Skip to content

fix(rpc)!: align eth_createAccessList response with execution-apis spec#962

Merged
songgaoye merged 10 commits into
developfrom
song/fix_eth_createAccessList
May 28, 2026
Merged

fix(rpc)!: align eth_createAccessList response with execution-apis spec#962
songgaoye merged 10 commits into
developfrom
song/fix_eth_createAccessList

Conversation

@songgaoye
Copy link
Copy Markdown

@songgaoye songgaoye commented May 25, 2026

Description

Fixes the eth_createAccessList response to align with the execution-apis spec.

Problems fixed

  1. gasUsed was decimal, not hexAccessListResult.GasUsed in x/evm/types was uint64, so the internal JSON wire format serialized it as 21000 instead of "0x5208". Changed to hexutil.Uint64.
  2. VM errors were silently dropped — when the simulated call reverted, res.VmError was never propagated. Added an Error field to AccessListResult and populated it from res.VmError.
  3. Accesslist field renamed to AccessList — corrects the capitalisation to match Go conventions and the spec field name.
  4. Pointer fields in rpc/types.AccessListResult changed to values*ethtypes.AccessList and *hexutil.Uint64 become value types, removing a potential null response for gasUsed.

Breaking changes

  • x/evm/types.AccessListResult: field Accesslist renamed to AccessList; GasUsed type changed from uint64 to hexutil.Uint64.
  • rpc/types.AccessListResult: AccessList and GasUsed changed from pointer to value types.

Files critical to review


For contributor use:

  • Targeted PR against correct branch (see CONTRIBUTING.md)
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Code follows the module structure standards.
  • Wrote unit and integration tests
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/) — n/a
  • Added relevant godoc comments — n/a
  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer

For admin use:

  • Added appropriate labels to PR (ex. WIP, R4R, docs, etc)
  • Reviewers assigned
  • Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

@songgaoye songgaoye requested a review from a team as a code owner May 25, 2026 09:37
@songgaoye songgaoye requested review from XinyuCRO and thomas-nguy and removed request for a team May 25, 2026 09:37
@github-actions

This comment has been minimized.

@github-actions github-actions Bot added the evm label May 25, 2026
@codecov
Copy link
Copy Markdown

codecov Bot commented May 25, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 44.69%. Comparing base (1ecc643) to head (5b40f3e).

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #962      +/-   ##
===========================================
+ Coverage    44.21%   44.69%   +0.47%     
===========================================
  Files          197      197              
  Lines        16927    16930       +3     
===========================================
+ Hits          7485     7567      +82     
+ Misses        8652     8555      -97     
- Partials       790      808      +18     
Files with missing lines Coverage Δ
rpc/backend/tx_info.go 60.35% <100.00%> (+2.77%) ⬆️
rpc/types/types.go 7.89% <ø> (ø)
x/evm/keeper/grpc_query.go 72.19% <100.00%> (+7.71%) ⬆️
x/evm/types/access_list.go 100.00% <ø> (ø)

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@songgaoye
Copy link
Copy Markdown
Author

@claude

@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 25, 2026

Claude finished @songgaoye's task in 2m 28s —— View job


PR Review: fix(rpc): align eth_createAccessList response with execution-apis spec

  • Gather context and read relevant files
  • Review changed files
  • Post review feedback

Summary

This PR correctly aligns eth_createAccessList with the execution-apis spec by:

  1. Changing GasUsed from *hexutil.Uint64 (pointer) to hexutil.Uint64 (value) in rpctypes.AccessListResult.
  2. Adding the Error field to both the RPC response type and the internal evmtypes.AccessListResult so VM errors are surfaced to callers instead of being silently dropped.
  3. Updating grpc_query.go to populate GasUsed as a hex-encoded value and propagate res.VmError.
  4. Adding meaningful test coverage for both success and error paths.

No state-breaking or consensus changes detected. This is a pure RPC-layer change.


Issues Found

Minor — test context leaks (rpc/backend/evm_query_client_test.go lines 336, 342)

