feat(wallet): add TxBuilder::replace_tx
#1799
Conversation
| // add previous fee | ||
| if let Ok(absolute) = self.wallet.calculate_fee(&tx) { | ||
| let rate = absolute / tx.weight(); | ||
| let previous_fee = PreviousFee { absolute, rate }; | ||
| self.params.bumping_fee = Some(previous_fee); | ||
| } |
There was a problem hiding this comment.
Should we still allow replacement if we can't determine the previous tx's fee/feerate?
There was a problem hiding this comment.
ConceptACK, ApproachACK.
Just need to make sure we don't use descendant outputs from tx we are trying to replace.
It's out of the scope of this PR, the inability to set both feerate/fee amount constraints together really bothers me. We should probably switch to the bdk_coin_select crate soon.
| // do not try to spend the outputs of the tx being replaced | ||
| self.params | ||
| .unspendable | ||
| .extend((0..tx.output.len()).map(|vout| OutPoint::new(txid, vout as u32))); |
There was a problem hiding this comment.
I think we also need to add the children outputs of the tx being replaced.
There was a problem hiding this comment.
Thanks, will fix.
crates/wallet/tests/wallet.rs
Outdated
There was a problem hiding this comment.
Maybe one more test to make sure we don't use descendant outputs of the tx we are trying to replace.
| /// - If none of the inputs are owned by this wallet | ||
| /// | ||
| /// [`finish`]: TxBuilder::finish | ||
| pub fn replace_tx(&mut self, txid: Txid) -> Result<&mut Self, ReplaceTxError> { |
There was a problem hiding this comment.
Should probably warn that we don't help keep the original tx's recipient.
- Add method `TxBuilder::previous_fee` for getting the previous feerate of the replaced tx
ValuedMammal
force-pushed
the
feat/replace-tx
branch
from
1e9caf4 to
3acc39b
Compare
| /// | ||
| /// # Note | ||
| /// | ||
| /// Aside from reusing one of the inputs, the method makes no assumptions about the |
There was a problem hiding this comment.
I would be wary of leaving this caveat up to documentation. The way I see it, one could either declare the recipient when calling replace_tx as a parameter or a new variant on the error path could be added for NoRecipient such that set_recipient must be called before replace_tx.
This PR adds a new method
TxBuilder::replace_txthat offers a simple way of creating replacements without relying on the mechanics ofbuild_fee_bump. See #1374 for context and motivating discussion.In summary, there are a few limitations with the current
build_fee_bump:In contrast
replace_txis useful when we only need to create a conflict and flexible enough to further tweak the parameters as desired.Notes to the reviewers
TxBuilder::add_utxosis modified to look for potentially spent outputs, provided that the builder is in a bump-fee context.Changelog notice
wallet: Added method
TxBuilder::replace_txChecklists
All Submissions:
cargo fmtandcargo clippybefore committingNew Features: