Use create_spend to calculate amount left to select and coin selection #863

edouardparis · 2023-12-08T16:56:50Z

Fixes #822. based on #865

darosior · 2023-12-11T14:01:05Z

Does this also fix #840?

edouardparis · 2023-12-11T14:02:19Z

Does this also fix #840?

Yes I believe you are right

edouardparis · 2023-12-11T14:19:45Z

So it does not crash anymore, but I think I should add a limit of 1000 to the form to just have an "invalid" form in red instead of having the big error banner.

pythcoiner · 2023-12-11T14:26:37Z

So it does not crash anymore, but I think I should add a limit of 1000 to the form to just have an "invalid" form in red instead of having the big error banner.

do this will 'lock' the step so the user cannot continue further? or just a hint?
1000 is just 3 times the sats/vbytes we experience few weeks ago, what if the fee go over this amount in few weeks/month?

darosior · 2023-12-11T14:32:09Z

do this will 'lock' the step so the user cannot continue further? or just a hint?

The user will encounter the same error if they click on "Next".

1000 is just 3 times the sats/vbytes we experience few weeks ago, what if the fee go over this amount in few weeks/month?

Yeah we might have to update our "max sane feerate", but that should be separate from this which just reuses what we use elsewhere.

pythcoiner · 2023-12-11T15:54:00Z

Yeah we might have to update our "max sane feerate", but that should be separate from this which just reuses what we use elsewhere.

maybe put in config file instead of hardcoding it?

jp1ac4

I've done some testing and it's looking good. I just had a few comments, mainly about using confirmed coins only for auto coin selection.

jp1ac4 · 2023-12-11T17:08:27Z

gui/src/app/state/spend/step.rs

