-
Notifications
You must be signed in to change notification settings - Fork 120
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
Add two unit tests for ProcessRewards
#806
base: main
Are you sure you want to change the base?
Conversation
WalkthroughThe changes involve updates to a test suite for the 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.
Address: authtypes.NewModuleAddress(types.TreasuryAccountName).String(), | ||
Coins: []sdk.Coin{{ | ||
Denom: TestRewardTokenDenom, | ||
Amount: sdkmath.NewInt(14_282_714), // 200 - 105.34 - 79.955 - 0.419 ~= 14.28 full coins |
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.
In the test case "three reward shares, enough treasury balance, $1.18 token price, 0.99 fee multiplier", the treasury account address is retrieved using authtypes.NewModuleAddress(types.TreasuryAccountName).String()
. This is inconsistent with the rest of the test cases, which use types.TreasuryModuleAddress.String()
. To maintain consistency and avoid potential issues, the same method should be used throughout the test cases.
Address: authtypes.NewModuleAddress(types.TreasuryAccountName).String(), | ||
Coins: []sdk.Coin{{ | ||
Denom: TestRewardTokenDenom, | ||
Amount: sdkmath.NewInt(138_094_240), // 200 - 105.34 - 79.955 - 0.419 ~= 138.094 full coins |
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.
In the test case "three reward shares, enough treasury balance, $1.18 token price, 0.33 fee multiplier", the treasury account address is retrieved using authtypes.NewModuleAddress(types.TreasuryAccountName).String()
. This is inconsistent with the rest of the test cases, which use types.TreasuryModuleAddress.String()
. To maintain consistency and avoid potential issues, the same method should be used throughout the test cases.
}, | ||
tokenPrice: tokenPrice1_18Usdc, | ||
treasuryAccountBalance: sdkmath.NewInt(200_000_000), // 200 full coins | ||
feeMultiplierPpm: 330_000, // 99% |
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 comment for the feeMultiplierPpm
in the test case "three reward shares, enough treasury balance, $1.18 token price, 0.33 fee multiplier" states "99%" which is incorrect. The actual value used in the test case is 330_000
, which corresponds to "33%". The comment should be updated to reflect the correct fee multiplier percentage to avoid confusion.
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.
Nit: Fix this comment
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.
@roy-dydx I've corrected the comment to accurately reflect the fee multiplier percentage:
- feeMultiplierPpm: 330_000, // 99%
+ feeMultiplierPpm: 330_000, // 33%
This change ensures that the comment now matches the actual value used in the test case.
}}, | ||
}, | ||
{ | ||
Address: authtypes.NewModuleAddress(types.TreasuryAccountName).String(), |
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.
Coderabbit made a good comment, so I'm calling it out (I personally ignore code rabbit these days): please use types.TreasuryModuleAddress.String()
here and below instead? (This was a recentish code change, maybe 3 weeks ago.)
Changelist
Added two unit tests:
Test Plan
[Describe how this PR was tested (if applicable)]
Author/Reviewer Checklist
state-breaking
label.PrepareProposal
orProcessProposal
, manually add the labelproposal-breaking
.feature:[feature-name]
.backport/[branch-name]
.refactor
,chore
,bug
.