Add Spending Policy API #612

BitcoinZavior · 2024-10-25T23:20:14Z

Description

Fixes the follwoing bug:

Encountering a Spending policy required: External error when attempting to build a transaction using a multi-sig descriptor. Currently, bdk-ffi does not provide a policy_path() in txBuilder, which is necessary for specifying the correct policy path during transaction creation.

The PR fix has the following changes:

Expose policies() from the wallet to retrieve available spending policies.
Add support for policy_path() in txBuilder to allow specifying the correct policy path when building the transaction.

Changelog notice

### Fixes

Resolved issue with building a transaction using a multi-sig descriptor resulted in a Spending policy error.
Added support for specifying a policy path in tx_builder using policy_path, enabling correct multi-sig spending policy specification during transaction creation.

Enhancements

Exposed policies() from the wallet to allow users to retrieve available spending policies for their wallet.

Checklists

All Submissions:

I've signed all my commits
I followed the contribution guidelines
I ran cargo fmt and cargo clippy before committing

New Features:

I've added tests for the new feature

Bugfixes:

I'm linking the issue being fixed by this PR
Fixes Spending Policy Required: External error on tx builder finish #605

reez

Nice! Thanks!!

A couple comments that are just notes and don't require any action, and then one question and two nits, but nothing that blocks me approving this or us merging as is:

ACK 4925a8d

Side question:

I see you checked I've added tests for the new feature in the PR template does that mean you are planning on adding a test (I didn't see one in current code)? Not even saying it's needed (and I'm assuming you tested on an external application since you opened Spending Policy Required: External error on tx builder finish #605 ). Note: I didn't test on an external app during this PR review.

reez · 2024-10-30T00:59:13Z

bdk-ffi/src/bdk.udl

