Skip to content

feat: add support for BIP-388 wallet policies#898

Open
trevarj wants to merge 4 commits intorust-bitcoin:masterfrom
trevarj:bip-388-wallet-policy
Open

feat: add support for BIP-388 wallet policies#898
trevarj wants to merge 4 commits intorust-bitcoin:masterfrom
trevarj:bip-388-wallet-policy

Conversation

@trevarj
Copy link

@trevarj trevarj commented Feb 12, 2026

Implemented BIP-388 by introducing a new type, WalletPolicy, which can be used
to create descriptor templates.

The idea is pretty simple and only really required making a new type that
implements MiniscriptKey, then the existing parser and Translator trait
takes care of the rest.

There are a few edge cases that require validation, which isn't so nice, but
works for now. Test cases that require BIP-390 and BIP-387's sortedmulti_a are
commented out and issues could arise with those test vectors when/if those BIPs
are implemented.

See WalletPolicy's doc and the unit tests for usage.

@trevarj
Copy link
Author

trevarj commented Feb 12, 2026

Closes #899

@apoelstra
Copy link
Member

concept ACK.

We do plan to implement musig and sortedmulti_a, but no timeline on that, and we can deal with it when we get to it.

We are also working on overhauling our validation framework more broadly, which I think/hope should eliminate the need for your hack. (IIRC there are other instances of this hack, or at least something similar, because of our weird separation between core::str::FromStr and crate::expression::FromTree, which I was able to address on my private branch.)

So I'd be okay to undraft this.

@trevarj trevarj force-pushed the bip-388-wallet-policy branch from 215e52e to 4a729f8 Compare February 12, 2026 14:33
@trevarj
Copy link
Author

trevarj commented Feb 12, 2026

Did a force push to address the incompatibilities with the MSRV.

@trevarj
Copy link
Author

trevarj commented Feb 12, 2026

We do plan to implement musig and sortedmulti_a, but no timeline on that, and we can deal with it when we get to it.

Nice, I got side-tracked and explored adding musig, but got too far down in the muck and wanted to finish this first.

We are also working on overhauling our validation framework more broadly, which I think/hope should eliminate the need for your hack. (IIRC there are other instances of this hack, or at least something similar, because of our weird separation between core::str::FromStr and crate::expression::FromTree, which I was able to address on my private branch.)

I will keep a lookout for this.

So I'd be okay to undraft this.

🫡

@trevarj trevarj marked this pull request as ready for review February 12, 2026 14:36
@tcharding
Copy link
Member

Trevor!! Can this be split up a bit mate? The wildcard display stuff, then maybe the XKeyParseError thing, then the rest?

@tcharding
Copy link
Member

API design suggestion but caveat this is not my repo so feel free to ignore me.

  • Would it be better to put validation inside from_descriptor, have from_descriptor_unchecked (if needed)?
  • Perhaps not even have the TryFroms? Or at least put the logic in the type methods and in the TryFrom just call the methods?

As far as semantics, it looks like the unit test prove the PR sufficiently to me.

@trevarj trevarj force-pushed the bip-388-wallet-policy branch from 4a729f8 to 6387817 Compare February 16, 2026 07:06
@trevarj
Copy link
Author

trevarj commented Feb 16, 2026

@tcharding

Trevor!!

TOBIN! Thank you for the review

Can this be split up a bit mate? The wildcard display stuff, then maybe the XKeyParseError thing, then the rest?

Split into four separate commits.

  • Would it be better to put validation inside from_descriptor, have from_descriptor_unchecked (if needed)?

Seems better to me. Easier to track down the logic than having to find the TryFrom.

Or at least put the logic in the type methods and in the TryFrom just call the methods?

Done

@trevarj trevarj requested a review from tcharding February 16, 2026 07:15
WalletPolicyParseFromString(String),
/// Couldn't set key info on WalletPolicy
WalletPolicyInvalidKeyInfo,
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know the style here in this repo, leaving for @apoelstra to comment. This is a single big error for the whole module unlike how we do it in rust-bitcoin.

Comment on lines +180 to +183
impl FromStr for WalletPolicy {
type Err = WalletPolicyError;
fn from_str(s: &str) -> Result<Self, Self::Err> { s.try_into() }
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Flagging that I'm not sure about the policy on this sort of thing in this repo. (by sort of thing I mean what could really be a blanket impl of FromStr using From<&str>.

@tcharding
Copy link
Member

Apart for the None/Some error thing my review only finds nits. I'm at my knowledge limit now so that is all I can offer I'm afraid.

- Instead of accepting a closure to convert the Key type to a String, added type
bounds to use Key: FromStr.
- Added new error type XKeyParseError, which will be a wrapper for the error
type on a Key's FromStr. XKeyParseError must implement From from that type.
- Change visibility of parse_xkey_deriv to pub(crate) for later usage.
- two new derivation_path functions, `derivation_path` and `derivation_paths`
that omit the path origin
- a `wildcard` accessor function to get the Wildcard value
Implemented BIP-388 by introducing a new type, `WalletPolicy`, which can be used
to create descriptor templates.

The idea is pretty simple and only really required making a new type that
implements `MiniscriptKey`, then the existing parser and `Translator` trait
takes care of the rest.

There are a few edge cases that require validation, which isn't so nice, but
works for now. Test cases that require BIP-390 and BIP-387's sortedmulti_a are
commented out and issues could arise with those test vectors when/if those BIPs
are implemented.

See `WalletPolicy`'s doc and the unit tests for usage.
@trevarj trevarj force-pushed the bip-388-wallet-policy branch from 6387817 to 945adb7 Compare February 17, 2026 08:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

Comments