Delegation PoC #504

cam-schultz · 2024-08-27T21:51:56Z

Why this should be merged

Partially fixes #441

How this works

Adds initialize/resend/complete methods for adding and removing delegators
Adds pack/unpack methods for SubnetValidatorWeightUpdate Warp messages
Unit tests for the above
Various cleanup items in the existing implementations (especially regarding nonce + weight management)

How this was tested

CI, new unit tests

How is this documented

N/A

minghinmatthewlam · 2024-08-28T19:51:52Z

contracts/staking/ERC20TokenStakingManager.sol

+     * @param validationID The ID of the validation period being delegated to.
+     * @param delegationAmount The amount to be delegated.
+     */
+    function initializeDelegatorRegistration(


did we want to offer delegation by default? Thought it was going to be an extension

Generally speaking, I prefer to make features optional rather than add complexity to the inheritance graph by adding extensions to support those features. Inheritance is best used as an abstraction tool to reduce shared code or provide polymorphism.

Regardless, for now I'm going to keep this as part of the PoSValidatorManager for the PoC, and we can revisit inheritance later.

I think more so thinking to allow Subnets to choose validator manager contract's that don't include delegation by default, and they can simply deploy the contract without delegation logic, as opposed to for reducing shared code.

I agree that validators should have the option to not support delegation. There are many ways to do that, such as providing that toggle as a constructor argument, or leaving it up to the individual validator to specify when they register. I'll enumerate those options and defer to a followup.

sounds good I'm good with deferring. Likely would push back as a constructor argument for turn on/off, since that increases contract bytecode and deployment costs.

I don't think we need to worry about deployment costs, as these contracts will be deployed by subnet owners

contracts/staking/ERC20TokenStakingManager.sol

contracts/staking/PoSValidatorManager.sol

minghinmatthewlam · 2024-08-28T19:58:59Z

contracts/staking/PoSValidatorManager.sol

+        );
+
+        // Ensure the delegator is not already registered
+        require(


will delegators be able to update their delegations and add more stake to the same validationID? Seems like a use case we should support

Let's defer this to a followup.

I think the safest way to this would be to come up with a unique delegation ID that takes into account the validatorID, the delegator address, and maybe the starting timestamp for uniquenes. Then add a separate delegation owner field, rather than using the delegator address for that purpose. That would also be more general, since the address that adds the delegation would not need to be the same address that claims rewards.

I'm not sure if this would make a big UX difference for delegators. Couldn't they just start a separate delegation with the additional stake in most cases?

Yeah, I agree with Cam here. Let's use the timestamp for uniqueness for the ID, that way one address can make multiple delegations, which prevents us from ever having to change the details of a delegation. We should have a way for any given address to access all of their delegations though

I'm not sure if this would make a big UX difference for delegators. Couldn't they just start a separate delegation with the additional stake in most cases?

In the current case they could but would have to use a separate address given this check. I do like the idea of the delegation owner field being tracked separately from the delegator address

good with deferring

contracts/staking/PoSValidatorManager.sol

geoff-vball

So far I've just gone over the implementations. I'll look over the tests next week :)

contracts/staking/ERC20TokenStakingManager.sol

contracts/staking/PoSValidatorManager.sol

geoff-vball · 2024-08-30T14:08:28Z

contracts/staking/PoSValidatorManager.sol

