Add "sortedmulti" descriptor support #171
Conversation
|
@sanket1729 Thanks for your review. Addressed the comments that I could and got CI passing. |
src/descriptor/mod.rs
Outdated
| let mut pks: Vec<bitcoin::PublicKey> = | ||
| self.pks.iter().map(|pk| pk.to_public_key()).collect(); | ||
| // Sort pubkeys lexigraphically according to BIP 67 | ||
| pks.sort_by(|a, b| a.to_string().partial_cmp(&b.to_string()).unwrap()); |
There was a problem hiding this comment.
Then with the above, this should just become pks.sort_by_key(Bip67Sorted).
There was a problem hiding this comment.
Did you mean pks.sort_by_key(|pk| Bip67Sorted(pk))?
According to the docs, the argument to sort_by_key needs to be FnMut, which we haven't implemented.
If I use a lambda, then it complains that return value of Bip67Sorted(pk) may outlive pk ... not sure how to fix that.
There was a problem hiding this comment.
I think it would be best to just use this for now:
pks.sort_by(|a, b| {
a.to_public_key()
.serialize()
.partial_cmp(&b.to_public_key().serialize())
.unwrap()
});
But get the upstream change merged and switch to pks.sort() as soon as upstream change is released.
There was a problem hiding this comment.
Doesn't make sense to create a new public struct to implement behavior that should be in rust-libsecp256k1
There was a problem hiding this comment.
pks.sort_by_key(|pk| Bip67Sorted(pk)) is identical to pks.sort_by_key(Bip67Sorted) except the latter uses fewer symbols. And struct constructors always implement all of the Fn traits.
Using a lambda or not should have no effect on this. What is the error you are getting?
There was a problem hiding this comment.
I can reproduce the lifetime error. The issue is that sort_by_key and sort_key_cached_key have their lifetimes set incorrectly by stdlib, so we do indeed need to use sort_by
There was a problem hiding this comment.
Ahh sorry, should have posted the full error message.
There was a problem hiding this comment.
Unresolving this for now since there is opposition from upstream to including a nonsense Core ordering in rust-bitcoin.
|
Thanks for the review @apoelstra. I added a few commits addressing most of your comments. Commit messages aren't very good because I imagine I'll squash all these commits once we're done making changes? |
|
Ok @justinmoon can you rebase on master and squash everything? You can break up all the commits however you want ... one big commit is fine ... but please make sure that every commit compiles by itself. |
|
Rebased and squashed to one big commit |
Closes #164