fix: allow preinstall in case account has a balance #1940
Conversation
thomas-nguy
requested review from
XinyuCRO and
calvinaco
and removed request for
a team
📝 WalkthroughWalkthroughUpdated dependencies and Nix metadata, added an UNRELEASED changelog entry about preinstall behavior, and extended an integration test to send an on‑chain transaction and assert its receipt before existing Create2 preinstall checks. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: defaults 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). (20)
🔇 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.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
integration_tests/test_preinstall.py (1)
14-23: Excellent test coverage for the fix!This test logic effectively validates the PR's objective by:
- Sending funds to the Create2 address (giving it a balance)
- Verifying the transaction succeeds
- Later confirming that preinstall code can still be registered at that address
The assertion for
gasUsed == 21000is correct for a simple ETH transfer.Optional: Add explicit balance verification for clarity.
Consider adding a balance check after the transaction to make the test intent more explicit:
receipt = w3.eth.wait_for_transaction_receipt(txhash) assert receipt.status == 1 assert receipt.gasUsed == 21000 + +# Verify the address now has a balance +balance = w3.eth.get_balance(create2address) +assert balance == 5This would make it explicit that the address has a balance before attempting the preinstall, further documenting the test's purpose.
📜 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 (4)
CHANGELOG.md(1 hunks)go.mod(4 hunks)gomod2nix.toml(3 hunks)integration_tests/test_preinstall.py(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: Build
go.mod
[error] 1-1: Command: nix develop .#rocksdb -c make test failed. go.mod specifies Go version 1.25.0, but no compatible Go attribute could be found.
⏰ 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). (1)
- GitHub Check: Run golangci-lint
🔇 Additional comments (4)
integration_tests/test_preinstall.py (1)
5-5: LGTM!The
ADDRSimport is necessary for accessing the validator address in the new test logic.CHANGELOG.md (1)
10-10: LGTM!The changelog entry is properly formatted and correctly placed in the UNRELEASED section.
gomod2nix.toml (1)
327-329: Dependency metadata updates are consistent with go.mod.The changes to this file reflect the dependency updates made in
go.mod, including:
- Ethermint source change to the personal fork (line 327-329)
- Multiple
golang.org/x/*version and hash updates (lines 759-782)google.golang.org/protobufupdate (lines 801-802)Note that if the issues identified in
go.mod(Go version and Ethermint fork) are addressed, this file will need to be regenerated usinggomod2nixto reflect the corrected dependencies.Also applies to: 759-782, 801-802
go.mod (1)
3-3: Go 1.25.0 is a valid, released version; verify that Go 1.25+ is installed in your build environment.Go 1.25.0 is a major release officially released on August 12, 2025. The error indicates a build environment configuration issue rather than an invalid version. Go 1.25.5 (the latest stable version as of December 2, 2025) is available and stable. Ensure your CI/CD pipeline and build environment have Go 1.25+ installed.
Likely an incorrect or invalid review comment.
| github.com/spf13/viper v1.21.0 | ||
| github.com/stretchr/testify v1.11.1 | ||
| golang.org/x/crypto v0.41.0 | ||
| golang.org/x/crypto v0.44.0 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's check the go.mod file to see all the dependency changes
echo "=== Checking go.mod file around line 42 and 45 ==="
sed -n '35,55p' go.mod
echo ""
echo "=== Checking go.mod file around line 263-268 ==="
sed -n '255,275p' go.mod
echo ""
echo "=== Searching for golang.org/x/crypto usage in Go files ==="
rg -n "golang.org/x/crypto" --type=go -C 1 | head -30
echo ""
echo "=== Searching for google.golang.org/protobuf usage in Go files ==="
rg -n "google.golang.org/protobuf" --type=go -C 1 | head -30Repository: crypto-org-chain/cronos
Length of output: 2584
Consider upgrading golang.org/x/crypto to v0.45.0 or later.
The current version v0.44.0 contains security vulnerabilities (CVE-2025-58181, CVE-2025-47914) that are fixed in v0.45.0 and later. The google.golang.org/protobuf v1.36.11 update is a stable patch release with no breaking changes.
🤖 Prompt for AI Agents
In go.mod around line 42, upgrade the module golang.org/x/crypto from v0.44.0 to
v0.45.0 (or later) to pick up security fixes; update the version in go.mod, run
`go get golang.org/x/[email protected]` (or `@latest`), then run `go mod tidy` to
refresh go.sum and resolve dependencies, and finally run the project's
tests/build to verify there are no regressions.
go.mod
Outdated
| github.com/ethereum/go-ethereum => github.com/crypto-org-chain/go-ethereum v1.10.20-0.20250815065500-a4fbafcae0dd | ||
| // develop | ||
| github.com/evmos/ethermint => github.com/crypto-org-chain/ethermint v0.22.1-0.20251208051817-9f5d1db3246d | ||
| github.com/evmos/ethermint => github.com/thomas-nguy/ethermint v0.4.2-0.20251216120354-91058302724d |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for references to both ethermint sources and any documentation explaining the fork choice
rg -i "(thomas-nguy|ethermint fork|archived)" --type=md -C 3Repository: crypto-org-chain/cronos
Length of output: 49
🏁 Script executed:
# Check git history for when and why this change was made
cd "$(git rev-parse --show-toplevel)" 2>/dev/null || true
git log --oneline -20 -- go.mod | head -20Repository: crypto-org-chain/cronos
Length of output: 88
🏁 Script executed:
# Look for any comments in go.mod or related files explaining the fork
grep -n -B 5 -A 5 "thomas-nguy/ethermint" go.modRepository: crypto-org-chain/cronos
Length of output: 872
🏁 Script executed:
# Check if there are any CHANGELOG, README, or docs explaining this choice
find . -type f \( -name "CHANGELOG*" -o -name "README*" -o -name "*.md" \) | xargs grep -l "ethermint\|fork" 2>/dev/null | head -10Repository: crypto-org-chain/cronos
Length of output: 348
🏁 Script executed:
grep -i "fork\|ethermint\|maintain\|upstream" README.md CONTRIBUTING.md 2>/dev/null | head -20Repository: crypto-org-chain/cronos
Length of output: 1527
🏁 Script executed:
# Check the specific commit referenced in the fork to understand what customizations exist
# The version is v0.4.2-0.20251216120354-91058302724d
# Try to find any code or comments explaining why this fork is used
rg -i "thomas-nguy\|ethermint.*custom\|ethermint.*fork" . --type=go -C 2 | head -30Repository: crypto-org-chain/cronos
Length of output: 49
🏁 Script executed:
# Check if there's a CHANGELOG or release notes explaining the fork
cat CHANGELOG.md 2>/dev/null | grep -i "ethermint\|fork" -C 2 | head -20Repository: crypto-org-chain/cronos
Length of output: 1197
🏁 Script executed:
# Look for any architecture decision records (ADR) or design docs explaining fork usage
fd -t f "ADR|adr|architecture|decision" docs/ 2>/dev/null | xargs grep -l "ethermint\|fork" 2>/dev/nullRepository: crypto-org-chain/cronos
Length of output: 182
🏁 Script executed:
# Check if there's a crypto-org-chain/ethermint fork mentioned in CHANGELOG or elsewhere
rg "crypto-org-chain.*ethermint" . -C 2 | head -40Repository: crypto-org-chain/cronos
Length of output: 3604
🏁 Script executed:
# Check the git history for the ethermint dependency to understand when/why it changed to thomas-nguy
git log --all --oneline -- go.mod | grep -i "ethermint" | head -5Repository: crypto-org-chain/cronos
Length of output: 319
🏁 Script executed:
# Look for any issues or PRs mentioning the ethermint fork choice
git log -p -- go.mod | grep -B 10 -A 10 "thomas-nguy\|ethermint" | head -50Repository: crypto-org-chain/cronos
Length of output: 49
🏁 Script executed:
# Get more detailed git log for go.mod to see when thomas-nguy fork was introduced
git log --all -p -- go.mod | grep -B 30 -A 5 "thomas-nguy/ethermint" | head -60Repository: crypto-org-chain/cronos
Length of output: 2110
🏁 Script executed:
# Check when the switch to thomas-nguy happened and what the commit message says
git log --all --format="%H %s" -- go.mod | head -3 | while read commit msg; do echo "=== $msg ==="; git show "$commit" | grep -A 5 -B 5 "ethermint" | head -20; doneRepository: crypto-org-chain/cronos
Length of output: 2572
🏁 Script executed:
# Look for any references to crypto-org-chain/ethermint vs thomas-nguy in recent commits
git log --all -S "ethermint" --oneline -- go.mod | head -10Repository: crypto-org-chain/cronos
Length of output: 624
🏁 Script executed:
# Check the specific commit that changed to thomas-nguy/ethermint
git show 398b200 --stat | head -20Repository: crypto-org-chain/cronos
Length of output: 1086
🏁 Script executed:
# Look for commit messages explaining the change from crypto-org-chain to thomas-nguy
git log --all --format="%H %ai %s" -- go.mod | head -20Repository: crypto-org-chain/cronos
Length of output: 2569
🏁 Script executed:
# Check if there are any PRs or issues referencing the fork change
git log --all -S "thomas-nguy/ethermint" --oneline | head -5Repository: crypto-org-chain/cronos
Length of output: 153
🏁 Script executed:
# Find the commit that introduced thomas-nguy/ethermint and show its full message
git log --all -S "thomas-nguy/ethermint" --format="%H %s %b" | head -100Repository: crypto-org-chain/cronos
Length of output: 153
Document the maintenance plan and rationale for switching to the personal Ethermint fork.
The upstream github.com/evmos/ethermint was archived on April 4, 2024 and is now read-only, which justifies using a fork for continued development. However, the recent switch from github.com/crypto-org-chain/ethermint to github.com/thomas-nguy/ethermint lacks explanation in documentation or comments. Add clarification in the codebase about:
- Why the fork was switched (was crypto-org-chain/ethermint no longer maintained?)
- Whether thomas-nguy/ethermint is actively maintained or if this is temporary
- Long-term maintenance responsibilities and update frequency
🤖 Prompt for AI Agents
In go.mod around line 306, the swap to the personal fork
github.com/thomas-nguy/ethermint lacks documented rationale and maintenance
plan; add a brief comment in the repo (adjacent to the go.mod entry or in a new
docs/DEPENDENCIES.md or MAINTAINERS.md) stating why we switched from the
archived upstream (evmos/ethermint), whether the previous crypto-org-chain fork
was unmaintained, whether thomas-nguy/ethermint is actively maintained or
temporary, who owns long‑term maintenance responsibilities, how often
updates/security pulls will be applied, and a contact or issue link for
follow-up. Ensure the note includes version pin justification and a plan for
reverting to upstream or adopting another fork in future.
Signed-off-by: Thomas <[email protected]>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1940 +/- ##
==========================================
+ Coverage 16.87% 18.51% +1.64%
==========================================
Files 72 75 +3
Lines 6163 5114 -1049
==========================================
- Hits 1040 947 -93
+ Misses 5000 4045 -955
+ Partials 123 122 -1 🚀 New features to boost your workflow:
|
1 participant
👮🏻👮🏻👮🏻 !!!! 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
Chores
Tests
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.