-
Notifications
You must be signed in to change notification settings - Fork 193
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(inflation): fix default inflation allocation params #1688
Conversation
WalkthroughThe codebase underwent a significant update to the inflation distribution parameters, altering the percentages allocated to the Community Pool, Staking Rewards, and Strategic Reserves. This change is reflected in both the constants and the expected values within test functions, indicating a shift in the inflation logic that could affect dependent systems. Additionally, some cleanup was performed in test files, removing unnecessary print statements and related checks. Changes
TipsChat with CodeRabbit Bot (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CommunityPool: sdk.NewDecWithPrec(35_159141, 8), // 35.159141% | ||
StakingRewards: sdk.NewDecWithPrec(27_757217, 8), // 27.757217% | ||
StrategicReserves: sdk.NewDecWithPrec(37_083642, 8), // 37.083642% |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The sum of the new percentage values for CommunityPool
, StakingRewards
, and StrategicReserves
exceeds 100%. This is not valid for a distribution where the total should be exactly 100%. Please correct the values to ensure they sum up to 100%.
- CommunityPool: sdk.NewDecWithPrec(35_159141, 8), // 35.159141%
- StakingRewards: sdk.NewDecWithPrec(27_757217, 8), // 27.757217%
- StrategicReserves: sdk.NewDecWithPrec(37_083642, 8), // 37.083642%
+ CommunityPool: sdk.NewDecWithPrec(<correct_value>, 8), // <correct_percentage>%
+ StakingRewards: sdk.NewDecWithPrec(<correct_value>, 8), // <correct_percentage>%
+ StrategicReserves: sdk.NewDecWithPrec(<correct_value>, 8), // <correct_percentage>%
Committable suggestion
❗ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
CommunityPool: sdk.NewDecWithPrec(35_159141, 8), // 35.159141% | |
StakingRewards: sdk.NewDecWithPrec(27_757217, 8), // 27.757217% | |
StrategicReserves: sdk.NewDecWithPrec(37_083642, 8), // 37.083642% | |
CommunityPool: sdk.NewDecWithPrec(<correct_value>, 8), // <correct_percentage>% | |
StakingRewards: sdk.NewDecWithPrec(<correct_value>, 8), // <correct_percentage>% | |
StrategicReserves: sdk.NewDecWithPrec(<correct_value>, 8), // <correct_percentage>% |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (1)
- CHANGELOG.md (1 hunks)
Additional comments: 1
CHANGELOG.md (1)
- 87-90: The changelog correctly documents the significant change to the default inflation allocation as per the pull request.
@@ -87,6 +87,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 | |||
* [#1659](https://github.com/NibiruChain/nibiru/pull/1659) - refactor(oracle): curate oracle default whitelist | |||
* [#1679](https://github.com/NibiruChain/nibiru/pull/1679) - test(perp): add more tests for perp module msg server | |||
* [#1686](https://github.com/NibiruChain/nibiru/pull/1686) - test(perp): add more tests for perp module msg server for DnR | |||
* [#1688](https://github.com/NibiruChain/nibiru/pull/1688) - fix(inflation): make default inflation allocation follow tokenomics |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The same entry for the fix related to default inflation allocation appears twice in the changelog, which seems to be an oversight and should be consolidated into a single entry.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (3)
- CHANGELOG.md (1 hunks)
- x/inflation/keeper/hooks_test.go (1 hunks)
- x/inflation/keeper/inflation_test.go (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- CHANGELOG.md
Additional comments: 5
x/inflation/keeper/hooks_test.go (2)
161-180: The changes in the test function
TestPeriodChangesSkippedEpochsAfterEpochEnd
correctly reflect the removal of debug print statements and related conditional logic as described in the summary. The remaining assertions and logic appear to be intact and relevant to the test's purpose.161-180: The test modifications align with the PR's objective to adjust inflation allocation parameters. The assertions in the test are checking the expected behavior of the inflation logic after epoch changes, which is crucial for validating the new tokenomics.
x/inflation/keeper/inflation_test.go (3)
37-47: The updated expected values in the test case reflect the new allocation percentages for the Community Pool, Staking Rewards, and Strategic Reserves as per the PR objective. This ensures that the test will validate the correct distribution of inflation.
59-69: The test case without a root account specified correctly reflects the scenario where the strategic reserves should remain in the module account, and the expected values are updated accordingly.
37-48: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [37-69]
The summary provided matches the changes in the hunks, indicating that the expected values for coin amounts in the
TestMintAndAllocateInflation
function have been updated to reflect the new inflation distribution parameters.
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #1688 +/- ##
==========================================
+ Coverage 74.19% 74.26% +0.06%
==========================================
Files 193 193
Lines 15444 15476 +32
==========================================
+ Hits 11459 11493 +34
+ Misses 3324 3318 -6
- Partials 661 665 +4
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
* fix: fix default inflation allocation * chore: changelog * fix: fix tests --------- Co-authored-by: Unique Divine <[email protected]>
Description
Make it follow tokenomics
Purpose
Why is this PR important?
Summary by CodeRabbit
New Features
Bug Fixes
Tests