-
Notifications
You must be signed in to change notification settings - Fork 1
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
feat(RewardsStreamerMP.t.sol): Make tests use calc functions and test… #83
Conversation
Currently this only affects functions related to MP, but all functions that are using magic numbers should have their own calculation function, which is tested by itself. Also, not all MPs are yet changed to use the calculation functions, so this is a WIP and I expect to finish this week. |
@3esmit this one needs some |
test/RewardsStreamerMP.t.sol
Outdated
@@ -760,7 +839,7 @@ contract StakeTest is RewardsStreamerMPTest { | |||
); | |||
|
|||
uint256 currentTime = vm.getBlockTimestamp(); | |||
uint256 timeToMaxMP = _calculateTimeToMPLimit(stakeAmount); | |||
uint256 timeToMaxMP = _calculateTimeToMP(stakeAmount, totalMaxMP - totalMP); |
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.
Any reason this is changed? I think reading "calculate time to max MP" is more intuitive than what this change is doing.
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.
Because calculateTimeToMP is more generic, and can be reused for other tests. While TimeToMPLimit would hardcode this function to always calculate time to max mp.
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.
We should at least rename this. "time to MP" doesn't make sense. I no longer understand what the function is doing without looking at its source.
test/RewardsStreamerMP.t.sol
Outdated
return timeInSeconds; | ||
} | ||
|
||
function _calculateTimeToMP(uint256 amount, uint256 target) internal view returns (uint256) { |
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.
We should probably rename this. "calculate time to MP" doesn't make a lot of sense
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.
renamed to _calculateTimeToAccureMP
781c3ea
to
3f1c79f
Compare
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.
Great changes!
Some minor comments to address. Then this can be merged!
// wait for 1 year | ||
uint256 currentTime = vm.getBlockTimestamp(); | ||
vm.warp(currentTime + (365 days)); | ||
|
||
vm.warp(currentTime + (warpLength)); |
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.
I thnk warplength
is not necessary here. It doesn't add any value.
… those functions chore(linter): added solhint-disable-next-line max-line-length to circumvent bug in linter chore(RewardsStreamerMP.t.sol): rename _calculateTimeToMP -> _calculateTimeToAccureMP
… those functions
Description
Fixes #81
Checklist
Ensure you completed all of the steps below before submitting your pull request:
pnpm adorno
?pnpm verify
?