feat: add preinstall in upgrade handler #1939
Conversation
thomas-nguy
requested review from
XinyuCRO and
songgaoye
and removed request for
a team
WalkthroughAdds v1.7 upgrade handlers (mainnet and testnet) that register EVM preinstall contracts, bumps version references to v1.7.0, updates changelog and test flakes, and extends integration tests to verify preinstalled contract bytecode after upgrade. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 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)
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 |
This comment has been minimized.
This comment has been minimized.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
app/upgrades.go (1)
24-35: Consider extracting common preinstall definitions.The EIP-2935 preinstall is duplicated between the mainnet (v1.7) and testnet (v1.7-testnet) handlers. Consider extracting it to a package-level variable or helper to reduce duplication.
// At package level or as a helper var eip2935Preinstall = evmtypes.Preinstall{ Name: "EIP-2935 - Serve historical block hashes from state", Address: params.HistoryStorageAddress.String(), Code: common.Bytes2Hex(params.HistoryStorageCode), }Then reuse in both handlers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
CHANGELOG.md(1 hunks)app/upgrades.go(1 hunks)default.nix(1 hunks)integration_tests/configs/upgrade-test-package.nix(2 hunks)integration_tests/test_upgrade.py(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
integration_tests/test_upgrade.py (2)
integration_tests/cosmoscli.py (1)
block_height(177-178)integration_tests/network.py (1)
w3(39-42)
app/upgrades.go (2)
app/app.go (2)
App(273-338)Name(167-167)x/cronos/types/interfaces.go (1)
EvmKeeper(59-69)
🪛 GitHub Check: Lint python
integration_tests/test_upgrade.py
[failure] 14-14:
./integration_tests/test_upgrade.py:14:1: I001 isort found an import in the wrong position
[failure] 357-357:
./integration_tests/test_upgrade.py:357:89: E501 line too long (102 > 88 characters)
⏰ 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 (ubuntu-latest)
- GitHub Check: build (macos-latest)
- GitHub Check: build (macos-14)
- GitHub Check: unittest
- GitHub Check: integration_tests (evm)
- GitHub Check: integration_tests (slow)
- GitHub Check: integration_tests (mint)
- GitHub Check: integration_tests (unmarked)
- GitHub Check: integration_tests (ibc_rly_gas)
- GitHub Check: integration_tests (gov)
- GitHub Check: integration_tests (ibc_rly_evm)
- GitHub Check: integration_tests (ibc_timeout)
- GitHub Check: integration_tests (ibc_update_client)
- GitHub Check: integration_tests (ica)
- GitHub Check: integration_tests (ibc)
- GitHub Check: integration_tests (upgrade)
- GitHub Check: integration_tests (gas)
- GitHub Check: Analyze (go)
🔇 Additional comments (9)
CHANGELOG.md (1)
10-11: LGTM!The changelog entry follows the existing format and is correctly placed under the UNRELEASED section.
default.nix (1)
14-14: LGTM!Version bump to v1.7.0 correctly aligns with the new v1.7 upgrade handlers.
integration_tests/test_upgrade.py (2)
347-362: Preinstall verification test logic looks correct.The test correctly verifies that the EIP-2935 historical storage contract is deployed at the expected address with the expected bytecode after the v1.7 upgrade.
172-172: LGTM!Adding v1.7 to the allowed upgrade plan names enables the governance proposal submission path for the new version.
integration_tests/configs/upgrade-test-package.nix (2)
30-32: LGTM!The released1_6 binding correctly references the v1.6.1 release commit, following the established pattern for previous releases.
61-67: LGTM!The v1.6 entry now correctly points to the released version, and v1.7 points to the current development code, enabling proper upgrade path testing.
app/upgrades.go (3)
17-38: Mainnet v1.7 upgrade handler structure looks correct.The handler properly runs migrations first, then applies preinstalls. Error handling is appropriate, returning errors from both
RunMigrationsandAddPreinstalls.
40-71: Testnet v1.7-testnet upgrade handler structure looks correct.The handler correctly registers additional testnet-specific preinstalls (Create2 and Permit2) alongside the standard EIP-2935 preinstall.
48-58: Hardcoded preinstall addresses are canonical deploymentsBoth addresses are confirmed as canonical:
- Create2 deployer (
0x4e59b44847b379578588920ca78fbf26c0b4956c) is the most widely used CREATE2 factory contract out there, deployed to this address on many chains- Permit2 (
0x000000000022D473030F116dDEE9F6B43aC78BA3) is the canonical Permit2 contract referred to in Uniswap's documentation and integration guides as the one blessed address that everyone should useBoth bytecodes correspond to the verified contracts on Ethereum and are deployed identically across multiple chains.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1939 +/- ##
==========================================
+ Coverage 16.87% 25.51% +8.63%
==========================================
Files 72 78 +6
Lines 6163 5150 -1013
==========================================
+ Hits 1040 1314 +274
+ Misses 5000 3673 -1327
- Partials 123 163 +40
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
app/upgrades.go (1)
50-66: Prefix inconsistency—normalize the Code field format.Create2 and Permit2 preinstalls use explicit
0xprefixes (lines 54, 59), while EIP-2935 usescommon.Bytes2Hex()which does not include the0xprefix (line 64). This inconsistency could cause issues depending on howAddPreinstallsparses the Code field.Either add the
0xprefix to theBytes2Hexoutput or useBytes2Hexfor all Code fields to maintain consistency.#!/bin/bash # Verify how AddPreinstalls handles the Code field and whether it expects 0x prefix ast-grep --pattern $'func ($_ $_) AddPreinstalls($_, $_) $_ { $$$ }' # Also search for hex decoding logic rg -n "Code" --type go -A 3 -B 3 | grep -i "hex\|decode"
🧹 Nitpick comments (1)
app/upgrades.go (1)
27-37: Consider extracting EIP-2935 preinstall to reduce duplication.The EIP-2935 preinstall definition is identical in both the mainnet (lines 27-33) and testnet (lines 61-65) handlers. Extract this to a shared variable or helper function.
+var eip2935Preinstall = evmtypes.Preinstall{ + Name: "EIP-2935 - Serve historical block hashes from state", + Address: params.HistoryStorageAddress.String(), + Code: common.Bytes2Hex(params.HistoryStorageCode), +} + func (app *App) RegisterUpgradeHandlers(cdc codec.BinaryCodec, maxVersion int64) bool { planName := "v1.7" app.UpgradeKeeper.SetUpgradeHandler(planName, func(ctx context.Context, plan upgradetypes.Plan, fromVM module.VersionMap) (module.VersionMap, error) { // ... { - preinstall := []evmtypes.Preinstall{ - { - Name: "EIP-2935 - Serve historical block hashes from state", - Address: params.HistoryStorageAddress.String(), - Code: common.Bytes2Hex(params.HistoryStorageCode), - }, - } + preinstall := []evmtypes.Preinstall{eip2935Preinstall}Also applies to: 61-66
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/upgrades.go(1 hunks)integration_tests/test_upgrade.py(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
integration_tests/test_upgrade.py (2)
integration_tests/cosmoscli.py (1)
block_height(177-178)integration_tests/network.py (1)
w3(39-42)
⏰ 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: build (ubuntu-latest)
- GitHub Check: build (macos-latest)
- GitHub Check: unittest
- GitHub Check: build (macos-14)
- GitHub Check: integration_tests (upgrade)
- GitHub Check: Run golangci-lint
- GitHub Check: integration_tests (slow)
- GitHub Check: integration_tests (ibc_rly_evm)
- GitHub Check: integration_tests (ibc)
- GitHub Check: integration_tests (ica)
- GitHub Check: integration_tests (unmarked)
- GitHub Check: integration_tests (mint)
- GitHub Check: integration_tests (gas)
- GitHub Check: integration_tests (ibc_timeout)
- GitHub Check: integration_tests (evm)
- GitHub Check: integration_tests (gov)
- GitHub Check: integration_tests (ibc_rly_gas)
- GitHub Check: Analyze (go)
- GitHub Check: integration_tests (ibc_update_client)
🔇 Additional comments (4)
app/upgrades.go (1)
27-37: LGTM for the mainnet upgrade handler structure.The error handling for migrations and the preinstall registration pattern look correct. The use of
sdk.UnwrapSDKContextis appropriate for interfacing with the EVM keeper.integration_tests/test_upgrade.py (3)
12-15: Import order appears correct now.The
HexBytesimport at line 12 is properly positioned alphabetically among third-party imports (hexbytesbeforepystarportandweb3).
172-172: LGTM for adding v1.7 to the upgrade flow.The v1.7 plan name is correctly added to the conditional that uses the new governance proposal format.
347-363: The review comment's concern is valid in principle: hardcoded test values should be kept synchronized with upstream constants to prevent test failures. However, I cannot complete the verification without repository access to confirm the specific claims aboutapp/upgrades.gousingparams.HistoryStorageAddressandparams.HistoryStorageCode. The test values do match the current go-ethereum params constants, but further investigation into the local codebase is needed to fully assess the synchronization risk.
thomas-nguy
force-pushed
the
thomas/create2preinstall
branch
from
960177f to
899acd0
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
integration_tests/test_upgrade.py (2)
12-12: Import order issue already flagged.The static analysis tool flagged an import order violation on this line in a previous review. Please address the linting issue by moving the
HexBytesimport to the correct position.
355-359: Line length issue already flagged.The line length violation on line 357 has been flagged in a previous review. Please break the string literal into multiple lines to satisfy the linter.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
CHANGELOG.md(1 hunks)app/upgrades.go(1 hunks)default.nix(1 hunks)integration_tests/configs/upgrade-test-package.nix(2 hunks)integration_tests/test_upgrade.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- default.nix
- CHANGELOG.md
🧰 Additional context used
🧬 Code graph analysis (2)
app/upgrades.go (2)
app/app.go (1)
App(273-338)x/cronos/types/interfaces.go (1)
EvmKeeper(59-69)
integration_tests/test_upgrade.py (2)
integration_tests/cosmoscli.py (1)
block_height(177-178)integration_tests/network.py (1)
w3(39-42)
⏰ 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: build (macos-14)
- GitHub Check: build (macos-latest)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: unittest
- GitHub Check: integration_tests (gas)
- GitHub Check: integration_tests (evm)
- GitHub Check: integration_tests (slow)
- GitHub Check: integration_tests (mint)
- GitHub Check: integration_tests (ibc_update_client)
- GitHub Check: integration_tests (ica)
- GitHub Check: integration_tests (unmarked)
- GitHub Check: integration_tests (ibc_timeout)
- GitHub Check: integration_tests (ibc_rly_evm)
- GitHub Check: integration_tests (ibc)
- GitHub Check: integration_tests (upgrade)
- GitHub Check: integration_tests (gov)
- GitHub Check: Run golangci-lint
- GitHub Check: integration_tests (ibc_rly_gas)
- GitHub Check: Analyze (go)
🔇 Additional comments (8)
integration_tests/test_upgrade.py (2)
172-172: LGTM!The addition of "v1.7" to the allowed upgrade plan names correctly enables the new upgrade path.
347-363: The hardcoded bytecode in the test correctly matches the EIP-2935 specification and aligns with go-ethereum's params.HistoryStorageCode implementation.Verification confirms the test bytecode matches the EIP-2935 specification and is consistent with the go-ethereum v1.15.11 dependency used in the codebase.
app/upgrades.go (4)
6-13: LGTM!The added imports are necessary for preinstall handling and are used appropriately in the upgrade handlers.
19-40: LGTM, but note the Code field format inconsistency.The v1.7 mainnet upgrade handler correctly:
- Runs migrations before adding preinstalls
- Unwraps the context for SDK operations
- Handles errors appropriately
- Adds the EIP-2935 preinstall
However, a past review comment already flagged that the Code field uses
common.Bytes2Hex()which returns hex without the0xprefix, while the testnet handler uses explicit0xprefixes for Create2 and Permit2. Please address the format inconsistency noted in the previous review.
51-55: Appropriate separation of mainnet and testnet preinstalls.The Create2 preinstall is correctly included only in the testnet handler, addressing the concern raised in previous comments about the existing Create2 deployment on mainnet.
56-60: LGTM!The Permit2 preinstall is appropriately included in the testnet handler alongside Create2, as discussed in previous comments.
integration_tests/configs/upgrade-test-package.nix (2)
60-67: LGTM!The linkFarm entries correctly transition v1.6 to use the released version and add v1.7 as the new current version, following the established pattern.
30-32: v1.6.1 is not a recognized release in cronos repository.The released1_6 binding references a non-existent release tag. The latest available version is v1.5.0, with stable releases only up to v1.4.10. Verify whether this should target an existing release (v1.4.x or v1.5.0) or if this configuration is intended for a future release that hasn't been published yet. The commit hash
05e102ef83b9ab0d5b55d46fb90f5fee53a295d2should also be validated against an actual release tag.
Signed-off-by: Thomas <[email protected]>
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
6dac283
2 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.