feat(StakeManager): implement multiplier points estimation #97
Conversation
0x-r4bbit
force-pushed
the
mp-estimate
branch
2 times, most recently
from
502254c
to
31dde1a
Compare
contracts/StakeManager.sol
Outdated
|
|
||
| StakeRewardEstimate public stakeRewardEstimate; | ||
|
|
||
| uint256 public currentEpochExpiredMP; |
There was a problem hiding this comment.
How about currentEpochMissedMP ?
| userReward += userEpochReward; | ||
| iEpoch.epochReward -= userEpochReward; | ||
| iEpoch.totalSupply -= userSupply; | ||
| //TODO: remove epoch when iEpoch.totalSupply reaches zero |
There was a problem hiding this comment.
@3esmit I think this TODO should be part of this PR
| } | ||
| account.epoch = userEpoch; | ||
| if (userReward > 0) { | ||
| pendingReward -= userReward; | ||
| stakedToken.transfer(account.rewardAddress, userReward); | ||
| } | ||
| mpDifference = account.totalMP - mpDifference; | ||
| mpDifference = account.totalMP - mpDifference; //TODO: optimize, this only needed for migration |
There was a problem hiding this comment.
Can we resolve this as well?
test/StakeManager.t.sol
Outdated
| assertEq(stakeManager.totalSupplyMP(), 0, "totalSupplyMP burned after unstaking"); | ||
| assertEq(totalMP, 0, "userMP burned after unstaking"); | ||
| } | ||
|
|
||
| function test_restakeOnLocked() public { | ||
| function _test_restakeOnLocked() public { |
There was a problem hiding this comment.
I wonder if we should remove all the tests related to restaking altogether.
There's also #103 , maybe we make it as part of that?
There was a problem hiding this comment.
I guess so, or refactor onto not tests.
| // only perform the assert below one epoch after *that* | ||
| if (i > 0) { | ||
| assertEq(totalMP, totalMPBefore, "must NOT increase MPs"); | ||
| } |
There was a problem hiding this comment.
@3esmit this is a fix I've introduced as it would otherwise immediately have a failing assertion.
Reason being is that we always have that 1 epoch after maxlimit epoch in which MPs will be minted, if there's accounts that started staking after the start time of any given epoch (which is likely 99% the case in reality).
So we have to check for greater 0 here to perform the assertion only after we've really minted the last MPs.
There was a problem hiding this comment.
Is not always, I am not sure about that. And in some cases (really small staking values), can be 2 epochs.
It should use the same calculations as in the smart contract, or better, use precalculated values and precalculated static checks.
0x-r4bbit
changed the title
0x-r4bbit
force-pushed
the
mp-estimate
branch
6 times, most recently
from
620a4b6
to
44d16c4
Compare
This commit introduces the internal accounting logic for accrueing multiplier points, that will later be used to determine how many experience points an account is eligible to. The majority of the work here was done by @3esmit.
0x-r4bbit
force-pushed
the
mp-estimate
branch
from
52a350f
to
9f3d8ff
Compare
contracts/StakeManager.sol
Outdated
| if (iEpoch.totalSupply == 0) { | ||
| pendingReward -= iEpoch.epochReward; | ||
| delete epochs[userEpoch]; | ||
| } |
There was a problem hiding this comment.
Hm.. This seems a bit odd.
We're subtracting iEpoch.epochReward from pendingReward, and we're also subtracting userReward from pendingReward in line 459.
Shouldn't we only subtract the userReward from pendingReward? And shouldn't we only do this once, not twice?
Also, this changes causes the rule to fail that subOfEpochRewards == pendingReward
Need to check this.
0x-r4bbit
force-pushed
the
mp-estimate
branch
from
9f3d8ff
to
530bec0
Compare
Description
This commit introduces the internal accounting logic for accrueing
multiplier points, that will later be used to determine how many
experience points an account is eligible to.
The majority of the work here was done by @3esmit.
Checklist
Ensure you completed all of the steps below before submitting your pull request:
forge snapshot?pnpm gas-report?pnpm lint?forge test?pnpm verify?