-
Notifications
You must be signed in to change notification settings - Fork 2
refactor(RewardStreamerMP): extract MP and Stake mathematical formula… #109
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
Conversation
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.
@3esmit great work!
There are some comments I'd like you and @gravityblast to review.
The overall direction is really good!
/// @notice The accrue rate period of time over which multiplier points are calculated. | ||
uint256 public constant ACCRUE_RATE = 12 seconds; | ||
/// @notice Minimal value to generate 1 multiplier point in the accrue rate period (rounded up). | ||
uint256 public constant MIN_BALANCE = (((YEAR * 100) - 1) / (MP_APY * ACCRUE_RATE)) + 1; |
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.
Where does this 100
come from? i It's not the MP_APY
Can we have this live in a constant as well?
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.
MP_APY stands for "Multiplier Points Annual Percentage Yield". The unit of MP_APY is Token % Per Year
, the unit of MIN_BALANCE is Token. In order to convert Token % Per Year to Token
, we need to remove the Percentage and Year.
I am unsure if a mathematical artifact should have a constant, otherwise we should call it "CENT", or something like that, and seems to complicate the visualization of the formula
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.
Okay I see, this makes more sense now.
I still think that the MIN_BALANCE
is something we should avoid.
We should rather check if MPs greater > 0 will be generated with the given stake amount and if not, simply not update the lastUpdateTime, so next time we calculate MPs we still calculate from the original last update.
See the comment here: #109 (comment)
2e124b4
to
6c9963f
Compare
@3esmit to move on with this I've now made the necessary changes myself and squashed your commits into one. |
…s to abstract contracts
6c9963f
to
0544c0e
Compare
…s to abstract contracts
Description
Closes #107
Checklist
Ensure you completed all of the steps below before submitting your pull request:
pnpm adorno
?pnpm verify
?