feat: test all available curves #186
Conversation
Signed-off-by: Smuu <18609909+Smuu@users.noreply.github.com>
Signed-off-by: Smuu <18609909+Smuu@users.noreply.github.com>
Signed-off-by: Smuu <18609909+Smuu@users.noreply.github.com>
Signed-off-by: Smuu <18609909+Smuu@users.noreply.github.com>
Signed-off-by: Smuu <18609909+Smuu@users.noreply.github.com>
Signed-off-by: Smuu <18609909+Smuu@users.noreply.github.com>
WalkthroughThis pull request introduces a comprehensive enhancement to the cryptographic key management system across multiple crates. The primary focus is on supporting multiple signing key algorithms (Ed25519, Secp256k1, and Secp256r1) by modifying configuration handling, transaction building, and key generation processes. Changes span configuration files, main application logic, and test suites, ensuring consistent algorithm specification and handling throughout the project. Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
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 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: 6
🧹 Nitpick comments (20)
crates/da/src/lib.rs (2)
3-3: Use consistent dependency management across crates.Replacing "ed25519_consensus" with "prism_keys" appears correct. Make sure the old dependency is fully removed from Cargo.toml to avoid confusion or duplication.
56-56: Clear error messaging.Relying on a single error variant (“Invalid signature length”) might mask more descriptive errors. Consider returning the underlying error message from “from_algorithm_and_bytes” for easier troubleshooting.
crates/cli/src/main.rs (6)
40-48: DRY up repeated checks.The verifying_key_algorithm validation is repeated in multiple places. Extracting this into a helper function could improve readability and prevent duplication.
- if verifying_key_algorithm.is_empty() { - ... - } - if !["ed25519", "secp256k1", "secp256r1"].contains(&verifying_key_algorithm) { - ... - } + fn validate_algorithm(algorithm: &str) -> std::io::Result<()> { + if algorithm.is_empty() { + return Err(...); + } + if !["ed25519", "secp256k1", "secp256r1"].contains(&algorithm) { + return Err(...); + } + Ok(()) + }
52-52: LightClient instantiation.Passing Some(prover_vk) is fine, though consider clarifying whether a None case is possible or how fallback logic is handled.
92-92: Passing cloned verifying key into config.Storing both signing_key and verifying_key in the config is valid, but ensure they remain in sync, especially if any rotation logic is introduced later.
98-98: Logging clarity.“prover verifying key: {}” is helpful. Consider adding algorithm info for completeness.
127-127: Key retrieval flow repeated.Same note about repeating the KeyChain retrieval logic. Consolidate if possible.
151-157: Careful error mapping.If invalid data is encountered, returning “invalid prover verifying key” is good but consider whether more context is needed, e.g. exact reason.
crates/keys/src/verifying_keys.rs (2)
77-77: Enumerate allowed algorithms.Rather than returning “Unexpected algorithm,” consider enumerating possible algorithms in the error message or providing a typed error.
216-222: Graceful fallback for unknown curves.Returning “Invalid curve type” is good. If you foresee additional curves in the future, plan for extension or clearer messages.
crates/node_types/prover/src/prover/tests.rs (3)
23-30: TransactionBuilder calls.register_service_with_random_keys, create_account_with_random_key_signed, etc. confirm that these random keys are properly seeded for deterministic test results if needed.
Line range hint
64-84: Process transaction logic.This block thoroughly tests insert and update flows. The random key approach is beneficial, though ensure any negative test cases are covered (e.g., invalid signatures).
112-116: Redundant block?Again, the same pattern. Possibly factor out a macro or parameterized test approach for DRY.
crates/tests/Cargo.toml (1)
22-24: Consider documenting supported curves in workspace README.The consistent addition of
prism-keysacross crates establishes a solid foundation for multi-curve support. Consider:
- Documenting supported curves in the workspace README
- Adding a compatibility matrix for different node types (prover, full node) and their supported curve combinations
- Including migration guides for users updating from ed25519-consensus
crates/keys/src/signatures.rs (1)
41-41: Improve error message for algorithm validationThe error message now includes the actual algorithm value, which is helpful for debugging. However, consider adding valid algorithm options to the message.
- _ => bail!("Unexpected algorithm for Signature: {}", algorithm), + _ => bail!("Unexpected algorithm for Signature: '{}'. Expected one of: ed25519, secp256k1, secp256r1", algorithm),crates/keys/src/signing_keys.rs (1)
66-66: Improve error message consistencyUpdate the error message to match the format suggested for
new_with_algorithm.- _ => bail!("Unexpected algorithm for SigningKey: {}", algorithm), + _ => bail!("Unexpected algorithm for SigningKey: '{}'. Expected one of: ed25519, secp256k1, secp256r1", algorithm),crates/tests/src/lib.rs (1)
Line range hint
89-104: Consider separating test scenariosThe test combines account creation and updates in a single loop. Consider separating these into distinct test cases for better clarity and easier debugging.
#[tokio::test] async fn test_account_creation() -> Result<()> { // Test only account creation } #[tokio::test] async fn test_account_updates() -> Result<()> { // Test only account updates }crates/cli/src/cfg.rs (1)
210-210: Consider documenting the configuration precedence.The configuration application logic correctly prioritizes command-line arguments over config file values, but this behavior should be documented.
Add a comment explaining the precedence:
+ // Command-line arguments take precedence over config file values verifying_key_algorithm: args.verifying_key_algorithm.unwrap_or(config.verifying_key_algorithm),crates/common/src/tree/mod.rs (2)
63-75: Consider using test parameterization to reduce duplication.While the current implementation provides good test coverage for all algorithms, there's significant duplication in the test functions. Consider using a test framework feature for parameterized tests to reduce code duplication.
Example approach using a test macro:
macro_rules! parameterized_test { ($test_name:ident, $test_fn:ident) => { #[test] fn $test_name() { for algorithm in ["ed25519", "secp256k1", "secp256r1"] { $test_fn(algorithm); } } }; } parameterized_test!(test_insert_and_get_all_algorithms, test_insert_and_get);Also applies to: 97-109, 142-154, 181-193, 220-232, 251-264, 318-330, 373-385, 405-417, 459-472
Line range hint
1-472: Well-structured implementation of multi-algorithm support.The changes effectively implement support for multiple cryptographic algorithms while maintaining backward compatibility and providing comprehensive test coverage. The architecture cleanly separates the algorithm-specific implementation details through the prism_keys abstraction.
Consider documenting the supported algorithms and their use cases in the project's documentation to help users make informed decisions about algorithm selection.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (17)
crates/cli/src/cfg.rs(4 hunks)crates/cli/src/main.rs(4 hunks)crates/common/src/transaction_builder.rs(5 hunks)crates/common/src/tree/mod.rs(11 hunks)crates/da/Cargo.toml(1 hunks)crates/da/src/lib.rs(2 hunks)crates/keys/src/lib.rs(2 hunks)crates/keys/src/signatures.rs(1 hunks)crates/keys/src/signing_keys.rs(2 hunks)crates/keys/src/verifying_keys.rs(4 hunks)crates/node_types/lightclient/Cargo.toml(1 hunks)crates/node_types/lightclient/src/lightclient.rs(2 hunks)crates/node_types/prover/Cargo.toml(0 hunks)crates/node_types/prover/src/prover/mod.rs(3 hunks)crates/node_types/prover/src/prover/tests.rs(7 hunks)crates/tests/Cargo.toml(1 hunks)crates/tests/src/lib.rs(5 hunks)
💤 Files with no reviewable changes (1)
- crates/node_types/prover/Cargo.toml
🔇 Additional comments (42)
crates/da/src/lib.rs (1)
53-53: Validate that the inserted algorithm matches the key.
Using vk.algorithm() to construct the signature is good, but ensure that the provided bytes truly align with that chosen algorithm to avoid subtle verification issues.
crates/cli/src/main.rs (6)
7-7: Import looks good.
Importing SigningKey and VerifyingKey from prism_keys centralizes cryptographic key usage. Ensure consistency with the rest of the codebase.
71-71: Key retrieval.
KeyStoreType::KeyChain(KeyChain).get_signing_key() is returning a raw key. Confirm if concurrency or thread-safety is a concern.
86-86: Verifying key derivation.
verifying_key = signing_key.verifying_key() is clean. Make sure no mismatch occurs between config-provided verifying key and the derived verifying key if both are used.
131-134: Bubble up error with clarity.
An empty verifying_key_algorithm leads to an InvalidInput error. The message is helpful, but ensure the CLI usage instructions mention this requirement.
136-141: Repeated algorithm checks.
Again, these checks appear multiple times in the file. Might be beneficial to unify them as a helper.
142-143: From bytes usage.
Consider verifying any special constraints (e.g., compressed vs. uncompressed for secp256k1/r1) if that might break usage.
crates/keys/src/verifying_keys.rs (2)
89-89: Matching signature type with key type.
This approach is robust. The pattern matching clarifies that a signature’s actual type must match the verifying key’s type.
199-200: Documentation clarity.
Stating that 32 bytes is for Ed25519, 33/65 for secp256 curves is a helpful reference.
crates/node_types/prover/src/prover/tests.rs (17)
11-11: Parameterizing tests by algorithm.
This is a good approach to test all supported algorithms thoroughly. Ensure test coverage for each.
15-15: Config with key algorithm.
Config::default_with_key_algorithm typically indicates test usage. Consider where production config usage might differ.
19-19: Mock transactions helper.
Parameterizing by algorithm ensures consistent coverage. Good usage of “service_id” to isolate test scenarios.
34-39: Multiple calls to validate_and_queue_update.
Testing the same transaction multiple times is helpful. We confirm duplicate submission logic.
50-52: Separate test for ed25519.
This pattern is repeated for each curve, ensuring thorough coverage.
54-62: Separate tests for secp256k1 and secp256r1.
Similarly, good coverage. No issues noted.
80-80: SigningKey::new_with_algorithm.
This is consistent with the rest of the code that parameterizes by algorithm.
103-108: Repeated test pattern.
Same concept as before for process_transactions. This repetition is acceptable for varied cryptographic coverage.
117-134: execute_block with conflicting transactions.
This is a well-structured test scenario for block-level validations.
142-154: Test multiple curves with invalid block transactions.
As with the prior logic, coverage is thorough.
156-179: execute_block.
Collective creation of mock transactions, verifying resulting proofs is robust. No immediate issues found.
Line range hint 180-188: Epoch finalization.
Testing epoch finalization across different algorithms further ensures correctness.
192-204: Separate curve tests for finalize_new_epoch.
Matches prior pattern. Good coverage.
Line range hint 206-249: test_restart_sync_from_scratch.
Combining DA-layer usage with spawning a Prover in the background is an effective concurrency test.
250-262: Additional coverage for different curves.
Again, consistent approach.
Line range hint 264-299: test_load_persisted_state.
This ensures persisted state can be reloaded properly. Good for ensuring data consistency.
300-313: Long form multi-curve testing.
Completes coverage for all supported algorithms on state loading.
crates/da/Cargo.toml (1)
24-24: LGTM! Verify DA proof compatibility.
The addition of prism-keys is consistent with the project-wide migration. Since this is the DA (Data Availability) module, ensure that the proof verification logic remains compatible with all supported curves.
✅ Verification successful
Let me gather more information about the cryptographic verification implementation to ensure compatibility.
Let me check one more thing to understand the algorithm and signature types being used.
Signature verification implementation is correctly migrated
The verification logic in the DA module is properly implemented using the new prism-keys dependency. The code correctly:
- Uses algorithm-agnostic types (
SigningKey,VerifyingKey,Signature) - Handles signature verification with proper error handling
- Maintains compatibility by using the verifying key's algorithm for signature creation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for proof verification implementations
rg -A 5 "verify.*proof|verify.*signature" crates/da/src/
Length of output: 775
Script:
#!/bin/bash
# Check the type definitions and imports related to VerifyingKey
ast-grep --pattern 'use $_::VerifyingKey;'
# Check the full implementation context
rg -B 5 -A 10 'verify_signature' crates/da/src/
Length of output: 2133
Script:
#!/bin/bash
# Check for signature and algorithm type usage
rg -B 2 -A 2 "Signature::from_algorithm_and_bytes|algorithm()" crates/da/src/
# Check imports to understand the cryptographic types
rg -B 2 "use prism_keys::" crates/da/src/
Length of output: 666
crates/tests/Cargo.toml (1)
24-24: LGTM! Ensure comprehensive curve testing.
The addition of prism-keys to the test crate is crucial. With the mock_prover feature available, this provides a good foundation for testing different cryptographic curves.
crates/keys/src/lib.rs (2)
16-16: LGTM! Good addition of secp256r1 support.
The import follows the established pattern of aliasing curve-specific signing keys.
126-137: LGTM! Comprehensive test coverage for secp256r1.
The test follows the same pattern as ed25519 and secp256k1 tests, ensuring consistent verification of key serialization and deserialization.
crates/node_types/lightclient/src/lightclient.rs (2)
2-2: LGTM! Good migration to generic VerifyingKey.
The change from ed25519-specific to generic VerifyingKey type enables multi-curve support.
75-75: LGTM! Correct ownership handling in signature verification.
Using clone() is appropriate here as verify_signature takes ownership of the key.
crates/cli/src/cfg.rs (2)
104-104: LGTM! Good default configuration.
The default to "ed25519" maintains backward compatibility while enabling opt-in support for other curves.
Also applies to: 115-115
39-41: 🛠️ Refactor suggestion
Consider adding validation for verifying_key_algorithm.
While the documentation clearly states the supported algorithms, there's no validation at the configuration level. This could lead to runtime errors if an invalid algorithm is provided.
Consider adding validation in the apply_command_line_args function:
verifying_key_algorithm: Option<String>,
+#[arg(long, value_parser = ["ed25519", "secp256k1", "secp256r1"])]
verifying_key_algorithm: Option<String>,crates/common/src/transaction_builder.rs (5)
87-93: LGTM! Method correctly updated to support multiple algorithms.
The change properly introduces algorithm flexibility while maintaining the existing functionality.
123-127: LGTM! Method signature and implementation correctly updated.
The changes maintain consistency with the new algorithm-flexible approach.
146-151: LGTM! Method properly updated for algorithm flexibility.
The implementation correctly handles the algorithm parameter and maintains consistency.
185-190: LGTM! Methods correctly updated to support algorithm parameter.
Both methods properly implement the algorithm flexibility changes.
Also applies to: 195-200
272-278: LGTM! Methods properly updated for algorithm support.
The changes maintain consistency with the algorithm-flexible approach.
Also applies to: 284-288
crates/node_types/prover/src/prover/mod.rs (3)
2-2: LGTM! Imports and type declarations properly updated.
The changes correctly switch from ed25519-specific types to algorithm-agnostic types.
Also applies to: 49-49
57-67: LGTM! Default implementation maintains backward compatibility.
The changes properly maintain ed25519 as the default while using the new API.
72-82: LGTM! Test helper method properly implemented.
The new method correctly enables testing with different algorithms while being appropriately scoped to test code.
| tokio = { workspace = true } | ||
| log = { workspace = true } | ||
| anyhow = { workspace = true } | ||
| prism-common = { workspace = true, features = ["test_utils"] } | ||
| prism-errors = { workspace = true } | ||
| prism-da = { workspace = true } | ||
| prism-keys = { workspace = true } |
There was a problem hiding this comment.
💡 Codebase verification
Tests needed for multiple curve support
Based on the codebase analysis, while the lightclient crate has integrated prism-keys for multiple curve support, there are currently no tests present. The codebase shows:
- No test modules or test functions in the source files
- No dedicated
testsdirectory - Core functionality using
VerifyingKeyfromprism-keysbut without test coverage
Action items:
- Add unit tests for signature verification with different curve types
- Add tests for the
verify_signaturefunctionality with various key types - Include integration tests for the full verification flow with different curves
🔗 Analysis chain
LGTM! Verify test coverage for multiple curves.
The addition of prism-keys aligns with the PR objective to support multiple cryptographic curves. Since this crate has test-related features, ensure comprehensive test coverage exists for all supported curves (ed25519, secp256k1, secp256r1).
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if tests exist for different curves
rg -A 5 "test.*curve|test.*key" crates/node_types/lightclient/src/
Length of output: 68
Script:
#!/bin/bash
# Let's check for test files and test functions more broadly
fd "test" crates/node_types/lightclient/
Length of output: 40
Script:
#!/bin/bash
# Look for test modules or functions in the codebase
rg -g "*.rs" "mod test|#\[test\]" crates/node_types/lightclient/src/
Length of output: 70
Script:
#!/bin/bash
# Check for key-related test assertions or verifications
rg -g "*.rs" "assert.*key|verify.*key" crates/node_types/lightclient/src/
Length of output: 650
Script:
#!/bin/bash
# Look for imports of different curve types in test modules
rg -g "*.rs" "use.*(?i)(ed25519|secp256k1|secp256r1)" crates/node_types/lightclient/src/
Length of output: 90
Script:
#!/bin/bash
# Let's check the actual source files to understand the structure
fd -e rs . crates/node_types/lightclient/src/
Length of output: 137
Script:
#!/bin/bash
# Look at the lib.rs or mod.rs to understand the module structure
rg -g "lib.rs|mod.rs" "pub mod|mod" crates/node_types/lightclient/src/
Length of output: 72
Script:
#!/bin/bash
# Check for any test configuration or test utilities
cat crates/node_types/lightclient/src/lib.rs
Length of output: 134
Script:
#!/bin/bash
# Let's examine the lightclient.rs file for test modules
cat crates/node_types/lightclient/src/lightclient.rs
Length of output: 5878
Script:
#!/bin/bash
# Check if tests might be in a separate tests directory
fd . crates/node_types/lightclient/tests/
Length of output: 167
Signed-off-by: Smuu <18609909+Smuu@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
crates/node_types/prover/src/prover/tests.rs (1)
11-17: Consider using test parameterization for better maintainability.The test suite has been properly updated to support multiple algorithms, but the current approach of creating separate test functions for each algorithm leads to code duplication. Consider using test parameterization for better maintainability.
Example using
rstest:use rstest::rstest; #[rstest] #[case("ed25519")] #[case("secp256k1")] #[case("secp256r1")] #[tokio::test] async fn test_create_test_prover(#[case] algorithm: &str) { let prover = create_test_prover(algorithm).await; // ... rest of the test }crates/tree/src/tests.rs (3)
Line range hint
44-459: Consider using test parameterization macros for better organization.The current approach of creating separate test functions for each algorithm creates maintenance overhead. Consider using test frameworks that support parameterized tests, such as
rstestor similar, to reduce code duplication and improve maintainability.Example approach:
use rstest::rstest; #[rstest] #[case("ed25519")] #[case("secp256k1")] #[case("secp256r1")] fn test_insert_and_get(#[case] algorithm: &str) { // ... test implementation ... }
Line range hint
58-90: Enhance error assertions in failure test cases.The error assertions in failure test cases (e.g.,
test_insert_for_nonexistent_service_fails) only check if an error occurred but don't verify the specific error type or message. This could allow incorrect errors to pass the test.Consider using more specific error assertions:
- assert!(insertion_result.is_err()); + match insertion_result { + Err(e) => assert!(e.to_string().contains("service not found")), + Ok(_) => panic!("Expected error for nonexistent service"), + }
Line range hint
406-459: Replace println! statements with test-appropriate logging.The batch writing tests contain multiple
println!statements which can make test output noisy. Consider using thetracingcrate's test features or conditional compilation for debug output.- println!("Inserting acc_1"); + trace!("Inserting acc_1");crates/node_types/prover/src/prover/mod.rs (1)
53-65: Consider making the default key algorithm configurable.The
Defaultimplementation hardcodesed25519as the key algorithm. Consider:
- Making the algorithm configurable via environment variables
- Adding a constant for the default algorithm
+const DEFAULT_KEY_ALGORITHM: &str = "ed25519"; impl Default for Config { fn default() -> Self { - let signing_key = SigningKey::new_ed25519(); + let algorithm = std::env::var("PRISM_KEY_ALGORITHM") + .unwrap_or_else(|_| DEFAULT_KEY_ALGORITHM.to_string()); + let signing_key = SigningKey::new_with_algorithm(&algorithm);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
crates/node_types/prover/Cargo.toml(0 hunks)crates/node_types/prover/src/prover/mod.rs(3 hunks)crates/node_types/prover/src/prover/tests.rs(7 hunks)crates/tree/src/tests.rs(11 hunks)
💤 Files with no reviewable changes (1)
- crates/node_types/prover/Cargo.toml
🔇 Additional comments (1)
crates/node_types/prover/src/prover/tests.rs (1)
80-80: Verify key generation across different algorithms.
The test suite creates signing keys for different algorithms. Let's verify that the key generation is working correctly for all supported algorithms.
Also applies to: 122-122, 223-223
Signed-off-by: Smuu <18609909+Smuu@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
crates/tree/src/tests.rs (1)
Line range hint
12-42: Add test cases for invalid algorithm strings.The current tests only verify valid algorithm strings. Consider adding negative test cases to verify proper error handling for invalid algorithms.
#[test] fn test_insert_and_get_invalid_algorithm() { let mut tree = KeyDirectoryTree::new(Arc::new(MockTreeStore::default())); let mut tx_builder = TransactionBuilder::new(); let result = tx_builder.register_service_with_random_keys("invalid_algo", "service_1"); assert!(result.is_err()); assert!(result.unwrap_err().to_string().contains("Invalid algorithm")); }
🧹 Nitpick comments (5)
crates/keys/src/signing_keys.rs (1)
66-66: Enhance error message clarity.Consider making the error message more descriptive by including the list of supported algorithms, similar to the message in
new_with_algorithm.- _ => bail!("Unexpected algorithm for SigningKey: {}", algorithm), + _ => bail!("Unexpected algorithm for SigningKey: '{}'. Expected one of: ed25519, secp256k1, secp256r1", algorithm),crates/cli/src/main.rs (1)
163-173: Consider moving validate_algorithm to the keys crate.This validation logic would be more appropriate in the
prism_keyscrate where it can be reused by other crates.crates/tree/src/tests.rs (3)
Line range hint
12-42: Consider improving test maintainability with shared test constants.To enhance test maintainability and readability, consider extracting commonly used test values into constants:
+ const TEST_ALGORITHMS: [&str; 3] = ["ed25519", "secp256k1", "secp256r1"]; + const TEST_SERVICE_ID: &str = "service_1"; + const TEST_ACCOUNT_ID: &str = "acc_1"; fn test_insert_and_get(algorithm: &str) { let mut tree = KeyDirectoryTree::new(Arc::new(MockTreeStore::default())); let mut tx_builder = TransactionBuilder::new(); - let service_tx = tx_builder.register_service_with_random_keys(algorithm, "service_1").commit(); + let service_tx = tx_builder.register_service_with_random_keys(algorithm, TEST_SERVICE_ID).commit(); // ... rest of the function }
Line range hint
406-444: Remove debug println statements from test code.The test contains multiple debug println statements that should be removed from the test code as they add noise to the test output.
fn test_batch_writing(algorithm: &str) { // ... setup code ... - println!("Inserting acc_1"); tree.process_transaction(account1_tx).unwrap(); - println!("Tree state after first insert: {:?}", tree.get_commitment()); // Try to get the first value immediately let get_result1 = tree.get(KeyHash::with::<TreeHasher>("acc_1")); - println!("Get result for key1 after first write: {:?}", get_result1); // ... rest of the function }
Line range hint
58-74: Verify error messages in failure cases.The test verifies that the operation fails but doesn't check the specific error message. Consider adding error message verification to ensure the correct error is being returned.
fn test_insert_for_nonexistent_service_fails(algorithm: &str) { // ... setup code ... let insertion_result = tree.process_transaction(invalid_account_tx); - assert!(insertion_result.is_err()); + let err = insertion_result.unwrap_err(); + assert!(err.to_string().contains("service not found"), + "Expected 'service not found' error, got: {}", err); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
crates/cli/src/main.rs(5 hunks)crates/keys/src/signing_keys.rs(2 hunks)crates/node_types/prover/src/prover/mod.rs(3 hunks)crates/tests/src/lib.rs(5 hunks)crates/tree/src/tests.rs(11 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/node_types/prover/src/prover/mod.rs
🔇 Additional comments (4)
crates/keys/src/signing_keys.rs (1)
34-41: LGTM! Well-implemented error handling.
The implementation correctly returns a Result type instead of panicking and provides clear error messages for unsupported algorithms.
crates/tests/src/lib.rs (2)
45-46: Parameterize test to cover all supported algorithms.
The test currently only verifies secp256r1. Consider parameterizing the test to cover all supported algorithms.
- let algorithm = "secp256r1";
+ for algorithm in ["ed25519", "secp256k1", "secp256r1"] {
+ test_light_client_prover_talking_with_algorithm(algorithm).await?;
+ }50-51: LGTM! Proper error handling implementation.
The code correctly handles potential errors from new_with_algorithm and transaction builder methods using the ? operator for error propagation.
Also applies to: 83-83, 96-96, 115-115
crates/cli/src/main.rs (1)
42-47: LGTM! Robust error handling implementation.
The error handling for verifying keys is well-implemented with proper error messages and type conversions.
Also applies to: 133-138
Signed-off-by: Smuu <18609909+Smuu@users.noreply.github.com>
crates/keys/src/signing_keys.rs
Outdated
| @@ -31,6 +31,15 @@ impl SigningKey { | |||
| SigningKey::Secp256r1(Secp256r1SigningKey::random(&mut OsRng)) | |||
| } | |||
|
|
|||
| pub fn new_with_algorithm(algorithm: &str) -> Self { | |||
| match algorithm { | |||
| "ed25519" => SigningKey::Ed25519(Box::new(Ed25519SigningKey::new(OsRng))), | |||
There was a problem hiding this comment.
Those lines could reuse SigningKey::new_ed25519 etc
crates/cli/src/main.rs
Outdated
| return Err(std::io::Error::new(std::io::ErrorKind::InvalidInput, "invalid verifying key algorithm")); | ||
| } | ||
|
|
||
| let prover_vk = VerifyingKey::from_algorithm_and_bytes(verifying_key_algorithm, config.verifying_key.unwrap().as_bytes()).unwrap(); |
There was a problem hiding this comment.
When we extend usage of algorithm like in this PR, we should probably consider using an enum instead of the plain string. We could, for example, convert it from a config str to an appropriate enum here, like
CryptoAlgorithm::try_from(config.verifying_key_algorithm), doing the validation in that try_from.
There was a problem hiding this comment.
This is a great idea, thank you! Applied that
There was a problem hiding this comment.
I've pulled this out into #188 , because it also affects things I'm working on currently. It will also keep the diff a little bit smaller for your PR here. Could you please rebase, after #188 is through?
Solution is basically the same, with small differences:
- I used CryptoAlgorithm instead of KeyAlgorithm (In naming, "Crypto" is a superset for signatures and keys for me)
- Some corresponding changes in CryptoPayload we discussed in Slack
Signed-off-by: Smuu <18609909+Smuu@users.noreply.github.com>
Signed-off-by: Smuu <18609909+Smuu@users.noreply.github.com>
Signed-off-by: Smuu <18609909+Smuu@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (12)
Cargo.toml (1)
111-111: Consider using workspace version for paste crateFor consistency with other dependencies, consider moving the paste crate version to the workspace.package section.
-paste = "1.0.15" +paste = { version = "1.0.15", workspace = true }crates/keys/src/lib.rs (1)
13-19: Consider making 'Placeholder' a hidden or deprecated variant.
If "Placeholder" is never intended for real usage, it could be best to hide or remove this variant to avoid confusion and reduce complexity.crates/tree/src/tests.rs (2)
5-5: Enhance discoverability of 'KeyAlgorithm' usage.
Importing both 'SigningKey' and 'KeyAlgorithm' from 'prism_keys' is fine, but consider a dedicated doc comment or README reference for these new enumerated algorithms, to help future contributors quickly identify the available options.
12-24: Mind the code duplication in test logic.
Although you are using macros to generate multi-algorithm tests, there's still some repeated setup across tests (e.g., new tree, new tx_builder). Consider extracting that into a common fixture or utility function to further DRY up your test code.crates/keys/src/signatures.rs (1)
31-42: Consolidate error messages for unexpected algorithms.
Currently, the code bails out with "Unexpected algorithm for Signature." If you're handling multiple unexpected algorithm paths in different modules, consider centralizing or reusing a single error message format for clarity and consistency.crates/keys/src/tests.rs (1)
1-140: LGTM: Comprehensive test coverage for all curvesThe test suite thoroughly covers key operations across all supported curves (Ed25519, Secp256k1, Secp256r1) including serialization, deserialization, and error cases.
Consider organizing the tests into sub-modules by curve type for better maintainability:
mod ed25519_tests { #[test] fn test_verifying_key_operations() { // Ed25519 specific tests } } mod secp256k1_tests { #[test] fn test_verifying_key_operations() { // Secp256k1 specific tests } }🧰 Tools
🪛 GitHub Check: clippy
[failure] 2-2:
module has the same name as its containing modulecrates/cli/src/main.rs (1)
Line range hint
1-169: Consider implementing a custom error typeThe current approach of converting all errors to
std::io::Errormakes it difficult to handle specific error cases. Consider implementing a custom error type:#[derive(Debug, thiserror::Error)] pub enum PrismError { #[error("Invalid algorithm: {0}")] InvalidAlgorithm(String), #[error("Key error: {0}")] KeyError(String), #[error("Configuration error: {0}")] ConfigError(String), #[error(transparent)] IoError(#[from] std::io::Error), } impl From<PrismError> for std::io::Error { fn from(err: PrismError) -> Self { std::io::Error::new(std::io::ErrorKind::Other, err.to_string()) } }crates/keys/src/verifying_keys.rs (2)
Line range hint
90-120: LGTM! Consider enhancing error messages.The signature verification implementation is correct for all supported curves. Each key type is properly handled with appropriate verification methods.
Consider enhancing error messages to include the actual and expected signature types:
- bail!("Invalid signature type"); + bail!("Invalid signature type: expected Ed25519, got {:?}", signature);
220-226: Enhance error handling for curve-specific failures.The current error message "Invalid curve type" doesn't provide enough context about which curves were attempted.
Consider enhancing the error message to include which curves failed:
- Err(anyhow!("Invalid curve type")) + Err(anyhow!("Failed to parse key as either Secp256k1 or Secp256r1"))crates/node_types/prover/src/prover/tests.rs (1)
209-236: LGTM! Efficient test generation with clear naming.The macro effectively generates tests for each curve while maintaining clear test names.
Consider adding a brief doc comment to explain the macro's purpose:
+/// Generates test functions for each supported key algorithm (Ed25519, Secp256k1, Secp256r1). macro_rules! generate_algorithm_tests {crates/node_types/prover/src/prover/mod.rs (2)
46-46: Update field documentation to reflect multi-curve support.The comment should be updated to indicate that the verifying key can now use different curves.
- /// Key used to verify incoming [`FinalizedEpochs`]. - /// This is not necessarily the counterpart to signing_key, as fullnodes must use the [`verifying_key`] of the prover. + /// Key used to verify incoming [`FinalizedEpochs`]. + /// This is not necessarily the counterpart to signing_key, as fullnodes must use the [`verifying_key`] of the prover. + /// Supports multiple curves (Ed25519, Secp256k1, Secp256r1) for verification.
54-64: Consider making the default key algorithm configurable.The default implementation hardcodes Ed25519. Consider making this configurable through a const or environment variable for better flexibility.
+ const DEFAULT_KEY_ALGORITHM: KeyAlgorithm = KeyAlgorithm::Ed25519; impl Default for Config { fn default() -> Self { - let signing_key = SigningKey::new_ed25519(); + let signing_key = SigningKey::new_with_algorithm(DEFAULT_KEY_ALGORITHM) + .expect("Failed to create signing key with default algorithm");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (14)
Cargo.toml(1 hunks)crates/cli/src/main.rs(5 hunks)crates/common/src/transaction_builder.rs(6 hunks)crates/keys/src/lib.rs(1 hunks)crates/keys/src/signatures.rs(3 hunks)crates/keys/src/signing_keys.rs(4 hunks)crates/keys/src/tests.rs(1 hunks)crates/keys/src/verifying_keys.rs(5 hunks)crates/node_types/prover/Cargo.toml(1 hunks)crates/node_types/prover/src/prover/mod.rs(3 hunks)crates/node_types/prover/src/prover/tests.rs(7 hunks)crates/tests/src/lib.rs(5 hunks)crates/tree/Cargo.toml(1 hunks)crates/tree/src/tests.rs(11 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/node_types/prover/Cargo.toml
🧰 Additional context used
🪛 GitHub Check: clippy
crates/keys/src/tests.rs
[failure] 2-2:
module has the same name as its containing module
crates/common/src/transaction_builder.rs
[failure] 88-88:
using clone on type KeyAlgorithm which implements the Copy trait
🔇 Additional comments (18)
crates/tree/Cargo.toml (1)
14-14: Verify the usage of the paste crate
The addition of the paste crate suggests code generation for curve-specific implementations. Let's verify its usage in the codebase.
✅ Verification successful
The paste crate is correctly used for test generation across multiple key algorithms
The paste crate is being used appropriately in the generate_algorithm_tests! macro to create test variants for Ed25519, Secp256k1, and Secp256r1 key algorithms. This is part of the PR's changes to support multiple curve implementations, where the macro automatically generates test function names by concatenating the base test name with the algorithm variant (e.g., test_insert_and_get_ed25519).
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usage of paste macros in the tree crate
rg "paste::" crates/tree/
# Search for paste macro invocations
rg "paste!\s*!\s*{" crates/tree/
Length of output: 201
Script:
#!/bin/bash
# Search for paste macro usage with a simpler pattern
rg "paste!" crates/tree/
# Look for test modules or test-related code that might use paste
ast-grep --pattern 'mod tests {
$$$
}' crates/tree/src/
Length of output: 135
Script:
#!/bin/bash
# Get the context around the paste macro usage in tests.rs
rg -B 5 -A 5 "paste::paste!" crates/tree/src/tests.rs
# Check if this is a new addition by looking at git diff
git diff origin/main -- crates/tree/src/tests.rs
Length of output: 14919
Cargo.toml (2)
Line range hint 67-76: Verify curve implementations and features
The PR aims to test different curves (Ed25519, Secp256k1, P256). Let's verify the implementations and feature flags:
✅ Verification successful
Let me gather more information about the curve implementations and their dependencies.
Curve implementations and dependencies are properly configured
The verification confirms that:
- All three curves (Ed25519, Secp256k1, and P256/Secp256r1) are properly implemented with:
- Appropriate dependencies in Cargo.toml
- Complete implementations for signing and verification
- Proper test coverage for all curves in multiple test files
- The
KeyAlgorithmenum correctly defines all supported curves - Dependencies have necessary feature flags enabled (e.g., "secp256k1/global-context", "p256/ecdsa")
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for curve-specific implementations
echo "Checking curve implementations..."
rg -t rust "impl.*(?:Ed25519|Secp256k1|P256)"
# Check for test coverage of different curves
echo "Checking test coverage..."
rg -t rust "(?:#\[test\]|test_).*(?:ed25519|secp256k1|p256)"
# Verify feature flags for curves
echo "Checking feature flags..."
fd -e toml | xargs rg "features.*(?:ed25519|secp256k1|p256)"
Length of output: 1775
Script:
#!/bin/bash
# Check for curve-related dependencies in Cargo.toml files
echo "Checking curve-related dependencies..."
fd -e toml | xargs rg "(?:ed25519|secp256k1|p256|k256|p384|p521|curve25519)"
# Check for key algorithm definitions and usages
echo "Checking key algorithm definitions..."
rg -t rust "(?:enum|struct) .*(?:KeyAlgorithm|Algorithm)" -A 5
# Check for curve-specific signing operations
echo "Checking signing implementations..."
rg -t rust "(?:sign|verify).*(?:Ed25519|Secp256k1|P256)" -A 2
Length of output: 1808
Line range hint 38-38: Review ZK compatibility with curves
The addition of crates/zk/sp1 and patched crypto libraries suggests ZK proof integration. Ensure all curves are compatible with SP1 ZK-VM:
Consider documenting which curves are supported in the ZK context and any performance implications.
Also applies to: 142-144
crates/keys/src/lib.rs (1)
24-31: Good use of anyhow errors for parsing!
This improves the readability and consistency of error messages when the user provides an unsupported algorithm string.
crates/tree/src/tests.rs (2)
283-303: Excellent approach with generate_algorithm_tests macro!
This neatly covers each algorithm variant without duplicating test functions.
316-327: Robust negative test checks.
Verifying nonexistence in the tree with an explicit check and corresponding proof ensures stronger guarantees about the correctness of your tree logic.
crates/keys/src/signatures.rs (1)
46-51: Great alignment between 'algorithm()' and 'from_algorithm_and_bytes()'.
Having these complementary methods ensures consistent handling of signature algorithms.
crates/keys/src/signing_keys.rs (3)
35-42: Return 'Result' for 'new_with_algorithm'?
Panic is avoided here with a bail, which is good. This approach is consistent with the rest of the methods relying on 'Result', ensuring better error handling.
71-75: Accurate returns from 'algorithm' method
This helps maintain clarity when converting signing keys back into user-facing data structures.
56-67: Consistent error handling for unknown algorithm
This is aligned with best practices, but be mindful if "Placeholder" is not truly unknown. Consider giving a more precise message if you differentiate between a recognized but unsupported variant vs. a truly unknown algorithm.
crates/tests/src/lib.rs (1)
14-14: LGTM: Key algorithm integration looks good
The changes correctly integrate the KeyAlgorithm enum and properly initialize the signing key with the specified algorithm.
Also applies to: 45-46, 50-51
crates/keys/src/verifying_keys.rs (2)
67-80: LGTM! Type-safe algorithm handling.
The change from string-based to enum-based algorithm selection improves type safety and eliminates potential runtime errors from invalid algorithm strings.
82-87: LGTM! Consistent use of KeyAlgorithm enum.
The method now returns a strongly-typed KeyAlgorithm instead of a string, maintaining consistency with the new type system.
crates/node_types/prover/src/prover/tests.rs (1)
Line range hint 11-103: LGTM! Comprehensive test coverage for all curves.
The test functions have been properly parameterized to test all supported curves while maintaining the same test logic.
crates/common/src/transaction_builder.rs (1)
Line range hint 83-275: LGTM! Consistent algorithm parameterization.
The TransactionBuilder methods have been consistently updated to support different key algorithms, improving flexibility while maintaining the existing functionality.
crates/node_types/prover/src/prover/mod.rs (3)
2-2: LGTM: Import changes align with multi-curve support.
The addition of prism_keys imports supports the PR's objective of testing different curves.
285-287: Previous review comment addressed.
The signature verification changes have been implemented as suggested in the previous review.
67-79: Verify test coverage for all supported key algorithms.
Let's ensure that tests exist for all supported key algorithms.
✅ Verification successful
Let me gather more information about the available key algorithms to ensure comprehensive test coverage.
Test coverage for key algorithms is comprehensive
The test files show that all supported key algorithms (Ed25519, Secp256k1, and Secp256r1) are being tested through parameterized test macros. Each test case is executed for all three production algorithms, as evidenced by:
- In
crates/node_types/prover/src/prover/tests.rs:
$test_fn(KeyAlgorithm::Ed25519).await;
$test_fn(KeyAlgorithm::Secp256k1).await;
$test_fn(KeyAlgorithm::Secp256r1).await;The create_test_prover and create_mock_transactions helper functions are used across multiple test scenarios with all key algorithm variants.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check test coverage for different key algorithms
# Look for test cases using different key algorithms
rg -A 10 "default_with_key_algorithm|new_with_algorithm" "crates/node_types/prover/src/prover/tests.rs"
# Look for test helper functions that might be using different algorithms
rg -A 5 "create_test_prover|create_mock_transactions" "crates/node_types/prover/src/prover/tests.rs"
Length of output: 5311
Script:
#!/bin/bash
# Look for KeyAlgorithm enum definition and its variants
rg -A 5 "enum KeyAlgorithm"
# Look for test cases that might be using different algorithm variants
rg "KeyAlgorithm::"
Length of output: 4638
| impl fmt::Display for KeyAlgorithm { | ||
| fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { | ||
| match self { | ||
| KeyAlgorithm::Ed25519 => write!(f, "Ed25519"), | ||
| KeyAlgorithm::Secp256k1 => write!(f, "Secp256k1"), | ||
| KeyAlgorithm::Secp256r1 => write!(f, "Secp256r1"), | ||
| KeyAlgorithm::Placeholder => write!(f, "Placeholder"), | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Based on the gathered evidence, I can now provide a final response:
Placeholder variant requires documentation and restricted usage
The Placeholder variant is currently used in both KeyAlgorithm and Signature enums, with implementations that suggest it's a special case (empty bytes in signatures, accepted in string parsing). However, there's no documentation explaining its purpose or usage restrictions. This could lead to security issues if used in production code.
- In
crates/keys/src/lib.rs: Add documentation explaining the purpose and usage constraints ofKeyAlgorithm::Placeholder - In
crates/keys/src/signatures.rs: EnsureSignature::Placeholderis properly restricted and documented - Consider using
#[doc(hidden)]or feature flags if this is meant for testing/internal use only
🔗 Analysis chain
Validate usage of 'Placeholder' in 'from_str'
Currently, 'Placeholder' can be accepted as a valid algorithm option. If it is only for debugging or future use, consider gating it behind a feature flag or clarifying its intended usage.
Let me search for more context about the usage and testing of this placeholder variant.
Let me check one more thing to see if there are any tests or documentation that might explain the purpose of the Placeholder variant.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Searching for all references to 'KeyAlgorithm::Placeholder' to confirm usage context
rg 'KeyAlgorithm::Placeholder' -A 5
Length of output: 874
Script:
#!/bin/bash
# Search for tests or any other usage context of the placeholder variant
rg -i "placeholder" crates/keys/src/ -A 5
# Also check the enum definition
ast-grep --pattern 'enum KeyAlgorithm {
$$$
}'
Length of output: 1801
Script:
#!/bin/bash
# Look for test modules and documentation
rg -U "mod tests \{.*?^\}" -A 10 crates/keys/src/lib.rs --multiline
# Check for any documentation comments
rg "^///" crates/keys/src/lib.rs
Length of output: 104
| @@ -78,7 +80,7 @@ async fn test_light_client_prover_talking() -> Result<()> { | |||
|
|
|||
| let mut transaction_builder = TransactionBuilder::new(); | |||
| let register_service_req = | |||
| transaction_builder.register_service_with_random_keys("test_service").commit(); | |||
| transaction_builder.register_service_with_random_keys(algorithm, "test_service").commit(); | |||
There was a problem hiding this comment.
Add error handling for transaction builder methods
The transaction builder methods may fail when creating keys with the specified algorithm. Consider handling potential errors to prevent panics.
- transaction_builder.register_service_with_random_keys(algorithm, "test_service").commit();
+ transaction_builder.register_service_with_random_keys(algorithm, "test_service")
+ .map_err(|e| anyhow::anyhow!("Failed to register service: {}", e))?
+ .commit();
- .create_account_with_random_key_signed(algorithm, random_user_id.as_str(), "test_service")
+ .create_account_with_random_key_signed(algorithm, random_user_id.as_str(), "test_service")
+ .map_err(|e| anyhow::anyhow!("Failed to create account: {}", e))?
- transaction_builder.add_random_key_verified_with_root(algorithm, acc_id).commit();
+ transaction_builder.add_random_key_verified_with_root(algorithm, acc_id)
+ .map_err(|e| anyhow::anyhow!("Failed to add key: {}", e))?
+ .commit();Also applies to: 96-96, 115-115
There was a problem hiding this comment.
error[E0599]: no method named map_err found for struct UncommittedTransaction in the current scope
--> crates/tests/src/lib.rs:84:14
|
82 | let register_service_req = transaction_builder
| _______________________-
83 | | .register_service_with_random_keys(algorithm, "test_service")
84 | | .map_err(|e| anyhow::anyhow!("Failed to register service: {}", e))?
| | -^^^^^^^ method not found in UncommittedTransaction<'_>
| ||
|
There was a problem hiding this comment.
@smuu, the error occurs because “register_service_with_random_keys(…)” currently returns an UncommittedTransaction rather than a Result<T, E>, so there is no .map_err(...) method to call. If you’d like to handle errors at that step, consider changing the function’s return type to something like Result<UncommittedTransaction<'_>, SomeError> so that you can use .map_err(...). Alternatively, you can handle potential errors inside the transaction builder itself or wherever the actual error condition might happen.
Signed-off-by: Smuu <18609909+Smuu@users.noreply.github.com>
Signed-off-by: Smuu <18609909+Smuu@users.noreply.github.com>
Signed-off-by: Smuu <18609909+Smuu@users.noreply.github.com>
Signed-off-by: Smuu <18609909+Smuu@users.noreply.github.com>
Signed-off-by: Smuu <18609909+Smuu@users.noreply.github.com>
Signed-off-by: Smuu <18609909+Smuu@users.noreply.github.com>
Signed-off-by: Smuu <18609909+Smuu@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
crates/common/src/transaction_builder.rs (2)
118-122: 🛠️ Refactor suggestionMaintain consistent error handling across account creation methods.
Similar to the service registration methods, these methods should also handle errors properly instead of using
expect.Additionally, verify that all callers of these methods are updated to handle the new algorithm parameter.
#!/bin/bash # Search for calls to account creation methods rg -A 2 "create_account_with_random_key(_signed)?" --type rustAlso applies to: 141-146
259-264: 🛠️ Refactor suggestionImplement consistent error handling in data signing methods.
These methods should follow the same error handling pattern as suggested for other methods.
Additionally, verify that all callers of these data signing methods are updated to handle the new algorithm parameter.
#!/bin/bash # Search for calls to data signing methods rg -A 2 "add_randomly_signed_data(_verified_with_root)?" --type rustAlso applies to: 270-274
🧹 Nitpick comments (12)
crates/tree/src/tests.rs (3)
23-23: Ensure error handling when committing transactions.Here, "create_account_with_random_key_signed(...).commit()" might fail at runtime if, for instance, account creation encounters unexpected conditions. Consider handling or propagating errors from commit() calls for more robust test error reporting.
115-115: Consider verifying the updated account properties.After adding a random key, it might be useful to assert other updated account fields to ensure that no regressions occur in the update logic.
207-207: Typographical error in panic message.In the panic string "Expetced insert proof for transaction," the word "Expetced" is misspelled. Consider fixing it for clarity.
Apply this diff to correct the spelling:
- panic!("Expetced insert proof for transaction"); + panic!("Expected insert proof for transaction");crates/keys/src/lib.rs (2)
18-18: Document the 'Placeholder' variant.The 'Placeholder' variant may be confusing without explanatory context. In production code, a lack of clarity about its intended usage could introduce security or maintenance risks. Consider adding documentation detailing how and when 'Placeholder' should be used.
23-33: Validate 'from_str' error messages.When users pass invalid algorithm strings, the error message from “Invalid algorithm: {}” might be insufficient for debugging. Consider returning or logging the set of supported algorithms to guide users toward valid options.
crates/tests/src/lib.rs (2)
37-42: Provide explicit fallback behavior for unsupported environment variables.If users set an unsupported string for “CURVE_ALGORITHM,” the code panics. Consider offering a clearer fallback or user message, such as “Unsupported curve. Must be one of: ed25519, secp256k1, secp256r1."
Line range hint
70-77: Repeated Docker environment setup for each curve.Spinning up and tearing down Docker in a loop ensures isolation for each algorithm test but can be time-consuming. If performance becomes a bottleneck, consider spinning up once, running all the curves in the same environment if feasible, then tearing down.
justfile (1)
70-77: Consider caching Docker images to speed up repeated integration tests.Repeatedly building and tearing down containers can be resource-intensive. If faster iteration is desired, caching or incremental builds could be leveraged to improve test feedback times.
ci/run-validator.sh (1)
102-103: Consider extracting common transaction flags to a variable.The
--feesand--broadcast-mode blockflags are duplicated in both light and bridge node transaction commands.+ TX_FLAGS="--fees 21000utia --broadcast-mode block" - --fees 21000utia \ - --broadcast-mode block + $TX_FLAGS - --fees 21000utia \ - --broadcast-mode block + $TX_FLAGSAlso applies to: 164-165
crates/node_types/prover/src/prover/tests.rs (1)
19-31: Consider parameterizing mock data generation.The mock transaction creation could be more flexible by accepting additional parameters for email domains and service IDs.
-fn create_mock_transactions(algorithm: KeyAlgorithm, service_id: String) -> Vec<Transaction> { +fn create_mock_transactions( + algorithm: KeyAlgorithm, + service_id: String, + user_emails: Vec<String>, +) -> Vec<Transaction> { let mut transaction_builder = TransactionBuilder::new(); + let mut transactions = vec![ + transaction_builder.register_service_with_random_keys(algorithm, &service_id).commit() + ]; - vec![ - transaction_builder.register_service_with_random_keys(algorithm, &service_id).commit(), - transaction_builder - .create_account_with_random_key_signed(algorithm, "user1@example.com", &service_id) - .commit(), - transaction_builder - .create_account_with_random_key_signed(algorithm, "user2@example.com", &service_id) - .commit(), - transaction_builder.add_random_key_verified_with_root(algorithm, "user1@example.com").commit(), - ] + for email in user_emails { + transactions.push( + transaction_builder + .create_account_with_random_key_signed(algorithm, &email, &service_id) + .commit() + ); + } + + // Add random key for first user + if let Some(first_user) = user_emails.first() { + transactions.push( + transaction_builder + .add_random_key_verified_with_root(algorithm, first_user) + .commit() + ); + } + + transactions }crates/common/src/transaction_builder.rs (2)
187-193: Consider explicit type conversion and proper error handling.Two suggestions for improvement:
- Make the type conversion explicit for better readability
- Handle errors properly instead of using
expectpub fn add_random_key( &mut self, algorithm: KeyAlgorithm, id: &str, signing_key: &SigningKey - ) -> UncommittedTransaction { - let random_key = SigningKey::new_with_algorithm(algorithm).expect("Failed to create random key").into(); + ) -> Result<UncommittedTransaction, Box<dyn std::error::Error>> { + let random_key = SigningKey::new_with_algorithm(algorithm) + .map_err(|e| format!("Failed to create random key: {}", e)) + .map(VerifyingKey::from)?; - self.add_key(id, random_key, signing_key) + Ok(self.add_key(id, random_key, signing_key))
Line range hint
1-391: Consider implementing a comprehensive error handling strategy.The current changes introduce multiple points where errors could occur during key generation. Consider:
- Creating a custom error type for the crate to handle key-related errors
- Implementing proper error propagation throughout the builder methods
- Documenting error scenarios in the public API
This would improve the reliability and usability of the library.
Example custom error type:
#[derive(Debug, thiserror::Error)] pub enum TransactionBuilderError { #[error("Failed to create key: {0}")] KeyCreationError(String), #[error("Invalid key algorithm: {0}")] InvalidAlgorithmError(String), // Add other error variants as needed }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
ci/run-validator.sh(3 hunks)crates/cli/src/main.rs(5 hunks)crates/common/src/transaction_builder.rs(6 hunks)crates/keys/src/lib.rs(1 hunks)crates/node_types/prover/src/prover/mod.rs(3 hunks)crates/node_types/prover/src/prover/tests.rs(7 hunks)crates/tests/src/lib.rs(7 hunks)crates/tree/src/tests.rs(11 hunks)justfile(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/node_types/prover/src/prover/mod.rs
🔇 Additional comments (6)
crates/tests/src/lib.rs (1)
58-58: Verify signing key creation errors.
“SigningKey::new_with_algorithm(algorithm)?” may return an error. Although you’re using the “?” operator to propagate it, consider whether you want more descriptive error handling or logs for diagnostic purposes.
ci/run-validator.sh (1)
171-174: Verify impact of sequential provisioning.
The change from parallel to sequential provisioning of bridge and light nodes could increase the total provisioning time.
Also applies to: 212-212
✅ Verification successful
Based on the search results, I can now make a determination:
Sequential provisioning is safe and intentional.
The code shows that both provision_bridge_nodes and provision_light_nodes functions wait for the genesis block before proceeding (genesis_hash=$(wait_for_block 1)). This indicates that the sequential ordering is intentional to ensure proper initialization order, where bridge nodes are provisioned first followed by light nodes.
No timing-related errors or race conditions were found between bridge and light nodes. The sequential provisioning appears to be a deliberate design choice for initialization order rather than a performance optimization.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if there are any timing-sensitive operations or dependencies between bridge and light nodes
# Look for timing dependencies in the codebase
rg -A 5 "provision.*node.*wait|sleep|timeout"
# Check for any documented timing requirements
fd -t f "README|CONTRIBUTING" -X grep -l "provision"
Length of output: 103304
Script:
#!/bin/bash
# Check for any timing dependencies or ordering requirements between bridge and light nodes
# Look for any timing-related code around provisioning
rg -A 5 "provision_bridge|provision_light"
# Check for any error handling or dependencies between the nodes
rg -A 5 "bridge.*node.*error|light.*node.*error"
# Look for any documentation about provisioning order
fd -t f "README|CONTRIBUTING|CHANGELOG" -X grep -i "provision.*order"
Length of output: 66632
crates/cli/src/main.rs (1)
40-51: Verify thread safety of key validation and creation.
The key validation and creation process involves multiple steps that could be affected by concurrent access.
Also applies to: 76-86, 132-162
✅ Verification successful
Let me generate additional scripts to check for thread safety aspects more thoroughly.
Based on the results, I can now generate the final response:
Key validation and creation are thread-safe by design
The key validation and creation process is stateless and does not share any mutable state. The code uses Arc only for thread-safe reference counting of immutable data, particularly for the node types and DA layer implementations. The key validation functions themselves don't use any shared state or synchronization primitives.
- Key validation occurs during node initialization before any concurrent operations begin
- The
VerifyingKeycreation is a pure function that operates only on its input parameters - No static/global state or synchronization primitives (Mutex/RwLock) are used in the key handling code
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential race conditions in key handling
# Look for concurrent access patterns
rg -A 5 "async|tokio::spawn|Arc::new|Mutex|RwLock"
# Check for any documented thread safety requirements
ast-grep --pattern 'struct $name {
$$$
$field: Arc<$_>
$$$
}'
Length of output: 65861
Script:
#!/bin/bash
# Check for concurrent access patterns in key validation and creation
# Look for key validation and creation functions
ast-grep --pattern 'fn validate_algorithm($$$) {
$$$
}'
# Look for key creation functions
ast-grep --pattern 'fn from_algorithm_and_bytes($$$) {
$$$
}'
# Check for any shared state or static variables
rg -A 5 "static|lazy_static|thread_local|Mutex|RwLock|Arc" crates/cli/src/
# Check for any async functions that might interact with key validation
rg -A 5 "async fn" crates/cli/src/
Length of output: 6443
crates/node_types/prover/src/prover/tests.rs (2)
209-228: LGTM! Well-designed test generation macro.
The macro effectively generates tests for each supported algorithm while maintaining readability and reducing code duplication.
230-236: Verify test coverage across algorithms.
Let's ensure that all test cases provide adequate coverage for each algorithm's specific behaviors.
✅ Verification successful
Let me generate additional verification scripts to check the test coverage across algorithms.
The previous search didn't provide enough context about the test coverage. These additional scripts will help us:
- Find algorithm-specific test cases and assertions in test files
- Identify the KeyAlgorithm enum variants that need coverage
- Locate test helper functions used by the test macros
This will give us a better picture of the test coverage across different algorithms.
Let me try searching in the correct directory structure.
The previous search failed because we were looking in the wrong directory. These new scripts will:
- Search for algorithm-specific tests in the crates directory
- Find the KeyAlgorithm enum definition
- Look for the test helper functions and macro usages mentioned in the review snippet
This should give us better insight into the test coverage.
Test coverage is comprehensive across all supported key algorithms.
The codebase demonstrates thorough test coverage across all supported key algorithms (Ed25519, Secp256k1, and Secp256r1) through:
- A
generate_algorithm_tests!macro that automatically generates test variants for each algorithm - Each test function is parameterized with
KeyAlgorithmand tests algorithm-specific behaviors - Core functionality tests covering:
- Basic operations (insert, update, get)
- Error cases (invalid transactions, nonexistent services)
- State management (persistence, sync)
- Block execution and epoch finalization
The test suite ensures that each algorithm implementation is verified against the same test cases, maintaining consistent behavior across all supported algorithms.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check test coverage for algorithm-specific code paths
# Look for algorithm-specific code paths
ast-grep --pattern 'match $var {
KeyAlgorithm::$_ => $$$
}'
# Check for any algorithm-specific test assertions
rg -A 5 "assert.*algorithm|match.*algorithm"
Length of output: 131013
Script:
#!/bin/bash
# Look for algorithm-specific test cases and assertions in test files
rg -g "*.rs" -A 5 "test.*algorithm|assert.*algorithm|KeyAlgorithm::" tests/
# Find algorithm enum definition and variants
ast-grep --pattern 'enum KeyAlgorithm {
$$$
}'
# Find test helper functions that may be used by the test macros
ast-grep --pattern 'fn test_$_($_, $_) {
$$$
}'
Length of output: 227
Script:
#!/bin/bash
# Look for algorithm-specific test cases and assertions
rg -g "*.rs" -A 5 "test.*algorithm|assert.*algorithm|KeyAlgorithm::" crates/
# Find algorithm enum definition and variants
ast-grep --pattern 'enum KeyAlgorithm {
$$$
}'
# Find test helper functions that may be used by the test macros
rg -g "*.rs" -A 5 "generate_algorithm_tests|test_validate_and_queue_update|test_process_transactions|test_execute_block|test_finalize_new_epoch|test_restart_sync_from_scratch|test_load_persisted_state" crates/
Length of output: 29933
crates/common/src/transaction_builder.rs (1)
9-9: LGTM: Import changes are correct.
The addition of KeyAlgorithm import is appropriate for the new functionality.
| let prover_vk = VerifyingKey::from_algorithm_and_bytes( | ||
| KeyAlgorithm::from_str(verifying_key_algorithm) | ||
| .map_err(|e| std::io::Error::new( | ||
| std::io::ErrorKind::InvalidData, | ||
| format!("Failed to create verifying key: {}", e) | ||
| ))?, | ||
| config.verifying_key.unwrap().as_bytes(), | ||
| ).map_err(|e| std::io::Error::new( | ||
| std::io::ErrorKind::InvalidData, format!("invalid prover verifying key: {}", e), | ||
| ))?; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Extract common key creation logic to reduce duplication.
The pattern of creating keys from algorithm and bytes is repeated multiple times with similar error handling.
Create a helper function:
fn create_key_from_algorithm<T>(
algorithm: &str,
bytes: &[u8],
creator: impl FnOnce(KeyAlgorithm, &[u8]) -> Result<T, Box<dyn std::error::Error>>,
) -> std::io::Result<T> {
let key_algorithm = KeyAlgorithm::from_str(algorithm)
.map_err(|e| std::io::Error::new(
std::io::ErrorKind::InvalidData,
format!("Failed to create key: {}", e)
))?;
creator(key_algorithm, bytes)
.map_err(|e| std::io::Error::new(
std::io::ErrorKind::InvalidData,
format!("Invalid key: {}", e)
))
}Also applies to: 78-85, 134-141, 151-162
| return Err(std::io::Error::new(std::io::ErrorKind::InvalidInput, "verifying key algorithm is required")); | ||
| } | ||
|
|
||
| if !SUPPORTED_ALGORITHMS.contains(&KeyAlgorithm::from_str(algorithm).unwrap()) { |
There was a problem hiding this comment.
Replace unwrap with proper error handling in validate_algorithm.
Using unwrap() on KeyAlgorithm::from_str could panic if the algorithm string is malformed.
- if !SUPPORTED_ALGORITHMS.contains(&KeyAlgorithm::from_str(algorithm).unwrap()) {
+ if let Ok(key_algorithm) = KeyAlgorithm::from_str(algorithm) {
+ if !SUPPORTED_ALGORITHMS.contains(&key_algorithm) {
+ return Err(std::io::Error::new(std::io::ErrorKind::InvalidInput, "unsupported verifying key algorithm"));
+ }
+ } else {
+ return Err(std::io::Error::new(std::io::ErrorKind::InvalidInput, "invalid verifying key algorithm format"));
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if !SUPPORTED_ALGORITHMS.contains(&KeyAlgorithm::from_str(algorithm).unwrap()) { | |
| if let Ok(key_algorithm) = KeyAlgorithm::from_str(algorithm) { | |
| if !SUPPORTED_ALGORITHMS.contains(&key_algorithm) { | |
| return Err(std::io::Error::new(std::io::ErrorKind::InvalidInput, "unsupported verifying key algorithm")); | |
| } | |
| } else { | |
| return Err(std::io::Error::new(std::io::ErrorKind::InvalidInput, "invalid verifying key algorithm format")); | |
| } |
| pub fn register_service_with_random_keys( | ||
| &mut self, | ||
| algorithm: KeyAlgorithm, | ||
| id: &str, | ||
| ) -> UncommittedTransaction { | ||
| let random_service_challenge_key = SigningKey::new_with_algorithm(algorithm).expect("Failed to create challenge key"); | ||
| let random_service_signing_key = SigningKey::new_with_algorithm(algorithm).expect("Failed to create signing key"); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider implementing proper error handling instead of using expect.
The current implementation uses expect which could cause panic in a library context. Consider propagating errors instead:
- pub fn register_service_with_random_keys(
- &mut self,
- algorithm: KeyAlgorithm,
- id: &str,
- ) -> UncommittedTransaction {
- let random_service_challenge_key = SigningKey::new_with_algorithm(algorithm).expect("Failed to create challenge key");
- let random_service_signing_key = SigningKey::new_with_algorithm(algorithm).expect("Failed to create signing key");
+ pub fn register_service_with_random_keys(
+ &mut self,
+ algorithm: KeyAlgorithm,
+ id: &str,
+ ) -> Result<UncommittedTransaction, Box<dyn std::error::Error>> {
+ let random_service_challenge_key = SigningKey::new_with_algorithm(algorithm)
+ .map_err(|e| format!("Failed to create challenge key: {}", e))?;
+ let random_service_signing_key = SigningKey::new_with_algorithm(algorithm)
+ .map_err(|e| format!("Failed to create signing key: {}", e))?;
+ Ok(self.register_service(id, random_service_challenge_key, random_service_signing_key))Committable suggestion skipped: line range outside the PR's diff.
Signed-off-by: Smuu <18609909+Smuu@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
crates/keys/src/tests.rs (2)
10-35: Consider parameterizing the tests to reduce code duplication.While the test logic is correct, the code is repetitive for each curve type. Consider using a test macro or parameterized tests to make the code more maintainable.
Example refactor using a macro:
macro_rules! test_verifying_key_reparsing { ($name:ident, $constructor:expr) => { #[test] fn $name() { let verifying_key = $constructor.verifying_key(); let re_parsed_verifying_key = VerifyingKey::from_algorithm_and_bytes( verifying_key.algorithm(), &verifying_key.to_bytes(), ) .unwrap(); assert_eq!(re_parsed_verifying_key, verifying_key); } }; } test_verifying_key_reparsing!(test_ed25519_verifying_key, SigningKey::new_ed25519()); test_verifying_key_reparsing!(test_secp256k1_verifying_key, SigningKey::new_secp256k1()); test_verifying_key_reparsing!(test_secp256r1_verifying_key, SigningKey::new_secp256r1());
1-140: Test coverage aligns with PR objectives but could be more comprehensive.The tests successfully verify that different curves (Ed25519, Secp256k1, Secp256r1) can be used interchangeably, which aligns with the PR objective of verifying if prover and full node can use different curves. However, consider:
- Adding integration tests that specifically simulate prover and full node interaction with different curve combinations
- Adding property-based tests to verify key compatibility across a wider range of scenarios
- Adding performance benchmarks to compare different curve combinations
Would you like help implementing any of these suggestions?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
crates/keys/src/tests.rs(1 hunks)crates/tests/Cargo.toml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/tests/Cargo.toml
🔇 Additional comments (2)
crates/keys/src/tests.rs (2)
1-8: LGTM: Imports are appropriate and secure.
The module correctly imports all necessary cryptographic libraries and uses OsRng for secure random number generation.
93-139: 🛠️ Refactor suggestion
Add more error cases for string parsing.
While the invalid length test is good, consider adding tests for:
- Invalid base64 strings
- Valid base64 but invalid key data
- Empty strings
- Very long strings
Also, verify that the base64 implementation is using a secure, constant-time comparison to prevent timing attacks.
✅ Verification successful
Let me check the base64 implementation details to verify the security concerns.
Let me check the actual base64 crate being used to verify its security properties.
Add more error test cases, but no security concerns with base64
The base64 implementation uses the standard Rust base64 crate (v0.22.0) which is well-maintained and secure by default. The constant-time concern is not applicable here as:
- The base64 crate uses constant-time operations internally
- Base64 encoding/decoding doesn't typically need timing attack protection as the encoded data is public
Agree with adding these test cases:
- Invalid base64 strings (e.g., non-base64 characters)
- Valid base64 but invalid key data (e.g., all zeros)
- Empty strings
- Very long strings (>44 chars which is the base64 length of 32 bytes)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the base64 implementation uses constant-time comparison
rg -A 5 "impl.*ToBase64|impl.*FromBase64"
Length of output: 1814
Script:
#!/bin/bash
# Check the base64 implementation details
cat crates/serde/src/base64.rs
Length of output: 1156
Script:
#!/bin/bash
# Check Cargo.toml for base64 dependency version
rg "base64.*=" -A 2 "Cargo.toml"
Length of output: 82
| #[test] | ||
| fn test_reparsed_signing_keys_are_equal_to_original() { | ||
| let signing_key_ed25519 = SigningKey::new_ed25519(); | ||
| let re_parsed_signing_key = SigningKey::from_algorithm_and_bytes( | ||
| signing_key_ed25519.algorithm(), | ||
| &signing_key_ed25519.to_bytes(), | ||
| ) | ||
| .unwrap(); | ||
| assert_eq!(re_parsed_signing_key, signing_key_ed25519); | ||
|
|
||
| let signing_key_secp256k1 = SigningKey::new_secp256k1(); | ||
| let re_parsed_signing_key = SigningKey::from_algorithm_and_bytes( | ||
| signing_key_secp256k1.algorithm(), | ||
| &signing_key_secp256k1.to_bytes(), | ||
| ) | ||
| .unwrap(); | ||
| assert_eq!(re_parsed_signing_key, signing_key_secp256k1); | ||
|
|
||
| let signing_key_secp256r1 = SigningKey::new_secp256r1(); | ||
| let re_parsed_signing_key = SigningKey::from_algorithm_and_bytes( | ||
| signing_key_secp256r1.algorithm(), | ||
| &signing_key_secp256r1.to_bytes(), | ||
| ) | ||
| .unwrap(); | ||
| assert_eq!(re_parsed_signing_key, signing_key_secp256r1); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add edge cases and consider parameterizing the tests.
While the happy path is well tested, consider:
- Adding tests for invalid key lengths
- Testing malformed key data
- Using parameterized tests to reduce code duplication (similar to the previous suggestion)
| #[test] | ||
| fn test_reparsed_signatures_are_equal_to_original() { | ||
| let message = b"test message"; | ||
|
|
||
| let signature_ed25519 = SigningKey::new_ed25519().sign(message); | ||
| let re_parsed_signature = Signature::from_algorithm_and_bytes( | ||
| signature_ed25519.algorithm(), | ||
| &signature_ed25519.to_bytes(), | ||
| ) | ||
| .unwrap(); | ||
| assert_eq!(re_parsed_signature, signature_ed25519); | ||
|
|
||
| let signature_secp256k1 = SigningKey::new_secp256k1().sign(message); | ||
| let re_parsed_signature = Signature::from_algorithm_and_bytes( | ||
| signature_secp256k1.algorithm(), | ||
| &signature_secp256k1.to_bytes(), | ||
| ) | ||
| .unwrap(); | ||
| assert_eq!(re_parsed_signature, signature_secp256k1); | ||
|
|
||
| let signature_secp256r1 = SigningKey::new_secp256r1().sign(message); | ||
| let re_parsed_signature = Signature::from_algorithm_and_bytes( | ||
| signature_secp256r1.algorithm(), | ||
| &signature_secp256r1.to_bytes(), | ||
| ) | ||
| .unwrap(); | ||
| assert_eq!(re_parsed_signature, signature_secp256r1); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance signature tests with additional test cases.
Consider adding tests for:
- Empty messages
- Large messages (e.g., > 1MB)
- Invalid signatures (to verify proper rejection)
- Modified messages (to verify signature mismatch detection)
Example test case for invalid signatures:
#[test]
fn test_signature_verification_fails_with_modified_message() {
let original_message = b"test message";
let modified_message = b"test message modified";
let signing_key = SigningKey::new_ed25519();
let signature = signing_key.sign(original_message);
assert!(!signing_key.verifying_key().verify(modified_message, &signature));
}
Questions:
Closes #135
Summary by CodeRabbit
New Features
VerifyingKeyand improved error handling in signature-related methods.Bug Fixes
Tests