Add Decay to stake amount#4
Conversation
35faa00 to
0fd8b2f
Compare
a80768d to
4093bad
Compare
|
|
||
| /// @notice The period over which stake fully decays to zero. After this period, the entire | ||
| /// original stake amount is considered decayed and is claimable by the fee recipient. | ||
| uint256 public constant DECAY_PERIOD = 365 days; |
There was a problem hiding this comment.
The decay period should match accessEnd
| /// @notice Convenience overload to claim for msg.sender. | ||
| function claim() external { | ||
| _claimDecay(msg.sender); | ||
| } |
There was a problem hiding this comment.
What is the use case for this function? I can't see an incentive to call it.
|
|
||
| // Fee collected from jailed amount for blocked addresses | ||
| if (isBlocklisted[_staker]) totalJailed -= decayed; | ||
| else totalStaked -= decayed; |
There was a problem hiding this comment.
Decayed stake should not affect capacity. I am not sure exactly how this affects the rest of the code, but we probably need to track the individual stake capacity seperately on the stakeInfo.
b5d5369 to
fe64913
Compare
| if (_stakingToken == address(0)) revert QueryTypeStakerFactory__InvalidTokenAddress(); | ||
| STAKING_TOKEN = IERC20(_stakingToken); | ||
| feeRecipient = _owner; | ||
| emit FeeRecipientUpdated(address(0), _owner); |
There was a problem hiding this comment.
We can probably abstract this to a internal function that is used here and in setFeeRecipient
|
|
||
| // Reset lockup and access periods | ||
| StakeInfo memory stakeInfo = stakes[msg.sender]; | ||
| StakeInfo storage stakeInfo = stakes[msg.sender]; |
There was a problem hiding this comment.
Does this need to be storage?
|
|
||
| // If there is no fee recipient yet, update lastClaimed and exit without modifying | ||
| if (_feeRecipient == address(0)) { | ||
| stakeInfo.lastClaimed = uint48(block.timestamp); |
There was a problem hiding this comment.
Why would last claimed need to be updated?
There was a problem hiding this comment.
It doesn't need to be, shouldn't be, now it only updates when the decay gets applied
| new QueryTypeStakerFactory(_owner, address(0)); | ||
| } | ||
|
|
||
| function testFuzz_EmitsFeeRecipientUpdatedEventWithArbitraryRecipient(address _newRecipient) |
There was a problem hiding this comment.
We should also have a test for emitting the event when the contract is created
There was a problem hiding this comment.
Update this to test its in the constructor and added a seperate test contract for the setFeeRecipient function
| returns (uint256 boundedTimeSkip) | ||
| { | ||
| // Ensure some stake remains after decay by capping time skip below access period end | ||
| return bound(_timeSkip, pool.lockupPeriod() + 1, pool.lockupPeriod() + pool.accessPeriod() - 1); |
There was a problem hiding this comment.
The stake can decay during the lockup
There was a problem hiding this comment.
Moved this to just the Unstake contract where we need it to skip into the time where it's unstakable
| assertEq(pool.totalStaked(), _stakeAmount, "Total staked incorrect"); | ||
| } | ||
|
|
||
| function testFuzz_ClaimOnBehalfSuccessfully( |
There was a problem hiding this comment.
| function testFuzz_ClaimOnBehalfSuccessfully( | |
| function testFuzz_ClaimArbitraryDecaySuccessfully( |
| assertEq(remaining.amount, _stakeAmount - decayed); | ||
| } | ||
|
|
||
| function test_ClaimWithNoStakeDoesNothing(address _nonStaker) public { |
There was a problem hiding this comment.
| function test_ClaimWithNoStakeDoesNothing(address _nonStaker) public { | |
| function testFuzz_ClaimWithNoStakeDoesNothing(address _nonStaker) public { |
| assertEq(pool.totalStaked(), 0); | ||
| } | ||
|
|
||
| function testFuzz_ClaimWhenFeeRecipientUnsetDoesNothing( |
There was a problem hiding this comment.
| function testFuzz_ClaimWhenFeeRecipientUnsetDoesNothing( | |
| function testFuzz_ClaimWhenFeeRecipientIsTheZeroAddrss( |
| assertEq(stakeAfter.lastClaimed, uint48(block.timestamp), "lastClaimed not updated"); | ||
| } | ||
|
|
||
| function testFuzz_ClaimWhenBlockedAndFeeRecipientUnsetKeepsJail( |
There was a problem hiding this comment.
This name is a little jumbled. We should try and make it clearer
| assertEq(lockupEnd, expectedLockupEnd); | ||
| assertEq(accessEnd, expectedAccessEnd); | ||
| assertEq(stakeInfo.lockupEnd, expectedLockupEnd); | ||
| assertEq(stakeInfo.accessEnd, expectedAccessEnd); |
There was a problem hiding this comment.
Should we assert the decayed value as well?
| if (decayed == 0) return 0; | ||
|
|
||
| address _feeRecipient = QueryTypeStakerFactory(FACTORY).feeRecipient(); | ||
| if (_feeRecipient == address(0)) return 0; |
There was a problem hiding this comment.
I think there may be a problem here. If no decay is applied while the fee recipient is the 0 address than once a fee recipient is added many stakes may have immediately decayed to 0. Since the lastClaimed will be when the stake was created. We should probably just remove this and burn the decayed stake if the fee recipient is zero. That way at least the behavior is consistent.
* Change naming
|
Coverage after merging decay-stake into main will be
Coverage Report
|
|||||||||||||||||||||||||
Resolves: #2
To do: