DescriptorPublicKey, second pass#131
Conversation
0160650 to
1b4feae
Compare
|
Added a commit to fix the 1.22.0 build (and fixed the actual build under 1.22) |
I took #116 into consideration as well, i actually implemented the second item in the list you all seem to have agreed upon. A rebased and cleaned up version of generalistic descriptor keys seems to also be a requirement for it ? |
176b7b2 to
de8345c
Compare
|
Makes sense, considering that I would have to rebase on master anyways, I might as well rebase on top of this PR. Working on it now.. |
|
@darosior do you consider the PR ready for another round of review? |
|
Yes
-------- Original Message --------
…On Sep 13, 2020, 23:36, Sebastian wrote:
***@***.***(https://github.com/darosior) do you consider the PR ready for another round of review?
—
You are receiving this because you were mentioned.
Reply to this email directly, [view it on GitHub](#131 (comment)), or [unsubscribe](https://github.com/notifications/unsubscribe-auth/AFLK3FZBR2RFHOVLBSKHFXDSFVCQVANCNFSM4RH3ERHQ).
|
sanket1729
left a comment
There was a problem hiding this comment.
Hi @darosior, I agree with the general idea of keeping things minimal in this PR so that easier for review.
I reviewed commit by commit, so of the issues had been addressed in later commits and you can safely ignore those.
de8345c to
51a2136
Compare
|
Thanks for the review, pushed the fixes. |
6b44af1 to
b49f735
Compare
98ab79d to
0c3850d
Compare
|
Now we only derive descriptors from an actual child number and not a path anymore. |
sgeisler
left a comment
There was a problem hiding this comment.
I think we reaches a pretty good state now (even though it doesn't do all I wanted anymore).
|
With this fuzztesting code fn do_test(data: &[u8]) {
let data_str = String::from_utf8_lossy(data);
if let Ok(dpk) = DescriptorPublicKey::from_str(&data_str) {
let output = dpk.to_string();
assert_eq!(data_str.to_lowercase(), output.to_lowercase());
}
}I can quickly get a crash by indexing into UTF-8 data which isn't ASCII. Also, it would be good to re-export the |
|
re adding a public key to Why not just make the |
|
That sounds like an awesome idea. The current version is a bit convoluted indeed, but I didn't see another option. That trick might work though. |
9fda12b to
881c574
Compare
It does work Added the fuzz target, fixed the parser management of unprintable characters and made |
881c574 to
9adac08
Compare
|
For what it worth, still running on this branch downstream without issue :) |
9adac08 to
b7c509e
Compare
A DescriptorPublicKey abstracts out the different public keys that can possibly be used in a descriptor, and their different forms (with / without source, wildcard in deriv. path, etc..). Co-Authored-By: Antoine Poinsot <darosior@protonmail.com> Signed-off-by: Antoine Poinsot <darosior@protonmail.com>
Signed-off-by: Antoine Poinsot <darosior@protonmail.com>
Co-Authored-by: Antoine Poinsot <darosior@protonmail.com>
Signed-off-by: Antoine Poinsot <darosior@protonmail.com>
Co-Authored-By: Andrew Poelstra <apoelstra@wpsoftware.net> Signed-off-by: Antoine Poinsot <darosior@protonmail.com>
b7c509e to
28158a3
Compare
dcb88a0 transactions: move tests to use DescriptorPublicKey::XPub (Antoine Poinsot) 007a8a4 Cargo.toml: just use bitcoin's secp256k1 (Antoine Poinsot) 426c3c4 scripts: take generalistic MiniscriptKey instead of PublicKey (Antoine Poinsot) d51654e Update to the latest rust-bitcoin (Antoine Poinsot) 1e24858 Cargo: rename the crate to revault_tx (Antoine Poinsot) Pull request description: This: - Renames the crate - Updates to rust-bitcoin 0.24 🎉 - Updates rust-miniscript to rust-bitcoin/rust-miniscript#131 and move our Miniscripts to use `DescriptorPublicKey`s ACKs for top commit: JSwambo: Ack dcb88a0 Tree-SHA512: ca0908a459b6129cb2292ad70ff7efd40f0d778302d9c8fcfffa2f89f2f069121853b91927e7b16cc0dc0c530a7da2373ab57b3bcad96168cdf6ec3510e4e6ad
This is a rebased, cleaned up and amended version of #93.
It's based on #130.
Here is the diff with a [rebased-on-master #93 ](https://github.com/darosior/rust-miniscript/commits/sgeisler_2020-06-descriptor-key_rebased) at commit cdfab02
@sgeisler i took the liberty to put myself as co-author of d84350d, which is the commit i mainly reworked - the rest were mostly rebase conflicts and fixes for 1.22.
Closes #93.