Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add descriptor templates #260

Merged
merged 5 commits into from
Dec 16, 2022

Conversation

thunderbiscuit
Copy link
Member

@thunderbiscuit thunderbiscuit commented Nov 22, 2022

Fixes #165.

Also relevant is bitcoindevkit/bdk#817

Description

This PR adds the descriptor templates as described in #165.

Links

Docs for BIP84 template

Changelog notice

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
  • I've added docs for the new feature

@thunderbiscuit thunderbiscuit self-assigned this Nov 22, 2022
@thunderbiscuit thunderbiscuit changed the title Add descriptor templates udl components Add descriptor templates Nov 22, 2022
@thunderbiscuit thunderbiscuit marked this pull request as draft November 22, 2022 19:47
@notmandatory
Copy link
Member

API design looks good to me.

bdk-ffi/src/bdk.udl Outdated Show resolved Hide resolved
@thunderbiscuit
Copy link
Member Author

thunderbiscuit commented Nov 23, 2022

This PR is surprisingly more complex to pull off than one might think at first glance. The core issue is that descriptors in BDK are built on fairly complex types and generics coming from rust-miniscript and rust-bitcoin.

Here are some notes I wrote for myself to keep track of the task at hand and the tradeoffs:

Types related to extended keys and descriptors in bdk, rust-bitcoin, and miniscript:

// bdk::keys::SinglePriv                 (miniscript) <- Struct
// bdk::keys::SinglePub                  (miniscript) <- Struct
// bdk::keys::DescriptorSecretKey        (miniscript) <- Enum
// bdk::keys::DescriptorPublicKey        (miniscript) <- Enum
// bdk::descriptor::DescriptorXKey       (miniscript) <- Struct
// bdk::descriptor::Descriptor           (miniscript) <- Enum

// bdk::keys::ExtendedKey                (bdk)        <- Enum
// bdk::keys::DerivableKey               (bdk)        <- Trait
// bdk::descriptor::IntoWalletDescriptor (bdk)        <- Trait
// bdk::keys::DescriptorKey              (bdk)        <- Enum
// bdk::descriptor::ExtendedDescriptor   (bdk)        <- Alias for Descriptor<DescriptorPublicKey>
// bdk::descriptor::template::DescriptorTemplateOut   <- Alias for (ExtendedDescriptor, KeyMap, ValidNetworks)

// bitcoin::util::bip32::ExtendedPrivKey (bitcoin)    <- Struct
// bitcoin::util::bip32::ExtendedPubKey  (bitcoin)    <- Struct

Bindings Wallet vs Rust BDK Wallet

In the bindings the Wallet currently takes descriptors in the form of strings whereas in Rust BDK, the main constructor is a generic function on the descriptor argument with a trait bound of IntoWalletDescriptor:

impl Wallet {
    pub fn new<E: IntoWalletDescriptor>(
        descriptor: E,
        change_descriptor: Option<E>,
        // ...
    ) -> Result<Self, Error> {}
}

This trait is implemented on a few types:

  1. &str
  2. &String
  3. (ExtendedDescriptor, KeyMap)
  4. DescriptorTemplateOut -> that's just an alias for (ExtendedDescriptor, KeyMap, ValidNetworks)
  5. ExtendedDescriptor -> that's just an alias for Descriptor<DescriptorPublicKey>, so only works with xpubs

Currently unsolved

The example DescriptorPR260 type I added here showcases one way to do this: we could have a Descriptor type which would simply wrap a DescriptorTemplateOut. We could even change the argument type on the Wallet to take this Descriptor type if we wanted. The problem is that I don't see a "clean" way to retrieve the descriptor as a string after we've used the template. The type that comes out of the Template::build() method is (ExtendedDescriptor, KeyMap, ValidNetworks), segreggating the extended public key and the private keys. Not clear to me how you'd just "print the private descriptor" yet, although I'm still playing around with it.

@thunderbiscuit
Copy link
Member Author

thunderbiscuit commented Nov 24, 2022

Good progress was made today. BIP44, 49, and 84 "private" templates work. A few outstanding discussion items:

  1. The fingerprint argument on the public version of the templates is mandatory, which means we'll need a way for users to get this fingerprint out of the DescriptorSecretKey struct if we want to keep the API the same.
  2. I think we need 2 as_string() methods: one that returns the public version of the descriptor, and one that returns the private version if there is one
  3. We can either ask users to always build a string out of the Descriptor type and give that to the wallet, or we could make the wallet take a descriptor of type Descriptor. This would be cleaner and nearer to the BDK API, but implies we need to have a constructor that takes a string on the Descriptor type. All of this is a bit different than in BDK, where the descriptor provided to the Wallet struct are generics and simply have a trait bound of IntoWalletDescriptor.
  4. What exact type the bindings Descriptor type should wrap is still up for debate: I am currently wrapping the DescriptorTemplateOut, (alias for (ExtendedDescriptor, KeyMap, ValidNetworks)), but that doesn't even work for public versions of the descriptors, so this will need to change no matter what.
  5. The Bip86 template is not something in BDK yet. I assume the best way to go about this is to open an issue/PR on Rust BDK to add it upstream, and so for now I have simply omitted it.

@thunderbiscuit
Copy link
Member Author

If bitcoindevkit/bdk/pull/821 gets merged I assume we might be ok with adding the dev-dependency on assert_matches as well. It feels much cleaner to me, and I have prepared an extra commit that brings that in.

// asserting on the string
assert_eq!(wallet2.unwrap_err().to_string(), "Descriptor(Key(InvalidNetwork))");

// assert_matches!
assert_matches!(wallet2.unwrap_err(), bdk::Error::Descriptor(Key(InvalidNetwork)))

@thunderbiscuit thunderbiscuit force-pushed the templates branch 2 times, most recently from d99e5d2 to 9c58ac4 Compare December 15, 2022 15:07
@thunderbiscuit thunderbiscuit force-pushed the templates branch 2 times, most recently from 0ef23f6 to 9b690f8 Compare December 16, 2022 16:44
bdk-ffi/src/lib.rs Outdated Show resolved Hide resolved
Copy link
Member

@notmandatory notmandatory left a comment

Choose a reason for hiding this comment

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

ACK bada9b8

@notmandatory notmandatory added the enhancement New feature or request label Dec 16, 2022
@thunderbiscuit thunderbiscuit merged commit bada9b8 into bitcoindevkit:master Dec 16, 2022
@thunderbiscuit thunderbiscuit deleted the templates branch November 14, 2023 15:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Create a type safe descriptor builder
2 participants