feat: support eip2935 #1882
feat: support eip2935 #1882
Conversation
thomas-nguy
requested review from
calvinaco and
songgaoye
and removed request for
a team
WalkthroughAdds EIP‑2935 support and tests: bumps a few Go module versions, adds a proto field for history serving, introduces a HistoryStorage preinstall integration test and helper Solidity contract, extends test utils and config, and updates the changelog. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Tester
participant Gov as Gov Module
participant Registry as Preinstall Registry
participant EVM as EVM Runtime
participant HS as HistoryStorage (address)
participant C as TestEip2935
rect rgba(230,240,255,0.35)
Tester->>Gov: Submit preinstall registration proposal (HistoryStorage)
Gov-->>Registry: On approval, register preinstall code at HS address
Registry-->>EVM: Update preinstall mapping
end
rect rgba(240,255,230,0.35)
Tester->>EVM: Deploy/Test C
Tester->>C: call blockhashOpcode(n)
C->>EVM: execute BLOCKHASH opcode
EVM->>HS: Query history storage / serve window
HS-->>EVM: Return stored block hash
EVM-->>C: Deliver bytes32 result
C-->>Tester: Return and verify hash
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
⏰ 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). (21)
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.
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1882 +/- ##
===========================================
+ Coverage 16.87% 35.79% +18.91%
===========================================
Files 72 127 +55
Lines 6163 11812 +5649
===========================================
+ Hits 1040 4228 +3188
- Misses 5000 7161 +2161
- Partials 123 423 +300 🚀 New features to boost your workflow:
|
![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
🧹 Nitpick comments (6)
CHANGELOG.md (1)
8-8: Clarify scope of EIP‑2935 changes in changelogConsider expanding this entry to mention the HistoryStorage preinstall registration and the new
history_serve_windowparam (plus the ethermint replace), for easier auditability.go.mod (1)
309-309: Ethermint replace points to a personal fork; verify provenance and pinningUsing
github.com/thomas-nguy/ethermintis risky for supply‑chain and long‑term maintenance. Prefer:
- A branch/tag under your org (e.g., crypto-org-chain/ethermint) or a specific commit SHA, and
- A tracking issue to upstream/backport.
Verify that gomod2nix is in sync and CI reproducible builds pass with this replace.
gomod2nix.toml (1)
318-321: Keep gomod2nix in lockstep with go.mod replaceThe module mapping now targets the ethermint fork. Ensure:
gomod2nixwas regenerated from this PR branch,- Nix builds work end‑to‑end in CI.
If you plan to move the fork under the org, remember to update this block accordingly.
integration_tests/test_eip2935.py (3)
1-11: Fix import order to satisfy isortGroup third‑party and local imports, and keep them sorted.
-from hexbytes import HexBytes - -from .cosmoscli import module_address -from .network import Cronos -from .utils import ( - CONTRACTS, - deploy_contract, - submit_gov_proposal, - w3_wait_for_block, -) +from hexbytes import HexBytes + +from .cosmoscli import module_address +from .network import Cronos +from .utils import CONTRACTS, deploy_contract, submit_gov_proposal, w3_wait_for_block
22-25: Wrap long hex string to pass line length limitsBreak the literal across more lines to satisfy E501.
- expected_history_storage_code = ( - "0x3373fffffffffffffffffffffffffffffffffffffffe14604657602036036042575f3560014303811160" - "4257611fff81430311604257611fff9006545f5260205ff35b5f5ffd5b5f35611fff60014303065500" - ) + expected_history_storage_code = ( + "0x3373fffffffffffffffffffffffffffffffffffffffe14604657602036036042575f3560014303811160" + "4257611fff81430311604257611fff9006545f5260205ff35b5f5ffd5b" + "5f35611fff60014303065500" + )
57-60: Use the loop index; fix unused variable and strengthen the checkCurrently
iis unused andstart + 0is constant. Compare multiple consecutive blocks.- for i in range(0, 9): - block = w3.eth.get_block(start + 0) - stored = contract.caller.blockhashOpcode(start + 0) + for i in range(0, 9): + block = w3.eth.get_block(start + i) + stored = contract.caller.blockhashOpcode(start + i) assert stored == block.hash
📜 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 (7)
CHANGELOG.md(1 hunks)go.mod(1 hunks)gomod2nix.toml(1 hunks)integration_tests/contracts/contracts/TestEip2935.sol(1 hunks)integration_tests/test_eip2935.py(1 hunks)integration_tests/utils.py(1 hunks)third_party/proto/ethermint/evm/v1/params.proto(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
integration_tests/test_eip2935.py (2)
integration_tests/utils.py (4)
module_address(746-748)deploy_contract(421-441)submit_gov_proposal(224-236)w3_wait_for_block(267-279)integration_tests/network.py (2)
Cronos(16-69)w3(39-42)
🪛 GitHub Check: Lint python
integration_tests/test_eip2935.py
[failure] 24-24:
./integration_tests/test_eip2935.py:24:89: E501 line too long (92 > 88 characters)
[failure] 23-23:
./integration_tests/test_eip2935.py:23:89: E501 line too long (96 > 88 characters)
[failure] 11-11:
./integration_tests/test_eip2935.py:11:1: I005 isort found an unexpected missing import
[failure] 10-10:
./integration_tests/test_eip2935.py:10:1: I001 isort found an import in the wrong position
[failure] 9-9:
./integration_tests/test_eip2935.py:9:1: I001 isort found an import in the wrong position
[failure] 8-8:
./integration_tests/test_eip2935.py:8:1: I001 isort found an import in the wrong position
[failure] 7-7:
./integration_tests/test_eip2935.py:7:1: I001 isort found an import in the wrong position
[failure] 6-6:
./integration_tests/test_eip2935.py:6:1: I001 isort found an import in the wrong position
[failure] 5-5:
./integration_tests/test_eip2935.py:5:1: I001 isort found an import in the wrong position
🪛 Ruff (0.13.2)
integration_tests/test_eip2935.py
57-57: Loop control variable i not used within loop body
Rename unused i to _i
(B007)
⏰ 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: Run golangci-lint
- GitHub Check: unittest
- GitHub Check: build (macos-latest)
- GitHub Check: build (macos-14)
- GitHub Check: gomod2nix
- GitHub Check: build (ubuntu-latest)
- GitHub Check: integration_tests (upgrade)
- GitHub Check: integration_tests (gas)
- GitHub Check: integration_tests (ibc_rly_gas)
- GitHub Check: integration_tests (slow)
- GitHub Check: integration_tests (ibc_rly_evm)
- GitHub Check: integration_tests (ibc_timeout)
- GitHub Check: integration_tests (ibc)
- GitHub Check: integration_tests (unmarked)
- GitHub Check: integration_tests (ibc_update_client)
- GitHub Check: integration_tests (gov)
- GitHub Check: integration_tests (ica)
🔇 Additional comments (4)
integration_tests/utils.py (1)
72-73: LGTM: contract artifact mapping addedThe new TestEip2935 artifact entry integrates cleanly with existing deployment helpers.
integration_tests/contracts/contracts/TestEip2935.sol (1)
1-9: LGTM: minimal helper for blockhashContract is concise and suitable for the test purpose.
integration_tests/test_eip2935.py (1)
32-44: Request: confirm preinstall msg type and authorityEnsure
/ethermint.evm.v1.MsgRegisterPreinstallsis routed/authorized in the app and thatmodule_address("gov")matches the expected authority on your ethermint fork, otherwise the proposal will fail at runtime.third_party/proto/ethermint/evm/v1/params.proto (1)
27-28: Integrate newhistory_serve_windowparam
- Regenerate protobufs (e.g. run your proto‐gen script) and commit updated
.pb.goforthird_party/proto/ethermint/evm/v1/params.proto.- Add
history_serve_windowtoDefaultParams()and include validation inParams.Validate().- Confirm field number 8 isn’t used upstream in Ethermint to avoid wire‐incompatibility.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
integration_tests/test_eip2935.py (2)
29-39: Clarify why only 4 blocks are checked instead of 5.The comment on line 29 states "we persist only the latest 5 block hashes," and line 30 says "Check that there is no history if query < current_block_number - 5." However, the loop at line 37 checks
range(0, 4), which validates only 4 blocks (start+0 through start+3).Since
header_hash_numis 5, consider checking 5 blocks to fully validate the boundary condition, or add a comment explaining why 4 blocks suffice (e.g., to avoid edge cases at the exact boundary).
74-77: Simplify redundant block waiting.Lines 75-77 introduce potentially redundant waiting:
- Line 75: Wait for current block + 10
- Line 76: Capture new current block as
start- Line 77: Wait for
start + 10(another 10 blocks)This results in waiting for approximately 20 blocks total. If the intent is to accumulate sufficient history after deploying the preinstall, consider consolidating or adding a comment explaining the two-phase wait.
Apply this diff to simplify:
- # Check that history < current_block_number - 5 is available - w3_wait_for_block(w3, w3.eth.block_number + 10, timeout=30) - start = w3.eth.block_number - w3_wait_for_block(w3, start + 10, timeout=30) + # Wait for sufficient blocks to accumulate history after preinstall + start = w3.eth.block_number + 10 + w3_wait_for_block(w3, start + 10, timeout=30)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
CHANGELOG.md(1 hunks)integration_tests/configs/eip2935.jsonnet(1 hunks)integration_tests/test_eip2935.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- CHANGELOG.md
🧰 Additional context used
🧬 Code graph analysis (1)
integration_tests/test_eip2935.py (2)
integration_tests/utils.py (4)
module_address(746-748)deploy_contract(421-441)submit_gov_proposal(224-236)w3_wait_for_block(267-279)integration_tests/network.py (3)
Cronos(16-69)setup_custom_cronos(155-197)w3(39-42)
🪛 Gitleaks (8.28.0)
integration_tests/configs/eip2935.jsonnet
[high] 122-122: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ 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). (19)
- GitHub Check: integration_tests (gov)
- GitHub Check: integration_tests (ica)
- GitHub Check: integration_tests (slow)
- GitHub Check: integration_tests (gas)
- GitHub Check: integration_tests (ibc_timeout)
- GitHub Check: integration_tests (ibc_update_client)
- GitHub Check: integration_tests (ibc_rly_gas)
- GitHub Check: integration_tests (upgrade)
- GitHub Check: integration_tests (ibc)
- GitHub Check: integration_tests (ibc_rly_evm)
- GitHub Check: integration_tests (unmarked)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: unittest
- GitHub Check: build (macos-latest)
- GitHub Check: build (macos-14)
- GitHub Check: gomod2nix
- GitHub Check: Run golangci-lint
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (3)
integration_tests/configs/eip2935.jsonnet (1)
119-123: False positive: Test encryption key, not a secret.The gitleaks warning flags this as a potential API key, but this is an
ageencryption public key used for testing the e2ee (end-to-end encryption) module. Test fixtures commonly include hardcoded keys for reproducibility.integration_tests/test_eip2935.py (2)
78-81: LGTM! Block hash validation logic is correct.The test correctly validates that after deploying the HistoryStorage preinstall, the
blockhashOpcodemethod returns actual block hashes (not zero bytes) for historical blocks beyond the initial 5-block window. The assertionstored == block.hashconfirms the contract retrieves the correct on-chain hash.
42-42: HistoryStorage address verified
The hardcoded address matches the canonical EIP-2935 HISTORY_STORAGE address.
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