feat(engine): forward eth_call ErrorResp.data into HostError.data (COW-1082)#58
Closed
brunota20 wants to merge 1 commit into
Closed
Conversation
…W-1082) The chain backend previously dropped alloy's structured `RpcError::ErrorResp` payload on the floor — the formatted error string went into `HostError.message`, but `HostError.data` stayed `None` and `HostError.code` was hard-coded to `-32603`. That made the twap-monitor's poll-time revert classifier inert on real traffic: `OrderNotValid` / `PollNever` / `PollTryAtBlock` / `PollTryAtEpoch` all fell through to `TryNextBlock` because `decode_revert_hex` only fires on a non-empty `err.data`. This change wires the structured payload through end-to-end. ## Changes - `crates/nexum-engine/src/host/provider_pool.rs`: when alloy's `provider.raw_request` fails with an `RpcError::ErrorResp`, the pool now captures both `payload.code` (as `Option<i64>` so we can distinguish "no ErrorResp" from "ErrorResp with code 0") and `payload.data` (as `Option<String>`, the JSON-encoded revert hex) and surfaces them on `ProviderError::Rpc`. Transport-side failures (timeout, websocket disconnect) leave both `None`. The two subscribe paths (`subscribe_blocks`, `subscribe_logs`) keep `code: None, data: None` since they don't carry an ErrorResp. - `crates/nexum-engine/src/host/impls/chain.rs`: extract the `ProviderError -> HostError` projection into a free helper `provider_error_to_host_error`. The `Rpc` arm forwards the structured `data` verbatim, preserves the node-reported code (saturating out-of-`i32` values to `-32603`), and falls back to `-32603` only when no `ErrorResp` was present. Five unit tests cover: revert with data, transport failure with `None`, out-of-range code, unknown-chain, and invalid-params. - `modules/twap-monitor/src/strategy.rs`: update the stale comment on the `decode_revert_hex` branch — that branch is now live on real traffic, the only `None` path is transport-level failures (which keep the safe `TryNextBlock` default). ## Why this is M4, not correctness No incorrect order is ever submitted (the contract reverts; the orderbook never sees a bad body). The issue is pruning efficiency: a permanently dead TWAP watch was re-polled every block until a submit eventually failed for an unrelated reason, and the local-store filled with `watch:` entries the strategy could otherwise drop on the first revert. With this fix the SDK-side classifier dispatches `Drop` / gate on the first revert, matching the documented expectation in `docs/adr/0007-upstream-protocol-logic-to-cow-rs.md`. ## Tests - 70/70 nexum-engine tests pass - 23/23 twap-monitor tests pass - 5/5 new chain.rs projection tests pass (revert-with-data, transport-fail, out-of-range-code, unknown-chain, invalid-params) - `cargo clippy -p nexum-engine -p twap-monitor --all-targets -- -D warnings` clean ## Surfaced by jeffersonBastos's PR #55 (M3 mirror) review, thread on `modules/twap-monitor/src/strategy.rs:189`. The mirror of this fix on the cow-api side is COW-1075 (already merged via PR #48). AI-assisted authoring with Claude (Opus 4.7); reviewed end-to-end and validated against the existing twap-monitor strategy tests before push.
This was referenced Jun 22, 2026
Author
This was referenced Jun 24, 2026
lgahdl
added a commit
that referenced
this pull request
Jul 10, 2026
Review follow-ups - three of the six tests could not fail for the property they claimed to verify: - run() now returns its (blocks, chain_logs) dispatch tally (the same numbers the shutdown log line reports). The no-starvation test asserts both queued events were drained instead of merely observing that run() terminates on the shutdown timer - a broken or reordered select arm now leaves a count at 0 and fails. - New direct test: a reconnect task parked on a dropped receiver exits with TaskExit::ReceiverGone on its own, joined on the bare handle. The previous test claimed to verify this through TaskSet::shutdown, which aborts every handle before joining, so ReceiverGone was never exercised; its doc comment now states the abort-then-join contract it actually proves. - harness_shutdown_after_push_completes_cleanly raced shutdown against dispatch and accepted either outcome. Replaced with harness_shutdown_preserves_completed_dispatch: prove the dispatch completed, tear down, then re-read the log record after teardown. - The harness no-starvation test now queues both event kinds before awaiting either, so the biased select genuinely arbitrates two ready streams instead of replaying the two pre-existing sequential tests. - New harness_delivers_blocks_in_push_order: blocks 7,8,9 pushed back-to-back must surface in the module's log records in that order - issue #56's ordering guarantee, previously untested. - run()-level tests wrapped in tokio::time::timeout so a shutdown-path hang fails in 10 s instead of stalling the suite to the CI job limit. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TfSsJPYDQ1GQGbnSVFxATx
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
RpcError::ErrorResppayload —HostError.datawas alwaysNoneandcodealways-32603. The twap-monitor'sdecode_revert_hexbranch was inert on real traffic; every revert fell through toTryNextBlock. This change wires the structured payload through end-to-end:provider_pool.rscapturespayload.code+payload.datafrome.as_error_resp()on the alloy side,chain.rsprojects them intoHostError, and the stale comment intwap-monitor/src/strategy.rsis updated to reflect the now-live code path.codeisOption<i64>on theProviderError::Rpcenvelope so we can distinguish "no ErrorResp" (transport-level failure) from "ErrorResp withcode = 0". Out-of-i32codes saturate to-32603(the previous Internal-error fallback). The newprovider_error_to_host_errorhelper makes the projection unit-testable.PollOutcome::DontTryAgain/ gate, rather than getting re-polled every block until something unrelated fails.Why this is M4
Surfaced by jeffersonBastos's PR #55 (M3 mirror) review, thread on
modules/twap-monitor/src/strategy.rs:189. Mirror of the cow-api-side fix in COW-1075 (PR #48, already merged) — same pattern applied to the chain side: structured error envelope →HostError.dataso the SDK classifier dispatches typed outcomes.Tests
chain.rsprojection tests (revert-with-data, transport-fail, out-of-range-code, unknown-chain, invalid-params)cargo clippy -p nexum-engine -p twap-monitor --all-targets -- -D warningscleanStacked on
PR #57 (
feat/baseline-latency-cow-1084) → upstream M4 epic (TBD).AI-assistance disclosure
Authored with Claude (Opus 4.7); reviewed end-to-end and validated against the existing twap-monitor strategy tests before push.