Refactor key derivation #1309
Refactor key derivation #1309
Conversation
Signed-off-by: dikel <dikelito@tutamail.com>
Signed-off-by: dikel <dikelito@tutamail.com>
Signed-off-by: dikel <dikelito@tutamail.com>
Signed-off-by: dikel <dikelito@tutamail.com>
Signed-off-by: dikel <dikelito@tutamail.com>
Signed-off-by: dikel <dikelito@tutamail.com>
dikel
requested review from
Sean-Tedrow-LB,
mehcode and
SimiHunjan
as code owners
Signed-off-by: dikel <dikelito@tutamail.com>
Codecov ReportBase: 74.40% // Head: 74.52% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## develop #1309 +/- ##
=============================================
+ Coverage 74.40% 74.52% +0.11%
- Complexity 2875 2892 +17
=============================================
Files 184 185 +1
Lines 11391 11435 +44
Branches 1121 1123 +2
=============================================
+ Hits 8476 8522 +46
+ Misses 2255 2253 -2
Partials 660 660
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
sdk/src/main/java/com/hedera/hashgraph/sdk/PrivateKeyECDSA.java
Outdated
Show resolved
Hide resolved
Signed-off-by: dikel <dikelito@tutamail.com>
Signed-off-by: dikel <dikelito@tutamail.com>
sdk/src/main/java/com/hedera/hashgraph/sdk/Utils/Bip32Utils.java
Outdated
Show resolved
Hide resolved
Signed-off-by: dikel <dikelito@tutamail.com>
Signed-off-by: dikel <dikelito@tutamail.com>
Signed-off-by: dikel <dikelito@tutamail.com>
| @@ -554,4 +542,48 @@ private byte[] wordsToLegacyEntropy2() throws BadMnemonicException { | |||
|
|
|||
| return entropy; | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
IMO it would make sense to have versions of these toStandard functions with default passphrase and index
toStandard(passphrase)(using index 0)toStandard()(using no passphrase and index 0)toStandard(index)(using no passphrase) << this one might be overkill, though
There was a problem hiding this comment.
We thought about having multiple versions of the toStandard methods but we decided it's better to have only one, because it cannot be done in the other SDKs. JS and Go don't support overloading of methods.
There was a problem hiding this comment.
I've fine with that logic, but this really needs to be folded into the standard proposal.
For example, I know the C++ SDK effort has already implemented the proposal as it currently exists, with default parameters.
| @@ -19,6 +19,8 @@ | |||
| */ | |||
| package com.hedera.hashgraph.sdk; | |||
|
|
|||
| import com.hedera.hashgraph.sdk.utils.Bip32Utils; | |||
| import org.bouncycastle.util.encoders.Hex; | |||
There was a problem hiding this comment.
Since this is the first implementation of the new standard, I think it would make sense to include a new set of standard "hedera" test vectors. The list I had come up with is detailed in the Tests section of the standards issue, bullet #2
In addition to defining new test vectors, this would also involve renaming test variables MNEMONIC3_STRING, MNEMONIC_LEGACY_STRING, MNEMONIC_STRING, and MNEMONIC_PRIVATE_KEY, so that it is clear what these actually are.
There was a problem hiding this comment.
We've already added the test vectors (toStandardED25519PrivateKey and toStandardECDSAsecp256k1PrivateKey methods in MnemonicTest. I've changed the variable names to be clearer.
There was a problem hiding this comment.
It's important that these particular test vectors are tracked somewhere so that all SDKs use the same ones. That will ensure that the Standard mnemonic->key functions are consistent across SDKs
Signed-off-by: dikel <dikelito@tutamail.com>
Signed-off-by: dikel <dikelito@tutamail.com>
Signed-off-by: dikel <dikelito@tutamail.com>
Signed-off-by: dikel <dikelito@tutamail.com>
Signed-off-by: dikel <dikelito@tutamail.com>
|
Kudos, SonarCloud Quality Gate passed! |
Description:
This PR refactors the derivation of private keys:
toStandard[Ed25519|ECDSAsecp256k1]PrivateKey()which use the correct derivation pathMnemonic.toPrivateKey()andPrivateKey.fromMnemonic()Related issue(s):
Fixes #1277, Fixes #1263
Checklist