@@ -401,6 +401,9 @@ interface Wallet {

  string descriptor_checksum(KeychainKind keychain);

+  [Throws=DescriptorError]
+  Policy? policies(KeychainKind keychain);


Looks good https://docs.rs/bdk_wallet/latest/bdk_wallet/struct.Wallet.html#method.policies

reez · 2024-10-30T01:01:00Z

bdk-ffi/src/bdk.udl

@@ -441,6 +444,10 @@ interface Wallet {

 interface Update {};

+interface Policy {
+  string id();


Is this the only field you need/want for your use case? Just curious really, since we can expose other fields later if needed obv.

Regardless, looks good https://docs.rs/bdk_wallet/latest/bdk_wallet/descriptor/policy/struct.Policy.html

Other fields have been exposed now.

reez · 2024-10-30T01:01:55Z

bdk-ffi/src/bdk.udl

@@ -456,6 +463,8 @@ interface TxBuilder {

  TxBuilder add_utxo(OutPoint outpoint);

+  TxBuilder policy_path(record<string, sequence<u64>> policy_path, KeychainKind keychain);


I think this is our first use of record https://mozilla.github.io/uniffi-rs/0.27/udl/builtin_types.html in the UDL

Looks good https://docs.rs/bdk_wallet/latest/bdk_wallet/tx_builder/struct.TxBuilder.html#method.policy_path

Yes, thanks!

reez · 2024-10-30T01:03:11Z

bdk-ffi/src/types.rs

 use bdk_wallet::AddressInfo as BdkAddressInfo;
 use bdk_wallet::Balance as BdkBalance;
 use bdk_wallet::KeychainKind;
 use bdk_wallet::LocalOutput as BdkLocalOutput;
 use bdk_wallet::Update as BdkUpdate;
-


nit: I think we can keep this blank line since we seem to break these use in groups

reez · 2024-10-30T01:05:22Z

bdk-ffi/src/wallet.rs

@@ -154,8 +161,7 @@ impl Wallet {

    pub(crate) fn sign(
        &self,
-        psbt: Arc<Psbt>,
-        // sign_options: Option<SignOptions>,


nit: might be nice to keep this on a separate line, but at the same time not any sort of blocker to approving this PR (and we might just remove the whole commented line before 1.0 beta anyway on bdk-ffi)

thunderbiscuit · 2024-10-30T18:31:16Z

I'm trying to understand this Policy type better. Reading the Wallet::policies example I imagine this policy to be a tree/list of policy nodes, each with an ID. But the Wallet::policies() method returns a single Policy that has a single id?

How do I know which path (u32) to choose if I'm not given the whole tree and options to choose from? I see that the Policy type potentially contains a field of type SatisfiableItem, which itself could be of the variant

Thresh {
    items: Vec<Policy>,
    threshold: usize,
},

Should we expose this field too so that users can query the satifiable item(s)?

I'm just not clear on this API and I want to make sure we get you what you need.

Follow up

I tried using one of the multisig examples in the BIP-383 test vectors:

wsh(multi(2,xprv9s21ZrQH143K31xYSDQpPDxsXRTUcvj2iNHm5NUtrGiGG5e2DtALGdso3pGz6ssrdK4PFmM8NSpSBHNqPqm55Qn3LqFtT2emdEXVYsCzC2U/2147483647/0,xprv9vHkqa6EV4sPZHYqZznhT2NPtPCjKuDKGY38FBWLvgaDx45zo9WQRUT3dKYnjwih2yJD9mkrocEZXo1ex8G81dwSM1fwqWpWkeS3v86pgKt/1/2/*,xprv9s21ZrQH143K3QTDL4LXw2F7HEK3wJUD2nW2nRk4stbPy6cq3jPPqjiChkVvvNKmPGJxWUtg6LnF5kejMRNNU3TGtRBeJgk33yuGBxrMPHi/10/20/30/40/*))

And I get this as a Policy:

Policy {
    id: "lrvxqx7c", 
    item: Multisig { 
        keys: [Fingerprint(bd16bee5), Fingerprint(5a61ff8e), Fingerprint(3442193e)],
        threshold: 2 
    }, 
    satisfaction: Partial {
        n: 3, 
        m: 2, 
        items: [], 
        sorted: Some(false),
        conditions: {} 
    },
    contribution: PartialComplete {
        n: 3, 
        m: 2, 
        items: [0, 1, 2], 
        sorted: Some(false),
        conditions: {
            [0, 1]: {Condition { csv: None, timelock: None }},
            [0, 2]: {Condition { csv: None, timelock: None }},
            [1, 2]: {Condition { csv: None, timelock: None }}
        } 
    } 
}

Is the vector you'd want to provide to the TxBuilder::policy_path method one of the options in the conditions field of the contribution field?

@afilini we might need your input here just to make sure we expose the correct behaviour and options.

…itten.

BitcoinZavior · 2024-11-07T00:32:02Z

Thanks @thunderbiscuit I have exposed other fields as well now.

BitcoinZavior · 2024-11-07T00:36:19Z

Should we expose this field too so that users can query the satifiable item(s)?

I have now exposed this field

BitcoinZavior · 2024-11-08T20:45:54Z

I'm trying to understand this Policy type better. Reading the Wallet::policies example I imagine this policy to be a tree/list of policy nodes, each with an ID. But the Wallet::policies() method returns a single Policy that has a single id?

How do I know which path (u32) to choose if I'm not given the whole tree and options to choose from? I see that the Policy type potentially contains a field of type SatisfiableItem, which itself could be of the variant
Thresh {
    items: Vec<Policy>,
    threshold: usize,
},
Should we expose this field too so that users can query the satifiable item(s)?

I'm just not clear on this API and I want to make sure we get you what you need.

@thunderbiscuit
I have now exposed that field too.

BitcoinZavior · 2024-11-08T20:50:18Z

I'm just not clear on this API and I want to make sure we get you what you need.

@thunderbiscuit Yes this is what we need. An integration test has been added here for the use case using the new exposed API:
https://github.com/LtbLightning/bdk-flutter/pull/147/files#diff-7c1bbba299e1163f3f58b2261196dbc0d886e22f40451fdb463ca1409d2b9ef6

thunderbiscuit · 2024-11-14T20:02:13Z

Thanks for the work @BitcoinZavior! I squashed and rebased. Merging now.

thunderbiscuit · 2024-11-14T20:09:27Z

For some reason I can't force push to your branch. But I added your commit in #626.

thunderbiscuit · 2024-11-15T16:49:24Z

Merged in #629. Thanks for the work @BitcoinZavior!

reez · 2024-11-15T16:50:38Z

Awesome work @BitcoinZavior !!!

BitcoinZavior added 3 commits October 19, 2024 16:51

feat: expose Wallet::policies method

61104c6

feat: expose TxBuilder::policy_path

c6c5eb9

feat: expose Policy::id method

4925a8d

BitcoinZavior marked this pull request as ready for review October 28, 2024 14:08

reez approved these changes Oct 30, 2024

View reviewed changes

BitcoinZavior added 7 commits October 31, 2024 17:23

feat: expose Policy::as_string & Policy::requires_path

1479893

feat: expose policy::item

c79b378

feat: expose policy::satisfaction

d22510f

feat: expose policy::contribution

437fcd0

Refactor policy_path in TxBuilder to prevent values from being overwr…

2eff412

…itten.

test: add test for tx_builder::policy_path method

a4f9a00

new lines corrected

e0251b4

Fix Clippy warning: simplify error handling

2b8592e

BitcoinZavior mentioned this pull request Nov 8, 2024

0.31.2 policy patch LtbLightning/bdk-flutter#147

Merged

Merge branch 'master' into add-policy-api

269b301

thunderbiscuit mentioned this pull request Nov 14, 2024

feat: expose Wallet::policies method #626

Closed

This was referenced Nov 14, 2024

Implement basic spending policy APIs #80

Closed

feat: expose Wallet::policies method #629

Merged

thunderbiscuit closed this Nov 15, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Spending Policy API #612

Add Spending Policy API #612

BitcoinZavior commented Oct 25, 2024 •

edited

Loading

reez left a comment •

edited

Loading

reez Oct 30, 2024

reez Oct 30, 2024

BitcoinZavior Nov 7, 2024

reez Oct 30, 2024

BitcoinZavior Nov 7, 2024

reez Oct 30, 2024

BitcoinZavior Nov 7, 2024

reez Oct 30, 2024

BitcoinZavior Nov 7, 2024

thunderbiscuit commented Oct 30, 2024 •

edited

Loading

BitcoinZavior commented Nov 7, 2024

BitcoinZavior commented Nov 7, 2024

BitcoinZavior commented Nov 8, 2024

BitcoinZavior commented Nov 8, 2024

thunderbiscuit commented Nov 14, 2024

thunderbiscuit commented Nov 14, 2024

thunderbiscuit commented Nov 15, 2024

reez commented Nov 15, 2024

		@@ -456,6 +463,8 @@ interface TxBuilder {

		TxBuilder add_utxo(OutPoint outpoint);

		TxBuilder policy_path(record<string, sequence<u64>> policy_path, KeychainKind keychain);

Add Spending Policy API #612

Add Spending Policy API #612

Conversation

BitcoinZavior commented Oct 25, 2024 • edited Loading

Description

Changelog notice

Enhancements

Checklists

All Submissions:

New Features:

Bugfixes:

reez left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thunderbiscuit commented Oct 30, 2024 • edited Loading

Follow up

BitcoinZavior commented Nov 7, 2024

BitcoinZavior commented Nov 7, 2024

BitcoinZavior commented Nov 8, 2024

BitcoinZavior commented Nov 8, 2024

thunderbiscuit commented Nov 14, 2024

thunderbiscuit commented Nov 14, 2024

thunderbiscuit commented Nov 15, 2024

reez commented Nov 15, 2024

BitcoinZavior commented Oct 25, 2024 •

edited

Loading

reez left a comment •

edited

Loading

thunderbiscuit commented Oct 30, 2024 •

edited

Loading