staking rewards to any #647

richardpringle · 2024-11-15T18:08:06Z

Why this should be merged

Closes #555

How this works

I use another mapping to set the reward-recipient.

TODO: I should probably add another method that allows the validator to change the recipient before completeEndValidation is called.

How this was tested

Unit tests
TODO: test delegator reward

How is this documented

TODO

contracts/validator-manager/PoSValidatorManager.sol

geoff-vball

This is looking good! When I had originally envisioned this, I was thinking of an external function that just lets the owner of a validation/delegation change their rewards address, which would obviate the need for passing around the address in the initialize functions. I think your way works just as well, except if the staker wants to change the reward address in between the initialize and complete steps. Having the rewards address change in a separate step would also let the caller verify the address is correctly set before calling to disburse the rewards.

We could also set rewardRecipient = owner when starting a staking period, which would prevent us from having to check if the address is 0 later.

contracts/validator-manager/PoSValidatorManager.sol

richardpringle · 2024-11-18T16:48:28Z

This is looking good! When I had originally envisioned this, I was thinking of an external function that just lets the owner of a validation/delegation change their rewards address, which would obviate the need for passing around the address in the initialize functions. I think your way works just as well, except if the staker wants to change the reward address in between the initialize and complete steps. Having the rewards address change in a separate step would also let the caller verify the address is correctly set before calling to disburse the rewards.

We could also set rewardRecipient = owner when starting a staking period, which would prevent us from having to check if the address is 0 later.

I still need to add the functionality to change the reward-recipient between initialize and complete.

As for always setting the reward-recipient (defaulting to the owner), it's cheaper not to set it. I'm happy to do things either way though. It's one of those things where we save the extra storage where it isn't necessary or we make the cost consistent (no matter where you withdraw the reward)

richardpringle · 2024-11-18T23:16:43Z

I think all that's left is exposing the changeRewardRecipient functions in the interface and some documentation.

richardpringle · 2024-11-18T23:18:26Z

I think all that's left is exposing the changeRewardRecipient functions in the interface and some documentation.

Oh actually, I think I need a couple more tests. Will leave comments in the code.

contracts/validator-manager/PoSValidatorManager.sol

contracts/validator-manager/interfaces/IPoSValidatorManager.sol

contracts/validator-manager/tests/PoAValidatorManagerTests.t.sol

contracts/validator-manager/PoSValidatorManager.sol

contracts/validator-manager/tests/PoSValidatorManagerTests.t.sol

iansuvak

Looks good to me overall! Just a couple of minor questions and a method name typo.

contracts/validator-manager/PoSValidatorManager.sol

contracts/validator-manager/interfaces/IPoSValidatorManager.sol

tests/utils/validator_manager.go

contracts/validator-manager/interfaces/IPoSValidatorManager.sol

geoff-vball · 2024-11-21T17:26:03Z

contracts/validator-manager/PoSValidatorManager.sol

+        if (rewardRecipient != address(0)) {
+            $._delegatorRewardRecipients[delegationID] = rewardRecipient;
+        }


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

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)

Yeah no, this is good :)

geoff-vball · 2024-11-21T17:29:45Z

contracts/validator-manager/PoSValidatorManager.sol

+            revert InvalidRewardRecipient(rewardRecipient);
+        }
+
+        PoSValidatorManagerStorage storage $ = _getPoSValidatorManagerStorage();


Idiomatically we always place this at the top of the function before any checks. If a call reverts, it uses all the gas anyway, so there are no potential savings.

What if the address is 0, though?

Either ordering gives exactly the same effects in all cases, since reverted transactions cause all gas to be use, and any side-effects are not saved. Happy to explain more if that doesn't explain it fully

geoff-vball · 2024-11-21T17:31:53Z

contracts/validator-manager/PoSValidatorManager.sol

+        if (rewardRecipient == address(0)) {
+            revert InvalidRewardRecipient(rewardRecipient);
+        }
+
+        PoSValidatorManagerStorage storage $ = _getPoSValidatorManagerStorage();
+
+        if ($._delegatorStakes[delegationID].owner != _msgSender()) {
+            revert UnauthorizedOwner(_msgSender());
+        }
+
+        if (rewardRecipient == _msgSender()) {
+            delete $._delegatorRewardRecipients[delegationID];
+        } else {
+            $._delegatorRewardRecipients[delegationID] = rewardRecipient;
+        }


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.

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?

bernard-avalabs · 2024-11-21T19:35:49Z

As for always setting the reward-recipient (defaulting to the owner), it's cheaper not to set it. I'm happy to do things either way though. It's one of those things where we save the extra storage where it isn't necessary or we make the cost consistent (no matter where you withdraw the reward)

My personal preference is to set the reward-recipient to the owner by default. It is a bit less error prone since we won't need to special case the 0 address. I imagine the storage cost is pretty small.

bernard-avalabs

Generally looks good to me. I left a comment about preferring to set the owner as the default reward recipient, but I'm fine either way.

richardpringle · 2024-11-21T21:05:00Z

As for always setting the reward-recipient (defaulting to the owner), it's cheaper not to set it. I'm happy to do things either way though. It's one of those things where we save the extra storage where it isn't necessary or we make the cost consistent (no matter where you withdraw the reward)

My personal preference is to set the reward-recipient to the owner by default. It is a bit less error prone since we won't need to special case the 0 address. I imagine the storage cost is pretty small.

🤔 I actually agree, I think that's the cleaner solution. However... this works, so I think it would make sense to do in a follow up (locking in the tests and everything now).

Thoughts @geoff-vball and @bernard-avalabs?

richardpringle commented Nov 15, 2024

View reviewed changes

contracts/validator-manager/PoSValidatorManager.sol Outdated Show resolved Hide resolved

richardpringle commented Nov 15, 2024

