-
Notifications
You must be signed in to change notification settings - Fork 154
Implement GetKey
for KeyMap
#765
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
ba5b2bf
to
4a3bd7e
Compare
cc @LLFourn |
81b4923
to
00342a1
Compare
Nice! Yeah, this is probably the right approach. In 00342a1 you have a bunch of Alternately, you could make |
Thanks @tcharding. It might be nice to be able to |
Yes I threw the |
Concept ACK |
Maybe in the longer term, a cleaner solution is to implement rust-bitcoin to implement impl_get_key_tuple!((T1, T2))
impl GetKey for Vec<T> where T: GetKey {
}
impl GetKey for BtreeSet<T>... |
Concept ACK. |
00342a1
to
4920626
Compare
Implemented Also made new take only the secret key. I did not remove any |
a97c37a
to
d2c954a
Compare
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.
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (2)
src/descriptor/key_map.rs:143
- The use of the
unreachable!
macro can be problematic if assumptions change. Consider handling this case more gracefully.
unreachable!("XPrv maps to XPrv")
src/descriptor/key_map.rs:22
- [nitpick] The name
KeyMap
is quite generic. Consider renaming it to something more descriptive likeDescriptorKeyMap
.
pub struct KeyMap {
if pk == request { | ||
match v { | ||
DescriptorSecretKey::Single(ref sk) => Some(sk.key), | ||
_ => unreachable!("Single maps to Single"), |
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 use of the unreachable!
macro can be problematic if assumptions change. Consider handling this case more gracefully.
_ => unreachable!("Single maps to Single"), | |
_ => return Err(GetKeyError::InvalidKey), |
Copilot uses AI. Check for mistakes.
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.
Lol, no
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.
AI is so awesome I think we are all going to be out of a job soon.
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.
Not if your job is to have fun :)
None | ||
} | ||
} | ||
_ => unreachable!("XPrv maps to XPrv"), |
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 use of the unreachable!
macro can be problematic if assumptions change. Consider handling this case more gracefully.
Copilot uses AI. Check for mistakes.
DescriptorSecretKey::MultiXPrv(xpriv) => { | ||
Some(xpriv.xkey.to_priv()) | ||
} | ||
_ => unreachable!("MultiXPrv maps to MultiXPrv"), |
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 use of the unreachable!
macro can be problematic if assumptions change. Consider handling this case more gracefully.
Copilot uses AI. Check for mistakes.
Too bad. copilot has been around long enough that you'd think it would work, but 100% of its suggestions are wrong and it's full of irritating woke language like "harmful" and "problematic". |
Ok(self.map.iter().find_map(|(k, v)| { | ||
match k { | ||
DescriptorPublicKey::Single(ref pk) => match key_request { | ||
KeyRequest::Pubkey(ref request) => match pk.key { |
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.
In d2c954a:
I think if you replace
match k {
Thing => match key_request {
Thing2 => ...
with
match (k, key_request) {
(Thing, Thing) => {
you can reduce the amount of indentation and should be able to reduce the number of branches.
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.
Yes, at least it looks better. I tried this, see: oleonardolima@4f304d1
:) . Still too far. Let's try it in another 6 months. |
@apoelstra @tcharding wdyt about #765 (comment)? I think that would solve future API requests like extend, merge etc |
concept ACK, though we haven't really looked at |
FTR I had totally forgotten about this PR until @ValuedMammal tagged me. I tried but am unable to engage my brain right now to implement the review suggestion from Andrew (#765 (comment)). |
I have no opinion ATM sorry mate simply because I haven't managed to context switch into miniscript properly right now. |
@tcharding I've been working on similar changes for bitcoindevkit/bdk_wallet#70 in bitcoindevkit/bdk_wallet#235, and I can continue the initial work from here and open another candidate PR. Is that fine? I discussed with @ValuedMammal, and it seems that we could implement the |
I've no real opinion mate, I don't work much on miniscript. I actually don't even know if this PR can go in or requires changes. |
) -> Result<DescriptorPublicKey, DescriptorKeyParseError> { | ||
let pk = sk.to_public(secp)?; | ||
if !self.map.contains_key(&pk) { | ||
self.map.insert(pk.clone(), sk); |
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.
In d2c954a:
Both the contains_key
check and the clone are unnecessary here.
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 had a dig and the BTreeMap::insert
is a little different to this one because the value in the map can differ but because our map is mapping pk to sk it cannot, right? If that assumption is correct then I believe the code is correct if we want to unconditionally return the public key.
src/descriptor/key_map.rs
Outdated
} | ||
_ => unreachable!("XPrv maps to XPrv"), | ||
}, | ||
_ => unreachable!("rust-bitcoin v0.32"), |
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.
In d2c954a:
This might not be unreachable
for every version of 0.32.x. You should just return None
here.
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 was very confused by what was in this PR until I found rust-bitcoin/rust-bitcoin#4238
I've returned None
as suggested and added a TODO because I think we can support XOnlyPublicKey
look up as well, right? I'm not familiar enough with the codebase and not sharp enough right now to work it out. If this requires any thought on your part just say and I'll come back and think harder.
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.
Also are you ok with TODO in the codebase here in miniscript or would you prefer an issue?
Create a `KeyMay` type to replace the current `BTreeMap` alias and implement `bitcoin::psbt::GetKey` for it. Close: rust-bitcoin#709
DescriptorSecretKey::XPrv(xpriv) => { | ||
// This clone goes away in next release of rust-bitcoin. | ||
if let Ok(Some(sk)) = xpriv.xkey.get_key(key_request.clone(), secp) | ||
{ | ||
Some(sk) | ||
} else { | ||
None | ||
} |
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 existing get_key
implementation for Xpriv
does not cover the scenario when the KeyRequest::Bip32(..)
uses the key origin information, and it'd need to be covered here (it's a scenario that's being tested by BDK).
I handled it this way in my implementation, see: oleonardolima@4f304d1#diff-9b4da42f4bda05e0c63a0bae838b0eae32dfc2d5511a0c8142db8acfa9337aa2R142-R160 it's tested by oleonardolima@4f304d1#diff-9b4da42f4bda05e0c63a0bae838b0eae32dfc2d5511a0c8142db8acfa9337aa2R391-R422
Ok(self.map.iter().find_map(|(k, v)| { | ||
match k { | ||
DescriptorPublicKey::Single(ref pk) => match key_request { | ||
KeyRequest::Pubkey(ref request) => match pk.key { |
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.
Yes, at least it looks better. I tried this, see: oleonardolima@4f304d1
Thanks for the review @oleonardolima, I'll look into your suggestions. |
Create a
KeyMap
type to replace the currentBTreeMap
alias and implementbitcoin::psbt::GetKey
for it.Close: #709