Conversation
| let secp = Secp256k1::new(); | ||
| let descriptor_public_key = self.0.to_public(&secp).unwrap(); | ||
| Arc::new(DescriptorPublicKey(descriptor_public_key)) | ||
| let descriptor_public_key = self.0.to_public(&secp).map_err(DescriptorKeyError::from)?; |
There was a problem hiding this comment.
Could we map the MultiXPrv case to InvalidKeyType, or preferably a dedicated MultiPath variant? The key parsed successfully; it is the conversion that is unsupported. Returning Parse makes binding callers inspect the error message to distinguish this case.
There was a problem hiding this comment.
The key parse error refers to what this function in particular is doing. So I won't say "the key parsed successfully" because it cannot parse the mutlixPriv to make it public. Hence, the parsing error. This is also consistent with rust-miniscript error as I shared in the PR description. So returning an invalidKeyType might be misleading.
I suspecting the parsing you are refering to is the one that happens before this function is used. i.e DescriptorSecretKey::from_string like in my test.
There was a problem hiding this comment.
Yeah, I see what you mean. I was referring to the earlier DescriptorSecretKey::from_string() parse, since at that point we already have a valid MultiXPrv.
But you're right that to_public() itself returns DescriptorKeyParseError upstream, so mapping that through the existing DescriptorKeyError::Parse conversion is consistent with the current FFI error mapping.
My concern was mainly about whether binding callers need a typed way to distinguish this particular conversion failure rather than inspecting the error message, not that the upstream error type itself was wrong.
I also noticed newer miniscript handles some MultiXPrv -> MultiXPub conversions directly and only errors for incompatible hardened derivation paths, so InvalidKeyType would probably be too broad here.
Given that, I'm fine with Parse for this PR if we're intentionally following the existing upstream/error mapping. Thanks for clarifying.
There was a problem hiding this comment.
Alright, thanks. Maybe if we update mini-script in the future we can update as well.
2 participants
Description
Addresses #1093
Notes to the reviewers
Documentation
bdk_walletbitcoinuniffiChangelog
Checklists
All Submissions:
cargo fmtandcargo clippybefore committingchangelog:*labelNew Features:
Bugfixes: