feat(evm): Surface on-chain revert data in failed transaction status#802
Open
dylankilkenny wants to merge 2 commits into
Open
feat(evm): Surface on-chain revert data in failed transaction status#802dylankilkenny wants to merge 2 commits into
dylankilkenny wants to merge 2 commits into
Conversation
When an EVM transaction transitions into Failed from a failed on-chain receipt, enrich status_reason with the revert payload as hex: "Transaction reverted on-chain (revert_data: 0x...)". Recovery runs only on the transition into Failed and is best-effort: prefer debug_traceTransaction (callTracer), fall back to eth_call at the execution block, and degrade to the existing generic string (preserved byte-for-byte) when neither yields data. Add a mockable EvmProviderTrait::get_call_revert_data that extracts the revert bytes from the JSON-RPC error before the lossy From<RpcError> conversion drops them. Gate all recovery on a new per-relayer EVM policy include_revert_data (default enabled), plumbed through config, domain model, API response, and OpenAPI. Signed-off-by: Dylan Kilkenny <dylankilkenny95@gmail.com>
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughAdds best-effort on-chain revert payload recovery into ChangesEVM Revert Data Recovery Feature
Sequence Diagram(s)sequenceDiagram
participant Poller as Transaction Poller
participant StatusHandler as handle_status_impl
participant RevertRecovery as recover_revert_data
participant Provider as EvmProvider
participant RPC as EVM RPC Node
Poller->>StatusHandler: receipt.status == 0 (Failed)
StatusHandler->>RevertRecovery: include_revert_data=true, tx_hash, receipt_block
RevertRecovery->>Provider: debug_traceTransaction(hash, callTracer)
Provider->>RPC: debug_traceTransaction
alt trace returns revert output
RPC-->>Provider: trace JSON with output bytes
Provider-->>RevertRecovery: Some(revert_bytes)
RevertRecovery-->>StatusHandler: status_reason = "reverted (revert_data: 0x...)"
else trace unavailable or empty output
RPC-->>Provider: error or empty
Provider-->>RevertRecovery: None
RevertRecovery->>Provider: get_call_revert_data(eth_call, block)
Provider->>RPC: eth_call at receipt block
alt eth_call reverts with data
RPC-->>Provider: RpcError::ErrorResp(data)
Provider-->>RevertRecovery: Some(Bytes)
RevertRecovery-->>StatusHandler: status_reason = "reverted (revert_data: 0x...)"
else no revert data
RPC-->>Provider: Ok or no data
Provider-->>RevertRecovery: None
RevertRecovery-->>StatusHandler: status_reason = "Transaction reverted on-chain (receipt status: failed)"
end
end
StatusHandler-->>Poller: updated transaction
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files
Flags with carried forward coverage won't be shown. Click here to find out more. @@ Coverage Diff @@
## main #802 +/- ##
==========================================
- Coverage 90.26% 89.55% -0.72%
==========================================
Files 292 301 +9
Lines 125079 128106 +3027
==========================================
+ Hits 112897 114719 +1822
- Misses 12182 13387 +1205
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
src/models/relayer/repository.rs (1)
365-373: ⚡ Quick winAdd explicit assertions for
include_revert_datapropagation in these conversion tests.You now set
include_revert_datain fixtures, but the tests don’t assert it after conversion/round-trip. That can let regressions slip through silently.Suggested patch
@@ if let RelayerNetworkPolicy::Evm(evm_policy) = repo_model.policies { + assert_eq!(evm_policy.include_revert_data, None); assert_eq!(evm_policy.gas_price_cap, Some(100_000_000_000)); assert_eq!(evm_policy.eip1559_pricing, Some(true)); } else { panic!("Expected EVM policy"); } @@ let repo_evm = RelayerRepoModel::from(original_evm.clone()); let recovered_evm = Relayer::from(repo_evm); @@ assert_eq!(original_evm.id, recovered_evm.id); assert_eq!(original_evm.network_type, recovered_evm.network_type); assert_eq!(original_evm.notification_id, recovered_evm.notification_id); + if let Some(RelayerNetworkPolicy::Evm(policy)) = recovered_evm.policies { + assert_eq!(policy.include_revert_data, None); + } else { + panic!("Expected EVM policy"); + }Also applies to: 896-904
🤖 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 `@src/models/relayer/repository.rs` around lines 365 - 373, The conversion tests for RelayerEvmPolicy fixtures do not include explicit assertions to verify that the include_revert_data field is properly propagated through round-trip conversions. Modify the test fixtures to set include_revert_data to a specific non-None value (e.g., Some(true) or Some(false)) instead of None, and then add explicit assertions after each conversion/round-trip operation in the test to verify that the include_revert_data field in the resulting RelayerEvmPolicy matches the expected value. Apply this change at both the primary location (lines 365-373) and the secondary location mentioned at lines 896-904.src/services/provider/evm/mod.rs (1)
1212-1248: 🏗️ Heavy liftAdd a test that exercises the concrete provider implementation.
These tests only stub
MockEvmProviderTrait, so they would pass even ifEvmProvider::get_call_revert_datamishandledRpcError::ErrorResp, block selection, or revert-data extraction. Add an implementation-level test with a mocked JSON-RPC response, or extract the error-response handling into a small helper and test that directly.🤖 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 `@src/services/provider/evm/mod.rs` around lines 1212 - 1248, The existing tests test_get_call_revert_data_returns_payload and test_get_call_revert_data_no_revert_returns_none only mock the EvmProviderTrait and would not catch bugs in the actual EvmProvider::get_call_revert_data implementation's handling of RpcError::ErrorResp, block selection, or revert-data extraction. Add an implementation-level test that mocks the JSON-RPC responses directly to test the concrete EvmProvider behavior, or extract the error-response and revert-data handling logic into a small helper function and add unit tests for that helper directly.src/domain/transaction/evm/status.rs (1)
1099-1105: Replace hard-codedtruewithDEFAULT_EVM_INCLUDE_REVERT_DATAconstant.The
.unwrap_or(true)at lines 1099-1105 should use the shared constant to maintain consistency with the relayer policy defaults. IfDEFAULT_EVM_INCLUDE_REVERT_DATAchanges in the future, this code path would still use the old default.♻️ Proposed fix
use crate::constants::{ get_evm_min_age_for_hash_recovery, get_evm_pending_recovery_trigger_timeout, get_evm_prepare_timeout, get_evm_resend_timeout, ARBITRUM_TIME_TO_RESUBMIT, - EVM_MIN_HASHES_FOR_RECOVERY, MAX_GAP_SCAN_RANGE, + DEFAULT_EVM_INCLUDE_REVERT_DATA, EVM_MIN_HASHES_FOR_RECOVERY, MAX_GAP_SCAN_RANGE, };.get_evm_policy() .include_revert_data - .unwrap_or(true) + .unwrap_or(DEFAULT_EVM_INCLUDE_REVERT_DATA)🤖 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 `@src/domain/transaction/evm/status.rs` around lines 1099 - 1105, Replace the hard-coded `true` value in the `.unwrap_or(true)` call on the `.include_revert_data` property access chain with the `DEFAULT_EVM_INCLUDE_REVERT_DATA` constant. This ensures consistency with the relayer policy defaults across the codebase, so that if the default value is updated in the future, all code paths automatically use the new default value rather than maintaining a separate hard-coded value.
🤖 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 `@src/domain/transaction/evm/status.rs`:
- Around line 1113-1115: The revert data recovered from
`recover_revert_data(tx).await` can be arbitrarily large (controlled by
RPC/contract), and when included in the format string for `status_reason`, it
can cause database or notification write failures. Bound the recovered hex
string to a reasonable maximum size before including it in the formatted error
message. When the hex exceeds the limit, truncate it and optionally append an
indicator (like "...") to show it was truncated. Apply this bounding logic in
the Some branch of the match expression before the format! call.
- Around line 67-73: The eth_call fallback request in the TransactionRequest
construction is dropping gas-price and EIP-1559 fee fields (gas_price,
max_fee_per_gas, max_priority_fee_per_gas), which causes contracts that branch
on tx.gasprice to return different revert reasons. Add these missing fee fields
to the TransactionRequest being constructed by extracting them from the evm_data
object (similar to how value and gas_limit are already being populated),
ensuring the fallback preserves complete transaction context. Additionally,
extend the fallback test to assert that the reconstructed request includes these
fee fields when no tracing is available.
In `@src/services/provider/evm/mod.rs`:
- Around line 558-560: The `Err(RpcError::ErrorResp(payload))` match arm
currently treats all error responses that lack revert data as `Ok(None)`,
bypassing the retry mechanism. Instead, check whether the error response payload
contains a retriable error code (such as -32002, -32005, -32603); if it does and
has no revert data, return `Err(ProviderError)` to allow retry logic and
provider failover to handle it. Only return `Ok(payload.as_revert_data())` when
revert bytes are present or when the error is non-retriable, ensuring retriable
errors are propagated as `Err` results rather than swallowed as successful
`None` responses.
---
Nitpick comments:
In `@src/domain/transaction/evm/status.rs`:
- Around line 1099-1105: Replace the hard-coded `true` value in the
`.unwrap_or(true)` call on the `.include_revert_data` property access chain with
the `DEFAULT_EVM_INCLUDE_REVERT_DATA` constant. This ensures consistency with
the relayer policy defaults across the codebase, so that if the default value is
updated in the future, all code paths automatically use the new default value
rather than maintaining a separate hard-coded value.
In `@src/models/relayer/repository.rs`:
- Around line 365-373: The conversion tests for RelayerEvmPolicy fixtures do not
include explicit assertions to verify that the include_revert_data field is
properly propagated through round-trip conversions. Modify the test fixtures to
set include_revert_data to a specific non-None value (e.g., Some(true) or
Some(false)) instead of None, and then add explicit assertions after each
conversion/round-trip operation in the test to verify that the
include_revert_data field in the resulting RelayerEvmPolicy matches the expected
value. Apply this change at both the primary location (lines 365-373) and the
secondary location mentioned at lines 896-904.
In `@src/services/provider/evm/mod.rs`:
- Around line 1212-1248: The existing tests
test_get_call_revert_data_returns_payload and
test_get_call_revert_data_no_revert_returns_none only mock the EvmProviderTrait
and would not catch bugs in the actual EvmProvider::get_call_revert_data
implementation's handling of RpcError::ErrorResp, block selection, or
revert-data extraction. Add an implementation-level test that mocks the JSON-RPC
responses directly to test the concrete EvmProvider behavior, or extract the
error-response and revert-data handling logic into a small helper function and
add unit tests for that helper directly.
🪄 Autofix (Beta)
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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5dce9d8e-bcff-4d70-8134-10afea0aebdc
📒 Files selected for processing (22)
CHANGELOG.mddocs/configuration/index.mdxdocs/evm.mdxopenapi.jsonsrc/api/controllers/relayer.rssrc/constants/relayer.rssrc/domain/relayer/evm/evm_relayer.rssrc/domain/relayer/evm/validations.rssrc/domain/transaction/evm/evm_transaction.rssrc/domain/transaction/evm/status.rssrc/jobs/handlers/notification_handler.rssrc/jobs/handlers/relayer_health_check_handler.rssrc/models/relayer/config.rssrc/models/relayer/mod.rssrc/models/relayer/repository.rssrc/models/relayer/request.rssrc/models/relayer/response.rssrc/repositories/relayer/mod.rssrc/repositories/relayer/relayer_in_memory.rssrc/repositories/relayer/relayer_redis.rssrc/services/provider/evm/mod.rssrc/utils/mocks.rs
- get_call_revert_data: forward non-revert ErrorResp as Err so retriable JSON-RPC codes (rate limit, internal error) hit retry/failover instead of being silently reported as "no revert data" - bound recovered revert hex embedded in status_reason to avoid oversized DB/notification writes from RPC/contract-controlled payloads - preserve fee context (gas price / EIP-1559 caps) in the eth_call fallback request so contracts branching on tx.gasprice revert as they did on-chain - use DEFAULT_EVM_INCLUDE_REVERT_DATA instead of hard-coded true - assert include_revert_data propagation in conversion round-trip tests Signed-off-by: Dylan Kilkenny <dylankilkenny95@gmail.com>
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.
What & why
Closes #781.
Today a failed EVM transaction records a generic
status_reason:Transaction reverted on-chain (receipt status: failed)— enough to know that it reverted, not why. Every downstream consumer (dashboards, alerting) has to re-derive the revert reason client-side viadebug_traceTransaction/eth_call, which only works on capable RPCs and adds a round-trip per failure.This change surfaces the on-chain revert payload (4-byte selector + ABI-encoded args) directly on the failed transaction:
How
On the transition into
Failed(only once per tx), recovery runs best-effort:debug_traceTransaction(hash, {tracer: callTracer}), reading the top-leveloutput.eth_callat the receipt's execution block; the revert bytes come from the JSON-RPC errordata.A new mockable
EvmProviderTrait::get_call_revert_dataextracts the revert bytes fromRpcError::ErrorRespbefore the lossyFrom<RpcError>conversion (which drops thedatafield).All recovery is gated by a new per-relayer EVM policy
include_revert_data(default enabled), plumbed through config → domain model → API response → OpenAPI, mirroringgas_limit_estimation. Set it tofalseto skip the extra RPC calls on cost-sensitive deployments.Recovery errors are always contained (logged, never propagated), so a recovery failure never changes the Failed outcome.
Tests
MockEvmProvider): trace-hit, eth_call fallback, both-fail→generic, already-Failed→skip, disabled→zero recovery RPCs, enabled (None/Some(true))→attempted.cargo test --lib,cargo fmt --check,cargo clippy --liball pass.Caveats / follow-ups
get_call_revert_data'sErrorResp→as_revert_dataextraction and thecallTracerJSON shape are not verified against a live node. Amockito/anvil test on the realEvmProviderwould close this gap.callTraceroutputsemantics are based on Geth docs; client quirks are why theeth_callfallback exists.Summary by CodeRabbit
Release Notes
New Features
include_revert_dataper-relayer policy option (enabled by default) to disable revert data recoveryDocumentation
API Updates