Fixes #486 #487
Merged
Fixes #486 #487
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
1. Add a precondition to PrivateKey.of that enforces the length to 33 bytes (starting with either 0x00 or 0xED). 2. Update BCKeyUtils to add/remove the padding for secp keys (the way we do for Ed keys). 3. Add a new function to PrivateKey called `valueWithoutPrefix` which returns the natural 32 private key bytes. 4. Update Secp256k1KeyPairService in Seed.java to prefix with private keys with 0x00 if only 32 bytes. 5. Validate that `PrivateKey.keyType()` works properly via unit tests.
sappenin
commented
Sep 28, 2023
xrpl4j-core/src/main/java/org/xrpl/xrpl4j/crypto/keys/Seed.java
Outdated
Show resolved
Hide resolved
nkramer44
reviewed
Sep 28, 2023
xrpl4j-core/src/main/java/org/xrpl/xrpl4j/crypto/keys/Seed.java
Outdated
Show resolved
Hide resolved
xrpl4j-core/src/main/java/org/xrpl/xrpl4j/crypto/keys/bc/BcKeyUtils.java
Outdated
Show resolved
Hide resolved
xrpl4j-core/src/main/java/org/xrpl/xrpl4j/crypto/keys/bc/BcKeyUtils.java
Outdated
Show resolved
Hide resolved
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #487 +/- ##
============================================
+ Coverage 90.88% 91.10% +0.21%
- Complexity 1596 1626 +30
============================================
Files 318 319 +1
Lines 4489 4553 +64
Branches 369 377 +8
============================================
+ Hits 4080 4148 +68
+ Misses 282 276 -6
- Partials 127 129 +2
☔ View full report in Codecov by Sentry. |
sappenin
commented
Oct 3, 2023
xrpl4j-core/src/main/java/org/xrpl/xrpl4j/codec/addresses/UnsignedByteArray.java
Outdated
Show resolved
Hide resolved
sappenin
commented
Oct 4, 2023
xrpl4j-core/src/main/java/org/xrpl/xrpl4j/crypto/keys/PrivateKey.java
Outdated
Show resolved
Hide resolved
sappenin
commented
Oct 4, 2023
xrpl4j-core/src/main/java/org/xrpl/xrpl4j/crypto/keys/Seed.java
Outdated
Show resolved
Hide resolved
sappenin
commented
Oct 4, 2023
xrpl4j-core/src/main/java/org/xrpl/xrpl4j/crypto/keys/bc/BcKeyUtils.java
Outdated
Show resolved
Hide resolved
nkramer44
requested changes
Oct 4, 2023
xrpl4j-core/src/main/java/org/xrpl/xrpl4j/codec/addresses/PrivateKeyCodec.java
Outdated
Show resolved
Hide resolved
xrpl4j-core/src/main/java/org/xrpl/xrpl4j/codec/addresses/PrivateKeyCodec.java
Outdated
Show resolved
Hide resolved
xrpl4j-core/src/main/java/org/xrpl/xrpl4j/codec/addresses/PrivateKeyCodec.java
Outdated
Show resolved
Hide resolved
xrpl4j-core/src/main/java/org/xrpl/xrpl4j/codec/addresses/PrivateKeyCodec.java
Outdated
Show resolved
Hide resolved
xrpl4j-core/src/main/java/org/xrpl/xrpl4j/codec/addresses/UnsignedByteArray.java
Outdated
Show resolved
Hide resolved
xrpl4j-core/src/main/java/org/xrpl/xrpl4j/crypto/keys/PrivateKey.java
Outdated
Show resolved
Hide resolved
xrpl4j-core/src/main/java/org/xrpl/xrpl4j/crypto/keys/PrivateKey.java
Outdated
Show resolved
Hide resolved
xrpl4j-core/src/main/java/org/xrpl/xrpl4j/crypto/keys/PrivateKey.java
Outdated
Show resolved
Hide resolved
| @@ -127,8 +127,7 @@ protected synchronized Signature ecDsaSign(final PrivateKey privateKey, final Un | |||
| Objects.requireNonNull(transactionBytes); | |||
|
|
|||
| final UnsignedByteArray messageHash = HashingUtils.sha512Half(transactionBytes); | |||
|
|
|||
| final BigInteger privateKeyInt = new BigInteger(privateKey.value().toByteArray()); | |||
| final BigInteger privateKeyInt = new BigInteger(privateKey.valueWithPrefixedBytes().toByteArray()); | |||
There was a problem hiding this comment.
I think this should probably be privateKey.valueWithNaturalBytes? Probably doesn't matter because the prefix will be 0x00 which won't affect the value of the BigInteger, but un-prefixed seems more correct
…gnedByteArray.java Co-authored-by: nkramer44 <[email protected]>
Calling UBA.toByteArray() will copy the bytes from the UBA inot a newly initialized byte array. This code was doing this twice, just to check the legnth. In addition, the length of the UBA was being checked, whereas the length of the byte[] should have been checked instead. Generally, these two lengths should be the same, but for correctness the byte[] length is preferred.
- valueWithNaturalBytes becomes naturalBytes - valueWithPrefixedBytes becomes prefixedBytes
sappenin
commented
Oct 5, 2023
xrpl4j-core/src/main/java/org/xrpl/xrpl4j/crypto/keys/bc/BcKeyUtils.java
Outdated
Show resolved
Hide resolved
sappenin
commented
Oct 5, 2023
xrpl4j-core/src/main/java/org/xrpl/xrpl4j/crypto/signing/bc/BcSignatureService.java
Outdated
Show resolved
Hide resolved
sappenin
commented
Oct 5, 2023
xrpl4j-core/src/test/java/org/xrpl/xrpl4j/crypto/keys/bc/BcKeyUtilsTest.java
Show resolved
Hide resolved
nkramer44
approved these changes
Oct 5, 2023
|
Merging this now that all tests passing except for two subsets of ITs.
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
valueWithoutPrefixwhich returns the natural 32 private key bytes.PrivateKey.keyType()works properly via unit tests.