chore: add validation for HeaderHashNum and HistoryServeWindow variables in evm params #1933
Conversation
randy-cro
requested review from
JayT106 and
calvinaco
and removed request for
a team
WalkthroughAdds "evm" to the CI integration test matrix, introduces a new Python integration test validating EVM parameter int64 overflow behavior, updates changelog, and bumps/updates several Go dependency pins and gomod2nix entries. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
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)
🚧 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). (22)
🔇 Additional comments (2)
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 #1933 +/- ##
===========================================
+ 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:
|
![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 (1)
integration_tests/test_evm_params_int_64_overflow.py (1)
33-62: Refine exception handling in test helper function.The current exception handling catches bare
Exceptionwhich is too broad. While this may work for testing, it could mask unexpected errors and make debugging harder.Consider catching more specific exceptions:
def submit_evm_param_update(cronos, params): """ Submit a governance proposal to update EVM module parameters. Args: cronos: Cronos cluster instance params: Complete params dict with updated values Returns: True if proposal passes, False if it fails """ authority = module_address("gov") msg = "/ethermint.evm.v1.MsgUpdateParams" try: submit_gov_proposal( cronos, msg, messages=[ { "@type": msg, "authority": authority, "params": params, } ], ) - return True - except (AssertionError, Exception) as e: + except AssertionError as e: print(f"Proposal failed as expected: {e}") return False + else: + return TrueThis change:
- Removes the broad
Exceptioncatch that could mask bugs- Moves the success return to an
elseblock for clarity- Only catches
AssertionErrorwhich is the expected failure mode fromsubmit_gov_proposal
📜 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 (5)
.github/workflows/test.yml(1 hunks)CHANGELOG.md(1 hunks)go.mod(2 hunks)gomod2nix.toml(2 hunks)integration_tests/test_evm_params_int_64_overflow.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
integration_tests/test_evm_params_int_64_overflow.py (2)
integration_tests/utils.py (1)
wait_for_new_blocks(134-142)integration_tests/cosmoscli.py (1)
query_params(1115-1126)
🪛 Ruff (0.14.7)
integration_tests/test_evm_params_int_64_overflow.py
59-59: Consider moving this statement to an else block
(TRY300)
60-60: Do not catch blind exception: Exception
(BLE001)
⏰ 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: Run golangci-lint
- GitHub Check: build (macos-14)
- GitHub Check: build (macos-latest)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: gomod2nix
- GitHub Check: unittest
- GitHub Check: integration_tests (slow)
- GitHub Check: integration_tests (evm)
- GitHub Check: integration_tests (gas)
- GitHub Check: integration_tests (gov)
- GitHub Check: integration_tests (ica)
- GitHub Check: integration_tests (mint)
- GitHub Check: integration_tests (ibc)
- GitHub Check: integration_tests (ibc_rly_gas)
- GitHub Check: integration_tests (ibc_timeout)
- GitHub Check: integration_tests (ibc_update_client)
- GitHub Check: integration_tests (unmarked)
- GitHub Check: integration_tests (upgrade)
- GitHub Check: integration_tests (ibc_rly_evm)
🔇 Additional comments (12)
CHANGELOG.md (1)
8-8: LGTM! Changelog entry properly documents the validation addition.The changelog entry correctly documents the addition of validation for EVM parameters under the UNRELEASED section.
.github/workflows/test.yml (1)
22-22: LGTM! Test matrix appropriately expanded to include EVM tests.The addition of "evm" to the test matrix ensures the new EVM parameter validation tests will be executed in CI.
go.mod (2)
306-306: Verify the ethermint fork switch from organization to personal repository.The module replacement mirrors the change in
gomod2nix.toml, switching fromcrypto-org-chain/etherminttorandy-cro/ethermint. This raises the same concerns about using a personal fork in production code.Please confirm:
- Is this temporary for testing or intended for production?
- What validation changes are in this fork that aren't in the main organization fork?
- What is the timeline for merging these changes back to the organization fork?
38-38: No action needed; cobra v1.10.2 is a safe patch upgrade.The upgrade from v1.10.1 to v1.10.2 includes only dependency path migration (gopkg.in/yaml.v3 to go.yaml.in/yaml/v3) and CI/CD linter fixes. No breaking changes.
integration_tests/test_evm_params_int_64_overflow.py (6)
1-24: LGTM! Well-documented test suite with correct constants.The module docstring clearly explains the test objectives and edge cases being validated. The constants MAX_INT64 and UINT64_MAX are correctly defined.
83-123: LGTM! Test properly validates zero value edge case.The test correctly verifies that zero is a valid value for both parameters, includes proper assertions, and confirms the chain continues producing blocks after the update.
125-161: LGTM! Test validates normal operation with typical values.The test establishes a baseline by verifying that typical valid values (100) work correctly for both parameters.
163-199: LGTM! Test properly validates the MAX_INT64 boundary case.The test correctly verifies that MAX_INT64 (the maximum valid int64 value) is accepted for both parameters and the chain continues functioning.
201-232: LGTM! Test correctly validates overflow rejection.The test properly verifies that values exceeding MAX_INT64 are rejected and that the parameters remain unchanged when an invalid proposal fails.
234-262: LGTM! Test validates rejection of extreme overflow values.The test properly verifies that UINT64_MAX is rejected, testing the most extreme overflow case.
gomod2nix.toml (2)
315-317: Verify the intentionality and scope of the ethermint fork switch to a personal repository.The module replacement has changed from
crypto-org-chain/etherminttorandy-cro/ethermint. This raises legitimate concerns about code review governance, long-term maintenance, and availability. Clarify:
- Is this fork switch intentional and approved by the team?
- What specific changes in
randy-cro/ethermintare required for this PR?- Is this a temporary workaround or permanent dependency, and if permanent, is there a plan to upstream changes?
628-629: Confirm cobra library version upgrade to v1.10.2.The cobra library has been upgraded from v1.10.1 to v1.10.2, which includes a migration from gopkg.in/yaml.v3 to go.yaml.in/yaml/v3 and CI/CD linter fixes. This is a safe, non-breaking maintenance update.
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
Tests
Chores
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.