Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

staking rewards to any #647

Merged
merged 10 commits into from
Nov 21, 2024
Merged

staking rewards to any #647

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
4941738
Send validation or delegation rewards to any address
richardpringle Nov 13, 2024
906be91
Add change-reward-recipient functionality
richardpringle Nov 18, 2024
e1aaa3a
Make sure changing delegator reward recipient fails for nil address
richardpringle Nov 20, 2024
5a5423b
Finish testing
richardpringle Nov 20, 2024
b68e477
Remove redundant test-methods
richardpringle Nov 20, 2024
4132377
Add new functions to PoSValdiatorManager interface
richardpringle Nov 20, 2024
a57283d
Fix Delegation typo
richardpringle Nov 21, 2024
efca4e7
Consolidate repeated delegator reward functionality
richardpringle Nov 21, 2024
4af5b9b
Add docs to new validator-manager interface methods
richardpringle Nov 21, 2024
8773bcb
Make fetching storage the first operation
richardpringle Nov 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
148 changes: 137 additions & 11 deletions abi-bindings/go/validator-manager/ERC20TokenStakingManager/ERC20TokenStakingManager.go
View file
Open in desktop

Large diffs are not rendered by default.

148 changes: 137 additions & 11 deletions abi-bindings/go/validator-manager/NativeTokenStakingManager/NativeTokenStakingManager.go
View file
Open in desktop

Large diffs are not rendered by default.

195 changes: 177 additions & 18 deletions contracts/validator-manager/PoSValidatorManager.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,8 +65,10 @@
mapping(bytes32 delegationID => Delegator) _delegatorStakes;
/// @notice Maps the delegation ID to its pending staking rewards.
mapping(bytes32 delegationID => uint256) _redeemableDelegatorRewards;
mapping(bytes32 delegationID => address) _delegatorRewardRecipients;
richardpringle marked this conversation as resolved.
Show resolved Hide resolved
/// @notice Maps the validation ID to its pending staking rewards.
mapping(bytes32 validationID => uint256) _redeemableValidatorRewards;
mapping(bytes32 validationID => address) _rewardRecipients;
}
// solhint-enable private-vars-leading-underscore

Expand All @@ -84,6 +86,7 @@
error InvalidDelegationID(bytes32 delegationID);
error InvalidDelegatorStatus(DelegatorStatus status);
error InvalidNonce(uint64 nonce);
error InvalidRewardRecipient(address rewardRecipient);
error InvalidStakeAmount(uint256 stakeAmount);
error InvalidMinStakeDuration(uint64 minStakeDuration);
error InvalidStakeMultiplier(uint8 maximumStakeMultiplier);
Expand Down Expand Up @@ -213,7 +216,36 @@
bool includeUptimeProof,
uint32 messageIndex
) external {
if (!_initializeEndPoSValidation(validationID, includeUptimeProof, messageIndex)) {
_initializeEndValidationWithCheck(
validationID, includeUptimeProof, messageIndex, address(0)
);
}

/**
* @notice See {IPoSValidatorManager-initializeEndValidation}.
*/
function initializeEndValidation(
richardpringle marked this conversation as resolved.
Show resolved Hide resolved
bytes32 validationID,
bool includeUptimeProof,
uint32 messageIndex,
address rewardRecipient
) external {
_initializeEndValidationWithCheck(
validationID, includeUptimeProof, messageIndex, rewardRecipient
);
}

function _initializeEndValidationWithCheck(
bytes32 validationID,
bool includeUptimeProof,
uint32 messageIndex,
address rewardRecipient
) internal {
if (
!_initializeEndPoSValidation(
validationID, includeUptimeProof, messageIndex, rewardRecipient
)
) {
revert ValidatorIneligibleForRewards(validationID);
}
}
Expand All @@ -227,7 +259,62 @@
uint32 messageIndex
) external {
// Ignore the return value here to force end validation, regardless of possible missed rewards
_initializeEndPoSValidation(validationID, includeUptimeProof, messageIndex);
_initializeEndPoSValidation(validationID, includeUptimeProof, messageIndex, address(0));
}

/**
* @notice See {IPoSValidatorManager-forceInitializeEndValidation}.
*/
function forceInitializeEndValidation(
bytes32 validationID,
bool includeUptimeProof,
uint32 messageIndex,
address rewardRecipient
) external {
// Ignore the return value here to force end validation, regardless of possible missed rewards
_initializeEndPoSValidation(validationID, includeUptimeProof, messageIndex, rewardRecipient);
}

