wallet-core: Add EIP2333 derivation #3529
Conversation
fed-franz
force-pushed
the
eip2333
branch
3 times, most recently
from
4d92d1b to
d729030
Compare
There was a problem hiding this comment.
Very good work!
I only have minor and mostly structural comments:
Please move the module under the keys module.
And maybe consider having all functions private except derive_bls_sk. The reasoning here is that in case we need them somewhen in the future, making them public will only cause a minor version update. But if we need to change them for some reason and they are public, the change will trigger a major version bump.
wallet-core/src/eip2333.rs
Outdated
| /// * `lamport_sk` - A container for the resulting lamport SK | ||
| /// | ||
| /// # Panics | ||
| /// Panics if the HKDF expansion fails due to an invalid output length. |
There was a problem hiding this comment.
Same here, I wonder what is an invalid output length.
wallet-core/src/eip2333.rs
Outdated
| // the master key to a child key with a given index. | ||
| #[test] | ||
| fn test_child_derivation() { | ||
| // All test cases are taken from the EIP2333 specification |
There was a problem hiding this comment.
Can you link the website of the test-data?
There was a problem hiding this comment.
The link to EIP2333 specification is provided at the beginning of the document. Should I duplicate it?
wallet-core/src/eip2333.rs
Outdated
| // salt = H(salt) | ||
| if sk.is_zero().into() { | ||
| salt = Sha256::digest(salt); | ||
| } |
There was a problem hiding this comment.
I don't understand why this if-statement is here
There was a problem hiding this comment.
To avoid computing the digest if the next iteration is not going to be executed.
Logically this digest should be computed at the beginning of each iteration, but it was a bit complicated to do it because the initialization value of salt has a different length.
Implements #3476