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

Require non-zero delegation fee #521

Closed
wants to merge 14 commits into from

Conversation

iansuvak
Copy link
Contributor

@iansuvak iansuvak commented Sep 4, 2024

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 mapping but we might consider adding it to the Validator struct.

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 without vm.expectRevert().

How is this documented

Comments

@iansuvak iansuvak requested a review from a team as a code owner September 4, 2024 20:25
@iansuvak iansuvak marked this pull request as draft September 4, 2024 20:25
@iansuvak iansuvak changed the title Require non-zero delegation fee [WIP] Require non-zero delegation fee Sep 4, 2024
Copy link
Contributor

@geoff-vball geoff-vball left a comment

Choose a reason for hiding this comment

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

.

tests/utils/staking.go Outdated Show resolved Hide resolved
tests/utils/staking.go Outdated Show resolved Hide resolved
tests/utils/staking.go Outdated Show resolved Hide resolved
@geoff-vball
Copy link
Contributor

geoff-vball commented Sep 5, 2024

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

@minghinmatthewlam
Copy link

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

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

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

Base automatically changed from delegation-poc to staking-contract September 5, 2024 16:50
@iansuvak iansuvak changed the title [WIP] Require non-zero delegation fee Require non-zero delegation fee Sep 6, 2024
@iansuvak iansuvak marked this pull request as ready for review September 6, 2024 17:01
Comment on lines +137 to +145
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);
}
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 add documentation to this function regarding when it can/should be called

tests/utils/staking.go Outdated Show resolved Hide resolved
@@ -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;
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

@@ -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;
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 not set on this stateful solution being the cleanest way to pass in delegationFeeRate to child test contracts. Happy to hear alternatives.

@iansuvak iansuvak closed this Sep 12, 2024
Copy link

@minghinmatthewlam minghinmatthewlam left a comment

Choose a reason for hiding this comment

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

my bad forgot this was a separate issue, also had an implementation in #544 with other parameterized values

@michaelkaplan13 michaelkaplan13 deleted the nonzero-delegation-fee branch September 20, 2024 21:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
audit scope Issues required as part of an upcoming audit scope L1 Staking Contract
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants