Introducing bip32::KeySource: wrapper for (Fingerprint, DerivationPath)#480
Introducing bip32::KeySource: wrapper for (Fingerprint, DerivationPath)#480apoelstra merged 2 commits intorust-bitcoin:masterfrom BP-WG:feat/keysource
bip32::KeySource: wrapper for (Fingerprint, DerivationPath)#480Conversation
sgeisler
left a comment
There was a problem hiding this comment.
ACK (I haven't checked if the introduction of the type is exhaustive).
|
I wouldn't say I'm against this, but this has the disadvantage that instances of let (fingerprint, derivation_path) = psbt.inputs[0].hd_keypaths.get(my_pk).unwrap();wouldn't work anymore. You could theoretically do: let KeySource(fingerprint, derivation_path) = psbt.inputs[0].hd_keypaths.get(my_pk).unwrap();but that means that you would have to import the type just to unpack it nicely. I would rather define this as a type alias, it achieves the same goal of not having to type the full tuple every time, while keeping the nice unpacking properties of tuples, and not making a breaking change in the API. |
|
I'd be fine with that too. |
|
Yep, alias will work better in this place, see new commit |
I did replacement with search over whole code for |
Add a "base64" feature that enables the `base64` dependency in `rust-bitcoin`. This enables `Psbt::from_str()`. Fix: rust-bitcoin#480
2c67681 Add base64 feature (Tobin C. Harding) Pull request description: Add a "base64" feature that enables the `base64` dependency in `rust-bitcoin`. This enables `Psbt::from_str()`. Fix: rust-bitcoin#480 ACKs for top commit: sanket1729: ACK 2c67681 Tree-SHA512: 24af82329cbbba0fff4bbb686f6108a879163dd44d7fa738677d94daf96a0a892f20f3780d4c2d6a17b183f0aa60423192340420f039d0b39bfde2422262151e
Combinations of
(Fingerprint, DerivationPath)are frequently used with PSBTs throughout this library and in many dependencies (like Lightning, Miniscript). It even had it's own serializer! So I think it is reasonable to introduce a type for representing this pair. After a brief discussion in rust-bitcoin/rust-miniscript#131 (comment) it was decided to name itKeySource, but we can change the name if needed.