Require explicit wildcard derivation for descriptor key types #853
Conversation
thunderbiscuit
force-pushed
the
fix/descriptor
branch
from
de38635 to
606d2e0
Compare
thunderbiscuit
force-pushed
the
fix/descriptor
branch
from
606d2e0 to
075073b
Compare
| xkey: xkey.into_xprv(network).unwrap(), | ||
| derivation_path: BdkDerivationPath::master(), | ||
| wildcard: Wildcard::Unhardened, | ||
| derivation_path: BdkDerivationPath::default(), |
There was a problem hiding this comment.
Just curious. Any reason why we are updating to default() here?
| /// Construct a secret descriptor using a mnemonic. | ||
| /// Construct a secret descriptor key using a mnemonic. | ||
| #[uniffi::constructor] | ||
| pub fn new(network: Network, mnemonic: &Mnemonic, password: Option<String>) -> Self { |
There was a problem hiding this comment.
I would have said since "wildcardness" is being set in the new() method why don't we also just pass a parameter that determines whether wildcard should be added on create or not. But I think you are right, there is no real reason why wildcard would be placed immediately/at end of the tprv...../*. This will make sense to be added after the extension has been added as like a step 3 like in your fn test_add_wildcard() test below.
From running the test on here, I believe it should work seamlessly too on the language bindings side (android, jvm...)
| pub fn new(network: Network, mnemonic: &Mnemonic, password: Option<String>) -> Self { | ||
| let secp = Secp256k1::new(); | ||
| let mnemonic = mnemonic.0.clone(); | ||
| let xkey: ExtendedKey = (mnemonic, password).into_extended_key().unwrap(); | ||
| let descriptor_public_key = BdkDescriptorPublicKey::XPub(DescriptorXKey { | ||
| origin: None, | ||
| xkey: xkey.into_xpub(network, &secp), | ||
| derivation_path: BdkDerivationPath::default(), | ||
| wildcard: Wildcard::None, | ||
| }); | ||
| Self(descriptor_public_key) | ||
| } |
There was a problem hiding this comment.
what do you think about this instead:
pub fn new(network: Network, mnemonic: &Mnemonic, password: Option<String>) -> Self {
let public_key = DescriptorSecretKey::new(network, mnemonic, password).as_public();
Self(public_key.0.clone())
}
| } | ||
| } | ||
|
|
||
| pub fn add_wildcard(&self) -> Result<Arc<Self>, DescriptorKeyError> { |
There was a problem hiding this comment.
after this line we could do:
if matches!(descriptor_x_key.wildcard, Wildcard::Unhardened) {
return Ok(Arc::new(Self(self.0.clone())));
}
but its not totally necessary and nothings wrong with the code as is; but if we already have a wildcard we can just return self at that point I think
(same logic can apply after line 167 too if needed)
|
I'll reply to the comments tomorrow thanks for the reviews! Just FYI this is a breaking change on the BDK libraries' API, and so would come in on the 3.0 release. Still I'd like to make sure it's good and everyone is happy with it sooner than later so it doesn't sit halfway done for 2 months before we all need to take another look/review because we forgot all about it. |
This was the only remaining Q I had (breaking change) and was just going to ask about it tomorrow at the call, but you addressed generally what I was wondering (plan) |
This is a small issue that's been on my mind for a very long time. Fixes #324.
Our
DescriptorSecretKeyandDescriptorPublicKeytypes, upon creation, build extended keys that have a wildcard added by default. For example, creating a DescriptorSecretKey currently gives you something like this:This is a bit weird because... well you wouldn't often add a wildcard right there as your first derivation step. None of the bips do this. Rather, you often add wildcards once you've fully derived your key at the index you want it at. Moreover, you could not use our API as is to create unwildcarded DescriptorSecretKeys or DescriptorPublicKeys. This is an odd limitation of the code as it stands, because in reality those are totally possible; we just didn't allow it with the current code. I suspect we didn't get a lot of feedback on this simply because it's not a part of the API that is used much.
This PR addresses that, and creates extended keys the way you'd expect them:
From there you can extend them however you want, including adding a wildcard to it if you'd like, using a new method on the type,
add_wildcard().Changelog notice
Checklists
All Submissions:
cargo fmtandcargo clippybefore committingNew Features: