-
Notifications
You must be signed in to change notification settings - Fork 141
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
descriptor: introduce several Taproot accessors #751
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -241,6 +241,40 @@ impl<Pk: MiniscriptKey> Descriptor<Pk> { | |
Ok(Descriptor::Tr(Tr::new(key, script)?)) | ||
} | ||
|
||
/// For a Taproot descriptor, returns the internal key. | ||
pub fn internal_key(&self) -> Option<&Pk> { | ||
if let Descriptor::Tr(ref tr) = self { | ||
Some(tr.internal_key()) | ||
} else { | ||
None | ||
} | ||
} | ||
|
||
/// For a Taproot descriptor, returns the [`TapTree`] describing the Taproot tree. | ||
/// | ||
/// To obtain the individual leaves of the tree, call [`TapTree::iter`] on the | ||
/// returned value. | ||
pub fn tap_tree(&self) -> Option<&TapTree<Pk>> { | ||
if let Descriptor::Tr(ref tr) = self { | ||
tr.tap_tree().as_ref() | ||
} else { | ||
None | ||
} | ||
} | ||
|
||
/// For a Taproot descriptor, returns an iterator over the scripts in the Taptree. | ||
/// | ||
/// If the descriptor is not a Taproot descriptor, **or** if the descriptor is a | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I find this kind of strange, I would prefer to differentiate these two cases so I can handle the 'not Taproot descriptor' as an error case if I was expecting it to be a Taproot. But I guess that if the distinction is important, one could also explicitly use (my suggested) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can differentiate them by calling There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, actually to distinguish a non-taproot descriptor from a keyspend-only descriptor, but without matching and without being able to distinguish further, you would need to call There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
In what scenarios would it make sense to have a code path that handles both taproot and non-taproot descriptors in that way? It seems to me that using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, I see from your other comment that you were thinking about already known-taproot descriptors, say if the same code initializes the tr descriptor and calls methods on it. It can make sense for such cases. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, if you are trying to "just sign a transaction" you might take an arbitrary descriptor, grab its scriptcode to produce a pre-Taproot sighash, then also iterate through all the Taproot branches (if any) to produce Taproot sighashes. In this case you could use the same code for both Taproot and non-Taproot descriptors. |
||
/// Taproot descriptor containing only a keyspend, returns an empty iterator. | ||
pub fn tap_tree_iter(&self) -> tr::TapTreeIter<Pk> { | ||
if let Descriptor::Tr(ref tr) = self { | ||
if let Some(ref tree) = tr.tap_tree() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why not use |
||
return tree.iter(); | ||
} | ||
} | ||
tr::TapTreeIter::empty() | ||
} | ||
|
||
/// Get the [DescriptorType] of [Descriptor] | ||
pub fn desc_type(&self) -> DescriptorType { | ||
match *self { | ||
|
@@ -696,7 +730,7 @@ impl Descriptor<DescriptorPublicKey> { | |
|
||
struct KeyMapWrapper<'a, C: secp256k1::Signing>(KeyMap, &'a secp256k1::Secp256k1<C>); | ||
|
||
impl<'a, C: secp256k1::Signing> Translator<String> for KeyMapWrapper<'a, C> { | ||
impl<C: secp256k1::Signing> Translator<String> for KeyMapWrapper<'_, C> { | ||
type TargetPk = DescriptorPublicKey; | ||
type Error = Error; | ||
|
||
|
@@ -744,7 +778,7 @@ impl Descriptor<DescriptorPublicKey> { | |
pub fn to_string_with_secret(&self, key_map: &KeyMap) -> String { | ||
struct KeyMapLookUp<'a>(&'a KeyMap); | ||
|
||
impl<'a> Translator<DescriptorPublicKey> for KeyMapLookUp<'a> { | ||
impl Translator<DescriptorPublicKey> for KeyMapLookUp<'_> { | ||
type TargetPk = String; | ||
type Error = core::convert::Infallible; | ||
|
||
|
@@ -907,7 +941,7 @@ impl Descriptor<DefiniteDescriptorKey> { | |
) -> Result<Descriptor<bitcoin::PublicKey>, ConversionError> { | ||
struct Derivator<'a, C: secp256k1::Verification>(&'a secp256k1::Secp256k1<C>); | ||
|
||
impl<'a, C: secp256k1::Verification> Translator<DefiniteDescriptorKey> for Derivator<'a, C> { | ||
impl<C: secp256k1::Verification> Translator<DefiniteDescriptorKey> for Derivator<'_, C> { | ||
type TargetPk = bitcoin::PublicKey; | ||
type Error = ConversionError; | ||
|
||
|
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.
Perhaps name it
tap_internal_key()
to keep it consistent with the other methods and to make it clearer that its taproot-related and taproot-only?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.
The other methods yield a
TapTree
and are named as such, with no prefix. It's just more obvious that a "taptree" is a taproot object while an "internal key" is less obvious.I can stick a
taproot_
prefix onto here if you want, though that makes the method name longer and won't make it look consistent with the other methods. Since Taproot is the "default" output type for the forseeable future, I would almost rather sticknon_taproot_
onto everything else..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.
If Taproot is considered the 'default' then not having a prefix definitely makes sense. I didn't think of it as such.
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.
Heh, well, today it's definitely not the default (as evidenced by you, and me, and miketwenty1, running into a tons of API issues just in the last couple months). But things are trending that way and all the extensions to Bitcoin I see on the horizon work within Taproot, so I think we should assume (for API purposes) that this is the way things are going.
Having said that, maybe the way to do this is to foreground
Tr
, as you say, and let Taproot-only users use that in place ofDescriptor
? Not sure.I can add a
taproot_
prefix here if you really want. But I weakly prefer not to.