-
Notifications
You must be signed in to change notification settings - Fork 16
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor ECDSA implementation #71
Conversation
WalkthroughThe recent changes in the Flow SDK enhance cryptographic functionalities, improve code clarity, and strengthen testing practices. Key updates include revamped cryptographic classes, streamlined data structures, and added validation checks for key handling. The test suite has been expanded to cover new scenarios, ensuring robust verification of signature and hashing algorithms. This comprehensive set of modifications aims to bolster security, usability, and maintainability across the SDK. Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (6)
- sdk/src/main/kotlin/org/onflow/flow/sdk/crypto/Crypto.kt (3 hunks)
- sdk/src/main/kotlin/org/onflow/flow/sdk/models.kt (29 hunks)
- sdk/src/test/kotlin/org/onflow/flow/sdk/crypto/HashTest.kt (2 hunks)
- sdk/src/test/kotlin/org/onflow/flow/sdk/crypto/SignTest.kt (1 hunks)
- sdk/src/test/kotlin/org/onflow/flow/sdk/models/HashSigningAlgorithmTest.kt (2 hunks)
- sdk/src/test/kotlin/org/onflow/flow/sdk/models/SignerTest.kt (1 hunks)
Files skipped from review due to trivial changes (1)
- sdk/src/test/kotlin/org/onflow/flow/sdk/models/SignerTest.kt
Additional comments not posted (44)
sdk/src/test/kotlin/org/onflow/flow/sdk/models/HashSigningAlgorithmTest.kt (6)
3-3
: Add import statement forHashAlgorithm
.The import statement for
HashAlgorithm
has been added, indicating its usage in the tests.
8-8
: Class renamed toSignatureAndHashingModelTest
.The class name change reflects an expanded focus on both signature and hashing algorithms.
31-31
: Update test method name for clarity.The method name has been updated to
Test signature fromCode
for better clarity.
38-39
: Update test method name and comment for clarity.The method name has been updated to
Test signature fromCadenceIndex
and the comment now references the correct documentation.
46-46
: Update test method name for clarity.The method name has been updated to
Test signature fromCode with invalid code
for better clarity.
51-51
: Update test method name for clarity.The method name has been updated to
Test signature fromCadenceIndex with invalid index
for better clarity.sdk/src/test/kotlin/org/onflow/flow/sdk/crypto/HashTest.kt (1)
9-9
: Update test method name for clarity.The method name has been updated to
Get default hasher
for better clarity.sdk/src/test/kotlin/org/onflow/flow/sdk/crypto/SignTest.kt (11)
3-3
: Add import statements forSignatureAlgorithm
andbytesToHex
.The import statements for
SignatureAlgorithm
andbytesToHex
have been added, indicating their usage in the tests.
10-15
: AddSupportedCurve
data class.The
SupportedCurve
data class encapsulates details about each curve, including the signature algorithm, private key size, public key size, and curve order.
18-28
: AddsupportedAlgos
list.The
supportedAlgos
list contains all supported curves of the library.
32-48
: Update test method for key pair generation.The method
Test KeyPair generation
iterates through supported algorithms and validates key pair generation and integrity.
52-73
: Update test method for key decoding.The method
Can decode keys correctly
validates the decoding functionality for both private and public keys.
77-101
: Update test method for private key decoding exceptions.The method
Private key decoding throws exception when invalid
validates that exceptions are thrown for invalid private keys.
106-134
: Update test method for public key decoding exceptions.The method
Public key decoding throws exception when invalid
validates that exceptions are thrown for invalid public keys.
138-151
: Add test method for public key derivation.The method
Test derivePublicKey
validates the derivation of public keys from private keys.
156-181
: Add test method for signer compatibility with hash algorithms.The method
Test signer compatibility with hash algorithms
validates that the signer can handle different hash algorithms and raises exceptions for unsupported ones.
185-209
: Add test method for signer correctness.The method
Test signer correctness
validates the correctness of signatures.
213-237
: Add test method for signer with invalid algorithm keys.The method
Test signer with invalid algo keys
validates that exceptions are thrown for invalid algorithm keys.sdk/src/main/kotlin/org/onflow/flow/sdk/crypto/Crypto.kt (14)
5-7
: Imports look good.The new imports for cryptographic parameters and signers from Bouncy Castle are necessary and correctly used.
Also applies to: 14-15, 18-19
37-43
: Changes toPrivateKey
data class are approved.The addition of
algo
andpublicKey
fields enhances the class by providing more context and usability.
47-80
: Changes toPublicKey
data class are approved.The addition of the
algo
field and theverify
method enhances the class by providing more context and functionality for signature verification.
88-94
: New methodcheckSupportedSignAlgo
is approved.The method correctly enforces algorithm compatibility checks and handles unsupported algorithms appropriately.
95-123
: RefactoredgenerateKeyPair
method is approved.The method correctly generates key pairs with the new fields in
PrivateKey
andPublicKey
, enhancing the integrity of key generation.
127-163
: RefactoreddecodePrivateKey
method is approved.The method correctly decodes private keys with the new fields in
PrivateKey
, enhancing the integrity of key decoding.
167-198
: RefactoreddecodePublicKey
method is approved.The method correctly decodes public keys with the new field in
PublicKey
, enhancing the integrity of key decoding.
204-208
: RefactoredgetSigner
andgetHasher
methods are approved.The methods correctly return instances of
Signer
andHasher
with the new implementations, enhancing the functionality.
210-301
: New and refactored methods inCrypto
object are approved.The methods correctly implement the intended functionality, enhancing the overall cryptographic capabilities of the SDK.
342-355
: Updates toHasherImpl
class are approved.The class correctly implements new hashing algorithms and validation checks, enhancing the hashing capabilities.
372-397
: RefactoredSignerImpl
class is approved.The class correctly implements the new signing process using
ECDSASigner
, enhancing the signing capabilities.
292-301
: NewformatSignature
method is approved.The method correctly formats signatures, ensuring proper handling of the resulting signature.
249-265
: NewderivePublicKey
method is approved.The method correctly derives the public key from the private key, enhancing the key management capabilities.
267-273
: NewcheckHashAlgoForSigning
method is approved.The method correctly enforces hash algorithm compatibility checks and handles unsupported algorithms appropriately.
sdk/src/main/kotlin/org/onflow/flow/sdk/models.kt (12)
19-21
: ReformattedFlowTransactionStatus
enum is approved.The reformatting improves readability without altering functionality.
58-60
: ReformattedSignatureAlgorithm
enum is approved.The reformatting improves readability without altering functionality.
142-147
: RefactoredFlowAccount
data class is approved.The refactoring simplifies the
builder
method definition without altering functionality.
152-161
: RefactoredgetKeyIndex
method inFlowAccount
data class is approved.The refactoring improves readability without altering functionality.
187-194
: RefactoredFlowAccountKey
data class is approved.The refactoring simplifies the
builder
method definition without altering functionality.
223-227
: RefactoredFlowEventResult
data class is approved.The refactoring simplifies the
builder
method definition without altering functionality.
Line range hint
486-530
:
RefactoredFlowTransaction
data class is approved.The refactoring simplifies the
builder
method and other method definitions without altering functionality.
576-579
: RefactoredFlowTransactionProposalKey
data class is approved.The refactoring simplifies the
builder
method definition without altering functionality.
600-603
: RefactoredFlowTransactionSignature
data class is approved.The refactoring simplifies the
builder
method definition without altering functionality.
Line range hint
621-657
:
RefactoredFlowBlockHeader
andFlowBlock
data classes are approved.The refactoring simplifies the
builder
method definitions without altering functionality.
Line range hint
777-803
:
RefactoredFlowCollectionGuarantee
andFlowBlockSeal
data classes are approved.The refactoring simplifies the
builder
method definitions without altering functionality.
819-821
: RefactoredFlowCollection
data class is approved.The refactoring simplifies the
builder
method definition without altering functionality.
This PR refactors the ECDSA implementation, fixes potential issues, and add proper testing (almost inexistent before the PR).
Changes:
PrivateKey
andPublicKey
fields - in particular allow accessing the public key from the private key.verify
to thePublicKey
type. The method verifies a signature against a message using an input hashing algorithm.ecCoupleComponentSize
which confuses the group order size and the prime field size.PublicKey
serialization which did not pad the point coordinates to the prime field order.PrivateKey
serialization is also padded to the group order size. Size checks are enforced in deserialization functions.models.kt
has mainly formatting changes only).Signer
does not implementHasher
as it doesn't make sense to use aSigner
to hash data.PrivateKey
andPublicKey
fields are updated (alternatively, it is safer to make the internal fields private and allow the construction ofPrivateKey
andPublicKey
only through functions and constructors that check the fields are compatible, but this would be a larger breaking change)PrivateKey
serialization is now padded to the order size. Deserializing a private key string also requires the input to be padded.SignerImpl
does not accept aHasher
input. TheHashAlgorithm
input is enough to get the hasher implementation.Summary by CodeRabbit
New Features
Bug Fixes
Documentation