refactor: Increase code reuse in hashchain and tree #146
Conversation
WalkthroughThe changes involve substantial modifications to the Changes
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: 4
🧹 Outside diff range and nitpick comments (1)
crates/common/src/hashchain.rs (1)
221-221: Replaceprintln!with a proper logging mechanism.Using
println!for logging is not ideal in production code. Consider using a logging crate likelogortracingfor better control over log levels and output destinations.Apply this diff to replace
println!withlog::debug!:- println!("Validating op {}", operation); + log::debug!("Validating operation: {:?}", operation);Make sure to include the logging crate in your dependencies and initialize the logger appropriately.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- crates/common/src/hashchain.rs (3 hunks)
🧰 Additional context used
🔇 Additional comments (1)
crates/common/src/hashchain.rs (1)
232-232: Ensure correct index usage inget_key_at_index.When retrieving the signing key with
get_key_at_index(args.signature.key_idx as usize), confirm thatkey_idxcorrectly corresponds to the desired entry. Off-by-one errors here could lead to signature verification failures or incorrect behavior.It's important to verify that
args.signature.key_idxaccurately reflects the position of the key in the hashchain. Consider adding checks or unit tests to validate this behavior.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
crates/common/src/hashchain.rs (1)
Line range hint
220-244: Refinements needed invalidate_operation_at_idxThe new
validate_operation_at_idxmethod is a good addition, but there are a few points to address:
Remove or replace the debug print statement:
- println!("Validating op {}", operation); + log::debug!("Validating operation: {}", operation);The
CreateAccountoperation check has been correctly updated to reference "CreateAccount" instead of "RegisterService". This resolves the issue mentioned in a previous review comment.Consider adding a check for the
RegisterServiceoperation to ensure it's only allowed as the first entry in an empty hashchain:Operation::RegisterService(_) => { - if idx > 0 { + if !self.entries.is_empty() { bail!("RegisterService operation must be the first entry"); } Ok(()) }These changes will improve the robustness and maintainability of the validation process.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- crates/common/src/hashchain.rs (3 hunks)
🧰 Additional context used
🔇 Additional comments (2)
crates/common/src/hashchain.rs (2)
108-125: Excellent refactoring ofverify_last_entrymethodThe refactoring of the
verify_last_entrymethod is a significant improvement:
- It uses a reverse iterator, which is more efficient for this use case.
- It handles edge cases (empty chain, single entry) explicitly.
- The logic for verifying the last entry against the second-to-last entry is clear and correct.
This refactoring enhances both the robustness and readability of the code.
211-218: Improved robustness invalidate_new_operationThe changes to
validate_new_operationare well-considered:
- The check for empty entries adds robustness to the validation process.
- The use of
validate_operation_at_idximproves code reusability and maintainability.These changes enhance the overall reliability of the operation validation process.
jns-ps
force-pushed
the
hashchain-code-reuse
branch
from
5af5344 to
10e4b30
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
crates/common/src/hashchain.rs (1)
108-125: Improved implementation ofverify_last_entryThe refactoring of
verify_last_entryis a significant improvement. It now handles different cases (empty, single entry, and multiple entries) more explicitly and efficiently. The use of a reverse iterator simplifies the logic.Consider extracting the validation logic for 2+ entries into a separate private method for improved readability:
fn validate_multiple_entries(&self, last_entry: &HashchainEntry, second_last_entry: &HashchainEntry, second_last_idx: usize) -> Result<()> { if last_entry.previous_hash != second_last_entry.hash { bail!("Previous hash mismatch for the last entry"); } self.validate_operation_at_idx(&last_entry.operation, second_last_idx) }Then, you can call this method in the main function:
if let Some((second_last_idx, second_last_entry)) = rev_iter.next() { self.validate_multiple_entries(last_entry, second_last_entry, second_last_idx) } else { // When there is only 1 item in the chain, validate insertion at idx 0 self.validate_operation_at_idx(&last_entry.operation, last_idx) }This separation of concerns could make the main function even more readable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- crates/common/src/hashchain.rs (3 hunks)
🧰 Additional context used
🔇 Additional comments (1)
crates/common/src/hashchain.rs (1)
168-178:⚠️ Potential issueOptimize
is_key_revoked_before_idxmethodThe new method provides a good abstraction for checking key revocation status, but there are two areas for improvement:
- The use of
skip(idx)is incorrect as it skips the entries we want to check.- The default return value of
truemight not be the desired behavior.Consider the following changes:
fn is_key_revoked_before_idx(&self, key: VerifyingKey, idx: usize) -> bool { self.iter() - .skip(idx) + .take(idx + 1) .rev() .find_map(|entry| match entry.operation.clone() { Operation::RevokeKey(args) if args.value == key => Some(true), Operation::AddKey(args) if args.value == key => Some(false), Operation::CreateAccount(args) if args.value == key => Some(false), _ => None, }) - .unwrap_or(true) + .unwrap_or(false) }These changes ensure that:
- All relevant entries up to
idxare checked.- A key is considered not revoked unless explicitly revoked by an operation.
jns-ps
force-pushed
the
hashchain-code-reuse
branch
2 times, most recently
from
0440bd6 to
aeebc35
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
crates/common/src/test_utils.rs (1)
Line range hint 266-269: Remove unnecessary clone of operation.
The operation is only used once in Hashchain::from_operation, so cloning it is unnecessary.
pub fn create_new_hashchain(id: &str, signing_key: &SigningKey, service: Service) -> Hashchain {
let op = Operation::new_create_account(id.to_string(), signing_key, service.id, &service.sk)
.unwrap();
- Hashchain::from_operation(op.clone()).unwrap()
+ Hashchain::from_operation(op).unwrap()
}crates/common/src/hashchain.rs (1)
207-211: Improve error message for RegisterService validation
The error message could be more informative by including the actual index where the operation was attempted.
Apply this change:
Operation::RegisterService(_) => {
if idx > 0 {
- bail!("RegisterService operation must be the first entry");
+ bail!("RegisterService operation must be the first entry, but was attempted at index {}", idx);
}
Ok(())
}crates/common/src/tree.rs (1)
Line range hint 477-481: Improve error message clarity for duplicate service registration.
The current error message "empty slot for ID" is confusing when indicating that a service already exists. Additionally, using DatabaseError::NotFoundError is semantically incorrect for this case.
-bail!(DatabaseError::NotFoundError(format!(
- "empty slot for ID {}",
- id
-)));
+bail!("Service with ID {} already exists", id);📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- crates/common/src/hashchain.rs (5 hunks)
- crates/common/src/test_utils.rs (1 hunks)
- crates/common/src/tree.rs (3 hunks)
🔇 Additional comments (3)
crates/common/src/test_utils.rs (1)
53-56: LGTM! Consistent with operation validation refactoring.
The change standardizes Hashchain creation by using from_operation with a register service operation, which aligns well with the PR's objective of reusing operation validation code.
crates/common/src/hashchain.rs (1)
219-224: 🛠️ Refactor suggestion
Optimize validation order in key operations
The key lookup is performed before checking if the key is revoked, which could be inefficient. Consider checking revocation status first to avoid unnecessary key lookups.
Apply this optimization:
Operation::AddKey(KeyOperationArgs { signature, .. })
| Operation::RevokeKey(KeyOperationArgs { signature, .. })
| Operation::AddData(AddDataArgs {
op_signature: signature,
..
}) => {
+ let signing_key = self.get_key_at_index(signature.key_idx)?;
+ if self.is_key_revoked_before_idx(signing_key.clone(), idx) {
+ bail!("The signing key is revoked");
+ }
- let signing_key = self.get_key_at_index(signature.key_idx)?;
- if self.is_key_revoked_before_idx(signing_key.clone(), idx) {
- bail!("The signing key is revoked");
- }
operation.verify_user_signature(signing_key.clone())
}Likely invalid or redundant comment.
crates/common/src/tree.rs (1)
Line range hint 590-824: Well-structured and comprehensive test coverage.
The test suite effectively covers:
- Happy path scenarios
- Error cases
- Edge cases
- Batch operations
- State transitions
jns-ps
force-pushed
the
hashchain-code-reuse
branch
from
aeebc35 to
77ac055
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
crates/common/src/hashchain.rs (1)
192-192: Consider enhancing CreateAccount error message for consistency.Other operations provide detailed error messages with context, but the CreateAccount operation uses a simple message. Consider making it consistent with other operations:
- bail!("CreateAccount operation must be the first entry"); + bail!( + "CreateAccount operation must be the first entry, but found {} existing entries", + self.entries.len() + );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
crates/common/src/hashchain.rs(3 hunks)crates/common/src/test_utils.rs(5 hunks)crates/common/src/tree.rs(10 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/common/src/test_utils.rs
- crates/common/src/tree.rs
🔇 Additional comments (2)
crates/common/src/hashchain.rs (2)
108-108: Default behavior in key revocation needs adjustment.
This issue was previously identified. The default return value of true could incorrectly mark keys as revoked when they haven't been explicitly revoked.
146-149: LGTM! Improved error messages with detailed context.
The enhanced error messages now provide better context by including the actual and expected hash values, making debugging easier.
Also applies to: 155-161, 173-179
jns-ps
changed the title
jns-ps
force-pushed
the
hashchain-code-reuse
branch
from
77ac055 to
5c55fef
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (2)
crates/common/src/hashchain.rs (1)
Line range hint
99-108: Consider adjusting the default behavior inis_key_invalid.The method currently defaults to considering keys as invalid (returns
true) when no matching operation is found. This might be too restrictive, especially for new keys or empty chains.Consider changing the default to
falseto assume keys are valid unless explicitly invalidated:}) - .unwrap_or(true) + .unwrap_or(false)crates/common/src/test_utils.rs (1)
194-207: Improve error handling in random insert creation.While the operation-centric approach is good, there are a few improvements that could be made:
- The expect message on line 200 is phrased as a success message but is used for error handling.
- The error handling could be more descriptive by including the actual error.
Consider this improvement:
- .expect("Creating account operation should succeed"); + .expect("Failed to create account operation");Also consider adding the error details:
- let proof = state.tree.insert(key_hash, operation).expect("Insert should succeed"); + let proof = state.tree.insert(key_hash, operation) + .expect("Failed to insert into tree: key_hash={key_hash}");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
crates/common/src/hashchain.rs(4 hunks)crates/common/src/test_utils.rs(5 hunks)crates/common/src/tree.rs(10 hunks)
🔇 Additional comments (4)
crates/common/src/test_utils.rs (1)
55-58: LGTM: Service registration refactor looks good.
The change to use Operation::new_register_service and Hashchain::from_operation aligns well with the operation-centric design pattern.
Let's verify the consistency of this pattern across the codebase:
✅ Verification successful
Let me gather more context about the usage patterns to ensure completeness of the verification.
Service registration refactor is correctly implemented
The verification confirms that:
- The new operation-centric approach using
Operation::new_register_serviceis consistently used across the codebase - All
register_serviceoccurrences in test files are using the new pattern through test utilities - The
RegisterServiceoperation is properly integrated into the operation handling system with appropriate validation logic
The changes align with the operation-centric design pattern and maintain consistency throughout the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining direct service registration calls
# that might have been missed in the refactor
# Search for direct service registration patterns
rg -l "register_service|RegisterService" --type rust
# Search for the new operation-based pattern
ast-grep --pattern 'Operation::new_register_service'
Length of output: 723
Script:
#!/bin/bash
# Check the implementation and usage context of register_service
rg "register_service" -B 2 -A 2 --type rust
# Check RegisterService struct/enum definition and usage
rg "RegisterService" -B 2 -A 2 --type rust
Length of output: 15222
crates/common/src/tree.rs (3)
75-75: LGTM! Improved type safety in proof structures.
The change from raw values to domain-specific types (Operation) enhances type safety and makes the code more maintainable.
Also applies to: 101-102
237-238: LGTM! Debug logging restored.
The debug logging has been readded as requested in the previous review.
Line range hint 333-362: LGTM! Improved error handling and type safety.
The update implementation now properly handles errors and validates operations before applying them.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor