feat: expose only_witness_utxo and add_foreign_utxo on TxBuilder #928
Conversation
reez
left a comment
reez
left a comment
There was a problem hiding this comment.
Thanks for adding, this is looking like the right shape to me, I left a couple comments to address, otherwise this is looking good
| psbt_input: Input, | ||
| satisfaction_weight: u64, | ||
| ) -> Result<Arc<Self>, AddForeignUtxoError> { | ||
| let bdk_outpoint: BdkOutPoint = outpoint.into(); |
There was a problem hiding this comment.
should we guard here if psbt_input lacks both witness_utxo and non_witness_utxo
if psbt_input.witness_utxo.is_none() && psbt_input.non_witness_utxo.is_none() {
return Err(AddForeignUtxoError::MissingUtxo);
}
would match upstream behavior at the method boundary instead of deferring failure to finish()
"This method returns errors… The psbt_input does not contain a witness_utxo or non_witness_utxo."
| }) | ||
| } | ||
|
|
||
| /// Add a foreign UTXO i.e. a UTXO not owned by this wallet. |
There was a problem hiding this comment.
Docs here don’t look identical to upstream add_foreign_utxo any reason for that?
| }) | ||
| } | ||
|
|
||
| /// Only fill witness_utxo. |
There was a problem hiding this comment.
Docs here don’t look identical to upstream only_witness_utxo any reason for that?
| } | ||
|
|
||
| #[test] | ||
| fn test_add_foreign_utxo_missing_witness_data() { |
There was a problem hiding this comment.
if we add the guard in add_foreign_utxo (per my code comment in tx_builder.rs) we should update the test for that too
There was a problem hiding this comment.
A few questions I'd like to clear up before moving forward. I also agree with @reez comments about matching docs!
I'm also wondering if the only_witness_utxo API is tied to the add_foreign_utxo in your use case in some way or if you just added it because it wasn't there and you saw it as an easy win (I don't mind adding it, I just want to make sure I understand the use for it).
Thanks for the hard work! I know it's a big one.
Note: it also needs a rebase.
| /// This is automatically set when working with SegWit transactions. The `witness_utxo` field | ||
| /// is enough to construct the signature hash. |
There was a problem hiding this comment.
I'm not sure how true this statement is, or at least it's not in the original docs, and your default value on the field is false. I'd remove this line for now. Was there a reason you added it?
| } | ||
| } | ||
|
|
||
| impl TryFrom<Input> for BdkInput { |
There was a problem hiding this comment.
I don't know if implementing the TryFrom trait here is required since we already have the From trait implemented. Is there a reason to use the psbt_input.try_into()?; instead of just .into() and returning the error on the TxBuilder if the conversion fails?
3 participants
Description
This PR adds the
only_witness_utxoandadd_foreign_utxoonTxBuildersince they are available inbdk, but missing inbdk-ffi. They are useful for bdk bindings to be used together with for example bark (Ark implementation from Second Tech) so a unilateral exit can be done with Cpfp transactions.Notes to the reviewers
Since the
add_foreign_utxofunction is still experimental, I added the same comments mentioning this and the caution that has to be taken. I think it is good to expose it already though for experimentation like with bark as mentioned before.Both functions seem to be working in an example in Dart to unilaterally exit from bark through a fork of both bdk-ffi and bdk-dart.
Documentation
bdk-ffiChecklists
All Submissions:
cargo fmtandcargo clippybefore committingNew Features: