fix: supply offsets for osmo token #8751
Conversation
WalkthroughThe changes in this pull request include modifications to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🧰 Additional context used🔇 Additional comments (9)
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: 1
🧹 Outside diff range and nitpick comments (6)
app/upgrades/v27/constants.go (1)
10-13: LGTM! Consider adding a comment forOsmoToken.The introduction of a constant block improves code organization. The
UpgradeNameconstant remains unchanged, and the newOsmoTokenconstant is a welcome addition.Consider adding a brief comment explaining the purpose of the
OsmoTokenconstant, similar to the existing comment forUpgradeName. For example:const ( // UpgradeName defines the on-chain upgrade name for the Osmosis v27 upgrade. UpgradeName = "v27" // OsmoToken defines the denomination for the OSMO token. OsmoToken = "uosmo" )app/upgrades/v27/upgrades_test.go (4)
23-25: LGTM: Constant definition is appropriate.The
v27UpgradeHeightconstant is well-defined for testing purposes.Consider adding a brief comment explaining the purpose of this constant, e.g.:
// v27UpgradeHeight represents the block height at which the v27 upgrade is simulated in tests const v27UpgradeHeight = int64(10)
36-50: LGTM: TestUpgrade method covers the main upgrade scenario.The method effectively sets up the test environment, executes the upgrade, and checks for potential issues.
Consider adding more specific assertions after the upgrade to verify the expected state changes. For example:
// Add after line 47 s.Require().Equal(v27UpgradeHeight, s.Ctx.BlockHeight(), "Block height should be equal to upgrade height after upgrade")
52-61: LGTM: dummyUpgrade function correctly simulates an upgrade.The function effectively sets up the upgrade scenario for testing purposes.
Consider adding a comment explaining the purpose of this function, and improve error handling:
// dummyUpgrade simulates an upgrade by scheduling a plan and updating the context func dummyUpgrade(s *UpgradeTestSuite) { // ... existing code ... plan, err := s.App.UpgradeKeeper.GetUpgradePlan(s.Ctx) s.Require().NoError(err) s.Require().Equal(v27.Upgrade.UpgradeName, plan.Name, "Upgrade plan name should match") }
63-77: LGTM: Supply offset test methods are well-implemented.The
PrepareSupplyOffsetTestandExecuteSupplyOffsetTestmethods effectively set up and verify the supply offset functionality.Consider adding more test cases to cover edge scenarios:
- Test with zero offsets
- Test with large (near max int64) offsets
- Test with negative total supply
Example:
func (s *UpgradeTestSuite) TestSupplyOffsetEdgeCases() { // Test with zero offsets s.App.BankKeeper.AddSupplyOffset(s.Ctx, v27.OsmoToken, osmomath.ZeroInt()) s.App.BankKeeper.AddSupplyOffsetOld(s.Ctx, v27.OsmoToken, osmomath.ZeroInt()) s.ExecuteSupplyOffsetTest() // Test with large offsets largeOffset := osmomath.NewIntFromUint64(^uint64(0) >> 1) // Max int64 s.App.BankKeeper.AddSupplyOffset(s.Ctx, v27.OsmoToken, largeOffset) s.ExecuteSupplyOffsetTest() // Test with negative total supply s.App.BankKeeper.AddSupplyOffset(s.Ctx, v27.OsmoToken, osmomath.NewInt(-1000000)) s.ExecuteSupplyOffsetTest() }app/upgrades/v27/upgrades.go (1)
26-26: Nitpick: Usekeepers.BankKeeperdirectly instead of aliasing tobk.Since
bkis only used in a few places within this function, you might consider usingkeepers.BankKeeperdirectly to improve readability and reduce unnecessary variables.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (3)
go.modis excluded by!**/*.modgo.sumis excluded by!**/*.sum,!**/*.sumgo.workis excluded by!**/*.work,!**/*.work
📒 Files selected for processing (3)
- app/upgrades/v27/constants.go (1 hunks)
- app/upgrades/v27/upgrades.go (1 hunks)
- app/upgrades/v27/upgrades_test.go (1 hunks)
🔇 Additional comments (4)
app/upgrades/v27/constants.go (1)
12-12: Verify the usage ofOsmoTokenconstant.The introduction of the
OsmoTokenconstant aligns with the PR objective of fixing supply offsets for the osmo token. However, it's not used within this file.Please ensure that this constant is utilized appropriately in other parts of the codebase related to the supply offset fix. Run the following script to verify its usage:
If the constant is not used, consider removing it or adding a TODO comment explaining its future use.
✅ Verification successful
Usage of
OsmoTokenis verified and appropriate.The
OsmoTokenconstant is properly utilized across relevant files in the codebase, aligning with the PR objectives for fixing supply offsets. The occurrences of the string"uosmo"in test and configuration files are appropriate and do not require replacement.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of OsmoToken constant in the codebase. # Test: Search for OsmoToken usage. Expect: Occurrences in files related to supply offset calculations or token operations. rg --type go -A 5 'OsmoToken' # Test: Search for hardcoded "uosmo" strings that could be replaced with OsmoToken. Expect: No occurrences or justified occurrences. rg --type go -A 5 '"uosmo"'Length of output: 21612
app/upgrades/v27/upgrades_test.go (3)
1-21: LGTM: Package declaration and imports are appropriate.The package name and imports are well-structured and relevant for the test suite's requirements.
27-34: LGTM: Test suite structure is well-defined.The
UpgradeTestSuitestruct andTestUpgradeTestSuitefunction are correctly implemented, following Go testing conventions.
1-77: Overall, the test suite is well-implemented and covers key functionality.The
app/upgrades/v27/upgrades_test.gofile provides a comprehensive test suite for the v27 upgrade, including the main upgrade scenario and supply offset functionality. The code is well-structured and follows Go testing conventions.To further enhance the test suite:
- Add more detailed comments explaining the purpose of each test function.
- Implement additional test cases to cover edge scenarios, especially for supply offset calculations.
- Consider adding benchmarks for performance-critical operations, if any.
To ensure comprehensive test coverage, run the following command:
This will help identify any areas that may need additional test cases.
| bk := keepers.BankKeeper | ||
|
|
||
| // Get the old offset from the old key: []byte{0x88} | ||
| offsetOld := bk.GetSupplyOffsetOld(ctx, OsmoToken) |
There was a problem hiding this comment.
Issue: Undefined variable OsmoToken.
The variable OsmoToken is used but not defined within this file. Please ensure that OsmoToken is properly defined or imported to prevent compilation errors.
|
|
||
| s.Require().Equal("500uosmo", coin.String()) | ||
| s.Require().Equal("500", offset.String()) | ||
| s.Require().Equal("0", oldOffset.String()) |
There was a problem hiding this comment.
Because the old value is removed from the store! I think we could delete the old state in a later migration to be safe? wdyt?
|
This pull request has been automatically marked as stale because it has not had any recent activity. It will be closed if no further activity occurs. Thank you! |
|
This should be good to merge right? @PaddyMc |
What is the purpose of the change
Fix the supply offset issues
cosmos-sdk PR => osmosis-labs/cosmos-sdk#629
How to test
before and after in-place-testnet
osmosisd q bank total-supply-of uosmo --output json | jqbeware of dragons: