Check for churn period seconds to pass when delegator ends #625
Conversation
geoff-vball
requested review from
iansuvak,
bernard-avalabs,
michaelkaplan13 and
cam-schultz
| @@ -691,6 +697,11 @@ abstract contract PoSValidatorManager is | |||
| revert InvalidDelegatorStatus(delegator.status); | |||
| } | |||
|
|
|||
| // Check that minimum stake duration has passed. | |||
| if (block.timestamp < delegator.startedAt + $._minimumStakeDuration) { | |||
There was a problem hiding this comment.
Is this check necessary given that we know the delegator is in PendingRemoved state?
There was a problem hiding this comment.
Oops. I didn't actually mean to add this check here. Thanks!
| // To prevent churn tracker abuse, check that one full churn period has passed, | ||
| // so a delegator may not stake twice in the same churn period. | ||
| if (block.timestamp < delegator.startedAt + _getChurnPeriodSeconds()) { | ||
| revert MinStakeDurationNotPassed(uint64(block.timestamp)); | ||
| } |
There was a problem hiding this comment.
I think this check should actually be moved into _completeEndDelegation itself. Currently I think there is still a corner case where _completeEndDelegation could be called from completeDelegatorRegistration prior to a churn period elapsing if the validator completed prior the delegation registration being completed.
There was a problem hiding this comment.
Otherwise, we could also just add the similar check in that case, but feels more robust and less error prone to include it in a single place within _completeEndDelegation (possibly with the downside of doing a redundant check in certain cases).
There was a problem hiding this comment.
Ahh yep, you're right
Why this should be merged
Makes delegators wait at least
churnPeriodSecondsto reclaim their stake in the case that their validator ended early. If the validator has not ended, the delegator must still waitminStakeDuration, whereminStakeDuration >= churnPeriodSeconds. This prevents the delegator from potentially staking multiple times in the same churn period.How this was tested
Unit tests
How is this documented
Comments