Require non-zero delegation fee #521
Conversation
iansuvak
requested review from
feuGeneA,
geoff-vball,
minghinmatthewlam,
bernard-avalabs,
michaelkaplan13 and
cam-schultz
iansuvak
changed the title
|
Looks like Cam opened a ticket for allowing validators to disable delegation. Not sure if you think it makes sense to implement here as well or want to defer to another PR #522 |
This was a followup from a comment to separate delegation as an extension to allow for people to choose validator managers that don't have delegation built in by default. Can still move forward here, and move all the delegation out altogether |
| @@ -38,6 +40,8 @@ abstract contract PoSValidatorManager is IPoSValidatorManager, ValidatorManager | |||
| _pendingEndDelegatorMessages; | |||
| /// @notice Maps the validationID to the uptime of the validator. | |||
| mapping(bytes32 validationID => uint64) _validatorUptimes; | |||
| /// @notice Maps the validationID to the delegation fee rate. | |||
| mapping(bytes32 validationID => uint256) _validatorDelegationFeeRates; | |||
There was a problem hiding this comment.
Do we want to track this here or in Validator? This makes it more specific to the PoS and maintains less state by not adding it to the delegation. The tradeoff that it's an additional thing that needs to be cleaned up once all delegators have exited.
There was a problem hiding this comment.
I do like keeping the staking specific settings in the staking contract. We should think about if this is also the simplest solution given the PoA -> PoS upgrade potential, which I think it does? If we allow Validators to set their fee rate to 0 to disable delegation, then we could automatically get the property of PoA validators not being able to be delegated to, I think?
There was a problem hiding this comment.
I don't think that I love overloading the function of the fee rate, but this might be the cheapest/cleanest way of doing this. I will give it a try and consider alternatives.
iansuvak
added this to the Staking Manager Audit Ready - all PRs should target staking-contract branch milestone
iansuvak
added
L1 Staking Contract
audit scope
iansuvak
changed the title
| function _setDelegationFeeRate(bytes32 validationID, uint256 feeRate) internal { | ||
| PoSValidatorManagerStorage storage $ = _getPoSValidatorManagerStorage(); | ||
| require( | ||
| feeRate >= $._minimumDelegationFeeRate && feeRate <= MAXIMUM_DELEGATION_FEE_RATE, | ||
| "PoSValidatorManager: invalid delegation fee rate" | ||
| ); | ||
| $._validatorDelegationFeeRates[validationID] = feeRate; | ||
| emit DelegationFeeRateSet(validationID, feeRate); | ||
| } |
There was a problem hiding this comment.
We should add documentation to this function regarding when it can/should be called
| @@ -38,6 +40,8 @@ abstract contract PoSValidatorManager is IPoSValidatorManager, ValidatorManager | |||
| _pendingEndDelegatorMessages; | |||
| /// @notice Maps the validationID to the uptime of the validator. | |||
| mapping(bytes32 validationID => uint64) _validatorUptimes; | |||
| /// @notice Maps the validationID to the delegation fee rate. | |||
| mapping(bytes32 validationID => uint256) _validatorDelegationFeeRates; | |||
There was a problem hiding this comment.
I do like keeping the staking specific settings in the staking contract. We should think about if this is also the simplest solution given the PoA -> PoS upgrade potential, which I think it does? If we allow Validators to set their fee rate to 0 to disable delegation, then we could automatically get the property of PoA validators not being able to be delegated to, I think?
minghinmatthewlam
removed this from the Staking Manager Audit Ready - all PRs should target staking-contract branch milestone
| @@ -22,6 +22,10 @@ abstract contract PoSValidatorManagerTest is ValidatorManagerTest { | |||
| address public constant DEFAULT_DELEGATOR_ADDRESS = | |||
| address(0x1234123412341234123412341234123412341234); | |||
|
|
|||
| // This is the rate that will be passed into the child contract `initializeValidatorRegistration` calls | |||
| // It is set here to avoid having to pass irrelevant initializers to the parent contract. | |||
| uint256 public delegationFeeRate; | |||
There was a problem hiding this comment.
I'm not set on this stateful solution being the cleanest way to pass in delegationFeeRate to child test contracts. Happy to hear alternatives.
Co-authored-by: Geoff Stuart <[email protected]> Signed-off-by: Ian Suvak <[email protected]>
There was a problem hiding this comment.
my bad forgot this was a separate issue, also had an implementation in #544 with other parameterized values
Why this should be merged
Fixes #505
How this works
Adds a new argument to the PoSValidatorManager initializer that sets a global minimum delegation fee rate and then enforces that any new validation respects it through a check in the initializer.
It stores the rates in a separate
mappingbut we might consider adding it to theValidatorstruct.How this was tested
Manually tested right now. I'm running into issues with
vm.expectRevert()presumably due to call nesting. Calls with invalid delegation fees that should revert fail both with and withoutvm.expectRevert().How is this documented
Comments