+                })
+                .collect()
+        } else {
+            self.coins


For automated coin selection, we currently only consider confirmed coins, so I think we'll need to filter for those here to keep the same behaviour (see also #826).

Ok, Thanks for the info.

jp1ac4 · 2023-12-11T17:27:03Z

gui/src/app/state/spend/step.rs

+                    SpendOutputAddress {
+                        addr: Address::from_str(&recipient.address.value)
+                            .expect("Checked before")
+                            .assume_checked(),


I'm not sure if this matters when creating the spend, but assume_checked would set the network of a signet address to testnet. Following #813, we ignore the network when looking up addresses, but there might be some other effect of having a different network set.

I do not think it matters, signet address are testnet address. https://github.com/wizardsardine/liana/blob/master/src/commands/mod.rs#L232 also address verification is done in the edit address message cycle and does not concern this PR.

Following #813, we ignore the network when looking up addresses

Since this is using the spend module it's not connected to the database anymore at all.

but there might be some other effect of having a different network set.

As long as we are only using this dummy spend for selecting coins, the address encoding should have any effect. In fact we could be taking Scripts instead of addresses in:

liana/src/spend.rs

Lines 385 to 389 in 3e0f82a

#[derive(Debug, Clone, PartialEq, Eq)]

pub struct SpendOutputAddress {

pub addr: bitcoin::Address,

pub info: Option<AddrInfo>,

}

This would avoid requiring this kind of ugliness from the callers.

jp1ac4 · 2023-12-11T17:42:44Z

gui/src/app/state/spend/step.rs

+    /// redraft calculates the amount left to select and auto selects coins
+    /// if the user did not select a coin manually
+    fn redraft(&mut self, daemon: Arc<dyn Daemon + Sync + Send>) {
+        if !self.form_values_are_valid() || self.recipients.is_empty() {


Perhaps in a follow-up we could also use this function for a self-transfer to check if the user sets a feerate that is too high and will not give any change.

Yes, we could also use this function to calculate the amount a max button feature.

Opened #868 to track this.

gui/src/app/view/spend/mod.rs

close wizardsardine#822

jp1ac4

ACK a732468.

dfc4342 gui: set max feerate in RBF modal (jp1ac4) Pull request description: This applies a similar change to feerate form validation as in #863 to ensure value is not too high. ![image](https://github.com/wizardsardine/liana/assets/121959000/caa31a99-8bc7-43bb-a2c2-a9d41d752f50) ACKs for top commit: edouardparis: utACK dfc4342 Tree-SHA512: 7f1fa5eb71ea2c5bc6b2c28bd79ba39e3be4b39fcade89e1978441937cbb75a9de8c52ea397fc0c7ef9ef3858995bca69a077b9c3625bb027d39e3d57bf3fa7f

@darosior

1339898 commands: include missing amount in response (jp1ac4) Pull request description: This PR follows a discussion around #873 (comment). The GUI uses the `InsufficientFunds` error to get the missing amount when the user is creating a new spend, but it is not straightforward to extract this information in a general way from the RPC error (see #822 (comment)) and instead the spend module's `create_spend` is currently used (see #863). With this PR, the missing amount will be included in the `createspend` response rather than as an error. These changes are based on suggestions from @darosior and @edouardparis. In a follow-up PR, the GUI should revert to using the `createspend` command to calculate the amount left to select. ACKs for top commit: darosior: re-ACK 1339898 Tree-SHA512: bf702d6b355339e96e719c1d95824e7941ac4fbaece4ec4cccd00b56ea4683ce7fb0cefc43faa5731b57e7935ef99da3a2c73b84aaeb9fa5f67703c799be2196

…sing amount cb5073c gui(spend): use create_spend_tx for missing amount (jp1ac4) fae32df gui: pass change_address param to createspend (jp1ac4) Pull request description: This PR is a follow-up to #927 and uses changes made in #938. It reverts to using the `createspend` command in the GUI for automated coin selection and to determine the amount left to select when creating a new spend (which was previously changed in #863). With this PR, the changes from #873 will become effective in the GUI so that (some) unconfirmed coins are used as candidates and any additional fee to pay for ancestors is included when calculating the amount left to select. ACKs for top commit: edouardparis: ACK cb5073c Tree-SHA512: d780b8a0238b595301701d889c45b263682867cdff1ec054872f717de7ae3d325fc5010c8df29333ae2a44ae2e92a86689d332a26ac7334c7e92fd9ffc7a6397

…ndidates c0d4320 spend: add warning about fee for ancestor (jp1ac4) a38c173 spend: return additional fee paid for ancestors (jp1ac4) 62bb4ad commands: include unconfirmed change as candidates (jp1ac4) b05b0f1 commands: add ancestor info for user-selected unconfirmed coins (jp1ac4) 94ef66c spend: increase candidate weight to pay for ancestor (jp1ac4) 5f00220 bitcoin: add mempool_entry to interface (jp1ac4) edbf00f bitcoin: add ancestor size and fees to mempool entry (jp1ac4) 0450322 func test: use utils function (jp1ac4) Pull request description: This is a first PR towards resolving #826 that adds unconfirmed change as coin selection candidates when creating a spend. As per #826 (comment), I haven't made any changes to the `rbfpsbt` command. We will also need to apply the same change in the GUI when selecting candidates for coin selection there (see #863 (comment)). ACKs for top commit: darosior: ACK c0d4320 Tree-SHA512: 8c17f5f8c32913f1ffae3a93ca3e8ee52ac40ee86790e41d73def5ed0c057e110e101797f778715fcd5f6bded1cd170618209323b5114a4f69c02d0ce066a2f2

edouardparis force-pushed the gui-spend-redraft-amount-left-to-select branch from c3eb82f to 076023d Compare December 11, 2023 14:01

edouardparis force-pushed the gui-spend-redraft-amount-left-to-select branch from 076023d to 6d0eca5 Compare December 11, 2023 14:09

edouardparis force-pushed the gui-spend-redraft-amount-left-to-select branch from f1f6d58 to c50c757 Compare December 11, 2023 16:20

edouardparis marked this pull request as ready for review December 11, 2023 16:21

edouardparis force-pushed the gui-spend-redraft-amount-left-to-select branch from c50c757 to eac460b Compare December 11, 2023 16:23

darosior requested a review from jp1ac4 December 11, 2023 16:31

jp1ac4 reviewed Dec 11, 2023

View reviewed changes

edouardparis added 2 commits December 12, 2023 10:07

Use create_spend to calculate amount left to select

4ccecd1

close wizardsardine#822

spend: check max feerate

a732468

edouardparis force-pushed the gui-spend-redraft-amount-left-to-select branch from eac460b to a732468 Compare December 12, 2023 09:15

This was referenced Dec 12, 2023

GUI: check if self-transfer feerate is too high #868

Open

When sending, have a max button to send the whole balance (sweep wallet) #546

Closed

edouardparis requested a review from jp1ac4 December 12, 2023 09:42

jp1ac4 approved these changes Dec 12, 2023

View reviewed changes

darosior merged commit 0346b7a into wizardsardine:master Dec 12, 2023
18 checks passed

edouardparis deleted the gui-spend-redraft-amount-left-to-select branch December 12, 2023 13:31

jp1ac4 mentioned this pull request Dec 12, 2023

gui: set max feerate in RBF modal #869

Merged

jp1ac4 mentioned this pull request Dec 13, 2023

commands: include unconfirmed change as coin selection candidates #873

Merged

pythcoiner mentioned this pull request Dec 20, 2023

store "max sane feerate" in config file instead hardcoding it #891

Open

jp1ac4 mentioned this pull request Jan 17, 2024

commands: include missing amount in spend response #927

Merged

jp1ac4 mentioned this pull request Jan 30, 2024

gui: use createspend command for coin selection and missing amount #947

Merged

darosior mentioned this pull request Mar 12, 2024

gui: Don't increment change index when running (auto) coin selection #840

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use create_spend to calculate amount left to select and coin selection #863

Use create_spend to calculate amount left to select and coin selection #863

edouardparis commented Dec 8, 2023 •

edited

Loading

darosior commented Dec 11, 2023

edouardparis commented Dec 11, 2023

edouardparis commented Dec 11, 2023

pythcoiner commented Dec 11, 2023

darosior commented Dec 11, 2023

pythcoiner commented Dec 11, 2023

jp1ac4 left a comment

jp1ac4 Dec 11, 2023

edouardparis Dec 12, 2023

jp1ac4 Dec 11, 2023

edouardparis Dec 12, 2023

darosior Dec 12, 2023

jp1ac4 Dec 11, 2023

edouardparis Dec 12, 2023

darosior Dec 12, 2023

jp1ac4 left a comment

	#[derive(Debug, Clone, PartialEq, Eq)]
	pub struct SpendOutputAddress {
	pub addr: bitcoin::Address,
	pub info: Option<AddrInfo>,
	}

Use create_spend to calculate amount left to select and coin selection #863

Use create_spend to calculate amount left to select and coin selection #863

Conversation

edouardparis commented Dec 8, 2023 • edited Loading

darosior commented Dec 11, 2023

edouardparis commented Dec 11, 2023

edouardparis commented Dec 11, 2023

pythcoiner commented Dec 11, 2023

darosior commented Dec 11, 2023

pythcoiner commented Dec 11, 2023

jp1ac4 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jp1ac4 left a comment

Choose a reason for hiding this comment

edouardparis commented Dec 8, 2023 •

edited

Loading