function changeValidatorRewardRecipient(
bytes32 validationID,
address rewardRecipient
) external {
PoSValidatorManagerStorage storage $ = _getPoSValidatorManagerStorage();

if (rewardRecipient == address(0)) {
revert InvalidRewardRecipient(rewardRecipient);
}

if ($._posValidatorInfo[validationID].owner != _msgSender()) {
revert UnauthorizedOwner(_msgSender());
richardpringle marked this conversation as resolved.
Show resolved Hide resolved
}

if (rewardRecipient == _msgSender()) {
delete $._rewardRecipients[validationID];
richardpringle marked this conversation as resolved.
Show resolved Hide resolved
} else {
$._rewardRecipients[validationID] = rewardRecipient;
}
}

function changeDelegatorRewardRecipient(
bytes32 delegationID,
address rewardRecipient
) external {
if (rewardRecipient == address(0)) {
revert InvalidRewardRecipient(rewardRecipient);
}

PoSValidatorManagerStorage storage $ = _getPoSValidatorManagerStorage();

if ($._delegatorStakes[delegationID].owner != _msgSender()) {
revert UnauthorizedOwner(_msgSender());
}
richardpringle marked this conversation as resolved.
Show resolved Hide resolved

if (rewardRecipient == _msgSender()) {
delete $._delegatorRewardRecipients[delegationID];
} else {
$._delegatorRewardRecipients[delegationID] = rewardRecipient;
}
Comment on lines +303 to +317
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be worth encapsulating this logic in a private function and having the external func call the private func. Then we could call the private func from initializeEndDelegation. Then we wouldn't have the different logical handling that I mentioned in another comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See this response. TL;DR I think I could clean that up in a follow up, but I think this works and solves the problem, can we land this PR before it diverges any further from main?

}

/**
Expand All @@ -238,7 +325,8 @@
function _initializeEndPoSValidation(
bytes32 validationID,
bool includeUptimeProof,
uint32 messageIndex
uint32 messageIndex,
address rewardRecipient
) internal returns (bool) {
PoSValidatorManagerStorage storage $ = _getPoSValidatorManagerStorage();

Expand Down Expand Up @@ -278,6 +366,11 @@
uptimeSeconds: uptimeSeconds
});
$._redeemableValidatorRewards[validationID] += reward;

if (rewardRecipient != address(0)) {
$._rewardRecipients[validationID] = rewardRecipient;
}

return (reward > 0);
}

Expand All @@ -296,9 +389,18 @@
}

address owner = $._posValidatorInfo[validationID].owner;

address rewardRecipient = $._rewardRecipients[validationID];

if (rewardRecipient == address(0)) {
rewardRecipient = owner;
} else {
delete $._rewardRecipients[validationID];
iansuvak marked this conversation as resolved.
Show resolved Hide resolved
}

// The validator can either be Completed or Invalidated here. We only grant rewards for Completed.
if (validator.status == ValidatorStatus.Completed) {
_withdrawValidationRewards(owner, validationID);
_withdrawValidationRewards(rewardRecipient, validationID);
}

// The stake is unlocked whether the validation period is completed or invalidated.
Expand Down Expand Up @@ -526,7 +628,36 @@
bool includeUptimeProof,
uint32 messageIndex
) external {
if (!_initializeEndDelegation(delegationID, includeUptimeProof, messageIndex)) {
_initializeEndDelegationWithCheck(
delegationID, includeUptimeProof, messageIndex, address(0)
);
}

/**
* @notice See {IPoSValidatorManager-initializeEndDelegation}.
*/
function initializeEndDelegation(
bytes32 delegationID,
bool includeUptimeProof,
uint32 messageIndex,
address rewardRecipient
) external {
_initializeEndDelegationWithCheck(
delegationID, includeUptimeProof, messageIndex, rewardRecipient
);
}

function _initializeEndDelegationWithCheck(
bytes32 delegationID,
bool includeUptimeProof,
uint32 messageIndex,
address rewardRecipient
) internal {
if (
!_initializeEndDelegation(
delegationID, includeUptimeProof, messageIndex, rewardRecipient
)
) {
revert DelegatorIneligibleForRewards(delegationID);
}
}
Expand All @@ -540,7 +671,20 @@
uint32 messageIndex
) external {
// Ignore the return value here to force end delegation, regardless of possible missed rewards
_initializeEndDelegation(delegationID, includeUptimeProof, messageIndex);
_initializeEndDelegation(delegationID, includeUptimeProof, messageIndex, address(0));
}

