fix(zk-prover): proof request failure bridges#1532
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughProofRequestActor now publishes terminal E3Failed events when proof signing or compute-request failures occur, using centralized helpers that emit CommitteeFinalized+DKGInvalidShares for DKG-path proofs (C0–C5) and CiphertextReady+DecryptionInvalidShares for decryption-path proofs (C6–C7). Docs and tests were updated accordingly. ChangesProof Failure Event Bridging
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/zk-prover/src/actors/proof_request.rs (1)
1505-1600: 💤 Low valueTests correctly verify E3Failed emission for failure paths.
The two tests appropriately validate that the failure bridging emits the expected terminal events. The test coverage focuses on one compute error path and one helper, which provides a smoke test of the new behavior.
Optional test improvements
Minor: Test context construction
Line 1553's
test_ctx(E3Failed{...})creates a context wrappingE3Failedwhen handling aComputeRequestError. While this doesn't affect correctness (the test validates the published event data, not the context), constructing the context with more representative data (e.g., the compute request itself) would improve test clarity.Optional: Broader coverage
Consider adding tests for:
- Signing failure paths (C1-C7)
fail_dkg_roundhelper directly (analogous to test 2's coverage offail_decryption_round)- Other proof types (C1-C5, C7) compute errors
These are low priority given the helpers are simple wrappers and the call sites follow a consistent pattern.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/zk-prover/src/actors/proof_request.rs` around lines 1505 - 1600, Tests correctly assert E3Failed events but the first test constructs the EventContext with an E3Failed payload instead of a representative compute request; update c0_compute_error_emits_e3_failed to call test_ctx with the actual ComputeRequest (the Zk ComputeRequest used in the TypedEvent) rather than E3Failed so the context matches the event being handled, i.e., locate the TypedEvent::new(...) in c0_compute_error_emits_e3_failed and replace the test_ctx(E3Failed { ... }) argument with the ComputeRequest (or a wrapper that converts that ComputeRequest into an Unsequenced EventContext) to produce a more realistic context; optionally add analogous tests for fail_dkg_round and other compute error variants (C1–C7) if you want broader coverage.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@crates/zk-prover/src/actors/proof_request.rs`:
- Around line 1505-1600: Tests correctly assert E3Failed events but the first
test constructs the EventContext with an E3Failed payload instead of a
representative compute request; update c0_compute_error_emits_e3_failed to call
test_ctx with the actual ComputeRequest (the Zk ComputeRequest used in the
TypedEvent) rather than E3Failed so the context matches the event being handled,
i.e., locate the TypedEvent::new(...) in c0_compute_error_emits_e3_failed and
replace the test_ctx(E3Failed { ... }) argument with the ComputeRequest (or a
wrapper that converts that ComputeRequest into an Unsequenced EventContext) to
produce a more realistic context; optionally add analogous tests for
fail_dkg_round and other compute error variants (C1–C7) if you want broader
coverage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ac761325-5248-409c-b194-3dbe171bf02e
📒 Files selected for processing (3)
agent/flow-trace/00_INDEX.mdagent/flow-trace/04_DKG_AND_COMPUTATION.mdcrates/zk-prover/src/actors/proof_request.rs
3 participants
closes #1533
Summary by CodeRabbit
Bug Fixes
Documentation
Tests