refactor(da): Proofs error enums implemented with thiserror #346
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the WalkthroughThe changes introduce structured error handling in the proof verification logic by replacing generic Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Batch
participant InsertProof
participant UpdateProof
participant ProofError
Caller->>Batch: verify()
Batch->>Batch: internal verification logic
alt InsertProof needed
Batch->>InsertProof: verify(service_challenge)
InsertProof-->>Batch: Result<(), ProofError>
end
alt UpdateProof needed
Batch->>UpdateProof: verify()
UpdateProof-->>Batch: Result<(), ProofError>
end
Batch-->>Caller: Result<(), ProofError>
Note right of ProofError: Structured error variants returned on failure
Suggested reviewers
Poem
✨ Finishing Touches🧪 Generate Unit Tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 2
🧹 Nitpick comments (1)
crates/tree/Cargo.toml (1)
28-31: Consider droppinganyhowfrom this crateWith
ProofError+thiserrorin place,prism-treeno longer needs the fullanyhowdependency (only theContextextension trait is used).
Replacing those few.context()calls with explicit mapping removes a heavy crate:-anyhow.workspace = trueand adjust code accordingly.
Reduces build time & binary size.
📜 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/common/src/operation.rs(1 hunks)crates/common/src/transaction.rs(0 hunks)crates/tree/Cargo.toml(1 hunks)crates/tree/src/proofs.rs(9 hunks)
💤 Files with no reviewable changes (1)
- crates/common/src/transaction.rs
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: unused dependencies
- GitHub Check: clippy
- GitHub Check: coverage
- GitHub Check: unit-test
- GitHub Check: integration-test
🔇 Additional comments (1)
crates/tree/src/proofs.rs (1)
128-137: Leakinganyhowback into the new API
verify()now returnsResult<_, ProofError>yet still calls.context()which converts the inner error toanyhow::Error, immediately string-ifying it. This re-introduces ananyhowdependency just for message decoration.Replace with custom context or direct mapping to keep the crate
anyhow-free:- self.non_membership_proof - .verify_nonexistence() - .context("Invalid NonMembershipProof") - .map_err(|e| ProofError::NonexistenceError(e.to_string()))?; + self.non_membership_proof + .verify_nonexistence() + .map_err(|e| ProofError::NonexistenceError(format!("Invalid NonMembershipProof: {e}")))?;[ suggest_optional_refactor ]
Codecov ReportAttention: Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
crates/tree/src/proofs.rs (1)
74-82:assert_eq!causes panics – return a typed error insteadUsing
assert_eq!here aborts execution on mismatch instead of returning a recoverableProofError.-assert_eq!(root, self.new_root); +if root != self.new_root { + return Err(ProofError::VerificationError( + "root mismatch after processing batch".to_string(), + )); +}
♻️ Duplicate comments (2)
crates/tree/src/proofs.rs (2)
42-44: Signature still relies on the shadowed aliasBecause of the import above, this line currently refers to the wrong
Result:pub fn verify(&self) -> Result<(), ProofError>After dropping the alias, update the signature:
-pub fn verify(&self) -> Result<(), ProofError> { +pub fn verify(&self) -> std::result::Result<(), ProofError> {Without the change, the file will not compile.
(The same fix is required forInsertProof::verifyandUpdateProof::verify.)
262-279:MissingServiceProofenum variant is dead code
MissingServiceProofis defined but never used (all call-sites raiseMissingServiceChallenge).
Keeping unused variants bloats the API surface and invites confusion.- #[error("service proof is missing from batch for create account verification: {0}")] - MissingServiceProof(String),Remove the variant (or wire it into
Batch::verify) and runcargo clippy -- -D dead_codeto prevent regressions.
🧹 Nitpick comments (1)
crates/tree/src/proofs.rs (1)
128-133: Still coupling toanyhow::Context; consider removing the dependency entirelyThe new error model no longer needs
anyhow, yet.context()keeps the dependency alive.
If you want to keep the rich-context behaviour, re-implement it via a helper, otherwise simplify:-self.non_membership_proof - .verify_nonexistence() - .context("Invalid NonMembershipProof") - .map_err(|e| ProofError::NonexistenceError(e.to_string()))?; +self.non_membership_proof + .verify_nonexistence() + .map_err(|e| ProofError::NonexistenceError(format!("Invalid NonMembershipProof: {e}")))?;This lets you drop the
anyhowcrate completely.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
crates/tree/Cargo.toml(1 hunks)crates/tree/src/proofs.rs(9 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/tree/Cargo.toml
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: clippy
- GitHub Check: unused dependencies
- GitHub Check: coverage
- GitHub Check: unit-test
- GitHub Check: integration-test
Summary by CodeRabbit
New Features
Refactor
Chores