refactor(common): Extracting common fields from Operation #154
Conversation
WalkthroughThe pull request introduces significant changes to 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 (
|
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (7)
crates/common/src/keys.rs (2)
16-22: LGTM! Consider adding documentation.The
Signatureenum is well-structured and type-safe. It properly encapsulates both signature types and includes a sensible default.Consider adding documentation comments to describe:
- The purpose of each variant
- When the Placeholder variant should be used
- Example usage
+/// Represents a cryptographic signature supported by the system. #[derive(Clone, Serialize, Deserialize, Debug, PartialEq, Eq, Default)] pub enum Signature { + /// A secp256k1 ECDSA signature, commonly used in Bitcoin and Ethereum Secp256k1(Secp256k1Signature), + /// An Ed25519 signature, commonly used in Cosmos, OpenSSH, and GnuPG Ed25519(Ed25519Signature), + /// A placeholder signature, used when a real signature is not yet available #[default] Placeholder, }
42-61: LGTM! Consider enhancing error messages.The signature verification logic is robust and type-safe. Good use of pattern matching to ensure signature type compatibility.
Consider making the error messages more specific by including the actual and expected signature types:
- Err(anyhow!("Invalid signature type")) + Err(anyhow!("Invalid signature type: expected Ed25519, got {:?}", signature))- Err(anyhow!("Invalid signature type")) + Err(anyhow!("Invalid signature type: expected Secp256k1, got {:?}", signature))crates/common/src/test_utils.rs (1)
55-62: LGTM with minor suggestions for error handling.The changes look good and align with the refactoring of the
Operationstructure. Since this is test code, usingunwrap()is generally acceptable. However, consider usingexpect()with descriptive messages to make test failures more informative.Apply this diff to improve error messages:
let hashchain = Hashchain::from_operation( Operation::new_register_service( service_id.clone(), ServiceChallenge::from(service_key.clone()), &service_key, - ).unwrap(), + ).expect("Failed to create register service operation"), - ).unwrap(); + ).expect("Failed to create hashchain from operation");crates/common/src/test_ops.rs (1)
90-95: Consider optimizing key handling and improving error handling.The current implementation clones the signing key and then passes it again as a reference. Consider either:
- Passing the key as a reference in both places to avoid unnecessary cloning, or
- Document why the clone is necessary if it serves a specific purpose.
Additionally, consider using
expectwith a descriptive message instead ofunwrapto provide better context during test failures.Here's a suggested improvement:
let op = Operation::new_register_service( id.to_string(), - service_signing_key.clone().into(), + (&service_signing_key).into(), &service_signing_key, ) - .unwrap(); + .expect("Failed to create register service operation");crates/common/src/tree.rs (2)
Line range hint
261-275: Enhance error handling for service challenge verification.The service challenge verification could benefit from more descriptive error messages and better error context to aid in debugging.
let creation_gate = match &service_last_entry.operation.variant { OperationType::RegisterService { creation_gate, .. } => creation_gate, _ => { - bail!("Service hashchain's last entry was not a RegisterService operation") + bail!("Expected RegisterService operation for service {}, found {:?}", + service_id, + service_last_entry.operation.variant) } }; let ServiceChallenge::Signed(service_pubkey) = creation_gate; let ServiceChallengeInput::Signed(challenge_signature) = &challenge; - service_pubkey.verify_signature( + service_pubkey.verify_signature( &bincode::serialize(&operation.without_challenge())?, challenge_signature, - )?; + ).context("Failed to verify service challenge signature")?;
276-278: Enhance debug logging.The debug logging could be more descriptive by including additional context about the operation being performed.
- debug!("creating new hashchain for user ID {}", operation.id); + debug!("creating new hashchain for user ID {} with service {}", operation.id, service_id);crates/common/src/operation.rs (1)
193-193: Remove unneededreturnstatementIn Rust, the
returnkeyword is optional for the last expression in a function. Removing it can improve readability.Apply this diff to address the Clippy warning:
- return self.new_key.as_ref(); + self.new_key.as_ref()🧰 Tools
🪛 GitHub Check: clippy
[failure] 193-193:
unneededreturnstatement
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
crates/common/src/hashchain.rs(5 hunks)crates/common/src/keys.rs(3 hunks)crates/common/src/operation.rs(5 hunks)crates/common/src/test_ops.rs(1 hunks)crates/common/src/test_utils.rs(1 hunks)crates/common/src/tree.rs(4 hunks)crates/node_types/prover/src/prover/mod.rs(2 hunks)crates/tests/src/lib.rs(1 hunks)
🧰 Additional context used
📓 Learnings (1)
crates/common/src/hashchain.rs (1)
Learnt from: distractedm1nd
PR: deltadevsde/prism#146
File: crates/common/src/hashchain.rs:164-174
Timestamp: 2024-11-01T07:51:25.629Z
Learning: In `crates/common/src/hashchain.rs`, extracting duplicated key validation logic between different operation types is not feasible without refactoring the underlying enum structs to use the same type.
🪛 GitHub Check: clippy
crates/common/src/operation.rs
[failure] 193-193:
unneeded return statement
🔇 Additional comments (13)
crates/tests/src/lib.rs (1)
121-123: Review security implications of using private key directly
The change from using ServiceChallenge::Signed(service.clone().vk) to directly passing &service.sk represents a significant shift in the security model. While this simplifies the API, exposing private keys directly in operations could have security implications.
Let's verify the usage pattern across the codebase:
Consider:
- Whether passing private keys directly aligns with the security requirements
- If we should maintain an abstraction layer (like ServiceChallenge) to control how keys are used
- Adding documentation explaining the security considerations of this change
crates/common/src/keys.rs (2)
135-142: LGTM! Clean implementation of signature creation.
The method properly constructs the appropriate Signature variant while maintaining the existing signing logic. The implementation aligns well with the verification changes.
Line range hint 16-144: Verify operation creation with new signature handling.
The changes to signature handling look good, but we should verify that all operation creation code has been updated to work with the new Signature type.
✅ Verification successful
Let me gather more specific information about operation creation and signature handling.
Based on the comprehensive results, I can now provide the final response:
Operation creation code is properly updated for new signature handling
The verification shows that all operation creation methods consistently use the new Signature type:
- All
Operation::new_*methods useSignature::Placeholderas initial value insert_signature()method properly uses the new signature typeSignatureBundleusage is properly contained to value signatures inAddDataoperations- No legacy byte array signature handling was found
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for direct usage of byte arrays or Vec<u8> for signatures
# which might indicate missed updates
# Look for old-style signature handling
rg -g '*.rs' 'Vec<u8>' -A 5 -B 5 | grep -i 'sign'
# Look for operation creation
ast-grep --pattern 'Operation::new_$_($$$)'
Length of output: 1661
Script:
#!/bin/bash
# Look for Operation struct definition and its methods
ast-grep --pattern 'impl Operation {
$$$
}'
# Look for SignatureBundle usage
rg -g '*.rs' 'SignatureBundle' -A 3 -B 3
# Look for specific operation creation methods
ast-grep --pattern 'fn new_$_($$$) -> Operation'
Length of output: 20046
crates/common/src/test_utils.rs (1)
55-62: Verify test coverage for all operation types.
Since this is part of a larger refactoring of the Operation structure, let's ensure we have adequate test coverage for all operation types.
crates/common/src/test_ops.rs (1)
Line range hint 1-300: Consider expanding test coverage for the refactored Operation structure.
Given the significant refactoring of the Operation type from an enum to a struct with common fields, consider:
- Adding test cases that explicitly verify the new common fields
- Including negative test cases for invalid field combinations
- Adding test utilities for the new
OperationTypeenum
Let's verify the test coverage:
crates/node_types/prover/src/prover/mod.rs (2)
22-22: LGTM: Import change aligns with Operation type refactoring
The addition of OperationType to the imports is consistent with the broader refactoring of the Operation type structure.
457-462: Security: Verify if RegisterService and CreateAccount require hashchain validation
The empty match arms for RegisterService and CreateAccount operations bypass hashchain validation. Please verify if this is intentional and document the reasoning.
crates/common/src/tree.rs (2)
16-16: LGTM! Import changes align with operation type restructuring.
The updated imports correctly reflect the new operation handling structure, importing the necessary types for the refactored operation processing.
229-238: LGTM! Clean pattern matching on operation variants.
The pattern matching on operation variants is well-structured and handles the different operation types appropriately.
crates/common/src/hashchain.rs (2)
129-131: Operation ID consistency check looks good
The check ensuring that operation.id matches self.id correctly prevents inconsistent operations from being added to the hashchain.
103-117: is_key_invalid method correctly identifies key validity
The logic in the is_key_invalid method appropriately checks for key revocations by iterating in reverse and returns the expected boolean value.
crates/common/src/operation.rs (2)
13-29: Well-structured Operation and OperationType definitions
The refactoring of the Operation struct and the introduction of the OperationType enum enhance the clarity and maintainability of the code. The use of enums to represent operation variants is a good practice for type safety.
223-248: Handle potential mismatches in signature validation
In validate_signature, ensure that the operation's state during signature verification matches the state when it was signed. Modifications to the operation after signing, such as placeholder replacements, could cause verification to fail.
Confirm that the without_signature and without_challenge methods correctly prepare the operation for signature verification without altering the signed content.
Would you like assistance in reviewing the signature verification logic?
| let hc_response = self.get_hashchain(&incoming_operation.id).await?; | ||
|
|
||
| let Found(mut hc, _) = hc_response else { | ||
| bail!("Hashchain not found for id: {}", incoming_operation.id()) | ||
| bail!("Hashchain not found for id: {}", incoming_operation.id) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling and pattern matching
Consider these improvements:
- Make the error message more descriptive by including the operation type
- Use a more concise pattern matching approach
- let Found(mut hc, _) = hc_response else {
- bail!("Hashchain not found for id: {}", incoming_operation.id)
- };
+ let Found(mut hc, _) = hc_response.else(|| {
+ bail!(
+ "Hashchain not found for {} operation with id: {}",
+ incoming_operation.variant,
+ incoming_operation.id
+ )
+ })?;Committable suggestion skipped: line range outside the PR's diff.
| if matches!(self.get(key_hash)?, Found(_, _)) { | ||
| bail!(DatabaseError::NotFoundError(format!( | ||
| "Account already exists for ID {}", | ||
| id | ||
| operation.id | ||
| ))); |
There was a problem hiding this comment.
Fix incorrect error type for existing account.
The code uses DatabaseError::NotFoundError when an account already exists, which is semantically incorrect. Consider using a more appropriate error type that better reflects the actual error condition.
- bail!(DatabaseError::NotFoundError(format!(
+ bail!(DatabaseError::AlreadyExistsError(format!(
"Account already exists for ID {}",
operation.id
)));📝 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 matches!(self.get(key_hash)?, Found(_, _)) { | |
| bail!(DatabaseError::NotFoundError(format!( | |
| "Account already exists for ID {}", | |
| id | |
| operation.id | |
| ))); | |
| if matches!(self.get(key_hash)?, Found(_, _)) { | |
| bail!(DatabaseError::AlreadyExistsError(format!( | |
| "Account already exists for ID {}", | |
| operation.id | |
| ))); |
| let vk = match &operation.variant { | ||
| OperationType::CreateAccount { .. } | OperationType::RegisterService { .. } => { | ||
| if !self.entries.is_empty() { | ||
| bail!("CreateAccount or RegisterService operation must be the first entry"); | ||
| } | ||
|
|
||
| operation.verify_user_signature(verifying_key) | ||
| &operation.new_key.clone().unwrap() |
There was a problem hiding this comment.
Avoid using unwrap() on new_key to prevent potential panics
In line 167~, the code uses operation.new_key.clone().unwrap(). If new_key is None, this will cause a panic. Since new_key is expected for CreateAccount and RegisterService operations, consider handling the None case explicitly to improve code safety.
Apply this diff to handle the None case:
- &operation.new_key.clone().unwrap()
+ if let Some(new_key) = &operation.new_key {
+ &new_key.clone()
+ } else {
+ bail!("Operation is missing required `new_key`")
+ }📝 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.
| let vk = match &operation.variant { | |
| OperationType::CreateAccount { .. } | OperationType::RegisterService { .. } => { | |
| if !self.entries.is_empty() { | |
| bail!("CreateAccount or RegisterService operation must be the first entry"); | |
| } | |
| operation.verify_user_signature(verifying_key) | |
| &operation.new_key.clone().unwrap() | |
| let vk = match &operation.variant { | |
| OperationType::CreateAccount { .. } | OperationType::RegisterService { .. } => { | |
| if !self.entries.is_empty() { | |
| bail!("CreateAccount or RegisterService operation must be the first entry"); | |
| } | |
| if let Some(new_key) = &operation.new_key { | |
| &new_key.clone() | |
| } else { | |
| bail!("Operation is missing required `new_key`") | |
| } |
| match &entry.operation.variant { | ||
| OperationType::RegisterService { .. } | OperationType::AddData { .. } => {} | ||
| OperationType::AddKey | OperationType::CreateAccount { .. } => { | ||
| let key = entry.operation.get_public_key().unwrap(); | ||
| valid_keys.insert(key.clone()); | ||
| } | ||
| Operation::RevokeKey(args) => { | ||
| valid_keys.remove(&args.value.clone()); | ||
| OperationType::RevokeKey => { | ||
| let key = entry.operation.get_public_key().unwrap(); | ||
| valid_keys.remove(&key.clone()); |
There was a problem hiding this comment.
Handle potential None values from get_public_key() to prevent panics
In lines 91~ and 95~, the code uses unwrap() on operation.get_public_key(). If get_public_key() returns None, this will cause a panic. It's safer to handle the Option properly to prevent potential runtime errors.
Apply this diff to handle the None case:
match &entry.operation.variant {
OperationType::RegisterService { .. } | OperationType::AddData { .. } => {}
OperationType::AddKey | OperationType::CreateAccount { .. } => {
- let key = entry.operation.get_public_key().unwrap();
+ if let Some(key) = entry.operation.get_public_key() {
valid_keys.insert(key.clone());
+ } else {
+ // Handle unexpected None value
+ continue;
+ }
}
OperationType::RevokeKey => {
- let key = entry.operation.get_public_key().unwrap();
+ if let Some(key) = entry.operation.get_public_key() {
valid_keys.remove(&key.clone());
+ } else {
+ // Handle unexpected None value
+ continue;
+ }
}
}📝 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.
| match &entry.operation.variant { | |
| OperationType::RegisterService { .. } | OperationType::AddData { .. } => {} | |
| OperationType::AddKey | OperationType::CreateAccount { .. } => { | |
| let key = entry.operation.get_public_key().unwrap(); | |
| valid_keys.insert(key.clone()); | |
| } | |
| Operation::RevokeKey(args) => { | |
| valid_keys.remove(&args.value.clone()); | |
| OperationType::RevokeKey => { | |
| let key = entry.operation.get_public_key().unwrap(); | |
| valid_keys.remove(&key.clone()); | |
| match &entry.operation.variant { | |
| OperationType::RegisterService { .. } | OperationType::AddData { .. } => {} | |
| OperationType::AddKey | OperationType::CreateAccount { .. } => { | |
| if let Some(key) = entry.operation.get_public_key() { | |
| valid_keys.insert(key.clone()); | |
| } else { | |
| // Handle unexpected None value | |
| continue; | |
| } | |
| } | |
| OperationType::RevokeKey => { | |
| if let Some(key) = entry.operation.get_public_key() { | |
| valid_keys.remove(&key.clone()); | |
| } else { | |
| // Handle unexpected None value | |
| continue; | |
| } | |
| } |
|
|
||
| op.insert_signature(signing_key)?; | ||
|
|
||
| Ok(op) |
There was a problem hiding this comment.
Ensure consistent signing after all fields are set
Similar to new_create_account, in new_register_service, consider signing the operation after all fields are fully initialized to ensure the signature covers the complete operation.
Verify the order of operations and adjust if necessary.
| if self.id.is_empty() { | ||
| bail!("Id is empty") | ||
| } | ||
|
|
||
| if signature.signature.is_empty() { | ||
| return Err(GeneralError::MissingArgumentError( | ||
| "signature is empty".to_string(), | ||
| ) | ||
| .into()); | ||
| match &self.variant { | ||
| OperationType::AddKey | OperationType::RevokeKey => (), | ||
| OperationType::RegisterService { .. } | OperationType::CreateAccount { .. } => { | ||
| if self.new_key.is_none() { | ||
| bail!("Creating a hashchain requires adding an initial key"); | ||
| } | ||
|
|
||
| Ok(()) | ||
| } | ||
| Operation::CreateAccount(CreateAccountArgs { id, challenge, .. }) => { | ||
| if id.is_empty() { | ||
| return Err( | ||
| GeneralError::MissingArgumentError("id is empty".to_string()).into(), | ||
| ); | ||
| OperationType::AddData { value, .. } => { | ||
| // TODO: Upper bound on value size | ||
| if value.is_empty() { | ||
| bail!("Adding external data requires a non-empty value"); | ||
| } | ||
| } | ||
| }; | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add upper bound on value size in validate_basic
In the AddData variant handling, there's a TODO comment regarding an upper bound on value size. Implementing this check can prevent potential issues with excessively large data.
Consider adding a size check like:
if value.len() > MAX_VALUE_SIZE {
bail!("Value exceeds maximum allowed size");
}Would you like me to help define MAX_VALUE_SIZE and implement this check?
| op.insert_signature(signing_key).context("Failed to insert signature")?; | ||
|
|
||
| let msg = bincode::serialize(&op).context("Failed to serialize operation")?; | ||
| let service_challenge = service_signer.sign(&msg); | ||
|
|
||
| match op { | ||
| Operation::CreateAccount(ref mut args) => { | ||
| args.challenge = ServiceChallengeInput::Signed(service_challenge); | ||
| } | ||
| _ => panic!("Operation should be CreateAccount"), | ||
| op.variant = OperationType::CreateAccount { | ||
| service_id, | ||
| challenge: ServiceChallengeInput::Signed(service_challenge), | ||
| }; | ||
|
|
There was a problem hiding this comment.
Potential signature mismatch due to modifying op.variant after signing
In new_create_account, the operation is signed using op.insert_signature(signing_key) before updating op.variant with the actual service_challenge. This means the signature does not cover the final state of the operation, which could lead to signature verification failures.
Consider updating op.variant with the service_challenge before signing to ensure the signature covers the complete operation:
let msg = bincode::serialize(&op).context("Failed to serialize operation")?;
let service_challenge = service_signer.sign(&msg);
- op.insert_signature(signing_key).context("Failed to insert signature")?;
op.variant = OperationType::CreateAccount {
service_id,
challenge: ServiceChallengeInput::Signed(service_challenge),
};
+ op.insert_signature(signing_key).context("Failed to insert signature")?;
📝 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.
| op.insert_signature(signing_key).context("Failed to insert signature")?; | |
| let msg = bincode::serialize(&op).context("Failed to serialize operation")?; | |
| let service_challenge = service_signer.sign(&msg); | |
| match op { | |
| Operation::CreateAccount(ref mut args) => { | |
| args.challenge = ServiceChallengeInput::Signed(service_challenge); | |
| } | |
| _ => panic!("Operation should be CreateAccount"), | |
| op.variant = OperationType::CreateAccount { | |
| service_id, | |
| challenge: ServiceChallengeInput::Signed(service_challenge), | |
| }; | |
| let msg = bincode::serialize(&op).context("Failed to serialize operation")?; | |
| let service_challenge = service_signer.sign(&msg); | |
| op.variant = OperationType::CreateAccount { | |
| service_id, | |
| challenge: ServiceChallengeInput::Signed(service_challenge), | |
| }; | |
| op.insert_signature(signing_key).context("Failed to insert signature")?; | |
draft for now
Summary by CodeRabbit
Release Notes
New Features
OperationTypeenum for better categorization of operations within the Hashchain.Signatureenum to enhance signature handling and type safety.Bug Fixes
Refactor
Tests