refactor(RewardStreamerMP): extract MP and Stake mathematical formula… #109
Conversation
…s to abstract contracts
chore(lint): Fix grammar
There was a problem hiding this comment.
@3esmit great work!
There are some comments I'd like you and @gravityblast to review.
The overall direction is really good!
src/RewardsStreamerMP.sol
Outdated
|
|
||
| // Rewards Streamer with Multiplier Points | ||
| contract RewardsStreamerMP is | ||
| Initializable, |
There was a problem hiding this comment.
I don't think we want to remove this.
There was a problem hiding this comment.
Removed by accident.
src/math/StakeMath.sol
Outdated
| returns (uint256 _deltaMp, uint256 _newLockEnd) | ||
| { | ||
| require(_balance > 0); | ||
| require(_increasedLockSeconds > 0); |
There was a problem hiding this comment.
These should probably be equivalent to the checks done here: https://github.com/vacp2p/staking-reward-streamer/pull/109/files#diff-b8371c7492fb939a38fb61c84fb63d70e61f70c633e1a09e6e059c8372ec3846L241-L253
So I guess it's kinda the same as the comment I made in stake() regarding error handling.
At the very least, these should throw custom errors.
| function _getAccountPendingdMP(Account storage account) internal view returns (uint256) { | ||
| if (account.maxMP == 0 || account.stakedBalance == 0) { | ||
| if (block.timestamp == account.lastMPUpdateTime) { |
| /// @notice Multiplier points absolute maximum percentage yield. | ||
| uint256 public constant MP_MPY_ABSOLUTE = 100 + (2 * (MAX_MULTIPLIER * MP_APY)); | ||
| /// @notice The accrue rate period of time over which multiplier points are calculated. | ||
| uint256 public constant ACCRUE_RATE = 12 seconds; |
There was a problem hiding this comment.
How do we guarantee that this rate is always 12 seconds?
There was a problem hiding this comment.
It's not guaranteed, it's the minimal possible accrue_rate possible. If one to call the contract every block, or at least 2 consecutive blocks, as the blocktime is 12 seconds, this would be the accure rate.
There was a problem hiding this comment.
or at least 2 consecutive blocks, as the blocktime is 12 seconds
We get faster soft confirmations on L2s tho.
The same has been discussed here #52 (comment)
Not sure if you ever followed up on the changes but back then, we fixed it by ensuring we only increase the lastUpdatedTime only if MPs were actually generated.
This way, you don't need to know about any block time at all.
Here are the changes we've made back then: https://github.com/vacp2p/staking-reward-streamer/pull/100/files#diff-b8371c7492fb939a38fb61c84fb63d70e61f70c633e1a09e6e059c8372ec3846R493-R496
What do you think?
| /// @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.
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.
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.
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)
| * @param _maxMP The maximum multiplier points. | ||
| * @return bonusMP The calculated bonus multiplier points. | ||
| */ | ||
| function _retrieveBonusMP(uint256 _balance, uint256 _maxMP) internal pure returns (uint256 bonusMP) { |
There was a problem hiding this comment.
This is not used anywhere. Where is this supposed to go?
There was a problem hiding this comment.
MultiplierPointMath.sol, similar to other library contracts, such as SafeMath.sol, may have functions that are not used, but are available to facilitate the development.
Currently this function is not used, however, it can be used to implement a view function which could separate the constituents of total MP.
| */ | ||
| function _timeToAccrueMP(uint256 _balance, uint256 _mp) internal pure returns (uint256 timeToReachMaxMP) { | ||
| return Math.mulDiv(_mp * 100, YEAR, _balance * MP_APY); | ||
| } |
There was a problem hiding this comment.
This is not used anywhere. Where is this supposed to go?
There was a problem hiding this comment.
MultiplierPointMath.sol, similar to other library contracts, such as SafeMath.sol, may have functions that are not used, but are available to facilitate the development.
_timeToAccrueMP is used in tests, and might be used in the main contracts if such calculation is needed.
…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?