[bugfix] Derive correct "signmessage" address #448
Conversation
tests/test_embit_utils.py
Outdated
| # CRAZY custom derivation path | ||
| (None, SC.CUSTOM_DERIVATION, False): "m/879345978543'/908327034508534983495'/9085098430894380959043'/0/5", | ||
| # CRAZY custom derivation paths | ||
| (None, SC.CUSTOM_DERIVATION, False, 5): "m/879345978543'/908327034508534983495'/9085098430894380959043'/0/5", |
There was a problem hiding this comment.
I believe the first 3 nodes of this derivation path are invalid because max hardened representation is 231-1, which is really representing the max child node 232-1. If we were to pass these thru to embit, it would raise an exception eventually because each child needs to fit into a 4byte serialization.
There was a problem hiding this comment.
maybe all tests above can also check for the 'index' element on return, if I'm understanding correctly that it's always there.
There was a problem hiding this comment.
Seemed unnecessarily fussy / repetitive to verify index in the base cases, but was a late addition here to verify the new index parsing logic in this PR.
Even with just three components in the tuple it's hard to keep straight when reading the code. The few 4-tuples added at the end make it even harder. Not super-concerned since it's not in the main app code, but an easier to follow refactor would eventually be nice.
There was a problem hiding this comment.
Slightly more realistic intentionally unrealistic test derivation path updated.
|
ACK tested tested before and indeed was not displaying the correct address but was signing for the correct one. [updated]: my ACK remains unchanged for commit 928c4ab; as a rule of thumb, a tests/ change would never alter that imo. |
|
ACK and Tested |
Prior implementation used the full address derivation path instead of the wallet address when deriving the xpub.
embit_utilsparsing