-
Notifications
You must be signed in to change notification settings - Fork 83
Adding descriptor generator #180
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
Adding descriptor generator #180
Conversation
09b4f4a to
ebf6be3
Compare
tvpeter
left a comment
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.
Good work Amos.
Most of the fns in utils should be in the handlers file, while most of those in handlers should be placed in the utils.
tvpeter
left a comment
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.
Thank you for your work on this, @AmosOO7.
If possible, please revert the imports you removed, as this will help in tracking your imports. Also, kindly remove any unnecessary comments and squash your commits into logical groupings. I have also left comments on other specific areas for your review.
Thank you!
6bff257 to
31f256a
Compare
tvpeter
left a comment
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.
Thank you for continuing to work on this.
I have left a couple of comments.
15495c4 to
6c8e714
Compare
Pull Request Test Coverage Report for Build 19111115390Details
💛 - Coveralls |
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 @AmosOO7 for continuing to work on this.
The checks for path and how a user can use it is a little bit confusing. I think we might need to split it into subcommands, similar to how key has generate, derive and restore. If you have any suggestions for the best subcategories, please feel free to share them in the conversation before continuing with the implementation. Otherwise, I will propose the subcategories this coming week to make it clearer. I've also left some additional comments for you.
Additionally, it would be a good idea to include documentation comments for each command option and parameter to help users better understand their functionalities.
Thank you.
|
After having a discussion with @tvpeter and following his suggestions. Breaking the Suggested Subcommands for
|
| Subcommand | Description |
|---|---|
| from-key | Generate descriptors from a provided Xprv or Xpub |
| from-mnemonic | Generate descriptors from a newly generated or provided BIP39 mnemonic |
| multipath | Generate multipath (wildcard derivation) descriptors from a key |
| info (optional/future) | Inspect a descriptor and show parsed structure (e.g., key origin, type, script) |
Detailed Breakdown
1. descriptor from-key
-
Input:
--type,<XPRV | XPUB> -
Optional:
--path m/84h/1h/0h(for custom derivation base) -
Output: external/internal descriptor strings
-
Optionally support both single and multipath modes via
--multipath
2. descriptor from-mnemonic
-
Input:
--type, -
Optional:
--mnemonic <WORDS>(if not, generate randomly) -
Output: same as
from-key, plus return mnemonic used
3. descriptor multipath
-
Input:
--type,<XPRV | XPUB> -
Output: wildcard descriptors (e.g.,
/0/*and/1/*)
4. (Optional) descriptor info
-
Input: descriptor string
-
Output: parsed structure, origin, derivation, checksum, script type, etc.
Example CLI Usage
From key
bdk-cli descriptor from-key --type 84 <XPRV | <XPUB>;
From mnemonic
bdk-cli descriptor from-mnemonic --type 86 (optional)<WORDS>
From key with multipath
bdk-cli descriptor multipath --type 44 <XPUB | <XPUB>;
Descriptor info (optional)
bdk-cli descriptor info "wpkh([abcd1234/84'/1'/0']xpub.../0/*)"
What do you think about the suggestions @tvpeter and @notmandatory ?
|
I think the following is more descriptive:
note:
Also, wait for confirmation from Notmandatory. |
|
@tvpeter I agree that having two Having the |
@AmosOO7, you should be able to update according to this. Thank you |
tvpeter
left a comment
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.
Hi @AmosOO7,
Thanks for continuing to push this PR. I have left comments, when you address them, I will continue with testing.
Thank you
|
@AmosOO7, can you please make time to update this PR? I was hoping it could make it into the next release. It would be great if you could push it forward. |
tvpeter
left a comment
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.
Thank you for your efforts on this PR @AmosOO7
I have left comments but don't worry, I will push changes so that it is easier for us to close the PR.
Thank you.
483a7f8 to
1452eaa
Compare
|
Unfortunately this PR needs a lot of cleanup, I've briefly discussed with @tvpeter and he'll help rework it when he has time. Problems I initially ran into:
|
- Created Subcommnds for the descriptor command; generate - Created function to get descriptors from mnemonics
- add generating multipath descriptors - add pretty formatting for descriptors
- remove multipath descriptor generation - fix descriptor type
1452eaa to
c8f23ba
Compare
@notmandatory for this 4th item, should I yank off the |
0f535b7 to
f6f77f8
Compare
f6f77f8 to
32a2c88
Compare
- remove generate subcommand - add descriptor to the repl
notmandatory
left a comment
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.
tACK 7d3720e
I tested:
- create each descriptor type
- create from mnemonic, xpub, xprv
- output with --pretty
Looks good, and should be helpful for new folks learning about descriptors.
tvpeter
left a comment
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 7d3720e
Pull Request: Add
descriptorSubcommandDescription
This PR introduces a new
descriptorsubcommand to the BDK CLI that enable users to generate and inspect Bitcoin wallet descriptors for internal and external paths, supporting common types (pkh, sh, wpkh, wsh, tr). It includes support for both single-path descriptors from extended keys (xprv/xpubortprv/tpub).This resolves issue #175
Features Implemented
descriptorsubcommand with:generate– generate new descriptors--typedefaults towshand a shortt--networkdefaulttestnetUsage Examples
1. Generate Descriptors from an Extended Private Key
2. Generate Descriptors from a Mnemonic (no key provided)
cargo fmtandcargo clippybefore committingNew Features:
CHANGELOG.md