fix: handle upgrade empty params #101
Conversation
WalkthroughThe pull request introduces updates to two files: Changes
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🧹 Outside diff range and nitpick comments (3)
x/evm/keeper/params.go (1)
22-23: LGTM! Consider adding a comment for context.The nil check is a good addition that aligns with the PR objective of handling empty GasRefundRatio during upgrades.
Consider adding a comment to explain why this nil check is necessary:
if err != nil { return math.LegacyZeroDec(), err + // Handle empty GasRefundRatio parameter during blockchain upgrades } else if params.GasRefundRatio.IsNil() { return math.LegacyZeroDec(), nil }app/upgrade.go (2)
Line range hint
21-24: Improve error handling robustnessThe current error handling approach has several potential issues:
- String-based error comparison is fragile and could break if the error message changes
- It might accidentally mask other errors containing the same string
Consider this more robust approach:
- if err := app.EVMKeeper.DeployERC20Wrapper(ctx); err != nil && - // ignore contract address collision error (contract already deployed) - !strings.Contains(err.Error(), vm.ErrContractAddressCollision.Error()) { + if err := app.EVMKeeper.DeployERC20Wrapper(ctx); err != nil { + // Explicitly handle the case where the contract is already deployed + if !errors.Is(err, vm.ErrContractAddressCollision) { + return nil, fmt.Errorf("failed to deploy ERC20 wrapper: %w", err) + } + // Log that we're ignoring a known condition + logger.Info("ERC20 wrapper contract already deployed, skipping deployment")Also, please add a comment explaining why it's safe to ignore the contract address collision error.
Line range hint
18-27: Consider additional upgrade handler safety measuresThe upgrade handler could benefit from additional safety measures:
- Validate upgrade preconditions before proceeding
- Handle potential state migrations
- Consider rollback mechanisms for partial failures
Would you like me to provide an example implementation with these safety measures?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
app/upgrade.go(1 hunks)x/evm/keeper/params.go(1 hunks)
🔇 Additional comments (1)
app/upgrade.go (1)
13-13: Verify version bump from 0.6.0 to 0.6.3
The version increment spans 3 minor versions. Please ensure this significant version change is properly documented.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #101 +/- ##
==========================================
- Coverage 28.34% 28.33% -0.01%
==========================================
Files 126 126
Lines 14022 14024 +2
==========================================
Hits 3974 3974
- Misses 9487 9489 +2
Partials 561 561
|
Description
If the chain upgraded via upgrade proposal, GasRefundRatio maybe empty. so we need to return zero dec instead of returning nil dec.
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!in the type prefix if API or client breaking changeReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
Summary by CodeRabbit