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 methods to Descriptor + DescriptorPublicKey #625

Merged
merged 4 commits into from
Nov 15, 2024

Conversation

reez
Copy link
Collaborator

@reez reez commented Nov 12, 2024

Description

https://docs.rs/bdk_wallet/latest/bdk_wallet/descriptor/enum.DescriptorPublicKey.html#method.master_fingerprint

https://docs.rs/bdk_wallet/latest/bdk_wallet/descriptor/enum.DescriptorPublicKey.html#method.is_multipath

https://docs.rs/bdk_wallet/latest/bdk_wallet/descriptor/enum.Descriptor.html#method.is_multipath

https://docs.rs/bdk_wallet/latest/bdk_wallet/descriptor/enum.Descriptor.html#method.into_single_descriptors

Would love for this to get into our beta6

Notes to the reviewers

Use Cases:

  • creating a wallet from a descriptor having the ability to check if it's multipath or not
  • creating a wallet from an xpub having the ability to construct a DescriptorPublicKey fromString then get the fingerprint

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

Bugfixes:

  • This pull request breaks the existing API
  • I've added tests to reproduce the issue which are now passing
  • I'm linking the issue being fixed by this PR

@reez reez force-pushed the multipath branch 3 times, most recently from d2f1246 to 80ca720 Compare November 13, 2024 18:58
@reez reez marked this pull request as ready for review November 13, 2024 19:07
@reez reez requested a review from thunderbiscuit November 13, 2024 19:07
@reez reez changed the title (draft) Add methods to DescriptorPublicKey Add methods to Descriptor + DescriptorPublicKey Nov 13, 2024
@rustaceanrob
Copy link
Contributor

Concept ACK

Cool additions

@@ -266,6 +266,10 @@ impl Descriptor {
let key_map = &self.key_map;
descriptor.to_string_with_secret(key_map)
}

pub(crate) fn is_multipath(&self) -> bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

Just out of curiosity, why are all of these access modifiers crate level? Clearly has no effect on the bindings but seems like a scope definition that doesn't actually apply

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

good question/point-

@reez
Copy link
Collaborator Author

reez commented Nov 14, 2024

Concept ACK

Cool additions

Thanks!

@reez
Copy link
Collaborator Author

reez commented Nov 14, 2024

I get a failure because cargo clippy doesn't like the naming convention, I think the best option to get this to pass are renaming:

    pub(crate) fn to_single_descriptors(&self) -> Result<Vec<Arc<Descriptor>>, MiniscriptError> {
    }

will do that right now just cuz I would like this to pass even though that naming isn't exactly equivalent to the function we are wrapping.

Copy link
Member

@thunderbiscuit thunderbiscuit left a comment

Choose a reason for hiding this comment

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

Just a tiny comment on the identation of the docstrings, but otherwise this is good to go IMO.

bdk-ffi/src/bdk.udl Outdated Show resolved Hide resolved
@reez reez force-pushed the multipath branch 2 times, most recently from 2d23e03 to b9a89ec Compare November 15, 2024 16:33
@reez
Copy link
Collaborator Author

reez commented Nov 15, 2024

Just a tiny comment on the identation of the docstrings, but otherwise this is good to go IMO.

Updated docstrings indentation, rebased, ready to merge 👍

@thunderbiscuit thunderbiscuit self-requested a review November 15, 2024 16:50
@thunderbiscuit
Copy link
Member

Sorry man I merged #629 before this one, so you'll have to rebase, but feel free to merge afterwards!

@reez reez merged commit 53f93ad into bitcoindevkit:master Nov 15, 2024
25 checks passed
@reez reez deleted the multipath branch November 15, 2024 16:53
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