-
Notifications
You must be signed in to change notification settings - Fork 79
Update compile
command to support creating taproot descriptors
#208
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
base: master
Are you sure you want to change the base?
Conversation
Pull Request Test Coverage Report for Build 16973164527Details
💛 - 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.
Thank you for working on this @va-an .
I have left a few comments for you.
Thank you
src/handlers.rs
Outdated
// This ensures the key path is effectively disabled and only script path can be used. | ||
// See https://github.com/bitcoin/bips/blob/master/bip-0341.mediawiki#constructing-and-spending-taproot-outputs | ||
let tree = TapTree::Leaf(Arc::new(taproot_policy)); | ||
Descriptor::new_tr(NUMS_UNSPENDABLE_KEY.to_string(), Some(tree)) |
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.
I guess the NUMS_UNSPENDABLE_KEY represents XonlyPublicKey, so it is better to parse it into an XonlyPublicKey and not used as a string (since strings are used for key placeholders) to avoid type mismatch.
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.
I've added validation that NUMS_UNSPENDABLE_KEY
is parsed as XOnlyPublicKey
.
Do you mean that nums_key should be passed as XOnlyPublicKey
to the new_tr
function?
let json_result = result.unwrap(); | ||
let descriptor = json_result.get("descriptor").unwrap().as_str().unwrap(); | ||
assert_eq!(descriptor, EXPECTED_AND_AB); | ||
} |
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.
can you also add test cases for invalid/invalid policies and other edge cases?
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.
Added some cases as separated test.
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.
@va-an Please rebase so I can test.
Thank you.
use bdk_wallet::miniscript::descriptor::TapTree; | ||
use std::sync::Arc; |
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.
Please move the imports to the top of the file and gate them.
assert!(EXPECTED_PK_A.contains(NUMS_UNSPENDABLE_KEY_HEX)); | ||
assert!(EXPECTED_AND_AB.contains(NUMS_UNSPENDABLE_KEY_HEX)); |
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.
I am wondering how much value these assert statements add when we are in control of the expected values ?
Description
Resolves #204.
Notes to the reviewers
For creating the tr descriptor, I used the NUMS pubkey proposed in BIP-341.
There is discussion about adding NUMS key to
rust-bicoin
, we can use it in the future from there.Also there is BIP draft for new descriptor key expression
unspendable()
for exacly this use case - we will simply use descriptortr(unspendable(), TREE)
.Changelog notice
Checklists
All Submissions:
cargo fmt
andcargo clippy
before committingNew Features:
CHANGELOG.md