Slip132: make x/tpub/prv classified as hash-class keys and add tests#39
Slip132: make x/tpub/prv classified as hash-class keys and add tests#39kaiwolfram wants to merge 1 commit intoBP-WG:masterfrom
Conversation
Codecov Report
@@ Coverage Diff @@
## master #39 +/- ##
========================================
+ Coverage 31.1% 40.9% +9.8%
========================================
Files 45 45
Lines 4745 5238 +493
========================================
+ Hits 1478 2144 +666
+ Misses 3267 3094 -173
Continue to review full report at Codecov.
|
dr-orlovsky
left a comment
dr-orlovsky
left a comment
There was a problem hiding this comment.
Thank you for this huge work! Tests looks good, however one breaking change from treating xpub/tpub types, which will affect all software which already uses this library - descriptors will stop working b/c of it. Pls see my comments and let me know what do you think.
Also, if it is not hard, can you please run cargo +nightly fmt --all and cargo clippy --all to get the CI passing?
| match kv.as_bytes() { | ||
| &VERSION_MAGIC_XPUB | &VERSION_MAGIC_XPRV | &VERSION_MAGIC_TPUB | ||
| | &VERSION_MAGIC_TPRV => None, | ||
| | &VERSION_MAGIC_TPRV => Some(KeyApplication::Hashed), |
There was a problem hiding this comment.
The idea with returning None here was the fact that Xpub's and related test types are allowed to be used in descriptors in any context, not just pre-segwit
There was a problem hiding this comment.
Makes sense, thanks for clarifying and sorry for the late reply. Maybe we should add a note to the docs then. Right now it says Returns None if the version is not recognized/unknown to the resolver. Using this library I'd be confused why xpub related types are unknown
| &VERSION_MAGIC_XPUB | &VERSION_MAGIC_XPRV => None, | ||
| &VERSION_MAGIC_TPUB | &VERSION_MAGIC_TPRV => None, | ||
| &VERSION_MAGIC_XPUB | &VERSION_MAGIC_XPRV => Some(vec![ | ||
| ChildNumber::Hardened { index: 44 }, | ||
| ChildNumber::Hardened { index: 0 }, | ||
| ]), | ||
| &VERSION_MAGIC_TPUB | &VERSION_MAGIC_TPRV => Some(vec![ | ||
| ChildNumber::Hardened { index: 44 }, | ||
| ChildNumber::Hardened { index: 1 }, | ||
| ]), |
|
I have to release a new 0.8 version today, so I created a PR with a non-breaking subset of your code #41. I am going to merge it - and we can continue discussion whether it is desirable to classify ordinary xpubs/tpubs as belonging to a specific Slip class here and than, if we decide to do so, merge that from this PR into v0.9 |
dr-orlovsky
added
question
dr-orlovsky
changed the title
dr-orlovsky
force-pushed
the
master
branch
3 times, most recently
from
b5a5f91 to
e989537
Compare
|
v0.10 is a new breaking version I am working on these days, so the parts of this PR which haven't got into #41 can be re-based onto the current master. @kaiwolfram are you still interested in working on this PR or should I do that by myself? |
Hi, it would be better if you do that. I'm currently busy with other stuff. Thank you. |
3 participants
Some tests for the slip132 lib to increase code coverage #11.
I changed
from_derivation_pathandto_derivation_pathbecause they didn't work with testnet paths.