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

feat(descriptor): add bip-86 template #388

Merged
merged 1 commit into from
Aug 15, 2023

Conversation

reez
Copy link
Collaborator

@reez reez commented Aug 3, 2023

Description

This PR adds the descriptor template for bip-86.

Links

Merged bdk Pull Request adding BIP-86 template

Docs for BIP-86 template

Notes to the reviewers

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 taproot-desc branch 3 times, most recently from ba8216a to 1f1902e Compare August 10, 2023 19:15
@reez reez marked this pull request as ready for review August 10, 2023 19:49
@reez reez added the enhancement New feature or request label Aug 10, 2023
@notmandatory
Copy link
Member

Looks good! we should make a local swift package with this branch and try it out in your example ios wallet.

@notmandatory notmandatory added this to the Release 0.30.0 milestone Aug 11, 2023
@reez
Copy link
Collaborator Author

reez commented Aug 11, 2023

Looks good! we should make a local swift package with this branch and try it out in your example ios wallet.

Can do!

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.

Tested. Looks good! Would you mind adding to the tests that start on line 266? Just for completeness since the other 3 templates are tested/showcased there.

I also think we can remove the comment on line 304; the templates are now showing correct coin types on testnet.

Otherwise ready to go IMO.

@reez
Copy link
Collaborator Author

reez commented Aug 11, 2023

Looks good! we should make a local swift package with this branch and try it out in your example ios wallet.

Just created a local swift package off of this branch and tried it out in the iOS wallet creating and using a bip-86 wallet, and it worked great.

Simulator Screenshot - iPhone 14 Pro - 2023-08-11 at 11 09 06

@reez
Copy link
Collaborator Author

reez commented Aug 11, 2023

Tested. Looks good! Would you mind adding to the tests that start on line 266? Just for completeness since the other 3 templates are tested/showcased there.

I also think we can remove the comment on line 304; the templates are now showing correct coin types on testnet.

Otherwise ready to go IMO.

Definitely! The code is updated now, and tests pass ✅.

  • Added tests for bip-86
  • removed comment on line 304



Note


I changed line 307


let template_private_84 = Descriptor::new_bip84(master, KeychainKind::External, Network::Testnet);


to


let template_private_84 = Descriptor::new_bip84(master.clone(), KeychainKind::External, Network::Testnet);


just because of the ownership rules since I added the line


let template_private_86 = Descriptor::new_bip86(master, KeychainKind::External, Network::Testnet);


after that line.


The other quick option was to just add .clone() to both of those lines, but I kept things similar to how it was before and just made the last of those lines (template_private_86) not have .clone().

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.

Tested ACK c3e8469.

@thunderbiscuit
Copy link
Member

thunderbiscuit commented Aug 15, 2023

@reez do you mind rebasing this on master? I'll merge right away. Sorry this took so long to review/get in.

@thunderbiscuit thunderbiscuit merged commit c3e8469 into bitcoindevkit:master Aug 15, 2023
18 checks passed
@reez reez deleted the taproot-desc branch August 16, 2023 16:04
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
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants