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.

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

Delegation PoC #504

Merged
merged 41 commits into from
Sep 5, 2024
Merged
Show file tree
Hide file tree
Changes from 26 commits
Commits
Show all changes
41 commits
Select commit Hold shift + click to select a range
24c72c6
rename blsPubKey param
cam-schultz Aug 27, 2024
12f8664
SubnetValidatorWeightUpdate packing
cam-schultz Aug 27, 2024
717fd66
delegation wip
cam-schultz Aug 27, 2024
319af42
cleanup
cam-schultz Aug 27, 2024
8ec0bbd
increment nonce on usage
cam-schultz Aug 27, 2024
76e3277
bug fixes
cam-schultz Aug 27, 2024
4ea1ff0
fix whitespace
cam-schultz Aug 27, 2024
abf330e
initializeRegisterDelegator tests passing
cam-schultz Aug 27, 2024
c290bca
update weight on complete step
cam-schultz Aug 27, 2024
040b9d0
tests passing
cam-schultz Aug 27, 2024
49abda9
format
cam-schultz Aug 27, 2024
f93b3eb
update bindings
cam-schultz Aug 27, 2024
a27e9db
cleanup, format, comments
cam-schultz Aug 27, 2024
5d8526f
do not restrict delegation amounts
cam-schultz Aug 28, 2024
de44d59
erc20 delegation e2e
cam-schultz Aug 28, 2024
9c9db92
native delegation e2e
cam-schultz Aug 28, 2024
070be82
Merge branch 'staking-contract' into delegation-poc
cam-schultz Aug 28, 2024
07a215f
remove weight checks
cam-schultz Aug 28, 2024
89da916
cleanup
cam-schultz Aug 28, 2024
305e475
test complete delegation out of order
cam-schultz Aug 29, 2024
f751b30
tests passing
cam-schultz Aug 29, 2024
ea1e56b
clean up events
cam-schultz Aug 29, 2024
b6706d7
cleanup helpers
cam-schultz Aug 29, 2024
54f3f7b
update bindings
cam-schultz Aug 29, 2024
31113ad
remove ordering test
cam-schultz Aug 29, 2024
94d4c1f
format+lint
cam-schultz Aug 29, 2024
036bfa6
assert instead of require
cam-schultz Aug 29, 2024
7d80231
Merge branch 'staking-contract' into delegation-poc
cam-schultz Aug 30, 2024
221e588
delete pending complete msg
cam-schultz Aug 30, 2024
66ba792
bindings
cam-schultz Aug 30, 2024
9571e51
fix e2e
cam-schultz Aug 30, 2024
38ba93a
mark storage getter private
cam-schultz Aug 30, 2024
b34d2e5
verify nonce
cam-schultz Sep 3, 2024
6b82cbf
clarify unix timestamp
cam-schultz Sep 3, 2024
bdef782
update comments
cam-schultz Sep 3, 2024
1f1238b
reorder fields
cam-schultz Sep 4, 2024
215f981
correct error prefixes
cam-schultz Sep 4, 2024
ba108a2
Merge branch 'staking-contract' into delegation-poc
cam-schultz Sep 4, 2024
e0a8f70
comment typo
cam-schultz Sep 4, 2024
68c9eaa
store starting validator weight
cam-schultz Sep 5, 2024
d63a131
Update contracts/staking/NativeTokenStakingManager.sol
cam-schultz Sep 5, 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

Large diffs are not rendered by default.

Large diffs are not rendered by default.

35 changes: 33 additions & 2 deletions abi-bindings/go/staking/PoAValidatorManager/PoAValidatorManager.go

Large diffs are not rendered by default.

21 changes: 20 additions & 1 deletion contracts/staking/ERC20TokenStakingManager.sol
Original file line number Diff line number Diff line change
Expand Up @@ -77,16 +77,35 @@ contract ERC20TokenStakingManager is
$._token = token;
}

/**
* @notice Begins the validator registration process. Locks the configured ERC20 in the contract as the stake.
* @param stakeAmount The amount to be staked.
* @param nodeID The node ID of the validator being registered.
* @param registrationExpiry The time at which the registration is no longer valid on the P-Chain.
cam-schultz marked this conversation as resolved.
Show resolved Hide resolved
* @param blsPublicKey The BLS public key of the validator.
*/
function initializeValidatorRegistration(
uint256 stakeAmount,
bytes32 nodeID,
uint64 registrationExpiry,
bytes memory blsPublicKey
) external override returns (bytes32 validationID) {
) external returns (bytes32 validationID) {
uint64 weight = _processStake(stakeAmount);
return _initializeValidatorRegistration(nodeID, weight, registrationExpiry, blsPublicKey);
}

/**
* @notice Begins the delegator registration process. Locks the configured ERC20 in the contract as the delegated stake.
* @param validationID The ID of the validation period being delegated to.
* @param delegationAmount The amount to be delegated.
*/
function initializeDelegatorRegistration(

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link

@minghinmatthewlam minghinmatthewlam Sep 4, 2024

Choose a reason for hiding this comment

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

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.

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 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.

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

bytes32 validationID,
uint256 delegationAmount
) external {
return _initializeDelegatorRegistration(validationID, _msgSender(), delegationAmount);
}

// Must be guarded with reentrancy guard for safe transfer from
function _lock(uint256 value) internal virtual override returns (uint256) {
return _getERC20StakingManagerStorage()._token.safeTransferFrom(value);
Expand Down
18 changes: 10 additions & 8 deletions contracts/staking/NativeTokenStakingManager.sol
Original file line number Diff line number Diff line change
Expand Up @@ -46,21 +46,23 @@ contract NativeTokenStakingManager is
* @notice Begins the validator registration process. Locks the provided native asset in the contract as the stake.
* @param nodeID The node ID of the validator being registered.
* @param registrationExpiry The time at which the registration is no longer valid on the P-Chain.
* @param signature The raw bytes of the Ed25519 signature over the concatenated bytes of
* [subnetID]+[nodeID]+[blsPublicKey]+[weight]+[balance]+[expiry]. This signature must correspond to the Ed25519
* public key that is used for the nodeID. This approach prevents nodeIDs from being unwillingly added to Subnets.
* balance is the minimum initial $nAVAX balance that must be attached to the validator serialized as a uint64.
* The signature field will be validated by the P-Chain. Implementations may choose to validate that the signature
* field is well-formed but it is not required.
* @param blsPublicKey The BLS public key of the validator.
*/
function initializeValidatorRegistration(
bytes32 nodeID,
uint64 registrationExpiry,
bytes memory signature
bytes memory blsPublicKey
) external payable returns (bytes32) {
uint64 weight = _processStake(msg.value);
return _initializeValidatorRegistration(nodeID, weight, registrationExpiry, blsPublicKey);
}

return _initializeValidatorRegistration(nodeID, weight, registrationExpiry, signature);
/**
* @notice Begins the delegator registration process. Locks the provided native asset in the contract as the delegated stake.
* @param validationID The ID of the validation period being delegated to.
*/
function initializeDelegatorRegistration(bytes32 validationID) external payable {
return _initializeDelegatorRegistration(validationID, _msgSender(), msg.value);
}

// solhint-enable ordering
Expand Down
192 changes: 187 additions & 5 deletions contracts/staking/PoSValidatorManager.sol
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,12 @@

pragma solidity 0.8.25;

import {IPoSValidatorManager} from "./interfaces/IPoSValidatorManager.sol";
import {
IPoSValidatorManager, Delegator, DelegatorStatus
} from "./interfaces/IPoSValidatorManager.sol";
import {PoSValidatorManagerSettings} from "./interfaces/IPoSValidatorManager.sol";
import {ValidatorManager} from "./ValidatorManager.sol";
import {Validator, ValidatorStatus} from "./interfaces/IValidatorManager.sol";
import {WarpMessage} from
"@avalabs/[email protected]/contracts/interfaces/IWarpMessenger.sol";
import {ValidatorMessages} from "./ValidatorMessages.sol";
Expand All @@ -17,10 +20,25 @@
// solhint-disable private-vars-leading-underscore
/// @custom:storage-location erc7201:avalanche-icm.storage.PoSValidatorManager
struct PoSValidatorManagerStorage {
/// @notice The minimum amount of stake required to be a validator.
uint256 _minimumStakeAmount;
/// @notice The maximum amount of stake allowed to be a validator.
uint256 _maximumStakeAmount;
/// @notice The minimum amount of time a validator must be staked for.
uint64 _minimumStakeDuration;
/// @notice The reward calculator for this validator manager.
IRewardCalculator _rewardCalculator;
/// @notice Maps the validationID to the uptime of the validator.
mapping(bytes32 validationID => uint64) _uptimes;
cam-schultz marked this conversation as resolved.
Show resolved Hide resolved
/// @notice Maps the validationID to a mapping of delegator address to delegator information.
mapping(bytes32 validationID => mapping(address delegator => Delegator)) _delegatorStakes;
Copy link
Collaborator

@michaelkaplan13 michaelkaplan13 Sep 3, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Choose a reason for hiding this comment

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

i like the (validationID, nonce) hash combo

/// @notice Maps the validationID to a mapping of delegator address to pending register delegator messages.
mapping(bytes32 validationID => mapping(address delegator => bytes))
_pendingRegisterDelegatorMessages;
/// @notice Maps the validationID to a mapping of delegator address to pending end delegator messages.
mapping(bytes32 validationID => mapping(address delegator => bytes))
_pendingEndDelegatorMessages;
/// @notice Maps the validationID to the uptime of the validator.
mapping(bytes32 validationID => uint64) _validatorUptimes;
}
// solhint-enable private-vars-leading-underscore
Expand Down Expand Up @@ -108,14 +126,12 @@
PoSValidatorManagerStorage storage $ = _getPoSValidatorManagerStorage();
// Lock the stake in the contract.
uint256 lockedValue = _lock(stakeAmount);

// Ensure the stake churn doesn't exceed the maximum churn rate.
uint64 weight = valueToWeight(lockedValue);
// Ensure the weight is within the valid range.

// Ensure the weight is within the valid range.
require(
weight >= $._minimumStakeAmount && weight <= $._maximumStakeAmount,
"PoSValidatorManager: Invalid stake amount"
"PoSValidatorManager: invalid stake amount"
);
return weight;
}
Expand All @@ -130,4 +146,170 @@

function _lock(uint256 value) internal virtual returns (uint256);
function _unlock(uint256 value, address to) internal virtual;

function _initializeDelegatorRegistration(
bytes32 validationID,
address delegator,
uint256 delegationAmount
) internal nonReentrant {
uint64 weight = valueToWeight(_lock(delegationAmount));
PoSValidatorManagerStorage storage $ = _getPoSValidatorManagerStorage();

// Ensure the validation period is active
Validator memory validator = _getValidator(validationID);
require(
validator.status == ValidatorStatus.Active, "ValidatorManager: validator not active"
);

// Ensure the delegator is not already registered
require(

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

good with deferring

$._delegatorStakes[validationID][delegator].weight == 0,
"ValidatorManager: delegator already registered"
cam-schultz marked this conversation as resolved.
Show resolved Hide resolved
);

_checkAndUpdateChurnTracker(weight);

// Update the validator weight
validator.weight += weight;
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Choose a reason for hiding this comment

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

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

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 went ahead and added that here, so that it doesn't fall through the cracks.

Choose a reason for hiding this comment

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

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.

_setValidator(validationID, validator);

// Submit the message to the Warp precompile.
uint64 nonce = _getAndIncrementNonce(validationID);
bytes memory setValidatorWeightPayload = ValidatorMessages
.packSetSubnetValidatorWeightMessage(validationID, nonce, validator.weight);
$._pendingRegisterDelegatorMessages[validationID][delegator] = setValidatorWeightPayload;
bytes32 messageID = WARP_MESSENGER.sendWarpMessage(setValidatorWeightPayload);

// Store the delegator information
$._delegatorStakes[validationID][delegator] = Delegator({
weight: weight,
startedAt: 0,
endedAt: 0,
status: DelegatorStatus.PendingAdded
});

emit DelegatorAdded({
validationID: validationID,
setWeightMessageID: messageID,
delegator: delegator,
delegatorWeight: weight,
validatorWeight: validator.weight,
nonce: nonce
});
}

function resendDelegatorRegistration(bytes32 validationID, address delegator) external {
_checkPendingRegisterDelegatorMessages(validationID, delegator);
PoSValidatorManagerStorage storage $ = _getPoSValidatorManagerStorage();
// Submit the message to the Warp precompile.
WARP_MESSENGER.sendWarpMessage($._pendingRegisterDelegatorMessages[validationID][delegator]);
}
Dismissed Show dismissed Hide dismissed

function completeDelegatorRegistration(uint32 messageIndex, address delegator) external {
PoSValidatorManagerStorage storage $ = _getPoSValidatorManagerStorage();

// Unpack the Warp message
WarpMessage memory warpMessage = _getPChainWarpMessage(messageIndex);
(bytes32 validationID, uint64 nonce,) =
ValidatorMessages.unpackSubnetValidatorWeightUpdateMessage(warpMessage.payload);
Comment on lines +213 to +214
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

_checkPendingRegisterDelegatorMessages(validationID, delegator);
delete $._pendingRegisterDelegatorMessages[validationID][delegator];

// Update the validator weight and delegator status
Validator memory validator = _getValidator(validationID);
require(validator.messageNonce >= nonce, "PoSValidatorManager: invalid nonce");

$._delegatorStakes[validationID][delegator].status = DelegatorStatus.Active;
$._delegatorStakes[validationID][delegator].startedAt = uint64(block.timestamp);

emit DelegatorRegistered({
validationID: validationID,
delegator: delegator,
nonce: nonce,
startTime: block.timestamp
});
}
Dismissed Show dismissed Hide dismissed

function initializeEndDelegation(bytes32 validationID) external {
PoSValidatorManagerStorage storage $ = _getPoSValidatorManagerStorage();

// Ensure the delegator is active
Delegator memory delegator = $._delegatorStakes[validationID][_msgSender()];
require(
delegator.status == DelegatorStatus.Active, "ValidatorManager: delegator not active"
);
delegator.status = DelegatorStatus.PendingRemoved;
delegator.endedAt = uint64(block.timestamp);

$._delegatorStakes[validationID][_msgSender()] = delegator;

Validator memory validator = _getValidator(validationID);
require(validator.weight > delegator.weight, "PoSValidatorManager: Invalid weight");
validator.weight -= delegator.weight;
_setValidator(validationID, validator);

// Submit the message to the Warp precompile.
uint64 nonce = _getAndIncrementNonce(validationID);
bytes memory setValidatorWeightPayload = ValidatorMessages
.packSetSubnetValidatorWeightMessage(validationID, nonce, validator.weight);
$._pendingEndDelegatorMessages[validationID][_msgSender()] = setValidatorWeightPayload;
bytes32 messageID = WARP_MESSENGER.sendWarpMessage(setValidatorWeightPayload);

emit DelegatorRemovalInitialized({
validationID: validationID,
setWeightMessageID: messageID,
delegator: _msgSender(),
validatorWeight: validator.weight,
nonce: nonce,
endTime: block.timestamp
});
}
Dismissed Show dismissed Hide dismissed

function resendEndDelegation(bytes32 validationID, address delegator) external {
_checkPendingEndDelegatorMessage(validationID, delegator);
PoSValidatorManagerStorage storage $ = _getPoSValidatorManagerStorage();
WARP_MESSENGER.sendWarpMessage($._pendingEndDelegatorMessages[validationID][delegator]);
}
Dismissed Show dismissed Hide dismissed

function completeEndDelegation(uint32 messageIndex, address delegator) external {
PoSValidatorManagerStorage storage $ = _getPoSValidatorManagerStorage();

// Unpack the Warp message
WarpMessage memory warpMessage = _getPChainWarpMessage(messageIndex);
(bytes32 validationID, uint64 nonce,) =
ValidatorMessages.unpackSubnetValidatorWeightUpdateMessage(warpMessage.payload);
_checkPendingEndDelegatorMessage(validationID, delegator);
delete $._pendingEndDelegatorMessages[validationID][delegator];

// 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);
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

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'm going to defer uptime/rewards handling since I think there are still some open design questions here.

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 implemented uptime handling here: #520

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

}
Dismissed Show dismissed Hide dismissed

function _checkPendingEndDelegatorMessage(
bytes32 validationID,
address delegator
) private view {
PoSValidatorManagerStorage storage $ = _getPoSValidatorManagerStorage();
require(
cam-schultz marked this conversation as resolved.
Show resolved Hide resolved
$._pendingEndDelegatorMessages[validationID][delegator].length > 0
&& $._delegatorStakes[validationID][delegator].status == DelegatorStatus.PendingRemoved,
"PoSValidatorManager: delegator removal not pending"
);
}

function _checkPendingRegisterDelegatorMessages(
bytes32 validationID,
address delegator
) private view {
PoSValidatorManagerStorage storage $ = _getPoSValidatorManagerStorage();
require(
$._pendingRegisterDelegatorMessages[validationID][delegator].length > 0
&& $._delegatorStakes[validationID][delegator].status == DelegatorStatus.PendingAdded,
"PoSValidatorManager: delegator registration not pending"
);
}
}
Loading