fix(wallet): Don't fail in build_fee_bump for missing parent txid
#337
Conversation
Clean up `test_add_foreign_utxo_bump_fee`. fix bitcoindevkit#325
Pull Request Test Coverage Report for Build 18917041894Details
💛 - Coveralls |
| let fee_rate = self | ||
| .calculate_fee_rate(&tx) | ||
| .map_err(|_| BuildFeeBumpError::FeeRateUnavailable)?; | ||
| let fee_rate = fee / tx.weight(); |
There was a problem hiding this comment.
What's the rationale to not use calculate_fee_rate?
There was a problem hiding this comment.
It seemed redundant since calculate_fee_rate already calls calculate_fee, so I chose to inline the implementation here.
| .script_pubkey(); | ||
| let witness_utxo = TxOut { | ||
| value: Amount::ZERO, | ||
| script_pubkey: ScriptBuf::new_p2a(), |
There was a problem hiding this comment.
new_p2a is not in rust-bitcoin 0.32.6, but in rust-bitcoin 0.32.7. Missing Cargo.toml update I guess
| } | ||
|
|
||
| #[test] | ||
| fn test_add_foreign_utxo_bump_fee() { |
There was a problem hiding this comment.
I would mention the type of script used in the first tx input, as I think is important for discoverability and also to illustrate the context of what this test is doing.
| let tx = psbt.unsigned_tx.clone(); | ||
| assert!(tx.input.iter().any(|txin| txin.previous_output == outpoint)); | ||
| let txid1 = tx.compute_txid(); | ||
| insert_tx(&mut wallet, tx); |
There was a problem hiding this comment.
A comment about the state in which this Tx is inserted from the perspective of the wallet would be helpful.
Like assume tx was broadcasted and is in mempool
| }; | ||
|
|
||
| let mut tx_builder = wallet.build_tx(); | ||
| tx_builder |
There was a problem hiding this comment.
The default fee rate of this tx is important but not expressed in the creation of the first tx of the test. A comment may be necessary here.
| graph | ||
| // Get previous transaction. | ||
| .get_tx(txin.previous_output.txid) | ||
| .ok_or(BuildFeeBumpError::UnknownUtxo(txin.previous_output)) |
There was a problem hiding this comment.
This is the source of the issue for #325, right?
| }) { | ||
| // This is a local utxo. | ||
| Some(&(keychain, derivation_index)) => { | ||
| let txout = prev_txout.ok_or(BuildFeeBumpError::UnknownUtxo(outpoint))?; |
There was a problem hiding this comment.
I think the error here is impossible to reach.
If the spk is present on the index, then this wallet collaborated on the creation of the transaction with the signature of that input, then that UTXO should have been already known by this wallet, right?
Also, we are getting this from the TxGraph itself and using an and_then call. All UnknownUtxos will be transformed into None and parsed by the other match arm before reaching this line.
There was a problem hiding this comment.
I will look for a way to rewrite it for clarity. I agree we should always have the txout, as otherwise we'd fail at calculate_fee.
3 participants
Description
This is a draft PR refactoring
build_fee_bumpin attempt to resolve #325.Notes to the reviewers
Changelog notice
Checklists
All Submissions:
just pbefore pushingNew Features:
Bugfixes: