fix: use empty access list if eth_createAccessList returns error#1799
fix: use empty access list if eth_createAccessList returns error#1799
Conversation
📝 WalkthroughWalkthroughBumped package version to v4.7.13 and changed gas estimation to ignore eth_createAccessList errors (returning the gas limit without an access list), removing the errors import; no other behavioral changes introduced. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
rollup/internal/controller/sender/estimategas.go (1)
156-183: Consider adding a unit test for the nil StorageKeys scenario.While the fix is correct, it would be valuable to add a test case that verifies
finetuneAccessListhandles entries with nilStorageKeyscorrectly, ensuring the returned access list always has non-nil slices.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rollup/internal/controller/sender/estimategas.go` around lines 156 - 183, Add a unit test for finetuneAccessList that constructs an input types.AccessList containing an entry whose StorageKeys is nil (and at least one entry matching the 'to' address and one not), call finetuneAccessList with a known gasLimitWithAccessList and to address, then assert the returned access list pointer is non-nil, every returned types.AccessTuple.StorageKeys is a non-nil slice (length may be zero), entries matching the removed-to logic are omitted from the new list and the gasLimitWithAccessList value is adjusted according to the removal rules; use the finetuneAccessList function and types.AccessList/types.AccessTuple types in the test to locate code under test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@rollup/internal/controller/sender/estimategas.go`:
- Around line 156-183: Add a unit test for finetuneAccessList that constructs an
input types.AccessList containing an entry whose StorageKeys is nil (and at
least one entry matching the 'to' address and one not), call finetuneAccessList
with a known gasLimitWithAccessList and to address, then assert the returned
access list pointer is non-nil, every returned types.AccessTuple.StorageKeys is
a non-nil slice (length may be zero), entries matching the removed-to logic are
omitted from the new list and the gasLimitWithAccessList value is adjusted
according to the removal rules; use the finetuneAccessList function and
types.AccessList/types.AccessTuple types in the test to locate code under test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 38a638bb-0d5c-4259-832c-02b23452f72d
📒 Files selected for processing (2)
common/version/version.gorollup/internal/controller/sender/estimategas.go
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #1799 +/- ##
===========================================
- Coverage 36.41% 36.41% -0.01%
===========================================
Files 248 248
Lines 21321 21325 +4
===========================================
Hits 7765 7765
- Misses 12733 12737 +4
Partials 823 823
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
rollup/internal/controller/sender/estimategas.go (1)
133-140: Add regression coverage for the new fallback contract.These branches now silently degrade to
EstimateGasinstead of failing the send path. Please add focused tests for bothrpcErrand non-emptyerrStrso this behavior change stays intentional and doesn’t regress unnoticed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rollup/internal/controller/sender/estimategas.go` around lines 133 - 140, Add regression tests that assert the send flow falls back to EstimateGas when CreateAccessList returns an RPC error or a non-empty errStr: mock s.gethClient.CreateAccessList to first return a non-nil rpcErr and verify the code path returns gasLimitWithoutAccessList and triggers the EstimateGas fallback, then a separate test where CreateAccessList returns rpcErr nil but errStr non-empty and assert the same fallback behavior; target the function that calls s.gethClient.CreateAccessList (the estimator in estimategas.go) and ensure both branches (rpcErr and non-empty errStr) are covered so future changes to CreateAccessList handling are caught.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@rollup/internal/controller/sender/estimategas.go`:
- Around line 133-140: Add regression tests that assert the send flow falls back
to EstimateGas when CreateAccessList returns an RPC error or a non-empty errStr:
mock s.gethClient.CreateAccessList to first return a non-nil rpcErr and verify
the code path returns gasLimitWithoutAccessList and triggers the EstimateGas
fallback, then a separate test where CreateAccessList returns rpcErr nil but
errStr non-empty and assert the same fallback behavior; target the function that
calls s.gethClient.CreateAccessList (the estimator in estimategas.go) and ensure
both branches (rpcErr and non-empty errStr) are covered so future changes to
CreateAccessList handling are caught.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ece63717-b7b8-4e5f-8829-e3d3f4e0baf1
📒 Files selected for processing (1)
rollup/internal/controller/sender/estimategas.go
Thegaram
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rollup/internal/controller/sender/estimategas.go (1)
164-185:⚠️ Potential issue | 🟠 MajorInitialize the rebuilt access list as empty, not nil.
The code normalizes
StorageKeysat the tuple level, but if every entry is filtered out,newAccessListremains nil. Since Go'sencoding/jsonmarshals nil slices asnulland non-nil empty slices as[], andtypes.AccessListhas no custom marshaling, the caller can still receivenullinstead of[]on the downstream signer/RPC path, which may cause compatibility issues.Initialize with a pre-allocated capacity instead:
Suggested fix
- var newAccessList types.AccessList + newAccessList := make(types.AccessList, 0, len(*accessList)) for _, entry := range *accessList {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rollup/internal/controller/sender/estimategas.go` around lines 164 - 185, newAccessList is currently declared nil so if every access-list entry is filtered it will marshal as JSON null; change its initialization to a non-nil empty slice (e.g., create newAccessList with make(types.AccessList, 0, len(*accessList)) or make(types.AccessList, 0)) before the loop to ensure it serializes as [] and keep the rest of the loop logic (the StorageKeys normalization and the return of &newAccessList) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@rollup/internal/controller/sender/estimategas.go`:
- Around line 133-145: Add unit tests that exercise the new fallback semantics
in estimateGasLimit and ensure getFeeData propagation remains correct: mock/stub
s.gethClient.CreateAccessList to return (1) a non-nil rpcErr, (2) a non-empty
errStr, and (3) a successful accessList with gasLimitWithAccessList; call
estimateGasLimit (or the higher-level getFeeData flow that invokes
estimateGasLimit) and assert that in cases (1) and (2) the function returns
gasLimitWithoutAccessList and no error (i.e., the code falls back to sending
without an access list), and in case (3) it returns the gasLimitWithAccessList
and the access list; ensure tests reference estimateGasLimit,
s.gethClient.CreateAccessList and getFeeData/sender submission path to validate
end-to-end behavior.
---
Outside diff comments:
In `@rollup/internal/controller/sender/estimategas.go`:
- Around line 164-185: newAccessList is currently declared nil so if every
access-list entry is filtered it will marshal as JSON null; change its
initialization to a non-nil empty slice (e.g., create newAccessList with
make(types.AccessList, 0, len(*accessList)) or make(types.AccessList, 0)) before
the loop to ensure it serializes as [] and keep the rest of the loop logic (the
StorageKeys normalization and the return of &newAccessList) unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 458faee6-0c81-49eb-aa37-ccfad99a350b
📒 Files selected for processing (1)
rollup/internal/controller/sender/estimategas.go
There was a problem hiding this comment.
♻️ Duplicate comments (1)
rollup/internal/controller/sender/estimategas.go (1)
133-145:⚠️ Potential issue | 🟠 MajorAdd regression coverage for the fallback path before merging.
These branches now silently downgrade
CreateAccessListfailures to “send without access list”, but this file still has 0% patch coverage. Please cover therpcErr,errStr, and success paths throughestimateGasLimitor the higher-level fee-data flow so this behavior change stays pinned down.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rollup/internal/controller/sender/estimategas.go` around lines 133 - 145, Add unit/integration tests exercising the CreateAccessList fallback by driving s.gethClient.CreateAccessList to return (1) a non-nil rpcErr, (2) a non-empty errStr, and (3) a successful accessList+gasLimitWithAccessList result, then assert estimateGasLimit (or the higher-level fee-data flow that calls estimateGasLimit) returns the gasLimitWithoutAccessList and no access list for cases (1) and (2) and the access-list gas for case (3). Mock or stub the gethClient used by sender to control CreateAccessList responses, and cover the rpcErr, errStr, and success branches so the fallback behavior is exercised.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@rollup/internal/controller/sender/estimategas.go`:
- Around line 133-145: Add unit/integration tests exercising the
CreateAccessList fallback by driving s.gethClient.CreateAccessList to return (1)
a non-nil rpcErr, (2) a non-empty errStr, and (3) a successful
accessList+gasLimitWithAccessList result, then assert estimateGasLimit (or the
higher-level fee-data flow that calls estimateGasLimit) returns the
gasLimitWithoutAccessList and no access list for cases (1) and (2) and the
access-list gas for case (3). Mock or stub the gethClient used by sender to
control CreateAccessList responses, and cover the rpcErr, errStr, and success
branches so the fallback behavior is exercised.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 33cebab8-b00e-458d-8630-539aef2bd9f7
📒 Files selected for processing (1)
rollup/internal/controller/sender/estimategas.go
3 participants
Purpose or design rationale of this PR
Symptom:
Root cause: ethereum/go-ethereum#33656 adds
slices.SortedFuncinside access list tracer, which returnsnilif the input slice is empty.Technically it's a bug in upstream geth, but we make our code more robust and defensive.
PR title
Your PR title must follow conventional commits (as we are doing squash merge for each PR), so it must start with one of the following types:
Deployment tag versioning
Has
tagincommon/version.gobeen updated or have you addedbump-versionlabel to this PR?Breaking change label
Does this PR have the
breaking-changelabel?Summary by CodeRabbit