Streamed rewards #74
Streamed rewards #74
Conversation
gravityblast
force-pushed
the
streamed-rewards
branch
from
5529222 to
ca9b6f1
Compare
| if (amount == 0) { | ||
| revert StakingManager__AmountCannotBeZero(); | ||
| } | ||
|
|
There was a problem hiding this comment.
To be very defensive, I'd suggest we:
- Either ensure
amountis at least1e18or - Ensure
rewardPerSecondis not0
While not super likely to happen, there's a chance to provide amount and duration such that rewardPerSecond will round down to 0
There was a problem hiding this comment.
Actually, just noticed we're no longer calculating rewardsPerSecond here. We should probably still have the check I've mentioned above.
src/RewardsStreamerMP.sol
Outdated
| // FIXME: If newTotalWeight > previousTotalWeight, then (previousTotalWeight) / (newTotalWeight) is less than 1. | ||
| // Multiplying rewardIndex by a fraction less than 1 reduces its value. | ||
| // Users who have not updated their accountRewardIndex may see a reduction in their pending rewards. | ||
| // Possible fix: adjust only when newTotalWeight < previousTotalWeight |
There was a problem hiding this comment.
@gravityblast I've been pondering on this comment for quite a while now and I can't quite understand what the issue here is. Can you give a concrete example that demonstrates the issue you're describing here?
gravityblast
force-pushed
the
streamed-rewards
branch
5 times, most recently
from
6b1e0a2 to
2108d88
Compare
gravityblast
force-pushed
the
streamed-rewards
branch
from
2108d88 to
1cfb7d5
Compare
… a real reward token balance
gravityblast
force-pushed
the
streamed-rewards
branch
from
1cfb7d5 to
e52b6fc
Compare
|
|
||
| RewardsStreamerMP.Account memory accountInfo = streamer.getAccount(p.account); | ||
|
|
||
| assertEq(accountInfo.stakedBalance, p.stakedBalance, "wrong account staked balance"); | ||
| assertEq(stakingToken.balanceOf(p.account), p.vaultBalance, "wrong vault balance"); | ||
| assertEq(accountInfo.accountRewardIndex, p.rewardIndex, "wrong account reward index"); | ||
| // assertEq(accountInfo.accountRewardIndex, p.rewardIndex, "wrong account reward index"); |
There was a problem hiding this comment.
We should remove the commented out lines
test/RewardsStreamerMP.t.sol
Outdated
| @@ -241,7 +224,7 @@ contract IntegrationTest is RewardsStreamerMPTest { | |||
|
|
|||
| // T3 | |||
| vm.prank(admin); | |||
| rewardToken.transfer(address(streamer), 1000e18); | |||
| // rewardToken.transfer(address(streamer), 1000e18); | |||
test/RewardsStreamerMP.t.sol
Outdated
| @@ -391,7 +370,7 @@ contract IntegrationTest is RewardsStreamerMPTest { | |||
|
|
|||
| // T6 | |||
| vm.prank(admin); | |||
| rewardToken.transfer(address(streamer), 1000e18); | |||
| // rewardToken.transfer(address(streamer), 1000e18); | |||
| streamer.setReward(2000e18, 10 days); | ||
| // accrued is 1000 from the previous reward and still 0 for the new one | ||
| assertEq(streamer.totalRewardsSupply(), 1000e18, "totalRewardsSupply should be 1000"); | ||
| assertEq(streamer.totalRewardsAccrued(), 1000e18); |
There was a problem hiding this comment.
Ah, interesting. Looking at these assertions, I expected it to be the other way around:
totalRewardsSupply()is what is "checkpointed"totalRewardsAccrued()is "real-time" balance that is accrued until this point in time
Also, the comment says [...] and still 0 for the new one but the 0 isn't reflected here anywhere.
Likely needs rewording?
There was a problem hiding this comment.
@0x-r4bbit yes so we have:
- 1000 accrued and saved to storage in
totalRewardsAccrued - 0 pending
totalRewardsSupplyistotalRewardsAccrued+ pending =1000
| // FIXME: this is needed to update the global state and account MP | ||
| // Later we should update the functions to use "real-time" values. | ||
| streamer.updateGlobalState(); | ||
| streamer.updateAccountMP(vaults[alice]); |
There was a problem hiding this comment.
re: the FIXME comment: can't we have the real-time functions be part of this PR?
There was a problem hiding this comment.
I'm preparing it for the next PR so we can merge this otherwise I have too many conflicts every day :-P
| uint256 tolerance = 300; // 300 wei | ||
|
|
||
| assertEq(streamer.totalRewardsSupply(), 100e18, "Total rewards supply mismatch"); | ||
| assertApproxEqAbs(streamer.rewardsBalanceOf(vaults[alice]), 100e18, tolerance); |
…PendingRewards and fix specs
Description
first PR part of #65
Update the current implementation to set a reward for a specific period (like 10K tokens for 1 year) and stream that dynamically.
Checklist
Ensure you completed all of the steps below before submitting your pull request:
pnpm adorno?pnpm verify?