From c50954e9105fd81231fcb17527941d650e9c9f42 Mon Sep 17 00:00:00 2001 From: jp1ac4 <121959000+jp1ac4@users.noreply.github.com> Date: Mon, 22 Jan 2024 13:56:31 +0000 Subject: [PATCH 1/6] gui(transactions): split strings across multiple lines This is easier to read and ensures `cargo fmt` works. --- gui/src/app/view/transactions.rs | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/gui/src/app/view/transactions.rs b/gui/src/app/view/transactions.rs index 8b201d9d..78161fdd 100644 --- a/gui/src/app/view/transactions.rs +++ b/gui/src/app/view/transactions.rs @@ -169,9 +169,14 @@ pub fn create_rbf_modal<'a>( confirm_button.on_press(Message::CreateRbf(super::CreateRbfMessage::Confirm)); } let help_text = if is_cancel { - "Replace the transaction with one paying a higher feerate that sends the coins back to us. There is no guarantee the original transaction won't get mined first. New inputs may be used for the replacement transaction." + "Replace the transaction with one paying a higher feerate \ + that sends the coins back to us. There is no guarantee the \ + original transaction won't get mined first. New inputs may \ + be used for the replacement transaction." } else { - "Replace the transaction with one paying a higher feerate to incentivize faster confirmation. New inputs may be used for the replacement transaction." + "Replace the transaction with one paying a higher feerate \ + to incentivize faster confirmation. New inputs may be used \ + for the replacement transaction." }; card::simple( Column::new() @@ -188,7 +193,8 @@ pub fn create_rbf_modal<'a>( Message::CreateRbf(CreateRbfMessage::FeerateEdited(msg)) }) .warning( - "Feerate must be greater than previous value and less than or equal to 1000 sats/vbyte", + "Feerate must be greater than previous value and \ + less than or equal to 1000 sats/vbyte", ) .size(20) .padding(10), From 9e5872c56f7877227d5116207f611c2e44618325 Mon Sep 17 00:00:00 2001 From: jp1ac4 <121959000+jp1ac4@users.noreply.github.com> Date: Thu, 25 Jan 2024 08:32:57 +0000 Subject: [PATCH 2/6] gui(transactions): reword rbf modal message --- gui/src/app/view/transactions.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/gui/src/app/view/transactions.rs b/gui/src/app/view/transactions.rs index 78161fdd..77ab9955 100644 --- a/gui/src/app/view/transactions.rs +++ b/gui/src/app/view/transactions.rs @@ -170,8 +170,8 @@ pub fn create_rbf_modal<'a>( } let help_text = if is_cancel { "Replace the transaction with one paying a higher feerate \ - that sends the coins back to us. There is no guarantee the \ - original transaction won't get mined first. New inputs may \ + that sends the coins back to your wallet. There is no guarantee \ + the original transaction won't get mined first. New inputs may \ be used for the replacement transaction." } else { "Replace the transaction with one paying a higher feerate \ From beabf08cbfe55e12a191d02c0a3370dc207fac86 Mon Sep 17 00:00:00 2001 From: jp1ac4 <121959000+jp1ac4@users.noreply.github.com> Date: Thu, 1 Feb 2024 13:49:19 +0000 Subject: [PATCH 3/6] gui: match on message for PsbtState --- gui/src/app/state/psbt.rs | 89 +++++++++++++++++---------------------- 1 file changed, 38 insertions(+), 51 deletions(-) diff --git a/gui/src/app/state/psbt.rs b/gui/src/app/state/psbt.rs index 9e8632ed..88072960 100644 --- a/gui/src/app/state/psbt.rs +++ b/gui/src/app/state/psbt.rs @@ -131,60 +131,47 @@ impl PsbtState { cache: &Cache, message: Message, ) -> Command { - match &message { - Message::View(view::Message::Spend(msg)) => match msg { - view::SpendTxMessage::Cancel => { - if let Some(PsbtAction::Sign(SignAction { display_modal, .. })) = - &mut self.action - { - *display_modal = false; - return Command::none(); - } + match message { + Message::View(view::Message::Spend(view::SpendTxMessage::Cancel)) => { + if let Some(PsbtAction::Sign(SignAction { display_modal, .. })) = &mut self.action { + *display_modal = false; + return Command::none(); + } - self.action = None; + self.action = None; + } + Message::View(view::Message::Spend(view::SpendTxMessage::Delete)) => { + self.action = Some(PsbtAction::Delete(DeleteAction::default())); + } + Message::View(view::Message::Spend(view::SpendTxMessage::Sign)) => { + if let Some(PsbtAction::Sign(SignAction { display_modal, .. })) = &mut self.action { + *display_modal = true; + return Command::none(); } - view::SpendTxMessage::Delete => { - self.action = Some(PsbtAction::Delete(DeleteAction::default())); - } - view::SpendTxMessage::Sign => { - if let Some(PsbtAction::Sign(SignAction { display_modal, .. })) = - &mut self.action - { - *display_modal = true; - return Command::none(); - } - let action = SignAction::new( - self.tx.signers(), - self.wallet.clone(), - cache.datadir_path.clone(), - cache.network, - self.saved, - ); - let cmd = action.load(daemon); - self.action = Some(PsbtAction::Sign(action)); - return cmd; - } - view::SpendTxMessage::EditPsbt => { - let action = UpdateAction::new(self.wallet.clone(), self.tx.psbt.to_string()); - let cmd = action.load(daemon); - self.action = Some(PsbtAction::Update(action)); - return cmd; - } - view::SpendTxMessage::Broadcast => { - self.action = Some(PsbtAction::Broadcast(BroadcastAction::default())); - } - view::SpendTxMessage::Save => { - self.action = Some(PsbtAction::Save(SaveAction::default())); - } - _ => { - if let Some(action) = self.action.as_mut() { - return action - .as_mut() - .update(daemon.clone(), message, &mut self.tx); - } - } - }, + let action = SignAction::new( + self.tx.signers(), + self.wallet.clone(), + cache.datadir_path.clone(), + cache.network, + self.saved, + ); + let cmd = action.load(daemon); + self.action = Some(PsbtAction::Sign(action)); + return cmd; + } + Message::View(view::Message::Spend(view::SpendTxMessage::EditPsbt)) => { + let action = UpdateAction::new(self.wallet.clone(), self.tx.psbt.to_string()); + let cmd = action.load(daemon); + self.action = Some(PsbtAction::Update(action)); + return cmd; + } + Message::View(view::Message::Spend(view::SpendTxMessage::Broadcast)) => { + self.action = Some(PsbtAction::Broadcast(BroadcastAction::default())); + } + Message::View(view::Message::Spend(view::SpendTxMessage::Save)) => { + self.action = Some(PsbtAction::Save(SaveAction::default())); + } Message::View(view::Message::Label(_, _)) | Message::LabelsUpdated(_) => { match self.labels_edited.update( daemon, From 881d9a74a29eef21aafd75f9851f588142e0091f Mon Sep 17 00:00:00 2001 From: jp1ac4 <121959000+jp1ac4@users.noreply.github.com> Date: Thu, 1 Feb 2024 16:25:17 +0000 Subject: [PATCH 4/6] gui(psbt): check for conflicting txs before broadcast --- gui/src/app/message.rs | 3 +- gui/src/app/state/psbt.rs | 45 +++++++++++++++++++++++-- gui/src/app/view/psbt.rs | 70 +++++++++++++++++++++++++++++++++++++-- 3 files changed, 111 insertions(+), 7 deletions(-) diff --git a/gui/src/app/message.rs b/gui/src/app/message.rs index 0a0ae5d2..d7495752 100644 --- a/gui/src/app/message.rs +++ b/gui/src/app/message.rs @@ -1,4 +1,4 @@ -use std::collections::HashMap; +use std::collections::{HashMap, HashSet}; use std::sync::Arc; use liana::{ @@ -42,4 +42,5 @@ pub enum Message { HistoryTransactions(Result, Error>), PendingTransactions(Result, Error>), LabelsUpdated(Result>, Error>), + BroadcastModal(Result, Error>), } diff --git a/gui/src/app/state/psbt.rs b/gui/src/app/state/psbt.rs index 88072960..3d18db5a 100644 --- a/gui/src/app/state/psbt.rs +++ b/gui/src/app/state/psbt.rs @@ -8,7 +8,7 @@ use iced::Subscription; use iced::Command; use liana::{ descriptors::LianaPolicy, - miniscript::bitcoin::{bip32::Fingerprint, psbt::Psbt, Network}, + miniscript::bitcoin::{bip32::Fingerprint, psbt::Psbt, Network, Txid}, }; use liana_ui::component::toast; @@ -167,7 +167,29 @@ impl PsbtState { return cmd; } Message::View(view::Message::Spend(view::SpendTxMessage::Broadcast)) => { - self.action = Some(PsbtAction::Broadcast(BroadcastAction::default())); + let outpoints: HashSet<_> = self.tx.coins.keys().cloned().collect(); + return Command::perform( + async move { + daemon + // TODO: filter for the outpoints in `tx.coins` when this is possible: + // https://github.com/wizardsardine/liana/issues/677 + .list_coins() + .map(|res| { + res.coins + .iter() + .filter_map(|c| { + if outpoints.contains(&c.outpoint) { + c.spend_info.map(|info| info.txid) + } else { + None + } + }) + .collect() + }) + .map_err(|e| e.into()) + }, + Message::BroadcastModal, + ); } Message::View(view::Message::Spend(view::SpendTxMessage::Save)) => { self.action = Some(PsbtAction::Save(SaveAction::default())); @@ -194,6 +216,17 @@ impl PsbtState { .update(daemon.clone(), message, &mut self.tx); } } + Message::BroadcastModal(res) => match res { + Ok(conflicting_txids) => { + self.action = Some(PsbtAction::Broadcast(BroadcastAction { + conflicting_txids, + ..Default::default() + })); + } + Err(e) => { + self.warning = Some(e); + } + }, _ => { if let Some(action) = self.action.as_mut() { return action @@ -277,6 +310,8 @@ impl Action for SaveAction { pub struct BroadcastAction { broadcast: bool, error: Option, + /// IDs of any directly conflicting transactions. + conflicting_txids: HashSet, } impl Action for BroadcastAction { @@ -314,7 +349,11 @@ impl Action for BroadcastAction { fn view<'a>(&'a self, content: Element<'a, view::Message>) -> Element<'a, view::Message> { modal::Modal::new( content, - view::psbt::broadcast_action(self.error.as_ref(), self.broadcast), + view::psbt::broadcast_action( + &self.conflicting_txids, + self.error.as_ref(), + self.broadcast, + ), ) .on_blur(Some(view::Message::Spend(view::SpendTxMessage::Cancel))) .into() diff --git a/gui/src/app/view/psbt.rs b/gui/src/app/view/psbt.rs index 081f37fe..a7035cad 100644 --- a/gui/src/app/view/psbt.rs +++ b/gui/src/app/view/psbt.rs @@ -140,7 +140,15 @@ pub fn save_action<'a>(warning: Option<&Error>, saved: bool) -> Element<'a, Mess } } -pub fn broadcast_action<'a>(warning: Option<&Error>, saved: bool) -> Element<'a, Message> { +/// Return the modal view to broadcast a transaction. +/// +/// `conflicting_txids` contains the IDs of any directly conflicting transactions +/// of the transaction to be broadcast. +pub fn broadcast_action<'a>( + conflicting_txids: &HashSet, + warning: Option<&Error>, + saved: bool, +) -> Element<'a, Message> { if saved { card::simple(text("Transaction is broadcast")) .width(Length::Fixed(400.0)) @@ -151,7 +159,59 @@ pub fn broadcast_action<'a>(warning: Option<&Error>, saved: bool) -> Element<'a, Column::new() .spacing(10) .push_maybe(warning.map(|w| warn(Some(w)))) - .push(text("Broadcast the transaction")) + .push(Container::new(h4_bold("Broadcast the transaction")).width(Length::Fill)) + .push_maybe(if conflicting_txids.is_empty() { + None + } else { + Some( + conflicting_txids.iter().fold( + Column::new() + .spacing(5) + .push(Row::new().spacing(10).push(icon::warning_icon()).push(text( + if conflicting_txids.len() > 1 { + "WARNING: Broadcasting this transaction \ + will invalidate some pending payments." + } else { + "WARNING: Broadcasting this transaction \ + will invalidate a pending payment." + }, + ))) + .push(Row::new().padding([0, 30]).push(text( + if conflicting_txids.len() > 1 { + "The following transactions are \ + spending one or more inputs \ + from the transaction to be \ + broadcast and will be \ + dropped, along with any other \ + transactions that depend on them:" + } else { + "The following transaction is \ + spending one or more inputs \ + from the transaction to be \ + broadcast and will be \ + dropped, along with any other \ + transactions that depend on it:" + }, + ))), + |col, txid| { + col.push( + Row::new() + .padding([0, 30]) + .spacing(5) + .align_items(Alignment::Center) + .push(text(txid.to_string())) + .push( + Button::new( + icon::clipboard_icon().style(color::GREY_3), + ) + .on_press(Message::Clipboard(txid.to_string())) + .style(theme::Button::TransparentBorder), + ), + ) + }, + ), + ) + }) .push( Row::new().push(Column::new().width(Length::Fill)).push( button::primary(None, "Broadcast") @@ -159,7 +219,11 @@ pub fn broadcast_action<'a>(warning: Option<&Error>, saved: bool) -> Element<'a, ), ), ) - .width(Length::Fixed(400.0)) + .width(Length::Fixed(if conflicting_txids.is_empty() { + 400.0 + } else { + 800.0 + })) .into() } } From dc817dee3be64bba5e8bee7a0ec646bb90e643d4 Mon Sep 17 00:00:00 2001 From: jp1ac4 <121959000+jp1ac4@users.noreply.github.com> Date: Wed, 31 Jan 2024 17:31:15 +0000 Subject: [PATCH 5/6] gui(transactions): check for direct descendants before rbf --- gui/src/app/message.rs | 1 + gui/src/app/state/transactions.rs | 50 +++++++++++++++++++++++--- gui/src/app/view/transactions.rs | 59 +++++++++++++++++++++++++++++-- 3 files changed, 104 insertions(+), 6 deletions(-) diff --git a/gui/src/app/message.rs b/gui/src/app/message.rs index d7495752..d8758e5b 100644 --- a/gui/src/app/message.rs +++ b/gui/src/app/message.rs @@ -43,4 +43,5 @@ pub enum Message { PendingTransactions(Result, Error>), LabelsUpdated(Result>, Error>), BroadcastModal(Result, Error>), + RbfModal(HistoryTransaction, bool, u64, Result, Error>), } diff --git a/gui/src/app/state/transactions.rs b/gui/src/app/state/transactions.rs index 17faa458..7dd66394 100644 --- a/gui/src/app/state/transactions.rs +++ b/gui/src/app/state/transactions.rs @@ -1,5 +1,5 @@ use std::{ - collections::HashMap, + collections::{HashMap, HashSet}, convert::TryInto, sync::Arc, time::{SystemTime, UNIX_EPOCH}, @@ -112,6 +112,16 @@ impl State for TransactionsPanel { } } }, + Message::RbfModal(tx, is_cancel, prev_feerate_vb, res) => match res { + Ok(descendant_txids) => { + let modal = + CreateRbfModal::new(tx, is_cancel, prev_feerate_vb, descendant_txids); + self.create_rbf_modal = Some(modal); + } + Err(e) => { + self.warning = e.into(); + } + }, Message::View(view::Message::Close) => { self.selected_tx = None; } @@ -129,8 +139,30 @@ impl State for TransactionsPanel { .to_sat() .checked_div(tx.tx.vsize().try_into().unwrap()) .unwrap(); - let modal = CreateRbfModal::new(tx.clone(), is_cancel, prev_feerate_vb); - self.create_rbf_modal = Some(modal); + let tx = tx.clone(); + let txid = tx.tx.txid(); + return Command::perform( + async move { + daemon + // TODO: filter for spending coins when this is possible: + // https://github.com/wizardsardine/liana/issues/677 + .list_coins() + .map(|res| { + res.coins + .iter() + .filter_map(|c| { + if c.outpoint.txid == txid { + c.spend_info.map(|info| info.txid) + } else { + None + } + }) + .collect() + }) + .map_err(|e| e.into()) + }, + move |res| Message::RbfModal(tx, is_cancel, prev_feerate_vb, res), + ); } } } @@ -247,6 +279,9 @@ pub struct CreateRbfModal { is_cancel: bool, /// Min feerate required for RBF. min_feerate_vb: u64, + /// IDs of any transactions from this wallet that are direct descendants of + /// the transaction to be replaced. + descendant_txids: HashSet, /// Feerate form value. feerate_val: form::Value, /// Parsed feerate. @@ -259,12 +294,18 @@ pub struct CreateRbfModal { } impl CreateRbfModal { - fn new(tx: model::HistoryTransaction, is_cancel: bool, prev_feerate_vb: u64) -> Self { + fn new( + tx: model::HistoryTransaction, + is_cancel: bool, + prev_feerate_vb: u64, + descendant_txids: HashSet, + ) -> Self { let min_feerate_vb = prev_feerate_vb.checked_add(1).unwrap(); Self { tx, is_cancel, min_feerate_vb, + descendant_txids, feerate_val: form::Value { valid: true, value: min_feerate_vb.to_string(), @@ -329,6 +370,7 @@ impl CreateRbfModal { content, view::transactions::create_rbf_modal( self.is_cancel, + &self.descendant_txids, &self.feerate_val, self.replacement_txid, self.warning.as_ref(), diff --git a/gui/src/app/view/transactions.rs b/gui/src/app/view/transactions.rs index 77ab9955..d69f926b 100644 --- a/gui/src/app/view/transactions.rs +++ b/gui/src/app/view/transactions.rs @@ -1,5 +1,5 @@ use chrono::NaiveDateTime; -use std::collections::HashMap; +use std::collections::{HashMap, HashSet}; use iced::{alignment, widget::tooltip, Alignment, Length}; @@ -157,8 +157,13 @@ fn tx_list_view(i: usize, tx: &HistoryTransaction) -> Element<'_, Message> { .into() } +/// Return the modal view for a new RBF transaction. +/// +/// `descendant_txids` contains the IDs of any transactions from this wallet that are +/// direct descendants of the transaction to be replaced. pub fn create_rbf_modal<'a>( is_cancel: bool, + descendant_txids: &HashSet, feerate: &form::Value, replacement_txid: Option, warning: Option<&'a Error>, @@ -183,6 +188,56 @@ pub fn create_rbf_modal<'a>( .spacing(10) .push(Container::new(h4_bold("Transaction replacement")).width(Length::Fill)) .push(Row::new().push(text(help_text))) + .push_maybe(if descendant_txids.is_empty() { + None + } else { + Some( + descendant_txids.iter().fold( + Column::new() + .spacing(5) + .push(Row::new().spacing(10).push(icon::warning_icon()).push(text( + if descendant_txids.len() > 1 { + "WARNING: Replacing this transaction \ + will invalidate some later payments." + } else { + "WARNING: Replacing this transaction \ + will invalidate a later payment." + }, + ))) + .push(Row::new().padding([0, 30]).push(text( + if descendant_txids.len() > 1 { + "The following transactions are \ + spending one or more outputs \ + from the transaction to be replaced \ + and will be dropped when the replacement \ + is broadcast, along with any other \ + transactions that depend on them:" + } else { + "The following transaction is \ + spending one or more outputs \ + from the transaction to be replaced \ + and will be dropped when the replacement \ + is broadcast, along with any other \ + transactions that depend on it:" + }, + ))), + |col, txid| { + col.push( + Row::new() + .padding([0, 30]) + .spacing(5) + .align_items(Alignment::Center) + .push(text(txid.to_string())) + .push( + Button::new(icon::clipboard_icon().style(color::GREY_3)) + .on_press(Message::Clipboard(txid.to_string())) + .style(theme::Button::TransparentBorder), + ), + ) + }, + ), + ) + }) .push_maybe(if !is_cancel { Some( Row::new() @@ -225,7 +280,7 @@ pub fn create_rbf_modal<'a>( ) })), ) - .width(Length::Fixed(600.0)) + .width(Length::Fixed(800.0)) .into() } From fde28bf571e4ba791617e62f0c124138cd58e1fe Mon Sep 17 00:00:00 2001 From: jp1ac4 <121959000+jp1ac4@users.noreply.github.com> Date: Thu, 1 Feb 2024 16:38:28 +0000 Subject: [PATCH 6/6] gui(transactions): get prev feerate from tx instead of param --- gui/src/app/message.rs | 2 +- gui/src/app/state/transactions.rs | 20 ++++++++++---------- 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/gui/src/app/message.rs b/gui/src/app/message.rs index d8758e5b..93fafab3 100644 --- a/gui/src/app/message.rs +++ b/gui/src/app/message.rs @@ -43,5 +43,5 @@ pub enum Message { PendingTransactions(Result, Error>), LabelsUpdated(Result>, Error>), BroadcastModal(Result, Error>), - RbfModal(HistoryTransaction, bool, u64, Result, Error>), + RbfModal(HistoryTransaction, bool, Result, Error>), } diff --git a/gui/src/app/state/transactions.rs b/gui/src/app/state/transactions.rs index 7dd66394..c8349754 100644 --- a/gui/src/app/state/transactions.rs +++ b/gui/src/app/state/transactions.rs @@ -112,10 +112,9 @@ impl State for TransactionsPanel { } } }, - Message::RbfModal(tx, is_cancel, prev_feerate_vb, res) => match res { + Message::RbfModal(tx, is_cancel, res) => match res { Ok(descendant_txids) => { - let modal = - CreateRbfModal::new(tx, is_cancel, prev_feerate_vb, descendant_txids); + let modal = CreateRbfModal::new(tx, is_cancel, descendant_txids); self.create_rbf_modal = Some(modal); } Err(e) => { @@ -134,11 +133,7 @@ impl State for TransactionsPanel { Message::View(view::Message::CreateRbf(view::CreateRbfMessage::New(is_cancel))) => { if let Some(idx) = self.selected_tx { if let Some(tx) = self.pending_txs.get(idx) { - if let Some(fee_amount) = tx.fee_amount { - let prev_feerate_vb = fee_amount - .to_sat() - .checked_div(tx.tx.vsize().try_into().unwrap()) - .unwrap(); + if tx.fee_amount.is_some() { let tx = tx.clone(); let txid = tx.tx.txid(); return Command::perform( @@ -161,7 +156,7 @@ impl State for TransactionsPanel { }) .map_err(|e| e.into()) }, - move |res| Message::RbfModal(tx, is_cancel, prev_feerate_vb, res), + move |res| Message::RbfModal(tx, is_cancel, res), ); } } @@ -297,9 +292,14 @@ impl CreateRbfModal { fn new( tx: model::HistoryTransaction, is_cancel: bool, - prev_feerate_vb: u64, descendant_txids: HashSet, ) -> Self { + let prev_feerate_vb = tx + .fee_amount + .expect("rbf should only be used on a transaction with fee amount set") + .to_sat() + .checked_div(tx.tx.vsize().try_into().expect("vsize must fit in u64")) + .expect("transaction vsize must be positive"); let min_feerate_vb = prev_feerate_vb.checked_add(1).unwrap(); Self { tx,