+        );
+
+        // Ensure the delegator is not already registered
+        require(


Yeah, I agree with Cam here. Let's use the timestamp for uniqueness for the ID, that way one address can make multiple delegations, which prevents us from ever having to change the details of a delegation. We should have a way for any given address to access all of their delegations though

geoff-vball · 2024-08-30T14:19:11Z

contracts/staking/PoSValidatorManager.sol

+        (bytes32 validationID, uint64 nonce,) =
+            ValidatorMessages.unpackSubnetValidatorWeightUpdateMessage(warpMessage.payload);


If we were to store the latest acked nonce for a given validationID, we could have a much cheaper function to complete a delegator registration if the nonce of the registration is less than or equal to the latest acked nonce, which would save on the warp message verification. Feel free to defer this

Going to defer this to a followup, since it is an optimization. Definitely think we should support this though.

geoff-vball · 2024-08-30T22:47:08Z

contracts/staking/PoSValidatorManager.sol

+        // Update the validator weight and delegator status
+        Validator memory validator = _getValidator(validationID);
+        require(validator.messageNonce >= nonce, "PoSValidatorManager: invalid nonce");
+        $._delegatorStakes[validationID][delegator].status = DelegatorStatus.Completed;
+
+        emit DelegationEnded(validationID, delegator, nonce);


We should be checking uptime, giving out rewards, and returning stake in here I think?

For uptime, I think anyone should be able to provide an uptime proof for a validator. Since we track the seconds of uptime, a proof can be provided basically at any point, and if the highest reported uptimeSeconds is at least 80% of the time from the validation period starting and the MIN(validation end, current time), then we can disburse rewards.

I'm going to defer uptime/rewards handling since I think there are still some open design questions here.

I implemented uptime handling here: #520

Happy to merge that into this branch and include it in this PR if you prefer.

contracts/staking/ValidatorManager.sol

contracts/staking/interfaces/IPoSValidatorManager.sol

geoff-vball · 2024-08-30T22:55:40Z

contracts/staking/interfaces/IPoSValidatorManager.sol

+     * @param validationID The ID of the validation period being ended.
+     * @param delegator The address of the delegator being removed.
+     */
+    function resendEndDelegation(bytes32 validationID, address delegator) external;


Technically our resends for starting or ending a delegation would only need to resend the current validator total weight with the highest nonce we've used so far. We don't need to store any of these messages, we just need to send our current state.

I'm going to leave this PR storing the pending messages in the contract state, both for consistency, and because there's a potential gas tradeoff to reconstructing the Warp message when calling the resend methods. I'll create an issue to quantify that and proceed accordingly.

I imagine the storage will cost more than the computation, and we should probably be optimizing for the happy path instead of the resend case. Happy to defer this to another ticket though

Yup, don't want to make any assumptions about gas cost, so will defer that analysis and any implementation changes to a followup item.

contracts/staking/interfaces/IPoSValidatorManager.sol

michaelkaplan13 · 2024-09-03T21:58:41Z

contracts/staking/PoSValidatorManager.sol

        IRewardCalculator _rewardCalculator;
+        /// @notice Maps the validationID to a mapping of delegator address to delegator information.
+        mapping(bytes32 validationID => mapping(address delegator => Delegator)) _delegatorStakes;


I think it may make more sense here to create a notion of a delegationID (similar to validationID) to key these maps on.

Theoretically, an address should be able to delegate to the same validation ID multiple times. This would also have the benefit of having a single ID to refer to a delegation rather than a composite key, and hopefully reduce the number of state lookups since there won't be nested mappings.

The delegationID could be a hash of the (validationID,nonce), where the nonce is the nonce used in the SetSubnetValidatorWeight message that added the delegator.

This idea came up previously. I agree that using a unique delegation ID rather than the address is the way to go, but going to defer that to a followup.

Using the nonce rather than a timestamp (as originally suggested in the linked conversation) seems like the way to go.

i like the (validationID, nonce) hash combo

bernard-avalabs

Generally makes sense to me. I assume there are no side effects from moving the weight update from completeEndDelegation to initializeEndDelegation.

contracts/staking/PoSValidatorManager.sol

geoff-vball · 2024-09-05T13:00:24Z

contracts/staking/PoSValidatorManager.sol

+        _checkAndUpdateChurnTracker(weight);
+
+        // Update the validator weight
+        validator.weight += weight;


I think we're going to need a new field to track the original weight of the validator. It'll be needed if the validator ends validation period to calculate their rewards, because right now we'd have to use the total weight with the delegations included.

sounds right to me, but we can defer to when handling delegation exit rewards

I went ahead and added that here, so that it doesn't fall through the cracks.

I would advocate to only add it in when needed because future changes might not need it or want to implement differently, for example, whether startTime is only needed for PoS, or using a separate mechanism as a whole. Not a big deal though now that's already added.

contracts/staking/NativeTokenStakingManager.sol

minghinmatthewlam · 2024-09-05T14:10:48Z

contracts/staking/PoSValidatorManager.sol

+        require(validator.messageNonce >= nonce, "PoSValidatorManager: invalid nonce");
+        // It should also be greater than or equal to the delegator's starting nonce
+        require(
+            $._delegatorStakes[validationID][delegator].startingNonce <= nonce,


why do we need start and ending nonce again, as opposed to saving latest nonce for delegator? Then the check here would instead check that Warp message received is latest nonce for delegator

nvm it's because P-chain is only willing to sign latest nonce weight update message. We keep a start nonce to check for registration completion, and ending nonce for ending completion.

minghinmatthewlam

LGTM

Co-authored-by: minghinmatthewlam <[email protected]> Signed-off-by: cam-schultz <[email protected]>

cam-schultz added 13 commits August 26, 2024 20:26

rename blsPubKey param

24c72c6

SubnetValidatorWeightUpdate packing

12f8664

delegation wip

717fd66

cleanup

319af42

increment nonce on usage

8ec0bbd

bug fixes

76e3277

fix whitespace

4ea1ff0

initializeRegisterDelegator tests passing

abf330e

update weight on complete step

c290bca

tests passing

040b9d0

format

49abda9

update bindings

f93b3eb

cleanup, format, comments

a27e9db

cam-schultz requested a review from a team as a code owner August 27, 2024 21:51

cam-schultz requested review from iansuvak, feuGeneA, geoff-vball, minghinmatthewlam, bernard-avalabs and michaelkaplan13 August 27, 2024 21:51

cam-schultz mentioned this pull request Aug 27, 2024

Simplify completeEndValidation #503

Merged

cam-schultz added 3 commits August 28, 2024 14:06

do not restrict delegation amounts

5d8526f

erc20 delegation e2e

de44d59

native delegation e2e

9c9db92

minghinmatthewlam reviewed Aug 28, 2024

View reviewed changes

contracts/staking/ERC20TokenStakingManager.sol Outdated Show resolved Hide resolved

minghinmatthewlam reviewed Aug 28, 2024

View reviewed changes

cam-schultz added 3 commits August 28, 2024 16:33

Merge branch 'staking-contract' into delegation-poc

070be82

remove weight checks

07a215f

cleanup

89da916

cam-schultz added 5 commits August 30, 2024 14:30

Merge branch 'staking-contract' into delegation-poc

7d80231

delete pending complete msg

221e588

bindings

66ba792

fix e2e

9571e51

mark storage getter private

38ba93a

geoff-vball reviewed Aug 30, 2024

View reviewed changes

meaghanfitzgerald added the L1 Staking Contract label Sep 3, 2024

cam-schultz added 3 commits September 3, 2024 12:52

verify nonce

b34d2e5

clarify unix timestamp

6b82cbf

update comments

bdef782

cam-schultz requested a review from geoff-vball September 3, 2024 20:11

michaelkaplan13 reviewed Sep 3, 2024

View reviewed changes

contracts/staking/interfaces/IPoSValidatorManager.sol Outdated Show resolved Hide resolved

michaelkaplan13 reviewed Sep 3, 2024

View reviewed changes

bernard-avalabs reviewed Sep 4, 2024

View reviewed changes

reorder fields

1f1238b

cam-schultz requested a review from michaelkaplan13 September 4, 2024 18:32

minghinmatthewlam reviewed Sep 4, 2024

View reviewed changes

contracts/staking/PoSValidatorManager.sol Outdated Show resolved Hide resolved

cam-schultz added 2 commits September 4, 2024 16:00

correct error prefixes

215f981

Merge branch 'staking-contract' into delegation-poc

ba108a2

cam-schultz requested review from bernard-avalabs and minghinmatthewlam September 4, 2024 21:37

comment typo

e0a8f70

geoff-vball reviewed Sep 5, 2024

View reviewed changes

minghinmatthewlam reviewed Sep 5, 2024

View reviewed changes

minghinmatthewlam approved these changes Sep 5, 2024

View reviewed changes

cam-schultz and others added 2 commits September 5, 2024 11:34

store starting validator weight

68c9eaa

Update contracts/staking/NativeTokenStakingManager.sol

d63a131

Co-authored-by: minghinmatthewlam <[email protected]> Signed-off-by: cam-schultz <[email protected]>

cam-schultz requested a review from geoff-vball September 5, 2024 16:40

cam-schultz merged commit 24b9c61 into staking-contract Sep 5, 2024
12 checks passed

cam-schultz deleted the delegation-poc branch September 5, 2024 16:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Delegation PoC #504

Delegation PoC #504

cam-schultz commented Aug 27, 2024

minghinmatthewlam Aug 28, 2024

cam-schultz Aug 28, 2024

minghinmatthewlam Sep 4, 2024 •

edited

Loading

cam-schultz Sep 4, 2024

minghinmatthewlam Sep 4, 2024

geoff-vball Sep 4, 2024

minghinmatthewlam Aug 28, 2024

cam-schultz Aug 28, 2024

bernard-avalabs Aug 29, 2024

geoff-vball Aug 30, 2024

iansuvak Aug 30, 2024

minghinmatthewlam Sep 4, 2024

geoff-vball left a comment

geoff-vball Aug 30, 2024

geoff-vball Aug 30, 2024

cam-schultz Sep 3, 2024

geoff-vball Aug 30, 2024

cam-schultz Sep 3, 2024

cam-schultz Sep 3, 2024

geoff-vball Aug 30, 2024

cam-schultz Sep 3, 2024

geoff-vball Sep 4, 2024

cam-schultz Sep 4, 2024

michaelkaplan13 Sep 3, 2024 •

edited

Loading

cam-schultz Sep 4, 2024

minghinmatthewlam Sep 4, 2024

bernard-avalabs left a comment

geoff-vball Sep 5, 2024

minghinmatthewlam Sep 5, 2024

cam-schultz Sep 5, 2024

minghinmatthewlam Sep 5, 2024

minghinmatthewlam Sep 5, 2024

minghinmatthewlam Sep 5, 2024

minghinmatthewlam left a comment

		(bytes32 validationID, uint64 nonce,) =
		ValidatorMessages.unpackSubnetValidatorWeightUpdateMessage(warpMessage.payload);

Delegation PoC #504

Delegation PoC #504

Conversation

cam-schultz commented Aug 27, 2024

Why this should be merged

How this works

How this was tested

How is this documented

Choose a reason for hiding this comment

Choose a reason for hiding this comment

minghinmatthewlam Sep 4, 2024 • edited Loading

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

Choose a reason for hiding this comment

geoff-vball 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelkaplan13 Sep 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bernard-avalabs 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

minghinmatthewlam left a comment

Choose a reason for hiding this comment

minghinmatthewlam Sep 4, 2024 •

edited

Loading

michaelkaplan13 Sep 3, 2024 •

edited

Loading