Repository navigation
fix(evm): Preserve JSON-RPC error data on plugin RPC responses - #915
Conversation
Alloy's RpcError→ProviderError mapping dropped error.data, so failed simulations (e.g. ERC-4337 FailedOp) reached plugins as opaque reverts. Pass the upstream data field through to JsonRpcError.
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
⛔ Files ignored due to path filters (1)
📒 Files selected for processing (15)
Included review availability: This review used your included allowance. 4 included reviews remain after this review. Your included PR review attempts over the past 7 days set your current allowance at 5 reviews per hour. WalkthroughProvider error conversions now retain optional upstream JSON-RPC error data. EVM and Stellar relayers pass that data into error responses when present. The JSON-RPC error model and OpenAPI schema describe the optional field. ChangesJSON-RPC Error Data
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix Sequence Diagram(s)sequenceDiagram
participant Provider as Upstream JSON-RPC provider
participant Conversion as Provider error conversion
participant Relayer as EVM relayer
participant Helper as create_error_response_with_data
participant Response as JsonRpcResponse
Provider->>Conversion: JSON-RPC error with optional data
Conversion->>Relayer: ProviderError::RpcErrorCode
Relayer->>Helper: error details and optional data
Helper->>Response: JsonRpcError with optional data
Suggested reviewers: Merge Risk: ⚪ Minimal · up to The change appears mergeable after normal checks; no actionable failure in JSON-RPC error-data forwarding is established. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
A rabbit hops through errors bright Comment |
Restore original CHANGELOG wording and exclude it from typos so pre-commit does not rewrite past release notes.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Exercise Stellar Call/raw-path mapping and ProviderError serialization so patch coverage includes the new RpcErrorCode.data field.
Piscina may run pool-executor from a temp path, so ambient require cannot see plugins/node_modules for compiler-externalized @openzeppelin/relayer-sdk.
Regression for compiler-externalized @openzeppelin/relayer-sdk resolving via pluginsRequire when executePlugin runs the plugin factory.
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The plugin loader and Stellar response boundary need regression coverage, and the changelog exclusion is overly broad.
Review effort: Balanced
Findings: 2
Open (3)
What changed in this PR
Preserves upstream JSON-RPC error data for plugin RPC responses and fixes SDK resolution in temporary Piscina workers.
Changes:
- Propagates JSON-RPC
datathrough EVM and Stellar provider errors. - Extends response models, OpenAPI schema, dependencies, and tests.
- Resolves externalized plugin SDK packages from
plugins/node_modules.
| File | Description |
|---|---|
Cargo.toml |
Enables JSON-RPC support and test dependency. |
Cargo.lock |
Locks dependency updates. |
CHANGELOG.md |
Documents the RPC-data fix. |
openapi.json |
Adds optional error data schema. |
plugins/lib/pool-executor.ts |
Fixes external package resolution. |
src/domain/relayer/evm/evm_relayer.rs |
Returns EVM provider error data. |
src/domain/relayer/evm/rpc_utils.rs |
Builds responses with optional data. |
src/domain/relayer/stellar/stellar_relayer.rs |
Returns Stellar provider error data. |
src/models/rpc/json_rpc.rs |
Adds the error data field. |
src/services/gas/fetchers/polygon_zkevm.rs |
Updates error construction. |
src/services/gas/handlers/polygon_zkevm.rs |
Updates error construction. |
src/services/provider/evm/mod.rs |
Updates EVM provider tests. |
src/services/provider/mod.rs |
Preserves Alloy RPC error data. |
src/services/provider/retry.rs |
Updates retry tests. |
src/services/provider/stellar/mod.rs |
Preserves Stellar RPC error data. |
src/utils/error_sanitization.rs |
Updates sanitization tests. |
typos.toml |
Excludes the changelog from typo checks. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Add Stellar relayer rpc data preservation coverage, exercise executePlugin require for the SDK, and allowlist legacy CHANGELOG typos instead of excluding the whole file.
Bump Node 20.20, piscina, axios, rustls, quinn-proto, and hickory-resolver. Ignore AWS-transitive rustls-webpki and aws-smithy-json advisories that need MSRV 1.94+ or rustls 0.22+ to remediate.
zeljkoX
left a comment
zeljkoX
left a comment
There was a problem hiding this comment.
Thanks, the core fix looks good. Keeping error.data on RpcErrorCode and passing it through is the right approach, and the anvil result (AA32 instead of OPAQUE_REVERT) confirms it works. A few things I'd like addressed before merge:
1. The SDK-resolution test doesn't catch the regression
pool-executor-sdk-require.test.ts passes even without the fix. Jest runs with cwd plugins/, and pool-executor is imported from plugins/lib/, so the old ambient require already reaches plugins/node_modules by walking up the tree. The test also can't tell cwd/plugins/package.json apart from cwd/package.json: under Jest the root is plugins/plugins/package.json, which doesn't exist and only resolves by walking up.
pluginsRequire is created when the worker module loads, so changing cwd after an in-process import does not affect it. For a real regression test, compile the worker into os.tmpdir() the way compileExecutorOnTheFly does, and spawn that file with cwd set to the parent of plugins/ (the layout the relayer actually uses). Alternatively, let the plugins root be passed in, so the test can set it explicitly. The first test only checks how Node resolves modules, not our code, and it could break on machines with a node_modules above the temp dir. I'd drop it.
2. Stellar forwards "data": null
In json_rpc_error_to_provider_error, error.get("data").cloned() keeps JSON null as Some(Value::Null), and skip_serializing_if = "Option::is_none" doesn't skip that, so clients get "data": null. The Alloy and jsonrpsee paths parse into an Option, which already turns null into None. Suggest:
data: error.get("data").filter(|v| !v.is_null()).cloned(),3. data skips sanitization
map_provider_error / sanitize_error_description deliberately hide upstream messages, but data now goes out to clients unchanged. That's fine for EVM revert hex, and the endpoint requires an API key. Stellar data, though, can include diagnostic events, and some providers put free-form text there (including nested objects, not only hex). A hex-only allowlist would drop those. Could we add a size cap, or at least a comment saying the exemption is intentional?
4. Changelog
Only the EVM change is listed. Please add the Stellar passthrough and the pool-executor fix. For the latter, it's worth noting that it only affects on-the-fly compilation (missing plugins/lib/pool-executor.js). The production image builds pool-executor.js at install time, so prod wasn't hitting this.
5. typos.toml
Adding these under [default.extend-words] allows the misspellings across the whole repo. Please don't exclude CHANGELOG.md entirely. Scope the allowlist to that file:
[type.changelog]
extend-glob = ["CHANGELOG.md"]
extend-words = { intristic = "intristic", exectution = "exectution", concurency = "concurency", transfering = "transfering", persistance = "persistance" }Nits
alloygets thejson-rpcfeature on the main dependency only so a test can build anErrorPayload. It's harmless (alloy-json-rpcis already pulled in viaalloy-provider), but it's a prod feature flag that exists for a test.jsonrpsee-types = "0.26.0"has to stay in step with the soroban client'sjsonrpsee-core. A comment next to it would save someone a confusing type mismatch later.- Callers still get
-32603/"Internal error", with the upstream code only indescription. That's fine for the AA plugin, which decodesdata. A follow-up could pass the original code through whendatais present.
The failing docker-scan check is the existing nodejs-20 CVE issue, not this PR.
zeljkoX
left a comment
zeljkoX
left a comment
There was a problem hiding this comment.
Approving, the core fix is good. Please take a look at the comments above (test coverage, Stellar null data, changelog, typos scoping) before merging.
Make the SDK require test fail without pluginsRequire by spawning a temp-compiled worker from the Relayer cwd, drop JSON-null and oversized error data, scope changelog typo allows, and document Stellar/plugin notes.
This reverts commit 70ebda4.
Summary
When proxying raw/plugin RPC calls, Alloy's
RpcError→ProviderErrorconversion dropped JSON-RPC errordata. Failed EVM simulations (including ERC-4337FailedOp) therefore reached plugins without a decodeable payload, forcing opaque-revert handling and blocking submission.This change:
error.dataonProviderError::RpcErrorCodeJsonRpcError.data(omitted when absent)Distinct from
include_revert_data(mined-txdebug_traceTransactionrecovery).Plugin pool: resolve SDK under Piscina temps
Validating the AA plugin against this Relayer branch also surfaced a separate pool-loader bug.
The plugin compiler leaves
@openzeppelin/relayer-sdkexternal (not bundled). At runtime the worker mustrequire()it fromplugins/node_modules. Whenpool-executor.jsis missing, Piscina compiles the worker on the fly intoos.tmpdir()and Node resolves modules from that temp path—sorequire('@openzeppelin/relayer-sdk')fails withMODULE_NOT_FOUNDeven though the SDK is installed underplugins/.Thin plugins that re-export a package (e.g. AA's
export { handler } from '@openzeppelin/relayer-plugin-aa') hit this path reliably. In-repo examples often still “work” when a prebuiltplugins/lib/pool-executor.jsis present, because thenrequirewalks up intoplugins/node_modules.Fix:
pool-executorpasses acreateRequirerooted atplugins/package.json(and a realplugins/__dirname) into the plugin factory, so externalized packages resolve correctly regardless of the worker file location.Testing Process
From<RpcError>keeps/dropsdatacorrectlycreate_error_response_with_dataserializesdataand omits it whenNonerpcreturns providerRpcErrorCode.dataon the JSON-RPC errorfix/preserve-rpc-error-data) + anvil: plugin pool loads AA;/plugins/aa/call/healthOK; expired paymaster submit →SIMULATION_FAILED/AA32(notOPAQUE_REVERT)Checklist
Note
If you are using Relayer in your stack, consider adding your team or organization to our list of Relayer Users in the Wild!