/**
* @notice See {IPoSValidatorManager-forceInitializeEndDelegation}.
*/
function forceInitializeEndDelegation(
bytes32 delegationID,
bool includeUptimeProof,
uint32 messageIndex,
address rewardRecipient
) external {
// Ignore the return value here to force end delegation, regardless of possible missed rewards
_initializeEndDelegation(delegationID, includeUptimeProof, messageIndex, rewardRecipient);
}

/**
Expand All @@ -551,7 +695,8 @@
function _initializeEndDelegation(
bytes32 delegationID,
bool includeUptimeProof,
uint32 messageIndex
uint32 messageIndex,
address rewardRecipient
) internal returns (bool) {
PoSValidatorManagerStorage storage $ = _getPoSValidatorManagerStorage();

Expand Down Expand Up @@ -598,17 +743,16 @@
($._delegatorStakes[delegationID].endingNonce,) =
_setValidatorWeight(validationID, validator.weight - delegator.weight);

uint256 reward = _calculateDelegationReward(delegator);
$._redeemableDelegatorRewards[delegationID] = reward;
uint256 reward =
_calculateAndSetDelegationReward(delegator, rewardRecipient, delegationID);

emit DelegatorRemovalInitialized({
delegationID: delegationID,
validationID: validationID
});
return (reward > 0);
} else if (validator.status == ValidatorStatus.Completed) {
$._redeemableDelegatorRewards[delegationID] = _calculateDelegationReward(delegator);

_calculateAndSetDelegationReward(delegator, rewardRecipient, delegationID);
_completeEndDelegation(delegationID);
// If the validator has completed, then no further uptimes may be submitted, so we always
// end the delegation.
Expand All @@ -619,40 +763,49 @@
}

/// @dev Calculates the reward owed to the delegator based on the state of the delegator and its corresponding validator.
function _calculateDelegationReward(Delegator memory delegator)
private
view
returns (uint256)
{
/// then set the reward and reward recipient in the storage.
function _calculateAndSetDelegationReward(
Delegator memory delegator,
address rewardRecipient,
bytes32 delegationID
) private returns (uint256) {
PoSValidatorManagerStorage storage $ = _getPoSValidatorManagerStorage();

Validator memory validator = getValidator(delegator.validationID);

uint64 delegationEndTime;
if (
validator.status == ValidatorStatus.PendingRemoved
|| validator.status == ValidatorStatus.Completed
) {
delegationEndTime = validator.endedAt;
} else if (validator.status == ValidatorStatus.Active) {
delegationEndTime = uint64(block.timestamp);
} else {
revert InvalidValidatorStatus(validator.status);
}

// Only give rewards in the case that the delegation started before the validator exited.
if (delegationEndTime <= delegator.startedAt) {
return 0;
}

return $._rewardCalculator.calculateReward({
uint256 reward = $._rewardCalculator.calculateReward({
stakeAmount: weightToValue(delegator.weight),
validatorStartTime: validator.startedAt,
stakingStartTime: delegator.startedAt,
stakingEndTime: delegationEndTime,
uptimeSeconds: $._posValidatorInfo[delegator.validationID].uptimeSeconds
});

$._redeemableDelegatorRewards[delegationID] = reward;

if (rewardRecipient != address(0)) {
$._delegatorRewardRecipients[delegationID] = rewardRecipient;
}
Comment on lines +803 to +805
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it maybe make sense to set this in initializeEndDelegation, so that we don't have to pass the parameter down another level?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually just changed this because of this comment.

Is there a problem passing this down? More expensive? I can revert my change, but this definitely is cleaner (ignoring gas cost)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah no, this is good :)


return reward;
}

Check notice

Code scanning / Slither

Block timestamp Low

PoSValidatorManager._calculateAndSetDelegationReward(Delegator,address,bytes32) uses timestamp for comparisons
Dangerous comparisons:
- delegationEndTime <= delegator.startedAt

/**
* @notice See {IPoSValidatorManager-resendUpdateDelegation}.
Expand Down Expand Up @@ -736,7 +889,13 @@
// Once this function completes, the delegation is completed so we can clear it from state now.
delete $._delegatorStakes[delegationID];

address rewardRecipient = delegator.owner;
address rewardRecipient = $._delegatorRewardRecipients[delegationID];

if (rewardRecipient == address(0)) {
rewardRecipient = delegator.owner;
} else {
delete $._delegatorRewardRecipients[delegationID];
}

(uint256 delegationRewards, uint256 validatorFees) =
_withdrawDelegationRewards(rewardRecipient, delegationID, validationID);
Expand Down
Loading