search

urbit / urbit-key-generation

Key derivation and HD wallet generation functions for Urbit
MIT License
15 stars 8 forks source link

Fix issue where we were hashing a seed string #12

Closed Fang- closed 5 years ago

Fang- commented 5 years ago

The first instance is in the childNodeFromSeed function. It returns the seed in a human readable hexadecimal format which we think is okay, but it also passes the childSeed to the walletFromSeed function as a hexadecimal string instead of a buffer, which is problematic because the walletFromSeed function will call hash on that string. This will work, but the result will not be the same as hashing the underlying seed directly. In other words, SHA-512(buffer) != SHA-512(buf2hex(buffer)).

This is that instance. Incorrect variable naming is no bueno! Pretty sure this fixes it.

Prior to making this change, one test failed. After making this change, two tests failed, because this changes the output to be more correct, which the tests don't yet account for.

(May or may not fix the tests myself, but putting this PR out so it can start being reviewed.

g-a-v-i-n commented 5 years ago

Nice catch, LGTM. I think we should probably have some kind of checklist for refactoring next time. @c-johnson

Fang- commented 5 years ago

I think we should probably have some kind of checklist for refactoring next time.

After this gets released, the checklist for future versions/revisions is just "does it give the same output for all kinds of inputs?"