From d9b8285fac1288e895d4b460646f73c664d1a2ff Mon Sep 17 00:00:00 2001 From: jp1ac4 <121959000+jp1ac4@users.noreply.github.com> Date: Fri, 1 Mar 2024 11:07:50 +0000 Subject: [PATCH 1/4] gui: fix cargo fmt --- gui/src/app/view/spend/mod.rs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/gui/src/app/view/spend/mod.rs b/gui/src/app/view/spend/mod.rs index be4d451f..70285ff3 100644 --- a/gui/src/app/view/spend/mod.rs +++ b/gui/src/app/view/spend/mod.rs @@ -176,7 +176,10 @@ pub fn create_spend_tx<'a>( Message::CreateSpend(CreateSpendMessage::FeerateEdited(msg)) }, ) - .warning("Feerate must be an integer less than or equal to 1000 sats/vbyte") + .warning( + "Feerate must be an integer less than \ + or equal to 1000 sats/vbyte", + ) .size(20) .padding(10), ) @@ -253,7 +256,8 @@ pub fn create_spend_tx<'a>( .width(Length::Fixed(100.0)), ) .push( - if is_valid && !duplicate + if is_valid + && !duplicate && (is_self_send || (total_amount < *balance_available && Some(&Amount::from_sat(0)) == amount_left)) From fba77a30e2aa9984cbda0fdaf40736ff1b0e57c4 Mon Sep 17 00:00:00 2001 From: jp1ac4 <121959000+jp1ac4@users.noreply.github.com> Date: Thu, 29 Feb 2024 16:14:38 +0000 Subject: [PATCH 2/4] gui: check if feerate is already set --- gui/src/app/view/spend/mod.rs | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/gui/src/app/view/spend/mod.rs b/gui/src/app/view/spend/mod.rs index 70285ff3..06731b18 100644 --- a/gui/src/app/view/spend/mod.rs +++ b/gui/src/app/view/spend/mod.rs @@ -220,8 +220,14 @@ pub fn create_spend_tx<'a>( .push(amount_with_size(amount_left, P2_SIZE)) .push(p2_regular("left to select").style(color::GREY_3)) } else { - Row::new() - .push(text("Feerate needs to be set.").style(color::GREY_3)) + Row::new().push( + text(if feerate.value.is_empty() || !feerate.valid { + "Feerate needs to be set." + } else { + "Add recipient details." + }) + .style(color::GREY_3), + ) }) .width(Length::Fill), ) From 83fa9a9c924bb1326b859f1d2d5aac58c291e960 Mon Sep 17 00:00:00 2001 From: jp1ac4 <121959000+jp1ac4@users.noreply.github.com> Date: Thu, 29 Feb 2024 16:19:51 +0000 Subject: [PATCH 3/4] gui: unset amount left to select if form invalid --- gui/src/app/state/spend/step.rs | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/gui/src/app/state/spend/step.rs b/gui/src/app/state/spend/step.rs index ff575cf9..37be938d 100644 --- a/gui/src/app/state/spend/step.rs +++ b/gui/src/app/state/spend/step.rs @@ -192,6 +192,13 @@ impl DefineSpend { /// if the user did not select a coin manually fn redraft(&mut self, daemon: Arc) { if !self.form_values_are_valid() || self.exists_duplicate() || self.recipients.is_empty() { + // The current form details are not valid to draft a spend, so remove any previously + // calculated amount as it will no longer be valid and could be misleading, e.g. if + // the user removes the amount from one of the recipients. + // We can leave any coins selected as they will either be automatically updated + // as soon as the form is valid or the user has selected these specific coins and + // so we should not touch them. + self.amount_left_to_select = None; return; } From 46cd0f4923c5fe59a5ff08134c850d9a9ceb345a Mon Sep 17 00:00:00 2001 From: jp1ac4 <121959000+jp1ac4@users.noreply.github.com> Date: Mon, 19 Feb 2024 13:26:46 +0000 Subject: [PATCH 4/4] gui: add max checkbox for spend recipients --- gui/src/app/state/spend/step.rs | 193 +++++++++++++++++++++++++++----- gui/src/app/view/message.rs | 1 + gui/src/app/view/spend/mod.rs | 61 ++++++++-- 3 files changed, 215 insertions(+), 40 deletions(-) diff --git a/gui/src/app/state/spend/step.rs b/gui/src/app/state/spend/step.rs index 37be938d..7ec15c18 100644 --- a/gui/src/app/state/spend/step.rs +++ b/gui/src/app/state/spend/step.rs @@ -1,6 +1,4 @@ -use std::collections::HashMap; -use std::str::FromStr; -use std::sync::Arc; +use std::{cmp::Ordering, collections::HashMap, str::FromStr, sync::Arc}; use iced::{Command, Subscription}; use liana::{ @@ -65,6 +63,9 @@ pub trait Step { pub struct DefineSpend { balance_available: Amount, recipients: Vec, + /// If set, this is the index of a recipient that should + /// receive the max amount. + send_max_to_recipient: Option, /// Will be `true` if coins for spend were manually selected by user. /// Otherwise, will be `false` (including for self-send). is_user_coin_selection: bool, @@ -123,6 +124,7 @@ impl DefineSpend { coins_labels: HashMap::new(), batch_label: form::Value::default(), recipients: vec![Recipient::default()], + send_max_to_recipient: None, is_user_coin_selection: false, // Start with auto-selection until user edits selection. is_valid: false, is_duplicate: false, @@ -162,12 +164,16 @@ impl DefineSpend { self } - fn form_values_are_valid(&self) -> bool { + // If `is_redraft`, the validation of recipients will take into account + // whether any should receive the max amount. Otherwise, all recipients + // will be fully validated. + fn form_values_are_valid(&self, is_redraft: bool) -> bool { self.feerate.valid && !self.feerate.value.is_empty() && (self.batch_label.valid || self.recipients.len() < 2) // Recipients will be empty for self-send. - && self.recipients.iter().all(|r| r.valid()) + && self.recipients.iter().enumerate().all(|(i, r)| + r.valid() || (is_redraft && self.send_max_to_recipient == Some(i) && r.address_valid())) } fn exists_duplicate(&self) -> bool { @@ -185,13 +191,16 @@ impl DefineSpend { fn check_valid(&mut self) { self.is_valid = - self.form_values_are_valid() && self.coins.iter().any(|(_, selected)| *selected); + self.form_values_are_valid(false) && self.coins.iter().any(|(_, selected)| *selected); self.is_duplicate = self.exists_duplicate(); } /// 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) { - if !self.form_values_are_valid() || self.exists_duplicate() || self.recipients.is_empty() { + if !self.form_values_are_valid(true) + || self.exists_duplicate() + || self.recipients.is_empty() + { // The current form details are not valid to draft a spend, so remove any previously // calculated amount as it will no longer be valid and could be misleading, e.g. if // the user removes the amount from one of the recipients. @@ -199,20 +208,51 @@ impl DefineSpend { // as soon as the form is valid or the user has selected these specific coins and // so we should not touch them. self.amount_left_to_select = None; + // Remove any max amount from a recipient as it could be misleading. + if let Some(i) = self.send_max_to_recipient { + self.recipients + .get_mut(i) + .expect("max has been requested for this recipient so it must exist") + .update( + self.network, + view::CreateSpendMessage::RecipientEdited(i, "amount", "".to_string()), + ); + } return; } let destinations: HashMap, u64> = self .recipients .iter() - .map(|recipient| { - ( - Address::from_str(&recipient.address.value).expect("Checked before"), - recipient.amount().expect("Checked before"), - ) + .enumerate() + .filter_map(|(i, recipient)| { + // A recipient that receives the max should be treated as change for coin selection. + // Note that we only give a change output if its value is above the dust + // threshold, but a user can only send payments above the same dust threshold, + // so using change output to determine the max amount for a recipient will + // not prevent a value that could otherwise be entered manually by the user. + if self.send_max_to_recipient == Some(i) { + None + } else { + Some(( + Address::from_str(&recipient.address.value).expect("Checked before"), + recipient.amount().expect("Checked before"), + )) + } }) .collect(); + let recipient_with_max = if let Some(i) = self.send_max_to_recipient { + Some(( + i, + self.recipients + .get_mut(i) + .expect("max has been requested for this recipient so it must exist"), + )) + } else { + None + }; + let outpoints = if self.is_user_coin_selection { let outpoints: Vec<_> = self .coins @@ -228,30 +268,53 @@ impl DefineSpend { ) .collect(); if outpoints.is_empty() { - // If the user has deselected all coins, simply set the amount left to select as the - // total destination value. Note this doesn't take account of the fee, but passing - // an empty list to `create_spend_tx` would use auto-selection and so we settle for - // this approximation. + // If the user has deselected all coins, set any recipient's max amount to 0. + if let Some((i, recipient)) = recipient_with_max { + recipient.update( + self.network, + view::CreateSpendMessage::RecipientEdited(i, "amount", "0".to_string()), + ); + } + // Simply set the amount left to select as the total destination value. Note this + // doesn't take account of the fee, but passing an empty list to `create_spend_tx` + // would use auto-selection and so we settle for this approximation. self.amount_left_to_select = Some(Amount::from_sat(destinations.values().sum())); return; } outpoints + } else if self.send_max_to_recipient.is_some() { + // If user has not selected coins, send the max available from all coins. + self.coins.iter().map(|(c, _)| c.outpoint).collect() } else { Vec::new() // pass empty list for auto-selection }; - // Use a fixed change address so that we don't increment the change index. - let dummy_address = self - .descriptor - .change_descriptor() - .derive(0.into(), &self.curve) - .address(self.network) - .as_unchecked() - .clone(); + // If sending the max to a recipient, use that recipient's address as the + // change address. + // Otherwise, use a fixed change address from the user's own wallet so that + // we don't increment the change index. + let change_address = if let Some((_, recipient)) = &recipient_with_max { + Address::from_str(&recipient.address.value) + .expect("Checked before") + .as_unchecked() + .clone() + } else { + self.descriptor + .change_descriptor() + .derive(0.into(), &self.curve) + .address(self.network) + .as_unchecked() + .clone() + }; let feerate_vb = self.feerate.value.parse::().expect("Checked before"); - match daemon.create_spend_tx(&outpoints, &destinations, feerate_vb, Some(dummy_address)) { + match daemon.create_spend_tx( + &outpoints, + &destinations, + feerate_vb, + Some(change_address.clone()), + ) { Ok(CreateSpendResult::Success { psbt, .. }) => { self.warning = None; if !self.is_user_coin_selection { @@ -268,6 +331,25 @@ impl DefineSpend { } // As coin selection was successful, we can assume there is nothing left to select. self.amount_left_to_select = Some(Amount::from_sat(0)); + if let Some((i, recipient)) = recipient_with_max { + // If there's no change output, any excess must be below the dust threshold + // and so the max available for this recipient is 0. + let amount = psbt + .unsigned_tx + .output + .iter() + .find(|o| { + o.script_pubkey + == change_address.clone().assume_checked().script_pubkey() + }) + .map(|change_output| change_output.value.to_btc()) + .unwrap_or(0.0) + .to_string(); + recipient.update( + self.network, + view::CreateSpendMessage::RecipientEdited(i, "amount", amount), + ); + } } // For coin selection error (insufficient funds), do not make any changes to // selected coins on screen and just show user how much is left to select. @@ -276,6 +358,25 @@ impl DefineSpend { // - select coins manually. Ok(CreateSpendResult::InsufficientFunds { missing }) => { self.amount_left_to_select = Some(Amount::from_sat(missing)); + if let Some((i, recipient)) = recipient_with_max { + let amount = Amount::from_sat(if destinations.is_empty() { + // If there are no other recipients, then the missing value will + // be the amount left to select in order to create an output at the dust + // threshold. Therefore, set this recipient's amount to this value so + // that the information shown is consistent. + // Otherwise, there are already insufficient funds for the other + // recipients and so the max available for this recipient is 0. + DUST_OUTPUT_SATS + } else { + 0 + }) + .to_btc() + .to_string(); + recipient.update( + self.network, + view::CreateSpendMessage::RecipientEdited(i, "amount", amount), + ); + } } Err(e) => { self.warning = Some(e.into()); @@ -307,6 +408,20 @@ impl Step for DefineSpend { self.batch_label.valid = true; self.batch_label.value = "".to_string(); } + if let Some(j) = self.send_max_to_recipient { + match j.cmp(&i) { + Ordering::Equal => { + self.send_max_to_recipient = None; + } + Ordering::Greater => { + self.send_max_to_recipient = Some( + j.checked_sub(1) + .expect("j must be greater than 0 in this case"), + ); + } + _ => {} + } + } } view::CreateSpendMessage::RecipientEdited(i, _, _) => { self.recipients @@ -377,6 +492,17 @@ impl Step for DefineSpend { self.is_user_coin_selection = true; } } + view::CreateSpendMessage::SendMaxToRecipient(i) => { + if self.recipients.get(i).is_some() { + if self.send_max_to_recipient == Some(i) { + // If already set to this recipient, then unset it. + self.send_max_to_recipient = None; + } else { + // Either it's set to some other recipient or not at all. + self.send_max_to_recipient = Some(i); + }; + } + } _ => {} } @@ -453,7 +579,11 @@ impl Step for DefineSpend { self.recipients .iter() .enumerate() - .map(|(i, recipient)| recipient.view(i).map(view::Message::CreateSpend)) + .map(|(i, recipient)| { + recipient + .view(i, self.send_max_to_recipient == Some(i)) + .map(view::Message::CreateSpend) + }) .collect(), Amount::from_sat( self.recipients @@ -509,9 +639,12 @@ impl Recipient { Ok(amount.to_sat()) } + fn address_valid(&self) -> bool { + !self.address.value.is_empty() && self.address.valid + } + fn valid(&self) -> bool { - !self.address.value.is_empty() - && self.address.valid + self.address_valid() && !self.amount.value.is_empty() && self.amount.valid && self.label.valid @@ -550,8 +683,8 @@ impl Recipient { }; } - fn view(&self, i: usize) -> Element { - view::spend::recipient_view(i, &self.address, &self.amount, &self.label) + fn view(&self, i: usize, is_max_selected: bool) -> Element { + view::spend::recipient_view(i, &self.address, &self.amount, &self.label, is_max_selected) } } diff --git a/gui/src/app/view/message.rs b/gui/src/app/view/message.rs index 8cbfb5f2..bde8a7c0 100644 --- a/gui/src/app/view/message.rs +++ b/gui/src/app/view/message.rs @@ -37,6 +37,7 @@ pub enum CreateSpendMessage { FeerateEdited(String), SelectPath(usize), Generate, + SendMaxToRecipient(usize), } #[derive(Debug, Clone)] diff --git a/gui/src/app/view/spend/mod.rs b/gui/src/app/view/spend/mod.rs index 06731b18..661e5b2d 100644 --- a/gui/src/app/view/spend/mod.rs +++ b/gui/src/app/view/spend/mod.rs @@ -2,7 +2,7 @@ use std::collections::HashMap; use iced::{ alignment, - widget::{checkbox, scrollable, Space}, + widget::{checkbox, scrollable, tooltip, Space}, Alignment, Length, }; @@ -215,10 +215,30 @@ pub fn create_spend_tx<'a>( )) .push(p2_regular("selected").style(color::GREY_3)) } else if let Some(amount_left) = amount_left { - Row::new() - .spacing(5) - .push(amount_with_size(amount_left, P2_SIZE)) - .push(p2_regular("left to select").style(color::GREY_3)) + if amount_left.to_sat() == 0 && !is_valid { + // If amount left is set, the current configuration must be redraftable. + // If it's not valid, either no coins are selected or there's a recipient + // with max selected and invalid amount. + if coins.iter().all(|(_, selected)| !selected) { + // This can happen if we have a single recipient + // and it has the max selected. + Row::new().push( + text("Select at least one coin.") + .style(color::GREY_3), + ) + } else { + // There must be a recipient with max selected and value 0. + Row::new().push( + text("Check max amount for recipient.") + .style(color::GREY_3), + ) + } + } else { + Row::new() + .spacing(5) + .push(amount_with_size(amount_left, P2_SIZE)) + .push(p2_regular("left to select").style(color::GREY_3)) + } } else { Row::new().push( text(if feerate.value.is_empty() || !feerate.valid { @@ -286,6 +306,7 @@ pub fn recipient_view<'a>( address: &form::Value, amount: &form::Value, label: &form::Value, + is_max_selected: bool, ) -> Element<'a, CreateSpendMessage> { Container::new( Column::new() @@ -338,7 +359,7 @@ pub fn recipient_view<'a>( ) .push( Row::new() - .align_items(Alignment::Start) + .align_items(Alignment::Center) .spacing(10) .push( Container::new(p1_bold("Amount")) @@ -346,16 +367,36 @@ pub fn recipient_view<'a>( .align_x(alignment::Horizontal::Right) .width(Length::Fixed(110.0)), ) - .push( - form::Form::new_trimmed("0.001 (in BTC)", amount, move |msg| { + .push_maybe(if is_max_selected { + Some( + Container::new( + text(amount.value.clone()).size(20).style(color::GREY_2), + ) + .padding(10) + .width(Length::Fill), + ) + } else { + None + }) + .push_maybe(if !is_max_selected { + Some(form::Form::new_trimmed("0.001 (in BTC)", amount, move |msg| { CreateSpendMessage::RecipientEdited(index, "amount", msg) }) .warning( "Invalid amount. (Note amounts lower than 0.00005 BTC are invalid.)", ) .size(20) - .padding(10), - ) + .padding(10)) + } else { + None + }) + .push(tooltip::Tooltip::new( + checkbox("MAX", is_max_selected, move |_| { + CreateSpendMessage::SendMaxToRecipient(index) + }), + "Total amount remaining after paying fee and any other recipients", + tooltip::Position::Left, + )) .width(Length::Fill), ), )