fix: allow preinstall in case account has a balance #1946
Conversation
thomas-nguy
requested review from
JayT106 and
songgaoye
and removed request for
a team
|
Warning Rate limit exceeded@thomas-nguy has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 0 minutes and 58 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
📝 WalkthroughWalkthroughUpdates dependency versions and lockfile hashes, adds a changelog entry, and expands the preinstall integration test to fund the preinstall address, register a Create2 preinstall via governance, and verify deployment behavior when the account has a balance. Changes
Sequence Diagram(s)(omitted — changes are dependency updates, lockfile sync, changelog, and an integration test augmentation; no new multi-component runtime control flow requiring visualization) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
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: 1
Fix all issues with AI Agents 🤖
In @integration_tests/test_preinstall.py:
- Around line 15-26: Break the long call to w3.eth.send_transaction into
multiple shorter lines to satisfy the 88-character limit and replace direct use
of w3.eth.send_transaction with the existing send_transaction helper from utils
for consistency; keep the same behavior by assigning the returned tx hash to
txhash, awaiting the receipt via w3.eth.wait_for_transaction_receipt(txhash) and
asserting receipt.status == 1 and receipt.gasUsed == 21000 (refer to symbols:
w3.eth.send_transaction, send_transaction, txhash,
w3.eth.wait_for_transaction_receipt, receipt.gasUsed).
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (4)
CHANGELOG.mdgo.modgomod2nix.tomlintegration_tests/test_preinstall.py
🧰 Additional context used
🧬 Code graph analysis (1)
integration_tests/test_preinstall.py (2)
integration_tests/utils.py (2)
submit_gov_proposal(226-238)send_transaction(468-471)integration_tests/network.py (1)
w3(39-42)
🪛 GitHub Check: Lint python
integration_tests/test_preinstall.py
[failure] 15-15:
./integration_tests/test_preinstall.py:15:89: E501 line too long (94 > 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). (18)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: build (macos-14)
- GitHub Check: build (macos-latest)
- GitHub Check: gomod2nix
- GitHub Check: integration_tests (evm)
- GitHub Check: integration_tests (gov)
- GitHub Check: integration_tests (ica)
- GitHub Check: integration_tests (mint)
- GitHub Check: integration_tests (ibc_rly_gas)
- GitHub Check: integration_tests (unmarked)
- GitHub Check: integration_tests (slow)
- GitHub Check: integration_tests (upgrade)
- GitHub Check: integration_tests (ibc_update_client)
- GitHub Check: integration_tests (gas)
- GitHub Check: integration_tests (ibc_timeout)
- GitHub Check: integration_tests (ibc_rly_evm)
- GitHub Check: integration_tests (ibc)
- GitHub Check: Run golangci-lint
🔇 Additional comments (7)
CHANGELOG.md (1)
16-16: LGTM! Changelog entry is clear and well-formatted.The changelog entry accurately describes the fix for allowing preinstall when an account has a balance, consistent with the PR objective and test changes.
integration_tests/test_preinstall.py (2)
5-5: LGTM! Import addition supports the new test scenario.The ADDRS import is correctly added to support funding the preinstall address in the test.
27-29: LGTM! Validation correctly ensures preinstall address starts empty.The check confirms the preinstall address has no code before the governance proposal, which is essential for validating the test scenario.
gomod2nix.toml (1)
1-820: Auto-generated file is consistent with go.mod updates.This file is auto-generated by gomod2nix and correctly reflects the dependency version updates from go.mod, including hash recalculations for:
- cosmos/ibc-go/v10: v10.4.0 → v10.5.0
- OpenTelemetry packages: v1.37.x → v1.38.0
- grpc: v1.75.1 → v1.78.0
- golang.org/x packages (crypto, net, oauth2, sync, sys, term, text)
- Various cloud and telemetry dependencies
No manual review required for this generated content.
go.mod (3)
54-54: OpenTelemetry and transitive dependency updates look substantial.The PR updates multiple indirect dependencies including:
- OpenTelemetry stack: v1.37.x → v1.38.0 (across otel, otel/metric, otel/sdk, otel/trace)
- golang.org/x packages: net (v0.43.0 → v0.47.0), oauth2 (v0.30.0 → v0.32.0), sys (v0.35.0 → v0.38.0), term (v0.34.0 → v0.37.0), text (v0.28.0 → v0.32.0), sync (v0.16.0 → v0.19.0)
- Google Cloud: compute/metadata (v0.7.0 → v0.9.0), GCP detectors (v1.29.0 → v1.30.0)
- Other: envoyproxy (v1.32.4 → v1.35.0), spiffe (v2.5.0 → v2.6.0)
These are primarily transitive dependencies. While generally safe, the large jump in golang.org/x/net (4 minor versions) and golang.org/x/sys (3 minor versions) should be tested to ensure no unexpected behavior changes, particularly around networking and system calls.
Please ensure integration tests pass successfully with these dependency updates, particularly:
- Network-related operations (RPC, P2P, IBC)
- OpenTelemetry instrumentation and metrics collection
- Any code that directly interacts with updated packages
Also applies to: 66-66, 90-90, 126-126, 134-134, 214-214, 227-227, 250-258, 263-268, 272-272
306-306: Note: ethermint replace directive updated.The ethermint dependency has been updated to a newer commit (
v0.22.1-0.20260105042515-e5b2b4df9e8d). This aligns with the preinstall functionality changes in this PR.
26-26: Verify coordination requirement for ibc-go v10.5.0 upgrade.The ibc-go v10.5.0 update introduces state-breaking changes (adds integrity validations for ProtoJSON-encoded ICA packets) and requires a coordinated chain upgrade. Since cronos actively uses ICA functionality (
x/cronos/keeper/precompiles/ica.go), verify that the chain upgrade is properly coordinated and any necessary migration steps are documented.The golang.org/x/crypto v0.44.0 update is a routine dependency refresh with no security fixes, deprecations, or API changes to address. The grpc v1.78.0 release includes a behavior change for unbracketed colons in hostnames (when using Go 1.26+), but cronos uses standard gRPC configuration, so this is unlikely to impact the current setup.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1946 +/- ##
==========================================
+ 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
🧹 Nitpick comments (1)
integration_tests/test_preinstall.py (1)
15-26: Line length issue resolved; test logic correctly validates the PR objective.The comment is now properly split across two lines, addressing the linter failure. The test correctly:
- Funds the preinstall address with 5 wei
- Verifies the transaction succeeds with standard gas usage (21000)
- Ensures the address remains empty before governance registration
This validates that preinstall registration works even when the account has a non-zero balance, which aligns with the PR objective.
Note: The past review suggested using the
send_transactionhelper from utils for consistency with the codebase pattern, but this wasn't addressed. While the current direct usage is functionally correct, consider this for future refactoring if consistency is a priority.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
integration_tests/test_preinstall.py
🧰 Additional context used
🧬 Code graph analysis (1)
integration_tests/test_preinstall.py (2)
integration_tests/utils.py (1)
submit_gov_proposal(226-238)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: Run golangci-lint
- GitHub Check: build (macos-14)
- GitHub Check: build (macos-latest)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: gomod2nix
- GitHub Check: integration_tests (ibc_rly_gas)
- GitHub Check: integration_tests (gov)
- GitHub Check: integration_tests (gas)
- GitHub Check: integration_tests (upgrade)
- GitHub Check: integration_tests (ibc_rly_evm)
- GitHub Check: integration_tests (unmarked)
- GitHub Check: integration_tests (ica)
- GitHub Check: integration_tests (slow)
- GitHub Check: integration_tests (evm)
- GitHub Check: integration_tests (ibc_update_client)
- GitHub Check: integration_tests (ibc)
- GitHub Check: integration_tests (mint)
- GitHub Check: integration_tests (ibc_timeout)
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (2)
integration_tests/test_preinstall.py (2)
5-5: LGTM - Import additions support the new test logic.The addition of
ADDRSandsubmit_gov_proposalimports is correct and both are properly utilized in the expanded test flow.
28-57: Excellent test coverage for the preinstall-with-balance scenario.The test flow is well-structured and comprehensive:
- Fund the preinstall address
- Verify it's empty (comment at line 28 clarifies intent)
- Register the preinstall via governance
- Verify the code is correctly deployed
This thoroughly validates that preinstall registration succeeds when the account has a pre-existing balance, which is exactly what the PR aims to fix.
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
Bug Fixes
Tests
Chores
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.