-
Notifications
You must be signed in to change notification settings - Fork 36
Multipath descriptor support (BIP 389) #275
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
Multipath descriptor support (BIP 389) #275
Conversation
Pull Request Test Coverage Report for Build 16654062685Details
💛 - Coveralls |
Thanks for working on this @schjonhaug. |
Glad to contribute, @ValuedMammal. Let me know if there’s anything I can improve! |
5893601
to
2911658
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for taking this one on, overall your PR looks great. I especially like the detailed docs and new validation to ensure only two indexes are used.
I have two small suggestions and you should do a rebase and then I think it's ready to merge.
2911658
to
7bbada5
Compare
I like the direction here and think this would be a great edition. In working with common hardware signer export files, these are very common descriptor representations. My one nit is to be careful naming the APIs with the more general |
@rustaceanrob what do you think about a little shorter name like |
@schjonhaug do you think you'll have time this week to make the small changes ValuedMammal and Rob suggested above? You also need to do a Or if you are OK with the changes but won't have time to work on it let me know and I can push a commit to your PR to wrap it up. Thanks! |
I created a commit you can copy or I can push here if it looks OK. Then all the commits should be squashed to one with a conventional commits style message. I think the name @rustaceanrob suggested is better even though it's long. Then later when we're able to support any |
- Add create_from_two_path_descriptor() method to create wallets from BIP 389 multipath descriptors - Support descriptors with exactly 2 paths (receive/change) using <0;1> syntax - Update MultiPath error to clarify requirement for exactly 2 paths - Add helper function make_two_path_descriptor_to_extract() for parsing two-path descriptors - Add comprehensive tests for two-path wallet creation and validation
7bbada5
to
9e3adf2
Compare
Thanks for the commit @notmandatory. I included it and renamed some functions and parameters to drive home the point that we are talking about two-path descriptors. |
Approach ACK |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 9e3adf2
Co-authored-by: ValuedMammal <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK da0b577
Thanks for getting this one wrapped up for the next release. I know there are downstream projects looking forward to having it.
Description
Key Features:
Wallet::create_multipath(descriptor)
following the same pattern ascreate()
andcreate_single()
Usage Example:
Notes to the reviewers
Design Decisions:
make_multipath_descriptor_to_extract()
helper following the same pattern as existingmake_descriptor_to_extract()
functioncreate_multipath()
method returnsCreateParams
just likecreate()
andcreate_single()
, maintaining the fluent builder patternImplementation Notes:
make_multipath_descriptor_to_extract()
twice (once for receive, once for change) which is intentional and follows the established pattern of lazy evaluationChangelog notice
Added:
Wallet::create_multipath()
method for creating wallets from BIP 389 multipath descriptorsCreateParams::new_multipath()
for multipath descriptor parameter creationChecklists
All Submissions:
just p
before pushingNew Features:
Bugfixes:
Closes #11