feat(sdk): including grovedb proof bytes when error in proof verification #2332
Conversation
WalkthroughThe changes introduce a new error variant, Changes
Possibly related PRs
Suggested labels
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: 1
🧹 Outside diff range and nitpick comments (2)
packages/rs-drive-proof-verifier/src/error.rs (2)
19-21: Enhance documentation for GroveDBError variantThe implementation looks good and aligns with the PR objectives. Consider expanding the documentation to include:
- Specific scenarios when this error occurs
- The significance of the proof_bytes field
- Example usage if applicable
/// GroveDB error, often for issues with proofs +/// +/// This variant is specifically used when proof verification fails in GroveDB operations. +/// The proof_bytes field contains the actual proof that failed verification, which can be +/// useful for debugging or client-side analysis.
19-21: Consider adding a From implementation for GroveDB errorsTo maintain consistency with other error conversions in this file (like
drive::error::ErrorandProtocolError), consider adding a dedicatedFromimplementation for converting GroveDB errors into this variant.Example implementation:
impl From<drive::grovedb::Error> for Error { fn from(error: drive::grovedb::Error) -> Self { Self::GroveDBError { proof_bytes: Vec::new(), // This would need appropriate proof bytes error: error.to_string(), } } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/rs-drive-proof-verifier/src/error.rs(2 hunks)packages/rs-drive-proof-verifier/src/proof.rs(26 hunks)
🔇 Additional comments (1)
packages/rs-drive-proof-verifier/src/error.rs (1)
2-2: Verify the usage of GroveDBProof import
The GroveDBProof import appears to be unused in this file. Let's verify its usage in related files.
✅ Verification successful
Let me check the actual usage of GroveDBProof in the error file and its relationship with the error types.
Let me check how the GroveDBProof is used in the SDK to understand the relationship between the error types and proof handling.
Based on the evidence gathered, I can now provide the final response:
The GroveDBProof import is correctly used and necessary
The GroveDBProof import is justified as it's used in the GroveDBError variant which includes proof_bytes. This error variant is designed to handle proof-related errors while preserving the proof bytes, which aligns with the PR objectives. The same type is also used in the SDK for proof handling and verification, showing a consistent pattern across the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for GroveDBProof usage in the codebase
rg "GroveDBProof" --type rust
Length of output: 375
Script:
#!/bin/bash
# Check the content of the error file to understand the context
cat packages/rs-drive-proof-verifier/src/error.rs
Length of output: 3823
Script:
#!/bin/bash
# Check the relevant section of the SDK file
rg -A 10 -B 10 "GroveDBProof" packages/rs-sdk/src/sdk.rs
Length of output: 1639
| .map_err(|e| match e { | ||
| drive::error::Error::GroveDB(e) => Error::GroveDBError { | ||
| proof_bytes: proof.grovedb_proof.clone(), | ||
| error: e.to_string(), | ||
| }, | ||
| _ => e.into(), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider refactoring repeated error mapping logic into a helper function
The error handling logic that maps drive::error::Error to Error::GroveDBError is duplicated across multiple methods (e.g., lines 277-282, 331-336, 402-407, etc.). Refactoring this logic into a helper function or macro can improve maintainability and reduce code duplication.
Apply this diff to simplify the error mapping within the selected line ranges:
-.map_err(|e| match e {
- drive::error::Error::GroveDB(e) => Error::GroveDBError {
- proof_bytes: proof.grovedb_proof.clone(),
- error: e.to_string(),
- },
- _ => e.into(),
-})?;
+.map_err(|e| map_drive_error(e, proof.grovedb_proof.clone()))?;Define the helper function outside the selected line ranges:
fn map_drive_error(e: drive::error::Error, proof_bytes: Vec<u8>) -> Error {
match e {
drive::error::Error::GroveDB(e) => Error::GroveDBError {
proof_bytes,
error: e.to_string(),
},
_ => e.into(),
}
}Also applies to: 331-336, 402-407, 535-540, 596-601, 650-655, 705-710, 757-762, 810-815, 866-871, 939-944, 1004-1009, 1095-1100, 1148-1156, 1208-1216, 1320-1325, 1408-1413, 1454-1459, 1502-1507, 1560-1565, 1610-1615, 1658-1663, 1706-1713, 1813-1818, 1864-1869, 1932-1937
QuantumExplorer
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
packages/rs-drive-proof-verifier/src/error.rs (1)
19-26: Enhance documentation for the GroveDBError variant.While the implementation is solid, the documentation could be more comprehensive by including:
- Specific scenarios when this error occurs
- Description of each field's purpose
- Guidance on error handling/resolution
Consider updating the documentation like this:
- /// GroveDB error, often for issues with proofs + /// Error that occurs during GroveDB proof verification operations. + /// + /// # Fields + /// * `proof_bytes` - The raw proof data that failed verification + /// * `height` - The block height at which the proof was generated + /// * `time_ms` - Time taken for the verification attempt in milliseconds + /// * `error` - Detailed description of what went wrong during verification
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/rs-drive-proof-verifier/src/error.rs(2 hunks)packages/rs-drive-proof-verifier/src/proof.rs(27 hunks)
🔇 Additional comments (5)
packages/rs-drive-proof-verifier/src/error.rs (1)
2-2: LGTM: Import statement is correctly placed and necessary.
The import of GroveDBProof aligns with the new error variant's requirements.
packages/rs-drive-proof-verifier/src/proof.rs (4)
1006-1007: Good validation of epoch value
The code properly validates that the epoch value doesn't exceed the maximum allowed value for a 16-bit integer.
1011-1014: Block info construction looks good
The construction of BlockInfo with metadata fields is correct and properly handles the epoch conversion.
1026-1033: State transition verification flow is well structured
The code properly:
- Verifies the state transition execution
- Maps any GroveDB errors with proof details
- Verifies the Tenderdash proof
- Returns the result with metadata and proof
Also applies to: 1036-1036, 1038-1038
277-284: 🛠️ Refactor suggestion
Consistent error handling pattern needs refactoring
The error mapping pattern is duplicated across multiple methods. This repetition violates the DRY principle and makes maintenance more difficult.
Create a helper function to handle the common error mapping pattern:
fn map_grove_db_error(e: drive::error::Error, proof_bytes: Vec<u8>, height: u64, time_ms: u64) -> Error {
match e {
drive::error::Error::GroveDB(e) => Error::GroveDBError {
proof_bytes,
height,
time_ms,
error: e.to_string(),
},
_ => e.into(),
}
}Then use it to simplify all error mappings:
- .map_err(|e| match e {
- drive::error::Error::GroveDB(e) => Error::GroveDBError {
- proof_bytes: proof.grovedb_proof.clone(),
- height: mtd.height,
- time_ms: mtd.time_ms,
- error: e.to_string(),
- },
- _ => e.into(),
- })?;
+ .map_err(|e| map_grove_db_error(e, proof.grovedb_proof.clone(), mtd.height, mtd.time_ms))?;Also applies to: 333-340, 406-413, 541-548, 604-611, 660-667, 717-724, 771-778, 826-833, 884-891, 959-966, 1026-1033, 1119-1126, 1174-1184, 1236-1245, 1350-1357, 1440-1447, 1488-1495, 1538-1545, 1598-1605, 1650-1657, 1700-1707, 1753-1761, 1859-1866, 1912-1919, 1982-1989
Issue being fixed or feature implemented
Proof errors in grovedb were not giving the client the actual proof, this fixes that.
What was done?
Returning a different error for proof errors.
How Has This Been Tested?
Not tested, but simple change.
Breaking Changes
Not breaking
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
New Features
GroveDBError, to enhance error handling related to GroveDB operations.Bug Fixes
maybe_from_proof_with_metadatamethods for better clarity and context in error messages.