View reviewed changes

contracts/validator-manager/PoSValidatorManager.sol Outdated Show resolved Hide resolved

richardpringle commented Nov 15, 2024

View reviewed changes

contracts/validator-manager/PoSValidatorManager.sol Show resolved Hide resolved

geoff-vball reviewed Nov 18, 2024

View reviewed changes

contracts/validator-manager/PoSValidatorManager.sol Show resolved Hide resolved

contracts/validator-manager/PoSValidatorManager.sol Show resolved Hide resolved

contracts/validator-manager/PoSValidatorManager.sol Outdated Show resolved Hide resolved

richardpringle force-pushed the staking-rewards-to-any branch 4 times, most recently from 79a1984 to 0380f82 Compare November 18, 2024 23:14

richardpringle commented Nov 18, 2024

View reviewed changes

contracts/validator-manager/PoSValidatorManager.sol Show resolved Hide resolved

richardpringle commented Nov 18, 2024

View reviewed changes

contracts/validator-manager/PoSValidatorManager.sol Show resolved Hide resolved

richardpringle force-pushed the staking-rewards-to-any branch from 0380f82 to 30f27e1 Compare November 18, 2024 23:25

richardpringle commented Nov 18, 2024

View reviewed changes

contracts/validator-manager/interfaces/IPoSValidatorManager.sol Show resolved Hide resolved

richardpringle commented Nov 18, 2024

View reviewed changes

contracts/validator-manager/tests/PoAValidatorManagerTests.t.sol Show resolved Hide resolved

geoff-vball reviewed Nov 19, 2024

View reviewed changes

contracts/validator-manager/PoSValidatorManager.sol Outdated Show resolved Hide resolved

contracts/validator-manager/PoSValidatorManager.sol Outdated Show resolved Hide resolved

richardpringle force-pushed the staking-rewards-to-any branch 3 times, most recently from ca5f8e0 to c290490 Compare November 19, 2024 17:59

richardpringle commented Nov 19, 2024

View reviewed changes

contracts/validator-manager/PoSValidatorManager.sol Show resolved Hide resolved

richardpringle commented Nov 19, 2024

View reviewed changes

contracts/validator-manager/tests/PoSValidatorManagerTests.t.sol Show resolved Hide resolved

richardpringle commented Nov 19, 2024

View reviewed changes

contracts/validator-manager/tests/PoSValidatorManagerTests.t.sol Outdated Show resolved Hide resolved

richardpringle marked this pull request as ready for review November 20, 2024 22:33

richardpringle requested a review from a team as a code owner November 20, 2024 22:33

richardpringle requested review from iansuvak, geoff-vball, bernard-avalabs, michaelkaplan13 and cam-schultz November 20, 2024 22:33

iansuvak reviewed Nov 21, 2024

View reviewed changes

contracts/validator-manager/PoSValidatorManager.sol Outdated Show resolved Hide resolved

contracts/validator-manager/PoSValidatorManager.sol Outdated Show resolved Hide resolved

contracts/validator-manager/PoSValidatorManager.sol Show resolved Hide resolved

iansuvak approved these changes Nov 21, 2024

View reviewed changes

geoff-vball reviewed Nov 21, 2024

View reviewed changes

bernard-avalabs reviewed Nov 21, 2024

View reviewed changes

richardpringle force-pushed the staking-rewards-to-any branch 2 times, most recently from 90cc2a6 to a388dc1 Compare November 21, 2024 19:58

richardpringle added 10 commits November 21, 2024 15:01

Send validation or delegation rewards to any address

4941738

Add change-reward-recipient functionality

906be91

Make sure changing delegator reward recipient fails for nil address

e1aaa3a

Finish testing

5a5423b

Remove redundant test-methods

b68e477

Add new functions to PoSValdiatorManager interface

4132377

Fix Delegation typo

a57283d

Consolidate repeated delegator reward functionality

efca4e7

Add docs to new validator-manager interface methods

4af5b9b

Make fetching storage the first operation

8773bcb

richardpringle force-pushed the staking-rewards-to-any branch from a388dc1 to 8773bcb Compare November 21, 2024 20:59

richardpringle requested a review from geoff-vball November 21, 2024 21:13

geoff-vball approved these changes Nov 21, 2024

View reviewed changes

richardpringle merged commit aec3442 into main Nov 21, 2024
17 checks passed

richardpringle deleted the staking-rewards-to-any branch November 21, 2024 22:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

staking rewards to any #647

staking rewards to any #647

richardpringle commented Nov 15, 2024 •

edited

Loading

geoff-vball left a comment

richardpringle commented Nov 18, 2024

richardpringle commented Nov 18, 2024

richardpringle commented Nov 18, 2024

iansuvak left a comment

geoff-vball Nov 21, 2024

richardpringle Nov 21, 2024

geoff-vball Nov 21, 2024

geoff-vball Nov 21, 2024

richardpringle Nov 21, 2024

geoff-vball Nov 21, 2024

geoff-vball Nov 21, 2024

richardpringle Nov 21, 2024

bernard-avalabs commented Nov 21, 2024

bernard-avalabs left a comment

richardpringle commented Nov 21, 2024

staking rewards to any #647

staking rewards to any #647

Conversation

richardpringle commented Nov 15, 2024 • edited Loading

Why this should be merged

How this works

How this was tested

How is this documented

geoff-vball left a comment

Choose a reason for hiding this comment

richardpringle commented Nov 18, 2024

richardpringle commented Nov 18, 2024

richardpringle commented Nov 18, 2024

iansuvak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bernard-avalabs commented Nov 21, 2024

bernard-avalabs left a comment

Choose a reason for hiding this comment

richardpringle commented Nov 21, 2024

richardpringle commented Nov 15, 2024 •

edited

Loading