fix(StakeManager): use mul by PRECISION and div back later to avoid precision loss in int divisions #67
Conversation
3esmit
changed the title
3esmit
force-pushed
the
fix-precision-loss-2
branch
from
af5ca34
to
547b910
Compare
3esmit
force-pushed
the
fix-precision-loss-2
branch
from
547b910
to
81a28da
Compare
…recision loss in int divisions
| @@ -324,8 +326,8 @@ contract StakeManager is Ownable { | |||
| //mint multiplier points to that epoch | |||
| _mintMP(account, iEpoch.startTime + EPOCH_SIZE, iEpoch); | |||
| uint256 userSupply = account.balance + account.currentMP; | |||
| uint256 userShare = userSupply / iEpoch.totalSupply; //TODO: fix precision loss; | |||
| uint256 userEpochReward = userShare * iEpoch.epochReward; | |||
| uint256 userShare = (userSupply * PRECISION) / iEpoch.totalSupply; //TODO: fix precision loss; | |||
There was a problem hiding this comment.
I believe we can now remove the TODO: comments no?
| userVault.stake(stakeAmount, 0); | ||
|
|
||
| assertEq(stakeManager.totalSupplyMP(), stakeAmount); | ||
| for (uint256 i = 0; i < 53; i++) { |
There was a problem hiding this comment.
Just cosmetics, but 53 here is somewhat of an arbitrary number. Would be better to give some semantic meaning using a variable or constant or something like that.
There was a problem hiding this comment.
53 is to complete a year. 53 weeks gives roughly 1 year.
| console.log("# START EPOCH", stakeManager.currentEpoch()); | ||
| console.log("# PND_REWARDS", stakeManager.pendingReward()); | ||
|
|
||
| for (uint256 i = 0; i < 3; i++) { |
There was a problem hiding this comment.
| for (uint256 i = 0; i < 3; i++) { | |
| for (uint256 i = 0; i < userVaults.length; i++) { |
There was a problem hiding this comment.
I just realized i isn't used anywhere, so I'm not sure this actually implied userVaults.length, since you're also using userVaults.length in the inner loop.
Is there any particular reason this is supposed to loop 3 times? If yes, can we add this via some constant or variable, so this isn't getting as confusing in the future?
| assertEq(lastMint, lastMintBefore + stakeManager.EPOCH_SIZE(), "must increaase lastMint"); | ||
| assertEq(epoch, epochBefore + 1, "must increase epoch"); | ||
| assertGt(currentMP, currentMPBefore, "must increase MPs"); | ||
| assertGt(rewards, rewardsBefore, "must increase rewards"); |
There was a problem hiding this comment.
These are great.
I wonder if we can make these assertions stronger.
Do you think it's feasible to also assert by how much MPs and rewards increase?
Or maybe a rough ballpark, so that one can say "it must increase by at least this amount" ?
Description
Alternative to #66;
This PR fixes #64 and fixes #24, uses a more simple solution than #66.
Checklist
Ensure you completed all of the steps below before submitting your pull request:
forge snapshot?pnpm lint?forge test?pnpm verify?