fix: Add default value of CancunTime and PragueTime in chain config #1851
Conversation
XinyuCRO
requested review from
JayT106 and
calvinaco
and removed request for
a team
WalkthroughAdds docs note about default Cancun/Prague times, bumps an ethermint dependency and its nix hash, modifies genesis to set cancun/prague times and add blobSchedule, introduces test helpers (funding + governance param removal), updates SELFDESTRUCT integration tests for pre/post-Cancun behavior, and adds receive handlers to test contracts. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Test as Integration Test
participant Utils as utils.remove_cancun_prague_params
participant Node as Cronos Node (RPC)
participant Gov as Governance Module
participant Params as Chain Params Store
Note over Test,Utils: remove_cancun_prague_params flow
Test->>Utils: call remove_cancun_prague_params(cronos)
Utils->>Node: query current chain_config
Node->>Utils: return chain_config (includes cancun_time/prague_time)
Utils->>Gov: submit MsgUpdateParams with cancun_time/prague_time removed
Gov->>Node: propose & execute param update (async)
Node->>Params: apply updated chain_config
Params->>Node: updated params confirmed
Node->>Utils: fetch params
Utils->>Test: assert cancun_time/prague_time absent
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
|
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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (3)
CHANGELOG.md(1 hunks)go.mod(1 hunks)gomod2nix.toml(1 hunks)
⏰ 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: Socket Security: Pull Request Alerts
- GitHub Check: gomod2nix
- GitHub Check: build (macos-14)
- GitHub Check: build (macos-latest)
- GitHub Check: integration_tests (gov)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: integration_tests (upgrade)
- GitHub Check: integration_tests (gas)
- GitHub Check: unittest
- GitHub Check: integration_tests (ibc)
- GitHub Check: integration_tests (unmarked)
- GitHub Check: integration_tests (ica)
- GitHub Check: integration_tests (slow)
- GitHub Check: integration_tests (ibc_rly_gas)
- GitHub Check: integration_tests (ibc_timeout)
- GitHub Check: integration_tests (ibc_rly_evm)
- GitHub Check: integration_tests (ibc_update_client)
- GitHub Check: Run golangci-lint
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (3)
CHANGELOG.md (1)
13-13: Whitespace consistency looks good.Adding a blank line after the “Improvements” heading matches the formatting used in other sections.
go.mod (1)
308-308: Verified ethermint version bump
- go.mod and gomod2nix.toml both reference v0.6.1-0.20250820110050-d940a537fb81
- Commit d940a537fb81 message confirms “add Cancun and Prague fork default value (#702)”
All checks pass.
gomod2nix.toml (1)
317-319: gomod2nix pinned to the new ethermint version and hash; good alignment.The module entry matches the go.mod replace and includes the updated sha256. Assuming CI’s nix build passes, this looks solid.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1851 +/- ##
===========================================
+ Coverage 16.87% 35.81% +18.94%
===========================================
Files 72 127 +55
Lines 6163 11812 +5649
===========================================
+ Hits 1040 4231 +3191
- Misses 5000 7158 +2158
- Partials 123 423 +300 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
integration_tests/test_gov_update_params.py (1)
53-55: Syntax error in params dict: broken string literal and missing comma.The
ibc_cro_denomvalue is split across lines and missing a trailing comma, which will prevent the test file from parsing.Apply this fix:
- "ibc_cro_denom": "ibc/6411AE2ADA1E73DB59DB151" - "A8988F9B7D5E7E233D8414DB6817F8F1A01600000", - "ibc_timeout": "96400000000000", + "ibc_cro_denom": "ibc/6411AE2ADA1E73DB59DB151A8988F9B7D5E7E233D8414DB6817F8F1A01600000", + "ibc_timeout": "96400000000000",
🧹 Nitpick comments (3)
integration_tests/test_gov_update_params.py (3)
18-21: Prefer tolerant removals for future-proofing (avoid KeyError).Direct
delraises if any key is missing on certain Ethermint versions. Use a single loop withpop(..., None)to keep the test resilient while preserving intent.- del p["chain_config"]["merge_netsplit_block"] - del p["chain_config"]["shanghai_time"] - del p["chain_config"]["cancun_time"] - del p["chain_config"]["prague_time"] + for k in ("merge_netsplit_block", "shanghai_time", "cancun_time", "prague_time"): + p["chain_config"].pop(k, None)
36-39: Assert presence plus defaulted value to validate the “default value” contract.The goal is to confirm the params are present and defaulted, not merely falsy. This also makes the intent explicit and avoids ambiguity if types ever change.
- assert not p["chain_config"]["merge_netsplit_block"] - assert not p["chain_config"]["shanghai_time"] - assert not p["chain_config"]["cancun_time"] - assert not p["chain_config"]["prague_time"] + for k in ("merge_netsplit_block", "shanghai_time", "cancun_time", "prague_time"): + assert k in p["chain_config"], f"{k} missing from chain_config" + assert not p["chain_config"][k]If
merge_netsplit_blockis not boolean on your chain (e.g., an int), consider replacing the last assert with explicit equality checks (e.g.,== 0) after confirming types fromcli.query_params("evm").
40-46: Make the error-message assertion robust to node/runtime variations.Error strings can vary slightly across geth/Ethermint versions. Matching case-insensitively on the opcode is less brittle.
- invalid_msg = "invalid opcode: PUSH0" + invalid_msg = "push0" with pytest.raises(ValueError) as e_info: contract.caller.randomTokenId() - assert invalid_msg in str(e_info.value) + assert invalid_msg in str(e_info.value).lower() with pytest.raises(ValueError) as e_info: deploy_contract(cronos.w3, CONTRACTS["Greeter"]) - assert invalid_msg in str(e_info.value) + assert invalid_msg in str(e_info.value).lower()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
CHANGELOG.md(1 hunks)integration_tests/test_gov_update_params.py(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- CHANGELOG.md
⏰ 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). (16)
- GitHub Check: Run golangci-lint
- GitHub Check: build (macos-14)
- GitHub Check: build (macos-latest)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: unittest
- GitHub Check: integration_tests (gov)
- GitHub Check: integration_tests (ibc_rly_evm)
- GitHub Check: integration_tests (upgrade)
- GitHub Check: integration_tests (slow)
- GitHub Check: integration_tests (ica)
- GitHub Check: integration_tests (gas)
- GitHub Check: integration_tests (ibc_rly_gas)
- GitHub Check: integration_tests (ibc_update_client)
- GitHub Check: integration_tests (ibc_timeout)
- GitHub Check: integration_tests (ibc)
- GitHub Check: integration_tests (unmarked)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
integration_tests/test_basic.py (1)
674-681: Pre-Cancun gating via governance is the right approach; thanks for splitting testsThis addresses the earlier suggestion to have separate pre-/post-EIP-6780 tests by explicitly disabling Cancun/Prague via governance before running the pre-Cancun assertions.
🧹 Nitpick comments (4)
scripts/geth-genesis.json (1)
20-31: Confirm blobSchedule schema/values match the client you run in CIThe blobSchedule section (target=3, max=6, baseFeeUpdateFraction=3338477) looks intentional, but these keys and the update fraction are client/version-specific. Two quick checks I recommend:
- Ensure your pinned Geth in CI accepts this exact schema.
- Document (or reference) where 3338477 comes from (upstream default or Ethermint override) to help future maintainers keep Cronos/Geth/Ethermint in sync.
If desired, I can add a short README note next to this genesis explaining the rationale.
integration_tests/contracts/contracts/TestSuicide.sol (1)
23-23: Destroyer’s receive() is optional for SELFDESTRUCT fundsSELFDESTRUCT transfers value without invoking fallback/receive on the beneficiary, so this is not strictly required. Keeping it is harmless and may help future scenarios where Destroyer is funded via normal transfers. If you prefer minimal bytecode, you could drop it.
- receive() external payable {} + // receive() not required for SELFDESTRUCT; keep only if you expect normal transfers. + // receive() external payable {}integration_tests/utils.py (1)
851-857: Fund helper works; consider making it resilient to pre-funded addresses and explicit about desired balanceCurrent behavior funds only when balance==0 and then asserts exact equality with fund. That matches your tests (newly deployed contracts). If you want this utility to be reusable elsewhere, consider topping up to a target balance and tolerating pre-funded accounts.
Optional, if you want that behavior:
-def fund_acc(w3, acc, fund=3000000000000000000): - addr = acc.address - if w3.eth.get_balance(addr, "latest") == 0: - tx = {"to": addr, "value": fund, "gasPrice": w3.eth.gas_price} - send_transaction(w3, tx) - assert w3.eth.get_balance(addr, "latest") == fund +def fund_acc(w3, acc, fund=3000000000000000000): + addr = acc.address + bal = w3.eth.get_balance(addr, "latest") + if bal < fund: + # top up to target + delta = fund - bal + tx = {"to": addr, "value": delta, "gasPrice": w3.eth.gas_price} + send_transaction(w3, tx) + assert w3.eth.get_balance(addr, "latest") >= fundintegration_tests/test_basic.py (1)
702-736: Post-Cancun assertions look correct; minor readability nitThe checks reflect EIP-6780 semantics (funds recovered, code intact). For readability, consider using to_wei to express 1 ETH.
- balance_wanted = 1000000000000000000 + balance_wanted = web3.Web3.to_wei(1, "ether")Optional additional coverage: add a case where a contract is created and selfdestructs in the same transaction (the exception in EIP-6780), asserting code deletion in that specific scenario. I can provide a small Factory contract and test if useful.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
integration_tests/contracts/contracts/TestSuicide.sol(2 hunks)integration_tests/test_basic.py(3 hunks)integration_tests/utils.py(1 hunks)scripts/geth-genesis.json(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
integration_tests/utils.py (3)
integration_tests/network.py (3)
w3(39-42)cosmos_cli(56-57)cosmos_cli(81-82)testground/benchmark/benchmark/testnet.py (1)
fund(36-76)integration_tests/cosmoscli.py (4)
address(302-312)query_params(1115-1126)module_address(37-44)submit_gov_proposal(1267-1347)
integration_tests/test_basic.py (1)
integration_tests/utils.py (5)
fund_acc(851-856)remove_cancun_prague_params(859-879)deploy_contract(414-434)contract_path(69-75)send_transaction(459-462)
⏰ 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 (ibc_rly_evm)
- GitHub Check: gomod2nix
- GitHub Check: integration_tests (upgrade)
- GitHub Check: integration_tests (gas)
- GitHub Check: integration_tests (slow)
- GitHub Check: build (macos-14)
- GitHub Check: integration_tests (ibc_rly_gas)
- GitHub Check: unittest
- GitHub Check: build (ubuntu-latest)
- GitHub Check: Run golangci-lint
- GitHub Check: build (macos-latest)
- GitHub Check: integration_tests (ibc)
- GitHub Check: integration_tests (ica)
- GitHub Check: integration_tests (ibc_update_client)
- GitHub Check: integration_tests (ibc_timeout)
- GitHub Check: integration_tests (unmarked)
- GitHub Check: integration_tests (gov)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (3)
scripts/geth-genesis.json (1)
18-19: Cancun/Prague set to activate at genesis looks correct and aligns with PR intentSetting cancunTime and pragueTime to 0 ensures both forks are active from genesis in the Geth-based test cluster, which matches the test split between pre/post semantics (Cronos disables via gov, Geth stays enabled). LGTM.
integration_tests/contracts/contracts/TestSuicide.sol (1)
9-9: Adding a payable receive() on Destroyee is necessary for clean ETH fundingThis unblocks direct value transfers in tests prior to SELFDESTRUCT. Good addition.
integration_tests/test_basic.py (1)
28-35: Good import split to externalize helpersImporting fund_acc and remove_cancun_prague_params from utils keeps tests concise and DRY. LGTM.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
integration_tests/utils.py (1)
859-880: Make Cancun/Prague param removal idempotent and assertions robust (absent vs. zero).Using del raises on missing keys and post-conditions via direct subscripting fail if the API omits fields or returns "0" (string). Use pop with defaults and assert on None/0/"0". This mirrors an earlier review; re-flagging here for completeness.
-def remove_cancun_prague_params(cronos): - from .cosmoscli import module_address as cosmos_module_address +def remove_cancun_prague_params(cronos): cli = cronos.cosmos_cli() p = cli.query_params("evm") - del p["chain_config"]["cancun_time"] - del p["chain_config"]["prague_time"] + # Tolerate missing keys to avoid KeyError and keep tests idempotent + cfg = p.get("chain_config") or {} + cfg.pop("cancun_time", None) + cfg.pop("prague_time", None) authority = cosmos_module_address("gov") msg = "/ethermint.evm.v1.MsgUpdateParams" submit_gov_proposal( cronos, msg, messages=[ { "@type": msg, "authority": authority, "params": p, } ], ) - p = cli.query_params("evm") - assert not p["chain_config"]["cancun_time"] - assert not p["chain_config"]["prague_time"] + cfg = cli.query_params("evm").get("chain_config") or {} + # Accept either absent or zero values (int or string) + assert cfg.get("cancun_time") in (None, 0, "0"), cfg + assert cfg.get("prague_time") in (None, 0, "0"), cfgAdditionally, Black flagged this hunk (BLK100). One likely fix is to move the import out of the function (see next comment).
🧹 Nitpick comments (2)
integration_tests/utils.py (2)
851-856: Top up to a target balance; avoid forcing legacy gasPrice.Current logic only funds when the balance is exactly zero and unconditionally sets gasPrice. For more resilient tests:
- Top up the difference when the balance is below the target.
- Let Web3 fill EIP-1559 fee fields (or legacy on non-1559 chains) by omitting gasPrice.
-def fund_acc(w3, acc, fund=3000000000000000000): - addr = acc.address - if w3.eth.get_balance(addr, "latest") == 0: - tx = {"to": addr, "value": fund, "gasPrice": w3.eth.gas_price} - send_transaction(w3, tx) - assert w3.eth.get_balance(addr, "latest") == fund +def fund_acc(w3, acc, fund=3000000000000000000): + addr = acc.address + bal = w3.eth.get_balance(addr, "latest") + if bal < fund: + # Rely on node defaults (EIP-1559 on supported chains, legacy otherwise). + tx = {"to": addr, "value": fund - bal} + send_transaction(w3, tx) + assert w3.eth.get_balance(addr, "latest") == fund
860-860: Move local import to module scope to satisfy Black and avoid runtime import overhead.Static analysis shows BLK100 on this block. Importing at module level also clarifies intent since a same-named helper exists in this file. Remove the in-function import and add this at the top with other imports:
from .cosmoscli import module_address as cosmos_module_addressAnd delete Line 860 inside the function:
- from .cosmoscli import module_address as cosmos_module_address
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
integration_tests/utils.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
integration_tests/utils.py (2)
integration_tests/network.py (3)
w3(39-42)cosmos_cli(56-57)cosmos_cli(81-82)integration_tests/cosmoscli.py (4)
address(302-312)module_address(37-44)query_params(1115-1126)submit_gov_proposal(1267-1347)
🪛 GitHub Check: Lint python
integration_tests/utils.py
[failure] 861-861:
./integration_tests/utils.py:861:1: BLK100 Black would make changes.
⏰ 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 (ica)
- GitHub Check: integration_tests (gov)
- GitHub Check: integration_tests (gas)
- GitHub Check: integration_tests (slow)
- GitHub Check: integration_tests (upgrade)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: integration_tests (ibc_update_client)
- GitHub Check: integration_tests (ibc_timeout)
- GitHub Check: integration_tests (ibc_rly_evm)
- GitHub Check: integration_tests (ibc)
- GitHub Check: integration_tests (unmarked)
- GitHub Check: integration_tests (ibc_rly_gas)
- GitHub Check: build (macos-latest)
- GitHub Check: build (macos-14)
- GitHub Check: gomod2nix
- GitHub Check: unittest
- GitHub Check: Run golangci-lint
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: semgrep-cloud-platform/scan
ethermint pr: crypto-org-chain/ethermint#702
Summary by CodeRabbit
Documentation
Chores
Tests
Contracts (tests)