SOV-3763 StakingRewardsOS #536
Conversation
| if (stopBlock > 0 && stopBlock < _block) { | ||
| uint256 previousWeightedStake = staking.getPriorWeightedStake( | ||
| _staker, | ||
| stopBlock, | ||
| _date | ||
| ); | ||
| if (previousWeightedStake < weightedStake) { | ||
| weightedStake = previousWeightedStake; | ||
| } | ||
| } |
There was a problem hiding this comment.
This logic should not be necessary anymore. The existing SOV staking rewards contract continues after the stopBlock, but increases of the stake are no longer considered. This osSOV staking rewards contract truly stops after the stop block, so this logic here is no longer required. I would just return 0 in case that (stopBlock > 0 && stopBlock < _block).
There was a problem hiding this comment.
good catch, thanks
but this icheck would be done on every call in a cycle which seems to be better to be moved to the calling function
so, just moved this validation from this function
| uint256 _date | ||
| ) internal view returns (uint256 weightedStake) { | ||
| weightedStake = staking.getPriorWeightedStake(_staker, _block, _date); | ||
| if (stopBlock > 0 && stopBlock < _block) { |
There was a problem hiding this comment.
probably can do this:
if(stopBlock == 0 || stopBlock > _block) return weightedStake;
uint256 previousWeightedStake = staking.getPriorWeightedStake(
_staker,
stopBlock,
_date
);
if (previousWeightedStake < weightedStake) {
weightedStake = previousWeightedStake;
}
There was a problem hiding this comment.
stop block would be checked on every iteration - more effective to have this check in the calling function, besides - separation of concerns. refactored.
| uint256 _date | ||
| ) internal view returns (uint256 weightedStake) { | ||
| weightedStake = staking.getPriorWeightedStake(_staker, _block, _date); | ||
| if (stopBlock > 0 && stopBlock < _block) { |
There was a problem hiding this comment.
stop block would be checked on every iteration - more effective to have this check in the calling function, besides - separation of concerns. refactored.
| msg.sender, | ||
| true, | ||
| _startTime | ||
| ); | ||
| require(withdrawTimestamp > 0 && amount > 0, "No valid reward"); | ||
| stakerNextWithdrawTimestamp[msg.sender] = withdrawTimestamp; | ||
| require(nextWithdrawTimestamp > 0 && amount > 0, "No valid reward"); |
There was a problem hiding this comment.
Can u please cover this edge in the unit test?
No description provided.