The cancel function from context.WithCancel is silently discarded (ctx, _ := context.WithCancel(...)). This will fire a go vet warning and technically leaks the cancel until the goroutine exits. Inline suggestions posted. Fix this →

Minor — GasUsed field naming inconsistency

evmtypes.AccessListResult (in x/evm/types/access_list.go) uses the JSON tag "accessList" but field name Accesslist (lowercase 's'). This inconsistency between the protobuf-internal struct and the RPC-facing rpctypes.AccessListResult (which uses AccessList with uppercase 'L') is a pre-existing issue, but it may confuse future readers. Not a functional bug since JSON tags control serialization.

Observation — coverage gap in x/evm/keeper/grpc_query.go

Codecov reports 4 uncovered lines in grpc_query.go. The new code path (lines 1011–1019) that now includes GasUsed hex-encoding and Error propagation is not covered by gRPC-level tests. The backend-level tests indirectly exercise this via mock, but a unit test hitting the keeper directly would be valuable.


Correctness

The core logic change is correct:

  • res.GasUsed (a uint64 from MsgEthereumTxResponse) is now cast to hexutil.Uint64 before marshaling, which will produce "0x5208" style hex strings in JSON — matching the spec.
  • res.VmError is propagated as Error in the result; the spec expects error to be omitted (or empty) on success and populated on revert, which omitempty on the Error string field handles correctly.
  • rpctypes.AccessListResult.GasUsed changing from *hexutil.Uint64 to hexutil.Uint64 is a breaking JSON serialization change for any existing callers that may rely on the pointer (e.g., null vs "0x0"). For an RPC response type that was already producing gasUsed, this is intentional and correct per spec, but worth noting for downstream consumers.

Verdict

LGTM with minor nits. The change is well-scoped, correctly fixes the spec alignment, and includes test coverage for the happy and error paths. The two inline comments flag context-cancel leaks in test helpers; everything else is cosmetic or pre-existing.
| Branch

Comment thread rpc/backend/evm_query_client_test.go Outdated
@songgaoye
Copy link
Copy Markdown
Author

@claude

@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 25, 2026

Claude finished @songgaoye's task in 2m 13s —— View job


PR Review: fix(rpc): align eth_createAccessList response with execution-apis spec

  • Gather context
  • Read changed files
  • Analyze for bugs, security, correctness
  • Post review feedback

Summary

This PR aligns eth_createAccessList with the execution-apis spec across three layers:

  1. evmtypes.AccessListResult (x/evm/types/access_list.go): Renamed AccesslistAccessList, changed GasUsed uint64hexutil.Uint64 (hex in JSON), added Error string.
  2. rpctypes.AccessListResult (rpc/types/types.go): Changed GasUsed *hexutil.Uint64hexutil.Uint64, added Error string.
  3. grpc_query.go: Cast res.GasUsed to hexutil.Uint64 at the convergence point, propagated res.VmError.
  4. tx_info.go: Updated field access to match the renamed/retyped fields, propagates res.Error.

No consensus/state machine changes. Pure RPC-layer change.


Issues

Minor — Context cancel leaks in new test helpers (rpc/backend/evm_query_client_test.go lines 336, 342)

ctx, _ := context.WithCancel(rpc.ContextWithHeight(1))

The cancel function is silently discarded. This pattern triggers go vet / staticcheck (SA4017 or lostcancel). The fix is:

ctx, cancel := context.WithCancel(rpc.ContextWithHeight(1))
defer cancel()

Note: the same pattern already existed in RegisterTraceCallError (line 354) — that pre-existing instance should also be fixed for consistency.

Minor — rpctypes.AccessListResult.AccessList remains a pointer (*ethtypes.AccessList)

Per the spec, accessList must always be an array (never null). With *ethtypes.AccessList, if the upstream response ever returns a nil access list (e.g., via the mock in tests using AccessList: nil), it serialises as null rather than [] when marshaled to the RPC client. The evmtypes.AccessListResult.AccessList is correctly a non-pointer (ethtypes.AccessList), but the RPC-facing wrapper still uses a pointer. This is pre-existing but a follow-up to make both consistent would close the spec gap completely.

