feat: decode evm errors [skip-line-limit]#1427
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds an EVM error decoder module and threads decoded error names into retry logic and logs; replaces hard-coded revert codes with symbolic names; removes subscription staleness checks; updates deployment artifacts, templates, CI, and integration scripts. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant RetryHandler as Retry Handler
participant Decoder as Error Decoder
participant Contract
participant Logger
Client->>RetryHandler: call_with_retry_and_decoder(operation)
RetryHandler->>Contract: submit transaction
Contract-->>RetryHandler: Err (raw revert / error string)
RetryHandler->>Decoder: decode_error_from_str(raw_err)
Decoder-->>RetryHandler: decoded_error (optional)
RetryHandler->>RetryHandler: should_retry_error(raw_err, decoded_error, retry_on_errors)
alt Retry condition met
RetryHandler->>Logger: Log retry attempt (uses display_error)
RetryHandler->>Contract: retry transaction
else Max retries exceeded
RetryHandler->>Logger: Log final failure (uses display_error)
RetryHandler-->>Client: return error
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip CodeRabbit can approve the review once all CodeRabbit's comments are resolved.Enable the |
![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: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/evm-helpers/src/error_decoder.rs`:
- Around line 49-52: The current decode_error_from_str implementation only picks
the longest 0x... blob via extract_revert_data and thus can miss short
selector-only custom errors when a longer unrelated hex (tx
hash/calldata/address) is present; change the logic in decode_error_from_str to
scan all hex blobs extracted from the input (use or extend extract_revert_data
to return all 0x tokens), try decode_error on each blob (short-to-long or try
all) and return the first successful decoding from decode_error, falling back to
None if none decode; update the analogous logic in the related function(s)
around lines 61-78 to the same approach, and add a regression test that feeds a
string containing both a short revert selector (e.g. CommitteeNotRequested
selector) and a longer unrelated hex token to ensure the short selector is
decoded correctly.
In `@crates/evm-helpers/src/retry.rs`:
- Around line 45-47: The code currently uses format!("{}", e) which loses the
anyhow source chain; change the construction of error_str in retry.rs (where
error_str, decode_error_from_str, and display_error are used) to preserve the
full source chain by formatting the error with a multi-line debug/pretty
representation (e.g., use format!("{e:#}") or equivalent) so that
decode_error_from_str can see inner revert payloads; update only the error_str
assignment so decoded.as_deref().unwrap_or(&error_str) continues to work.
In `@crates/evm/src/enclave_sol_writer.rs`:
- Around line 225-228: The retry predicate passed to send_tx_with_retry
currently only matches the decoded revert string "CiphertextOutputNotPublished",
which is brittle if decoding fails or the message text changes; update the retry
logic used in the closure given to send_tx_with_retry (the call site around
send_tx_with_retry and the string "CiphertextOutputNotPublished") to also detect
the raw revert selector or numeric error code fallback (e.g., match the revert
selector bytes/prefix or known error id) and consider unknown/undecodable
reverts that match that selector as retryable so retries still occur when
decoding fails or message text differs.
In `@tests/integration/persist.sh`:
- Line 16: The readiness loop in tests/integration/persist.sh uses curl
targeting 0.0.0.0 which is a bind address and can hang on some runners; update
the curl POST command inside the until loop (the line starting with "until curl
-sf -X POST ...") to use localhost instead of 0.0.0.0 so the readiness check
reliably connects like tests/integration/base.sh.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7915be2d-6fa0-468d-9f79-3c4300c2d3ff
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockexamples/CRISP/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (22)
crates/evm-helpers/Cargo.tomlcrates/evm-helpers/src/error_decoder.rscrates/evm-helpers/src/lib.rscrates/evm-helpers/src/retry.rscrates/evm/Cargo.tomlcrates/evm/src/ciphernode_registry_sol.rscrates/evm/src/enclave_sol_writer.rscrates/evm/src/error_decoder.rscrates/evm/src/evm_read_interface.rscrates/evm/src/helpers.rscrates/evm/src/lib.rscrates/evm/src/slashing_manager_sol_writer.rsexamples/CRISP/server/src/cli/commands.rsexamples/CRISP/server/src/server/indexer.rspackages/enclave-contracts/deployed_contracts.jsontemplates/default/deployed_contracts.jsontemplates/default/enclave.config.yamltemplates/default/tests/integration.spec.tstests/integration/base.shtests/integration/enclave.config.yamltests/integration/fns.shtests/integration/persist.sh
💤 Files with no reviewable changes (1)
- crates/evm/src/evm_read_interface.rs
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/evm-helpers/src/retry.rs (1)
14-21: Consider extracting shared retry logic.The
should_retry_errorfunction is duplicated betweenhelpers.rs(crates/evm) and this file. While acceptable given the crate boundary, if more shared retry logic emerges, consider extracting to a common location.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/evm-helpers/src/retry.rs` around lines 14 - 21, The should_retry_error function is duplicated between retry.rs and helpers.rs; extract this shared retry logic into a single reusable location (e.g., a new module or crate-level utility) and have both retry.rs and helpers.rs call that centralized function; specifically, move the implementation of should_retry_error into a shared module (e.g., evm_utils::retry or a common crate) and replace the local definitions in retry.rs and helpers.rs with calls to that shared function to avoid duplication and keep behavior consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@crates/evm-helpers/src/retry.rs`:
- Around line 14-21: The should_retry_error function is duplicated between
retry.rs and helpers.rs; extract this shared retry logic into a single reusable
location (e.g., a new module or crate-level utility) and have both retry.rs and
helpers.rs call that centralized function; specifically, move the implementation
of should_retry_error into a shared module (e.g., evm_utils::retry or a common
crate) and replace the local definitions in retry.rs and helpers.rs with calls
to that shared function to avoid duplication and keep behavior consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2de45838-2c70-4842-ae4c-b6049c23b803
📒 Files selected for processing (4)
crates/evm-helpers/src/retry.rscrates/evm/src/error_decoder.rscrates/evm/src/helpers.rstests/integration/persist.sh
ctrlc03
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/enclave-contracts/.gitignore`:
- Around line 14-17: Remove the trailing whitespace at the end of the artifacts
un-ignore line for Enclave.json (the line containing
"!/artifacts/contracts/Enclave.sol/Enclave.json") so it exactly matches the
other un-ignored artifact lines; optionally, for consistency with other
sections, add an explicit un-ignore of the ".sol/" directory (e.g.,
"!/artifacts/contracts/Enclave.sol/") before the Enclave.json entry to mirror
the pattern used for other contracts like CiphernodeRegistryOwnable.json.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bf611da9-7386-4060-a231-55bafa5f1551
📒 Files selected for processing (3)
packages/enclave-contracts/.gitignorepackages/enclave-contracts/artifacts/contracts/Enclave.sol/Enclave.jsonpackages/enclave-contracts/artifacts/contracts/registry/CiphernodeRegistryOwnable.sol/CiphernodeRegistryOwnable.json
✅ Files skipped from review due to trivial changes (1)
- packages/enclave-contracts/artifacts/contracts/registry/CiphernodeRegistryOwnable.sol/CiphernodeRegistryOwnable.json
ctrlc03
force-pushed
the
feat/evm-errors
branch
from
5a6aeed to
82a3daf
Compare
ctrlc03
changed the title
ctrlc03
force-pushed
the
feat/evm-errors
branch
from
1378b97 to
cb71d6d
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
crates/evm-helpers/src/retry.rs (1)
58-66: Verify the borrow ofdecode_erroracross async boundary.The reference
decode_fn = &decode_erroris taken outside theasync moveblock and used inside it. This works because:
decode_erroris owned bycall_with_retry_and_decoder's stack frame- The reference is
Copy, so it can be moved into the async blockretry_with_backoffawaits completion before returning, sodecode_erroroutlives all usagesThe pattern is sound, but the comment on line 58 could be clearer about the actual reason (avoiding the closure being consumed on each retry iteration since
D: Fn, notFnOnce).📝 Suggested comment improvement
- // Decode before entering the async block to avoid moving the closure + // Borrow the decoder here; the reference is Copy and can move into the async block + // without consuming the closure, allowing it to be reused across retry iterations let decode_fn = &decode_error;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/evm-helpers/src/retry.rs` around lines 58 - 66, Update the comment describing why we take a reference to decode_error before the async block to clearly state that we're preventing the closure from being moved/consumed on each retry because D implements Fn (not FnOnce), rather than implying lifetime juggling across the async boundary; mention the specific symbols decode_error and decode_fn and that this keeps the decoder callable across retry iterations inside call_with_retry_and_decoder/retry_with_backoff without cloning or consuming the closure.crates/evm/src/error_decoder.rs (1)
60-65: Consider using{:#}for consistency with retry logic.
format_evm_erroruses{err:?}(Debug format) whilehelpers.rsandretry.rsuse{e:#}(alternate Display). Both work, but{:?}produces a more verbose "Caused by:" chain format, whereas{:#}produces a colon-separated chain on a single line.If this function is used in log output alongside the retry logic, the formatting inconsistency could be visually jarring. This is a minor stylistic concern.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/evm/src/error_decoder.rs` around lines 60 - 65, The function format_evm_error currently formats the anyhow error using the Debug formatter "{err:?}", causing a verbose multi-line "Caused by:" chain; change it to use the alternate Display formatting (e.g. format!("{err:#}") or "{:#}") so it matches the style used in helpers.rs and retry.rs and produces a single-line colon-separated chain, then pass that string into decode_error_from_str as before (function: format_evm_error; helper: decode_error_from_str).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/enclave-contracts/deployed_contracts.json`:
- Around line 257-304: Remove the stale/orphaned verifier entries from
deployed_contracts.json: delete the objects for DkgESmShareComputationVerifier,
DkgShareDecryptionVerifier, DkgShareEncryptionVerifier,
DkgSkShareComputationVerifier, ThresholdPkGenerationVerifier, and
ThresholdShareDecryptionVerifier; ensure the JSON remains syntactically valid
after removal (no trailing commas) and that remaining verifier entries (e.g.,
RecursiveAggregationFoldVerifier, ThresholdDecryptedSharesAggregationBnVerifier,
ThresholdDecryptedSharesAggregationModVerifier, ThresholdPkAggregationVerifier,
ThresholdUserDataEncryptionCt0Verifier, ThresholdUserDataEncryptionCt1Verifier)
are untouched so the deployment script can continue to discover actual contracts
dynamically.
---
Nitpick comments:
In `@crates/evm-helpers/src/retry.rs`:
- Around line 58-66: Update the comment describing why we take a reference to
decode_error before the async block to clearly state that we're preventing the
closure from being moved/consumed on each retry because D implements Fn (not
FnOnce), rather than implying lifetime juggling across the async boundary;
mention the specific symbols decode_error and decode_fn and that this keeps the
decoder callable across retry iterations inside
call_with_retry_and_decoder/retry_with_backoff without cloning or consuming the
closure.
In `@crates/evm/src/error_decoder.rs`:
- Around line 60-65: The function format_evm_error currently formats the anyhow
error using the Debug formatter "{err:?}", causing a verbose multi-line "Caused
by:" chain; change it to use the alternate Display formatting (e.g.
format!("{err:#}") or "{:#}") so it matches the style used in helpers.rs and
retry.rs and produces a single-line colon-separated chain, then pass that string
into decode_error_from_str as before (function: format_evm_error; helper:
decode_error_from_str).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5101bf86-a2da-4f80-84de-dcabfbd75002
📒 Files selected for processing (20)
.github/workflows/ci.ymlcrates/evm-helpers/src/retry.rscrates/evm/src/ciphernode_registry_sol.rscrates/evm/src/enclave_sol_writer.rscrates/evm/src/error_decoder.rscrates/evm/src/evm_read_interface.rscrates/evm/src/helpers.rscrates/evm/src/lib.rscrates/evm/src/slashing_manager_sol_writer.rspackages/enclave-contracts/.gitignorepackages/enclave-contracts/artifacts/contracts/Enclave.sol/Enclave.jsonpackages/enclave-contracts/artifacts/contracts/registry/CiphernodeRegistryOwnable.sol/CiphernodeRegistryOwnable.jsonpackages/enclave-contracts/deployed_contracts.jsontemplates/default/deployed_contracts.jsontemplates/default/enclave.config.yamltemplates/default/tests/integration.spec.tstests/integration/base.shtests/integration/enclave.config.yamltests/integration/fns.shtests/integration/persist.sh
💤 Files with no reviewable changes (1)
- crates/evm/src/evm_read_interface.rs
🚧 Files skipped from review as they are similar to previous changes (6)
- crates/evm/src/ciphernode_registry_sol.rs
- templates/default/tests/integration.spec.ts
- .github/workflows/ci.yml
- tests/integration/fns.sh
- templates/default/deployed_contracts.json
- tests/integration/persist.sh
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
3 participants
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.
fix #148
Summary by CodeRabbit
New Features
Bug Fixes
Chores