feat: support create_access_list #1932
Conversation
thomas-nguy
requested review from
JayT106 and
XinyuCRO
and removed request for
a team
WalkthroughAdds an access-list integration test and a small Solidity test contract, registers the contract in test assets, updates related integration test code, and bumps/redirects ethermint module versions; changelog notes create_access_list support. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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 |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
gomod2nix.toml (1)
315-317: gomod2nix Ethermint pin matches go.mod; remember to realign when reverting the forkThe updated version/hash and
replaced = "github.com/thomas-nguy/ethermint"are consistent with the go.mod replace, so Nix builds should stay in sync. Once Ethermint changes are moved to an org‑managed repo (see go.mod comment), this entry will also need to be updated back to the org fork.
🧹 Nitpick comments (2)
CHANGELOG.md (1)
5-9: Changelog entry is fine; optional wording tweakThe new improvement entry is formatted consistently. If you want to be more explicit, you could rename it to “Support
eth_createAccessList” to match the JSON‑RPC method name, but it’s not strictly necessary.integration_tests/test_basic.py (1)
1126-1195: Access‑list test covers the right behavior; consider computing the mapping slot dynamicallyThe structure of
test_access_listis solid:
- It verifies that
eth_sendTransactionwithaccessListround‑trips viaeth_getTransaction.- It then uses
eth_createAccessListon aTestAccessList.touchSlotscall and asserts that the contract address and 3 storage keys (2 plain slots + 1 mapping entry) are present.Two small improvements to make the test less brittle:
- Compute the mapping storage key instead of hard‑coding it
Hard‑coding
"0x30939a3d..."ties the test to a specific storage layout andADDRS["validator"]. If either changes, the test silently becomes wrong. You can derive the key using the standard mapping formula:- storage_keys = access_list["result"]["accessList"][0]["storageKeys"] - assert len(storage_keys) == 3 - assert ( - "0x0000000000000000000000000000000000000000000000000000000000000000".lower() - in [k.lower() for k in storage_keys] - ) - assert ( - "0x0000000000000000000000000000000000000000000000000000000000000001".lower() - in [k.lower() for k in storage_keys] - ) - assert ( - "0x30939a3db93623b4b889c7802487ba975ac3bc59182f4c2dff55b302788334ec".lower() - in [k.lower() for k in storage_keys] - ) + storage_keys = access_list["result"]["accessList"][0]["storageKeys"] + assert len(storage_keys) == 3 + normalized_keys = [k.lower() for k in storage_keys] + + # Slots 0 and 1 (slotA, slotB) + assert "0x" + "0" * 64 in normalized_keys + assert "0x" + "0" * 63 + "1" in normalized_keys + + # balances[msg.sender] mapping slot: keccak(address, 2) + mapping_slot = 2 + mapping_key = Web3.solidity_keccak( + ["address", "uint256"], [ADDRS["validator"], mapping_slot] + ).hex().lower() + assert mapping_key in normalized_keysThis keeps the assertion logically equivalent but decouples it from the magic constant.
- Refresh the inline comment
The comment
# Build data field for set(123)no longer matchestouchSlots(123, 456). Renaming it (e.g., “Build data field for touchSlots(123, 456)”) will avoid confusion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (6)
CHANGELOG.md(1 hunks)go.mod(1 hunks)gomod2nix.toml(1 hunks)integration_tests/contracts/contracts/TestAccessList.sol(1 hunks)integration_tests/test_basic.py(1 hunks)integration_tests/utils.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
integration_tests/test_basic.py (2)
integration_tests/network.py (1)
w3(39-42)integration_tests/utils.py (2)
send_transaction(468-471)deploy_contract(423-443)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (18)
- GitHub Check: build (macos-14)
- GitHub Check: gomod2nix
- GitHub Check: build (ubuntu-latest)
- GitHub Check: build (macos-latest)
- GitHub Check: unittest
- GitHub Check: Run golangci-lint
- GitHub Check: integration_tests (gov)
- GitHub Check: integration_tests (mint)
- GitHub Check: integration_tests (upgrade)
- GitHub Check: integration_tests (gas)
- GitHub Check: integration_tests (ica)
- GitHub Check: integration_tests (slow)
- GitHub Check: integration_tests (ibc_timeout)
- GitHub Check: integration_tests (ibc_update_client)
- GitHub Check: integration_tests (ibc_rly_gas)
- GitHub Check: integration_tests (ibc_rly_evm)
- GitHub Check: integration_tests (unmarked)
- GitHub Check: integration_tests (ibc)
🔇 Additional comments (3)
integration_tests/utils.py (1)
49-75: TestAccessList wiring into TEST_CONTRACTS looks correctThe new
"TestAccessList": "TestAccessList.sol"entry matches the artifact path convention (contracts/artifacts/contracts/<name>.sol/<name>.json) and integrates cleanly with the existingCONTRACTSmapping.integration_tests/contracts/contracts/TestAccessList.sol (1)
1-14: TestAccessList storage layout matches the intended access‑list test
slotA,slotB, and thebalancesmapping are laid out in distinct slots, andtouchSlotswrites to each of them, which is exactly what you need for exercisingeth_createAccessListover three storage keys. The contract is minimal and fit for purpose.go.mod (1)
293-307: Avoid pinning Ethermint to a personal fork in main go.modReplacing
github.com/evmos/ethermintwithgithub.com/thomas-nguy/ethermintis reasonable for development, but merging intomainties the build and supply chain to an individual user account rather than an org‑controlled repo. This creates maintenance, security, and sustainability risks.Consider:
- Moving changes to
github.com/crypto-org-chain/ethermintand updating thereplacedirective there, or- Establishing a timeline to merge these changes upstream and removing the replacement once complete.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1932 +/- ##
===========================================
+ Coverage 16.87% 37.13% +20.25%
===========================================
Files 72 127 +55
Lines 6163 9757 +3594
===========================================
+ Hits 1040 3623 +2583
- Misses 5000 5713 +713
- Partials 123 421 +298 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (3)
go.mod(2 hunks)gomod2nix.toml(2 hunks)integration_tests/test_basic.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- integration_tests/test_basic.py
- gomod2nix.toml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: integration_tests (ibc_timeout)
- GitHub Check: integration_tests (mint)
- GitHub Check: integration_tests (gas)
- GitHub Check: integration_tests (slow)
- GitHub Check: integration_tests (gov)
- GitHub Check: integration_tests (ibc_update_client)
- GitHub Check: integration_tests (upgrade)
- GitHub Check: integration_tests (ica)
- GitHub Check: integration_tests (ibc_rly_gas)
- GitHub Check: integration_tests (ibc_rly_evm)
- GitHub Check: build (macos-14)
- GitHub Check: integration_tests (unmarked)
- GitHub Check: integration_tests (ibc)
- GitHub Check: build (macos-latest)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: unittest
- GitHub Check: gomod2nix
- GitHub Check: Run golangci-lint
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (1)
go.mod (1)
38-38: Minor version bump for spf13/cobra — routine maintenance.The cobra CLI framework has been bumped from v1.10.1 to v1.10.2. This is a patch-level update and represents routine maintenance.
thomas-nguy
force-pushed
the
thomas/access-list-support
branch
from
4b79feb to
e1ed497
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
integration_tests/contracts/contracts/TestAccessList.sol (1)
9-13: Considerexternalvisibility for minor gas savings.Since
touchSlotsis not called internally, declaring it asexternalinstead ofpublicwould provide a minor gas optimization. However, for a test contract, this is an optional refinement.- function touchSlots(uint256 a, uint256 b) public { + function touchSlots(uint256 a, uint256 b) external { slotA = a; slotB = b; balances[msg.sender] = a + b; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (6)
CHANGELOG.md(1 hunks)go.mod(2 hunks)gomod2nix.toml(1 hunks)integration_tests/contracts/contracts/TestAccessList.sol(1 hunks)integration_tests/test_basic.py(1 hunks)integration_tests/utils.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- CHANGELOG.md
- integration_tests/utils.py
- go.mod
- gomod2nix.toml
- integration_tests/test_basic.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (17)
- GitHub Check: Socket Security: Pull Request Alerts
- GitHub Check: integration_tests (gas)
- GitHub Check: integration_tests (mint)
- GitHub Check: integration_tests (evm)
- GitHub Check: integration_tests (slow)
- GitHub Check: integration_tests (gov)
- GitHub Check: integration_tests (ica)
- GitHub Check: integration_tests (upgrade)
- GitHub Check: integration_tests (ibc)
- GitHub Check: integration_tests (unmarked)
- GitHub Check: integration_tests (ibc_rly_evm)
- GitHub Check: integration_tests (ibc_timeout)
- GitHub Check: integration_tests (ibc_rly_gas)
- GitHub Check: integration_tests (ibc_update_client)
- GitHub Check: build (macos-14)
- GitHub Check: build (macos-latest)
- GitHub Check: Run golangci-lint
3 participants
👮🏻👮🏻👮🏻 !!!! REFERENCE THE PROBLEM YOUR ARE SOLVING IN THE PR TITLE AND DESCRIBE YOUR SOLUTION HERE !!!! DO NOT FORGET !!!! 👮🏻👮🏻👮🏻
PR Checklist:
make)make test)go fmt)golangci-lint run)go list -json -m all | nancy sleuth)Thank you for your code, it's appreciated! :)
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.