feat(common): secp256r1 support #152
Conversation
WalkthroughThe pull request introduces several modifications to the Changes
Possibly related PRs
Suggested reviewers
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (5)
crates/common/Cargo.toml (1)
Line range hint
27-29: Consider adding a secp256r1 feature flagSimilar to how secp256k1 has its own feature flag, consider adding a feature flag for secp256r1 to maintain consistency and allow optional compilation.
[features] default = [] test_utils = [] secp256k1 = ["secp256k1/global-context", "secp256k1/rand-std"] +secp256r1 = ["p256/ecdsa"]crates/node_types/prover/src/prover/tests.rs (1)
87-88: LGTM: Clear separation of signing and verifying keys.The explicit type annotation and separate variables improve code clarity. Consider adding a comment explaining why we need both keys in this test scenario.
let new_signing_key = create_mock_signing_key(); +// Create verifying key for validation, while keeping signing key for later operations let new_verifying_key: VerifyingKey = new_signing_key.clone().into();crates/common/src/test_ops.rs (1)
Line range hint
1-300: Consider adding test coverage for secp256r1.Since this PR adds secp256r1 support, consider extending the test utilities to explicitly verify operations with both secp256k1 and secp256r1 keys. This would help ensure the new key type works correctly throughout the operation lifecycle.
Would you like me to help create additional test cases that exercise secp256r1 keys?
crates/common/src/operation.rs (1)
140-140: LGTM! Consider documenting the key conversion behavior.The change to use
into()is correct and consistent with the new conversion pattern. Theclone()is necessary as the signing key is borrowed.Consider adding a doc comment explaining the key conversion behavior:
/// Creates a new CreateAccount operation. /// The signing key is converted to a verifying key using the From<SigningKey> implementation. pub fn new_create_account(...)crates/common/src/keys.rs (1)
33-33: Add documentation for theSecp256r1variant.Consider adding a doc comment to the
Secp256r1variant for consistency and clarity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
Cargo.toml(1 hunks)crates/common/Cargo.toml(1 hunks)crates/common/src/keys.rs(7 hunks)crates/common/src/operation.rs(2 hunks)crates/common/src/test_ops.rs(1 hunks)crates/common/src/test_utils.rs(4 hunks)crates/node_types/prover/src/prover/tests.rs(3 hunks)crates/tests/src/lib.rs(2 hunks)
🔇 Additional comments (16)
crates/common/Cargo.toml (1)
23-23: LGTM: p256 dependency addition
The addition of the p256 crate with workspace inheritance aligns with the PR objective of adding secp256r1 support.
Cargo.toml (1)
102-102: LGTM! The p256 dependency is correctly configured.
The p256 crate version 0.13.2 with "serde" and "ecdsa" features is appropriate for implementing secp256r1 support.
crates/tests/src/lib.rs (2)
12-12: LGTM: Import addition aligns with new key handling approach
The explicit import of VerifyingKey supports the PR's transition to using type conversions instead of method calls.
48-48: LGTM: Key conversion change follows new pattern
The change from verifying_key() to explicit into() conversion aligns with the PR's objective of streamlining key type conversions.
Let's verify the conversion implementation exists:
✅ Verification successful
Conversion implementation verified and correctly handles all key types
The From<SigningKey> implementation for VerifyingKey exists in crates/common/src/keys.rs and properly handles conversions for all supported key types (Ed25519, Secp256k1, Secp256r1) using their respective verification/public key derivation methods.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify From<SigningKey> for VerifyingKey implementation exists
# Expected: Find the implementation in the keys module
ast-grep --pattern 'impl From<SigningKey> for VerifyingKey'
Length of output: 703
crates/node_types/prover/src/prover/tests.rs (4)
2-2: LGTM: Import aligns with the new key handling approach.
The explicit import of VerifyingKey supports the transition from using verifying_key() method to using Into trait implementations.
62-62: LGTM: Correctly updated to use the new key conversion approach.
The change from verifying_key() to into() aligns with the PR's objective of streamlining key type conversions.
94-99: LGTM: Operations correctly use appropriate key types.
The operations are properly updated to use the new key handling approach while maintaining the test's original purpose of validating the handling of conflicting operations in the same block.
2-2: Verify consistent key handling across test files.
Let's ensure all test files have been updated to use the new key conversion approach.
✅ Verification successful
Based on the results, I can now generate the final response:
Key handling is consistent across test files
The codebase shows a consistent pattern:
- All test files correctly use the
Intotrait for key conversions - The only remaining
verifying_key()calls are inkeys.rswhere they are part of the implementation, not test code
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining uses of verifying_key() method in test files
# and verify consistent usage of Into trait for key conversions
echo "Checking for any remaining verifying_key() calls:"
rg "verifying_key\(\)" --type rust
echo -e "\nVerifying consistent key conversion patterns in tests:"
rg "into\(\)" --type rust -g "*test*.rs"
Length of output: 1332
crates/common/src/test_utils.rs (4)
111-111: LGTM! Clear and explicit type conversion.
The explicit type annotation with into() improves code clarity while maintaining the intended functionality.
216-216: LGTM! Clear type conversion in test utility.
The explicit type annotation with into() maintains consistency with the new conversion pattern while improving code clarity.
148-148: LGTM! Verify signature validation.
The key conversion change is correct. Since this affects signature creation, ensure the signature validation still works correctly with the converted keys.
Let's verify the signature validation tests:
#!/bin/bash
# Description: Verify signature validation tests
# Test: Search for signature validation test cases
rg -A 5 'test.*signature.*valid'64-64: LGTM! Verify the key conversion behavior.
The change to use into() aligns with the new From<SigningKey> implementation for VerifyingKey.
Let's verify the key conversion implementation:
crates/common/src/keys.rs (4)
7-11: LGTM!
The imports for p256::ecdsa are appropriate for adding Secp256r1 support.
17-17: LGTM!
Importing sha2::Sha256 is necessary for computing SHA-256 digests.
36-52: LGTM!
The Hash implementation correctly handles the new Secp256r1 variant.
61-61: LGTM!
The as_bytes method correctly serializes the Secp256r1 verifying key using to_sec1_bytes().
distractedm1nd
force-pushed
the
secp256r1
branch
from
4a62ab7 to
96b7343
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
crates/common/src/keys.rs (2)
Line range hint
233-254: Add test coverage for secp256r1 operationsWhile the existing tests were properly updated, we should add specific test cases for secp256r1:
- Signing and verification
- Key conversion
- String serialization/deserialization
Would you like me to provide the implementation for these test cases?
Line range hint
154-190: Add secp256r1 support to TryFrom implementationThe current implementation only handles Ed25519 (32 bytes) and Secp256k1 (33/65 bytes) keys. It should be extended to support secp256r1 keys as well.
Here's a suggested implementation:
match bytes.len() { 32 => { let vk = Ed25519VerifyingKey::try_from(bytes.as_slice()) .map_err(|e| anyhow!("Invalid Ed25519 key: {}", e))?; Ok(VerifyingKey::Ed25519(vk)) } 33 | 65 => { - let vk = Secp256k1VerifyingKey::from_slice(bytes.as_slice()) - .map_err(|e| anyhow!("Invalid Secp256k1 key: {}", e))?; - Ok(VerifyingKey::Secp256k1(vk)) + // Try parsing as Secp256k1 first + if let Ok(vk) = Secp256k1VerifyingKey::from_slice(bytes.as_slice()) { + return Ok(VerifyingKey::Secp256k1(vk)); + } + // Try parsing as Secp256r1 + let vk = Secp256r1VerifyingKey::from_sec1_bytes(bytes.as_slice()) + .map_err(|e| anyhow!("Invalid key: neither Secp256k1 nor Secp256r1: {}", e))?; + Ok(VerifyingKey::Secp256r1(vk)) } _ => Err(anyhow!("Invalid public key length")), }crates/common/src/transaction_builder.rs (1)
134-143: Consider removing unnecessary clone.The conversion to VerifyingKey and its usage in hash computation is correct. However,
vk.clone()on line 143 appears unnecessary sincevkis already owned and not used after this point.Consider this optimization:
- vk.clone(), + vk,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
Cargo.toml(1 hunks)crates/common/Cargo.toml(1 hunks)crates/common/src/keys.rs(8 hunks)crates/common/src/operation.rs(1 hunks)crates/common/src/test_utils.rs(9 hunks)crates/common/src/transaction_builder.rs(3 hunks)crates/node_types/prover/src/prover/tests.rs(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- Cargo.toml
- crates/common/Cargo.toml
- crates/common/src/operation.rs
- crates/node_types/prover/src/prover/tests.rs
🔇 Additional comments (11)
crates/common/src/test_utils.rs (6)
57-57: LGTM: Idiomatic type conversion using into()
The changes properly implement the transition from verifying_key() to into() for SigningKey to VerifyingKey conversion, making the code more idiomatic.
Also applies to: 65-65, 76-76
87-87: LGTM: Consistent type conversion in account creation
The changes maintain consistency in using into() for type conversion while preserving the original functionality.
Also applies to: 91-91, 100-100
152-152: LGTM: Clear and consistent type conversion
The change maintains consistency with the new pattern while using clear variable naming.
185-185: LGTM: Proper type conversion in SignatureBundle
The change correctly implements the type conversion while maintaining necessary ownership semantics.
218-219: LGTM: Consistent updates in random insert logic
The changes maintain consistency in type conversion while preserving good code formatting and readability.
Also applies to: 224-224, 236-236
265-265: LGTM: Clean type conversion in update logic
The change maintains consistency with the new pattern while using clear variable naming.
crates/common/src/keys.rs (4)
7-11: LGTM: Clean integration of secp256r1 dependencies
The p256 ECDSA imports and Signature enum extension are well-structured and maintain consistency with the existing implementation.
Also applies to: 29-29
41-42: LGTM: Well-documented secp256r1 support with proper Hash implementation
The documentation clearly indicates the use cases for secp256r1 (TLS, X.509 PKI, Passkeys), and the Hash implementation correctly maintains uniqueness with a distinct discriminator.
Also applies to: 45-61
108-118: LGTM: Consistent implementation of secp256r1 signing operations
The SigningKey implementation maintains consistency with the verification side:
- Proper SHA-256 digest usage
- Clean conversion implementations
- Consistent error handling
Also applies to: 203-203, 216-221
93-103: Verify DER signature format handling across the codebase
The implementation correctly uses DER format for secp256r1 signatures. Let's verify this is handled consistently across the codebase.
✅ Verification successful
Let me gather more context about signature handling in the codebase.
Based on the comprehensive search results, I can now provide the final response:
DER signature format is consistently handled for secp256r1 signatures
The codebase shows consistent and correct handling of signature formats:
- In
keys.rs, secp256r1 signatures are properly converted to DER format during verification - Other signature types (Ed25519, Secp256k1) use their appropriate formats
- No raw byte manipulation of secp256r1 signatures was found that could bypass DER encoding
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other signature format conversions to ensure consistency
rg -l "to_der|from_der" --type rust
# Search for any direct byte manipulation of signatures that might need DER handling
ast-grep --pattern 'signature.$$$bytes$$$'
Length of output: 108
Script:
#!/bin/bash
# Look for all signature-related code with context
rg "Signature" -A 5 -B 5 --type rust
# Look specifically for signature verification implementations
ast-grep --pattern 'verify$$$signature$$$'
# Search for any signature encoding/decoding
rg "encode|decode|serialize|deserialize" --type rust -A 3 -B 3 | grep -i "signature"
Length of output: 22682
crates/common/src/transaction_builder.rs (1)
171-171: LGTM! Efficient chaining of operations.
The direct conversion of the mock signing key to VerifyingKey using method chaining is clean and efficient.
This PR adds secp256r1 support.
It also removes
verifying_keymethod fromSigningKey, instead opting for aFrom<SigningKey>impl forVerifyingKey.Secp256k1Signaturealso now uses der format, likeSecp256r1SignatureSummary by CodeRabbit
New Features
Secp256r1signature scheme, enhancing cryptographic capabilities.p256, to improve functionality.Bug Fixes
Refactor
into()method for key management.TransactionBuildermethods to utilizeVerifyingKeydirectly for better maintainability.