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."
There was a problem hiding this comment.
I can add it if that's the recommended/preferred way. But it means repetition of checks between the original bdk code and bdk-ffi, meaning that it are also two places to maintain if changed. While it gives the same outcome and behavior for the end user if we just let it fail from the original code. Let me know if you would prefer it though and I'll add it.
There was a problem hiding this comment.
I think it would make sense to match the bdk workflow, but in this case we also know that:
- This TxBuilder workflow will be completely replaced by the new
Wallet::create_psbtmethod in 3.0 - I don't think there are many users of this method other than you @kumulynja
- We can fix this later on in a patch or minor release since it would be a non-breaking change
I'm ok with this for now.
There was a problem hiding this comment.
is the decision on this to:
- not address if our next release is 3.0
- track & address if our next release is 2.x
There was a problem hiding this comment.
I think yes that would make sense. What's your take on it?
| } | ||
|
|
||
| #[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.
| } | ||
| } | ||
|
|
||
| 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?
There was a problem hiding this comment.
I only found: From<&BdkInput> for Input, not the other way around. That's why I did this. Let me know if I missed the From to use or if there is a better way to do this.
There was a problem hiding this comment.
No that's my bad I didn't realize you were going the other way sorry. All good!
|
@kumulynja I'm wondering if you have time to address questions and comments on this PR this week. It's basically our last merge before the 2.3 release. No pressure, I could just release 2.3 without it and it would make it probably into 2.4, but I know some folks are waiting for it so if possible I would include it if you have time to update it. Just let me know. |
|
@thunderbiscuit Sorry, I planned to do it during the passed weekend, but didn't get to it. I will try to get it done before the end of the week still. Thanks for the comments and follow up. |
Yes, I needed both to make the bdk bindings compatible with bark (Ark protocol from Second), so since the |
thunderbiscuit
force-pushed
the
missing-tx-builder-methods
branch
2 times, most recently
from
7c38002 to
83b2030
Compare
| psbt_input: Input, | ||
| satisfaction_weight: u64, | ||
| ) -> Result<Arc<Self>, AddForeignUtxoError> { | ||
| let bdk_outpoint: BdkOutPoint = outpoint.into(); |
There was a problem hiding this comment.
I think it would make sense to match the bdk workflow, but in this case we also know that:
- This TxBuilder workflow will be completely replaced by the new
Wallet::create_psbtmethod in 3.0 - I don't think there are many users of this method other than you @kumulynja
- We can fix this later on in a patch or minor release since it would be a non-breaking change
I'm ok with this for now.
thunderbiscuit
force-pushed
the
missing-tx-builder-methods
branch
from
83b2030 to
86aea5d
Compare
|
One more thing: the tests here require live access to the Signet network, and historically that's been a problem for us (I also notice it's using MutinyNet, also likely to go down at some point). In general it'd be great to migrate this to a fully regtest setup, probably leveraging the bdk test_env crate. |
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: