feat(storage): InMemoryDatabase for tests #133
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Warning Rate limit exceeded@distractedm1nd has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 6 minutes and 17 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe changes introduce significant modifications across various files, enhancing functionalities related to hashchains, operations, and service registration. A new in-memory database structure is established, and workspace dependencies are updated in the Changes
Possibly related PRs
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: 25
🧹 Outside diff range and nitpick comments (17)
crates/nova/src/utils.rs (1)
166-176: Significant changes tocreate_ppfunction structureThe function now includes service registration before account creation, which aligns with the PR objective of implementing an InMemoryDatabase for tests. This change adds complexity to the test setup process.
Consider the following:
- Document the reason for this change in a comment to help future maintainers understand the context.
- Evaluate if this change affects any existing tests or functionalities that depend on the
create_ppfunction.crates/common/src/test_utils.rs (1)
33-33: Consider renaming theregistrationfield for clarityThe
registrationfield in theServicestruct holds aTestAccount. Renaming it to something more descriptive, likeregistration_account, could improve code readability.crates/common/src/hashchain.rs (1)
146-146: Address the TODO regardingRegisterServicepermissioning.The TODO comment indicates that
RegisterServiceshould not be permissionless until state bloat metrics are available. Implementing permission checks now can prevent potential misuse and align with security best practices.Would you like assistance in implementing the necessary permissioning logic or should I open a GitHub issue to track this task?
crates/prism/src/node_types/sequencer.rs (11)
Line range hint
320-358: Improve Error Handling and Consistency inprove_epochMethodIn the
prove_epochmethod, consider the following improvements:
Error Handling: Currently, if the proof generation or verification fails, the error is propagated up the call stack. It might be beneficial to add context to these errors for better debugging.
Consistency in Feature Flags: The use of
#[cfg(not(feature = "groth16"))]and#[cfg(feature = "groth16")]could be condensed for clarity using anifstatement inside a single block, although this depends on whether conditional compilation is necessary here.Code Duplication: The code for proof generation is duplicated in both conditional blocks. You could refactor this to eliminate duplication.
Proposed refactored code:
async fn prove_epoch( &self, height: u64, prev_commitment: Digest, new_commitment: Digest, proofs: Vec<Proof>, ) -> Result<FinalizedEpoch> { let batch = Batch { prev_root: prev_commitment, new_root: new_commitment, proofs, }; let mut stdin = SP1Stdin::new(); stdin.write(&batch); let client = self.prover_client.read().await; info!("generating proof for epoch height {}", height); let proof = { let prover = client.prove(&self.proving_key, stdin); #[cfg(feature = "groth16")] { prover.groth16().run()? } #[cfg(not(feature = "groth16"))] { prover.run()? } }; info!("successfully generated proof for epoch height {}", height); client.verify(&proof, &self.verifying_key)?; info!("verified proof for epoch height {}", height); let mut epoch_json = FinalizedEpoch { height, prev_commitment, current_commitment: new_commitment, proof, signature: None, }; epoch_json.insert_signature(&self.key); Ok(epoch_json) }
360-362: Simplify Methodget_commitmentThe method
get_commitmentcan be simplified by removing unnecessary context wrapping if not needed.Simplified code:
pub async fn get_commitment(&self) -> Result<Digest> { let tree = self.tree.read().await; tree.get_commitment() }
Line range hint
365-371: Avoid NestedResultin Return Type ofget_hashchainThe return type
Result<Result<Hashchain, NonMembershipProof>>is nested, which can be confusing. Consider flattening theResulttypes for better clarity.Rewritten method signature:
pub async fn get_hashchain( &self, id: &str, ) -> Result<Option<Hashchain>> { let tree = self.tree.read().await; let hashed_id = Digest::hash(id); let key_hash = KeyHash::with::<Hasher>(hashed_id); match tree.get(key_hash) { Ok(hashchain) => Ok(Some(hashchain)), Err(NonMembershipProof) => Ok(None), Err(e) => Err(e), } }Additionally, change the parameter from
&Stringto&strto follow Rust's best practices for borrowing strings.
Line range hint
393-399: Add Validation forRegisterServiceandCreateAccountOperationsIn the
validate_and_queue_updatemethod, the match arms forOperation::RegisterServiceandOperation::CreateAccountare empty, meaning no validation is performed. Consider adding validation logic to ensure these operations are valid before queuing.Suggested addition:
match incoming_operation { Operation::RegisterService(_) | Operation::CreateAccount(_) => { // Perform necessary validation here incoming_operation.verify_signature()?; }, Operation::AddKey(_) | Operation::RevokeKey(_) => { // Existing validation logic } };This ensures that signatures and other critical aspects of all operation types are validated.
418-420: Organize Imports According to ConventionGrouping imports logically improves readability. Consider grouping the
usestatements by their crates and alphabetically within each group.Organized imports:
use keystore_rs::create_signing_key; use prism_common::{operation::PublicKey, test_utils::create_mock_signing_key}; use prism_da::memory::InMemoryDataAvailabilityLayer; use prism_storage::inmemory::InMemoryDatabase;
437-439: Remove Redundant Clones in Test CodeIn the test
test_validate_and_queue_update, theservice_key.clone()may be unnecessary ifservice_keyimplementsCopyor is not used after being moved.Simplified code:
let op = Operation::new_register_service("service_id".to_string(), service_key.into());
447-451: Avoid Unnecessary Cloning ofsequencerChaining multiple
.clone()calls onsequenceris unnecessary ifsequencerimplementsCloneand is not moved. Consider borrowing instead.Simplified code:
sequencer .validate_and_queue_update(&op) .await .unwrap();
462-464: Remove Redundant Cloning ofsigning_keyThe variable
original_pubkeycan be obtained without cloningsigning_keyif it's not used afterward.Simplified code:
let original_pubkey = signing_key.into(); let service_key = create_mock_signing_key();
488-492: Optimize Key Conversion without CloningWhen converting
new_keyintopubkey, cloning might be unnecessary.Simplified code:
let pubkey = new_key.verifying_key().into(); let add_key_op = Operation::new_add_key("[email protected]".to_string(), pubkey, &signing_key, 0) .unwrap();
560-561: Avoid Unnecessary Conversion in Key AssignmentIn test code, avoid unnecessary
.into()calls if the types align without conversion.Simplified code:
let new_key = create_mock_signing_key(); let service_key = create_mock_signing_key();
599-601: Consistent Spacing Between Test SectionsMaintain consistent spacing to improve readability in test functions.
Add a newline after variable declarations:
let service_key = create_mock_signing_key(); let operations = vec![ // operations ];crates/common/src/tree.rs (3)
Line range hint
93-97: Avoid usingassert!in production code within theFromimplementationThe use of
assert!(N <= 32, "Input array must not exceed 32 bytes");in theFrom<[u8; N]> for Digestimplementation can cause a panic at runtime if the condition is not met. It is advisable to handle this scenario without causing a panic, perhaps by returning aResultor adjusting the implementation to safely handle arrays larger than 32 bytes.Consider modifying the implementation to return a
Digestwithout panicking:impl<const N: usize> From<[u8; N]> for Digest { fn from(value: [u8; N]) -> Self { let mut digest = [0u8; 32]; let len = N.min(32); digest[..len].copy_from_slice(&value[..len]); Self(digest) } }
381-382: Improve error message clarity when service hashchain retrieval failsThe error message "Failed to get hashchain for service ID {}" may not provide sufficient context if the service does not exist. Consider specifying whether the service hashchain does not exist or if there was an error retrieving it.
736-740: Optimize logging output intest_batch_writingThe test includes multiple
println!statements for debugging purposes. Consider using a logger or removing these statements if they are no longer needed to keep the test output clean.
📜 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 (12)
- crates/common/Cargo.toml (1 hunks)
- crates/common/src/hashchain.rs (7 hunks)
- crates/common/src/operation.rs (8 hunks)
- crates/common/src/test_utils.rs (9 hunks)
- crates/common/src/tree.rs (11 hunks)
- crates/nova/src/utils.rs (1 hunks)
- crates/prism/src/da/mod.rs (0 hunks)
- crates/prism/src/main.rs (2 hunks)
- crates/prism/src/node_types/sequencer.rs (6 hunks)
- crates/prism/tests/integration_tests.rs (2 hunks)
- crates/storage/src/inmemory.rs (1 hunks)
- crates/storage/src/lib.rs (1 hunks)
💤 Files not reviewed due to no reviewable changes (1)
- crates/prism/src/da/mod.rs
🔇 Additional comments (21)
crates/storage/src/lib.rs (1)
2-2: LGTM! Consider re-exporting the InMemoryDatabase.The addition of the
inmemorymodule aligns well with the PR objectives and expands the available storage options without modifying existing functionality. This is a good approach for introducing an in-memory database for testing purposes.Consider re-exporting the
InMemoryDatabasestruct (assuming it exists) similar to howDatabaseandRedisConnectionare re-exported. This would provide a consistent API for users of this crate. You could add something like:pub use crate::inmemory::InMemoryDatabase;to the existing re-exports.
Let's verify the contents of the new
inmemorymodule:This will help us understand the structure of the
InMemoryDatabaseand confirm if a re-export would be beneficial.✅ Verification successful
LGTM! Re-exporting
InMemoryDatabasewould enhance the crate's API consistency.The addition of the
inmemorymodule effectively introduces an in-memory storage option for testing, aligning with the PR objectives. Re-exporting theInMemoryDatabasestruct will provide a consistent and convenient API for users, similar to existing re-exports likeDatabaseandRedisConnection.You can add the following line to
crates/storage/src/lib.rs:pub use crate::inmemory::InMemoryDatabase;🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check the contents of the inmemory module # Test: Verify the existence and contents of the inmemory module cat crates/storage/src/inmemory.rsLength of output: 3660
crates/common/Cargo.toml (1)
21-21: LGTM! Verify base64 crate usage.The addition of the base64 crate as a workspace dependency is appropriate and consistent with the existing dependency declarations. This addition likely supports the implementation of the InMemoryDatabase for tests.
To ensure the base64 crate is being utilized effectively, please run the following script to check its usage across the codebase:
This will help confirm that the newly added dependency is being used appropriately in the project.
crates/prism/src/main.rs (1)
10-11: LGTM: New import added for PublicKeyThe addition of
use prism_common::operation::PublicKey;is appropriate given the changes in the main function for handling the verifying key.crates/prism/tests/integration_tests.rs (2)
28-28: Please clarify the SigningKey conversion.The change from using the verifying key to directly using the signing key for the
valuefield simplifies the code. However, it's crucial to ensure that this modification doesn't compromise security or alter the intended functionality.Could you please clarify:
- Is
SigningKeythe correct type for thevaluefield inCreateAccountArgs?- Does the
into()method correctly convertSigningKeyto the expected type?To verify the correctness of this change, please run the following script:
#!/bin/bash # Description: Verify the type compatibility and conversion of SigningKey # Test: Check if SigningKey implements the correct trait for conversion ast-grep --lang rust --pattern 'impl From<SigningKey> for $type { $$$ }' # Test: Verify usage of SigningKey in CreateAccountArgs rg --type rust -A 5 'struct CreateAccountArgs'This will help confirm that the
SigningKeyis correctly implemented and used in theCreateAccountArgsstruct.
20-20: LGTM: InMemoryDatabase implementation looks good.The change from Redis to an in-memory database aligns with the PR objective and should improve test performance. This modification reduces external dependencies, making the tests more isolated and potentially faster.
To ensure this change doesn't affect test behavior, please run the following script:
This will help confirm that the switch to InMemoryDatabase hasn't introduced any regressions in the test suite.
Also applies to: 55-55
crates/nova/src/utils.rs (1)
Line range hint
1-576: Recommend comprehensive test reviewWhile the changes are localized to the
create_ppfunction, they introduce a new structure with service registration that might affect how the utility functions in this file are used in tests.To ensure compatibility with existing test setups:
- Review all tests that use the
create_ppfunction:#!/bin/bash # Search for tests using create_pp rg "create_pp\(" --type rust --glob "**/tests/**"
- Verify if any tests need to be updated to accommodate the new service registration step:
#!/bin/bash # Search for test setup code that might need updating rg "TestTreeState::default\(\)" --type rust --glob "**/tests/**"crates/storage/src/inmemory.rs (1)
75-88:⚠️ Potential issueMaintain consistent lock acquisition order to prevent deadlocks
In
write_node_batch, you acquire locks onnodesandvaluesin a specific order. If other methods acquire these locks in a different order, it could lead to a deadlock.Ensure that all methods in your codebase acquire locks in the same order. For example, always lock
nodesbeforevalues. Review other parts of the code to confirm consistent locking order.Run the following script to check for consistent locking order:
This script checks for instances where
nodesis locked, and within the next 5 lines,valuesis also locked. Ensure that no methods lockvaluesbeforenodes.✅ Verification successful
Lock acquisition order is consistent and prevents potential deadlocks.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that locks are always acquired in the same order. # Search for occurrences of locking `nodes` and `values` rg --type rust 'self\.nodes\.lock\(\)' -A 5 | grep 'self\.values\.lock\('Length of output: 99
Script:
#!/bin/bash # Description: Verify that locks are always acquired in the same order. # Search for occurrences of locking `nodes` followed by `values` rg --type rust 'self\.nodes\.lock\(\)' -A 5 | grep -E 'self\.values\.lock\('Length of output: 239
Script:
#!/bin/bash # Description: Verify that no locks are acquired in the order `values` before `nodes` to prevent deadlocks. # Search for occurrences of locking `values` followed by `nodes` rg --type rust 'self\.values\.lock\(\)' -A 5 | grep -E 'self\.nodes\.lock\('Length of output: 78
crates/common/src/test_utils.rs (1)
28-35: Verify ownership and lifetimes in theServicestructThe
Servicestruct contains aregistrationfield of typeTestAccount, which in turn contains ownership of aHashchain. Ensure that the cloning and references are managed correctly to prevent ownership conflicts or lifetime issues.Run the following script to check for potential ownership and lifetime issues:
crates/common/src/hashchain.rs (5)
65-69: Implementation offrom_operationmethod is correct.The
from_operationmethod properly initializes a newHashchainand performs the providedOperation, adhering to a functional approach and improving code modularity.
71-88: Refactoredcreate_accountmethod enhances code clarity.Transforming
create_accountinto an associated function that returns aHashchainremoves the need for mutable state and aligns with functional programming best practices. This improves code readability and maintainability.
90-98: Addition ofregister_servicemethod improves functionality.The new
register_servicemethod encapsulates the service registration logic within theHashchain, promoting a cleaner and more modular code structure.
156-161: Inconsistent serialization in signature verification.In the
CreateAccountoperation (lines 156-161), the signature verification serializes the operation usingwithout_signature().without_challenge(), while inAddKeyandRevokeKeyoperations (lines 168-171), it uses onlywithout_signature(). Please verify that excluding the challenge inCreateAccountis intentional and that it does not compromise the integrity of the signature verification process.Also applies to: 168-171
284-286: Ensure the consistency of key hashing.The
get_keyhashmethod hashesself.idto produce theKeyHash. Verify thatself.idis intended to be used as the unique identifier for key hashing and that this approach is consistent with the rest of the system's design.crates/common/src/tree.rs (8)
545-548: Verify the testtest_insert_and_getfor proper account insertionIn the test
test_insert_and_get, it seems that the service account is not inserted before inserting the account, which might not reflect the actual logic where the service must exist prior to account creation.Ensure that the service is properly inserted before inserting the account. Add the following line before inserting the account:
tree_state.insert_account(service.registration.clone()).unwrap();
560-565: Clarify the test case intest_insert_for_nonexistent_service_failsIn this test, the service is registered but not inserted into the tree before attempting to insert an account. Ensure that this test correctly validates that account insertion fails when the service does not exist in the tree.
572-574: Potential issue with falsifying service signing key in testIn
test_insert_with_invalid_service_challenge_fails, the service signing key is replaced with a mock key to simulate an invalid challenge. Ensure that this approach accurately reflects an invalid service challenge and doesn't introduce unintended side effects.Review the implementation to confirm that replacing
service.skeffectively simulates an invalid service challenge without affecting other parts of the test.
595-597: Check error handling intest_insert_duplicate_keyThe test attempts to insert an account that already exists. Confirm that the error handling correctly identifies the duplicate key and that the appropriate error is returned.
628-632: Ensure service insertion intest_update_non_existing_keyIn
test_update_non_existing_key, the account is created but not inserted into the tree before attempting an update. This should result in an error, as expected. Verify that the test accurately checks the update failure due to a non-existing key.
655-659: Confirm the sequence of inserts and updates intest_multiple_inserts_and_updatesThe test performs multiple inserts and updates. Ensure that the service is inserted before accounts and that accounts are updated correctly. This helps validate the integrity of sequential operations.
682-686: Validate interleaved operations intest_interleaved_inserts_and_updatesThis test interleaves inserts and updates between two accounts. Verify that the operations maintain the correct state in the tree and that the final root hash matches the expected value.
718-724: Check root hash changes intest_root_hash_changesThe test checks that the root hash changes after inserting a new account. Ensure that the root hash before and after the insertion are indeed different, confirming that the tree is updating correctly.
distractedm1nd
force-pushed
the
inmemory_db
branch
from
2d525dd
to
95a5549
Compare
based on top of #130
Summary by CodeRabbit
Release Notes
New Features
InMemoryDatabasefor efficient in-memory data storage.Bug Fixes
Documentation
Refactor