gui: add warnings for descendant & conflicting transactions that will be dropped #930
Conversation
jp1ac4
force-pushed
the
gui-rbf-warn-descendant-coin-spending
branch
2 times, most recently
from
bd31e14
to
6f90d74
Compare
There was a problem hiding this comment.
Approach ACK.
These warnings are generated in the GUI using coins data from the DB and so will not appear if the DB coins have not yet been updated. Would resolving #887 help ensure the coins are updated before running these checks?
In most cases yes. It wouldn't cover it if it was just broadcast from another wallet (one such realistic scenario would be in the case of a multi-user wallet such as a multisig). But i think that's fine.
gui/src/app/view/psbt.rs
Outdated
| "WARNING: The following transaction{} spending one or more \ | ||
| inputs from the transaction to be broadcast and will be \ | ||
| dropped, along with any other transactions that depend on {}:", |
There was a problem hiding this comment.
Let's have a higher level first sentence before the technical transaction dependency description? I'm thinking something along the lines of "WARNING: replacing this transaction would invalidate some later payments. [...]".
gui/src/app/view/transactions.rs
Outdated
| "WARNING: The following transaction{} 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 {}:", |
jp1ac4
force-pushed
the
gui-rbf-warn-descendant-coin-spending
branch
from
6f90d74
to
802e0ee
Compare
|
I've made some changes to the wording as suggested. I also added the option to copy the transaction ID with a copy icon. Here are some screenshots for the broadcast warning:
For cancelling a transaction:
And for bumping the fee: I also reworded the existing "cancel transaction" message to mention sending coins "back to your wallet" instead of "back to us". |
|
That looks very good. Thank you for all the screenshots. |
gui/src/app/state/transactions.rs
Outdated
| @@ -126,8 +127,24 @@ impl State for TransactionsPanel { | |||
| .to_sat() | |||
| .checked_div(tx.tx.vsize().try_into().unwrap()) | |||
| .unwrap(); | |||
| let modal = | |||
| CreateRbfModal::new(tx.tx.txid(), is_cancel, prev_feerate_vb); | |||
| let descendant_txids: HashSet<_> = cache | |||
There was a problem hiding this comment.
I would like we do not use the cache coins for this usage. cache is generally used to display something like the previous balance, while the new balance is calculated in order to not display zero values.
I think it would be good instead, to have CreateRbfModal::new() -> (modal, command)
and give the command result to modal.update so it updates its conflicting txids.
The command would be an iced Command that use the daemon.list_coins and filter it and returns the Message::ConflictingRbfTransactions(Result<Vec, Error>)
What do you think ?
There was a problem hiding this comment.
That sounds good thanks, I'll make this change. After we implement #677, then we'll be able to retrieve just the spending coins in this call.
There was a problem hiding this comment.
I've now made the change to use daemon.list_coins and pass the result to another message.
In the end, I didn't pass the result to modal.update because when I tried this, the modal would open without any warnings and then update shortly afterwards, and I felt it was better that the warnings would be there as soon as the modal opened. So the modal is created using the result of list_coins.
I made a similar change for the set of conflicting transactions when broadcasting a transaction.
This is easier to read and ensures `cargo fmt` works.
jp1ac4
force-pushed
the
gui-rbf-warn-descendant-coin-spending
branch
from
802e0ee
to
fde28bf
Compare
|
As well as the changes discussed above in #930 (comment), I've also removed the In order for the new |
This is for #903.
It adds two warnings to the GUI:
when creating a new RBF if there are any descendant transactions of the transaction to be replaced:
when broadcasting a transaction if there are any conflicting transactions
These warnings are generated in the GUI using coins data from the DB and so will not appear if the DB coins have not yet been updated. Would resolving #887 help ensure the coins are updated before running these checks?