Observation — TestCreateAccessList in grpc_query_test.go lacks a VmError path

The keeper-level test (x/evm/keeper/grpc_query_test.go:TestCreateAccessList) only exercises the success path. A test case that deploys a contract that reverts on call would verify Error is correctly populated at the gRPC level and would close the Codecov gap on the 4 uncovered lines in grpc_query.go.


Correctness

The core changes are correct:

  • hexutil.Uint64(res.GasUsed) produces "0x5208"-style JSON — matches the spec.
  • res.VmError is now exposed as Error in both the internal and RPC result structs; omitempty ensures it is omitted on success.
  • Changing GasUsed from *hexutil.Uint64 to hexutil.Uint64 in rpctypes.AccessListResult is a breaking JSON serialisation change — callers that expected "gasUsed": null on zero gas will now get "gasUsed": "0x0". This is intentional and correct per spec.
  • The field rename AccesslistAccessList in evmtypes.AccessListResult is purely internal (JSON tag "accessList" unchanged), so no wire-format breakage.

Verdict

LGTM with minor nits. The fix is well-scoped and correct. The two inline review suggestions from the previous pass (context cancel leaks on lines 336 and 342) are the only actionable items in new code. Everything else is cosmetic or pre-existing.
| Branch

@crypto-org-chain crypto-org-chain deleted a comment from github-actions Bot May 25, 2026
@songgaoye
Copy link
Copy Markdown
Author

@claude

@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 28, 2026

Claude finished @songgaoye's task in 1m 8s —— View job


PR Review: fix(rpc): align eth_createAccessList response with execution-apis spec

  • Gather context
  • Read changed files
  • Check status of previously flagged issues
  • Post review feedback

Summary

Re-reviewing the current state of the branch after previous rounds of feedback.

All previously flagged issues have been addressed:

Context cancel leaks (previously flagged) — ✅ Fixed. Both RegisterCreateAccessList (line 338) and RegisterCreateAccessListError (line 345) in rpc/backend/evm_query_client_test.go now correctly use ctx, cancel := context.WithCancel(...) followed by defer cancel().

VmError path in keeper test (previously flagged as gap) — ✅ Addressed. TestCreateAccessList_VmError (x/evm/keeper/grpc_query_test.go:2783) exercises the revert path end-to-end: calls transfer with a zero-balance caller, unmarshals the gRPC response, and asserts result.Error is non-empty and result.GasUsed is non-zero.

AccessList pointer in rpctypes.AccessListResult (previously flagged) — ✅ Fixed. rpc/types/types.go:196 now uses ethtypes.AccessList (value, not pointer), matching the spec requirement that accessList is always an array and never null.


Current State

The implementation is clean across all three layers:

  • evmtypes.AccessListResult (x/evm/types/access_list.go:73-77): non-pointer AccessList, hexutil.Uint64 GasUsed, Error string with omitempty.
  • rpctypes.AccessListResult (rpc/types/types.go:195-199): same shape, spec-compliant.
  • grpc_query.go:1011-1015: hexutil.Uint64(res.GasUsed) cast and res.VmError propagation are correct.
  • tx_info.go:683-687: passes through res.AccessList, res.GasUsed, res.Error directly — no loss of information.

Verdict

LGTM. All actionable feedback from previous reviews has been addressed. The change is correct, well-tested, and spec-compliant. No state-breaking or consensus changes detected — this is a pure RPC-layer fix.
| Branch

@songgaoye songgaoye changed the title fix(rpc): align eth_createAccessList response with execution-apis spec fix(rpc)!: align eth_createAccessList response with execution-apis spec May 28, 2026
@songgaoye songgaoye merged commit f3a58a0 into develop May 28, 2026
52 of 54 checks passed
@songgaoye songgaoye deleted the song/fix_eth_createAccessList branch May 28, 2026 05:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants