fix: wrap dkg on chain txs for failures - #1790
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe change adds idempotent transaction submission for registry operations. It detects state settled by another sender, preserves unresolved errors, updates handlers for outcomes without receipts, and documents concurrent committee finalization. ChangesRegistry transaction idempotency
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant RegistryHandler
participant RegistryEffects
participant send_tx_idempotent
participant CiphernodeRegistry
RegistryHandler->>RegistryEffects: request registry operation
RegistryEffects->>send_tx_idempotent: submit transaction with retry handling
send_tx_idempotent->>CiphernodeRegistry: check desired state after failure
CiphernodeRegistry-->>send_tx_idempotent: state settled or work remains
send_tx_idempotent-->>RegistryEffects: return Mined or AlreadySettled
RegistryEffects-->>RegistryHandler: provide outcome for logging and continuation
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
crates/evm/src/helpers.rs (1)
397-417: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider retrying the settlement check.
settledruns one time. If the state read hits a transient RPC failure, a benign concurrent settlement becomes a hard error, and the caller reports it on the bus. The transaction path already usesretry_with_backoff. A short retry around the state read would make the benign case survive a single RPC blip.This is optional. The current behavior fails closed, which is safe.
🤖 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/evm/src/helpers.rs` around lines 397 - 417, Optionally wrap the settled state read in a short retry-with-backoff flow, reusing the existing retry mechanism, so transient RPC failures are retried before returning the settlement-check error. Update the settled() call in the post-send_tx_with_retry path while preserving the existing AlreadySettled, original error, and contextual failure outcomes.crates/evm/src/ciphernode_registry/effects.rs (1)
79-83: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider distinguishing a read failure from "not settled".
Both probes map every non-matching error to
Ok(false), including RPC transport failures.send_tx_idempotentmodels a failed state check separately through theErr(check_error)arm, which adds the context "the on-chain state check after the failure also failed". With these implementations that arm is unreachable, so an operator cannot tell a genuine "work remains" result from an unreadable chain.The propagated error is still the original transaction error, so behavior stays safe. This is a diagnostics improvement.
♻️ Proposed change for `committee_finalization_settled`
let contract = ICiphernodeRegistry::new(contract_address, provider.provider()); match contract.finalizeCommittee(e3_id_u256).call().await { Ok(_) => Ok(false), - Err(err) => Ok(reverts_with( - &anyhow::Error::from(err), - "CommitteeAlreadyFinalized", - )), + Err(err) => { + let err = anyhow::Error::from(err); + if reverts_with(&err, "CommitteeAlreadyFinalized") { + return Ok(true); + } + // A decoded revert that is not the settled marker means work remains. + if decode_error_from_str(&format!("{err:?}")).is_some() { + return Ok(false); + } + // An undecodable error is a read failure, not a state answer. + Err(err) + } }Also applies to: 99-103
🤖 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/evm/src/ciphernode_registry/effects.rs` around lines 79 - 83, Update committee_finalization_settled and the corresponding second probe so only a valid on-chain response indicating “not settled” returns Ok(false); propagate RPC/transport and other read failures as Err instead of converting them to false. Preserve the existing NodeAlreadySubmitted revert handling and allow send_tx_idempotent’s Err(check_error) path to report failed state checks.
🤖 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.
Inline comments:
In `@crates/evm/src/ciphernode_registry/effects.rs`:
- Around line 91-104: Update committee_finalization_settled to distinguish a
CommitteeAlreadyFinalized revert caused by a previously failed committee from
one finalized successfully: query the committee stage and return false for the
Failed stage before treating the revert as settled. Preserve the existing true
result only for genuinely finalized committees, and do not add a .from(...) call
to finalizeCommittee.
---
Nitpick comments:
In `@crates/evm/src/ciphernode_registry/effects.rs`:
- Around line 79-83: Update committee_finalization_settled and the corresponding
second probe so only a valid on-chain response indicating “not settled” returns
Ok(false); propagate RPC/transport and other read failures as Err instead of
converting them to false. Preserve the existing NodeAlreadySubmitted revert
handling and allow send_tx_idempotent’s Err(check_error) path to report failed
state checks.
In `@crates/evm/src/helpers.rs`:
- Around line 397-417: Optionally wrap the settled state read in a short
retry-with-backoff flow, reusing the existing retry mechanism, so transient RPC
failures are retried before returning the settlement-check error. Update the
settled() call in the post-send_tx_with_retry path while preserving the existing
AlreadySettled, original error, and contextual failure outcomes.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 2a8d1e7c-0d45-402c-b00f-92033e4f9eb7
📒 Files selected for processing (6)
agent/flow-trace/03_E3_REQUEST_AND_COMMITTEE.mdagent/flow-trace/04_DKG_AND_COMPUTATION.mdcrates/evm/src/ciphernode_registry/actor.rscrates/evm/src/ciphernode_registry/effects.rscrates/evm/src/ciphernode_registry/handlers.rscrates/evm/src/helpers.rs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Bug Fixes
New Features