add upgrade handler for v1.5.1; fixes #1273; fixes #1274 #1275
Conversation
joe-bowman
requested review from
faddat,
minhngoc274 and
tuantran1702
as code owners
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThe recent changes introduce a new upgrade to version Changes
Assessment against linked issues
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (4)
- app/upgrades/types.go (1 hunks)
- app/upgrades/upgrades.go (1 hunks)
- app/upgrades/v1_5.go (2 hunks)
- app/upgrades_test.go (2 hunks)
Check Runs (1)
lint completed (3)
- app/upgrades/v1_5.go: 60-60: File is not
gofumpt-ed (gofumpt) - app/upgrades/v1_5.go: 73-73: File is not
gofumpt-ed (gofumpt)
Files skipped from review due to trivial changes (1)
- app/upgrades/types.go
Additional comments: 4
app/upgrades/upgrades.go (1)
- 28-28: The addition of the
V010501UpgradeNameentry with its correspondingV010501UpgradeHandleris consistent with the PR objectives to address the issues Claims IBC Denoms computed incorrectly #1273 and Low RR restitution records created with status 0 #1274. This change integrates well with the existing upgrade pattern in the file. However, ensure that theV010501UpgradeHandlerfunction is implemented correctly elsewhere in the codebase to address the specific issues mentioned. It's also important to verify that all necessary tests have been added to cover the new upgrade logic.app/upgrades/v1_5.go (2)
- 133-176: The
resetLiquidClaimsDenomsfunction addresses the issue of incorrect IBC denomination calculations by iterating over zones and updating protocol data. The logic appears to align with the objectives. However, there are a couple of points to consider:
- The error handling within the
IterateZonescallback (line 158) sets a sharederrvariable and returnstrueto stop iteration. This approach can lead to partial updates if an error occurs midway. Consider accumulating errors or ensuring atomicity of operations where feasible.- The use of hardcoded channel mappings (lines 139-148) might limit flexibility. If these channels are subject to change, consider fetching them from a more dynamic source or configuration.
Consider reviewing the error handling strategy to ensure consistency and reliability during the upgrade process. Additionally, evaluate the maintainability of hardcoded channel mappings.
- 60-60: The file formatting hints suggest running
gofumpt, a stricter version ofgofmt, to ensure consistent formatting according to Go best practices. This includes removing extra lines and ensuring consistent indentation. While the specific lines mentioned might not directly correspond to formatting issues, it's a good practice to run the entire file throughgofumptas part of the final cleanup before merging.Consider running the file through
gofumptto ensure consistent Go formatting.Also applies to: 73-73
app/upgrades_test.go (1)
- 726-746: The implementation of
TestV010501UpgradeHandler()appears to be thorough and well-structured. It tests the upgrade handler for version v0.10.501, focusing on the correct handling of protocol data related to liquid token denominations for theosmosis-1chain. The test validates the presence and correctness of theLiquidAllowedDenomProtocolDataafter the upgrade handler execution, ensuring the upgrade logic functions as expected.However, it's important to ensure that the test covers all critical paths and edge cases related to the upgrade process. Consider adding more assertions to verify the state of other affected components or data structures, if any, to ensure comprehensive coverage.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1275 +/- ##
=======================================
Coverage 63.29% 63.29%
=======================================
Files 170 170
Lines 11261 11261
=======================================
Hits 7128 7128
Misses 3409 3409
Partials 724 724
Flags with carried forward coverage won't be shown. Click here to find out more. |
1. Summary
Fixes #1273
Fixes #1274
2.Type of change
3. Implementation details
Summary by CodeRabbit