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

FEATURES: June Epoch Upgrade PR #364

Merged
merged 8 commits into from Jun 26, 2023
Merged

FEATURES: June Epoch Upgrade PR #364

merged 8 commits into from Jun 26, 2023

Conversation

joshuahannan
Copy link
Member

@joshuahannan joshuahannan commented Apr 18, 2023

Includes multiple upgrades that will be going out near the June spork

Introduce Delegator Staking Minimums

Closes #363

Based on FLIP78-Delegator Staking Minimums

Adds functionality for enforcing that delegators are above a minimum stake to be staked in an epoch.
If a delegator is below the minimum, they will be unstaked during the move tokens operation.
The minimum is stored in key-0 of the existing staking minimum dictionary.
Delegators cannot register unless they provide the minimum.

The current minimum is expected to be 50, but that value can and will probably change in the future.

This change will be breaking for anyone who is using the basic register delegator transaction and the Locked Tokens register delegator transaction because it adds a new parameter to specify the number of tokens to commit.

Use capabilities for epoch admins

Adds EpochOperations resource interfaces for the FlowIDTableStaking, FlowClusterQC, and FlowDKG Admin resources so that these resources can be moved to the service account on the production networks. These interfaces restrict the functionality that is accessible by the epoch smart contracts and epoch account to only that which is required for automated epoch functionality.

New paths are used for the capabilities so that the resources can remain in the same paths for testing purposes

Also moves resetEpoch() into the FlowEpoch.Admin resource instead of the FlowEpoch.Heartbeat resource.

@@ -77,6 +77,13 @@ pub contract FlowIDTableStaking {

/// The minimum amount of tokens that each node type has to stake
/// in order to be considered valid
/// Keys:
/// 0 - Delegators
Copy link
Member

Choose a reason for hiding this comment

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

I am not a big fan of combining the delegator minimum stake with the per-role stake minimums. I think we should prioritize conceptual precision over the convenience of re-using an existing storage slot.

The staking contract has the business logic concepts:

  • a node role represents the set of responsibilities and capabilities associated with a node running in the network (collection, consensus, access, verification, execution)
  • a staking mechanism is a way to stake tokens and receive rewards (registration, delegation)

I can register a node with a particular role. Then someone can delegate tokens to that node. I cannot register a node with role delegator. They are conceptually distinct and should be differentiated in the business logic.

In addition, several comments are now incorrect or imprecise with this change.

/// Checks if the amount of tokens is greater than the minimum staking requirement for the specified role

/// The minimum amount of tokens that each node type has to stake in order to be considered valid

Delegators are not a node type or role.

I am being a bit pedantic here, but this is a core system contract. It is important that it is as easy as possible to read and reason about, both for maintainers and community members. Conflating delegation minimums with node role minimums introduces a fairly small additional cognitive overhead to reasoning about the contract ("node roles but also delegator minimums"), but the but also moments add up to making bugs more difficult to spot, and the behaviour less comprehensible to community members.

Concretely, I would suggest to store the delegator minimum stake as a separate config field with distinct documentation, setters, and getters.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the feedback! That is a great point. I do like the idea of keeping it a part of the shared field with the node minimums though because it saves some lines of code and streamlines the contract in other small ways, but I understand the potential confusion. The field is just called minimumStakeRequired, so it could include delegators if we wanted to expand the definition of the term role and fix the documentation. I'm a little torn. I'll see if I can get another opinion.

Copy link
Contributor

Choose a reason for hiding this comment

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

I kind of agree with what @jordanschalm & @joshuahannan said here and currently the key of the dictionary minimumStakeRequired represents the staker role, That causes confusion and to avoid it we may need to correct the documentation and make the key for a staking actor not for the role type staking because in the role we are colluding with the blockchain node's role with simple network participants (i.e. Delegator).

Copy link
Contributor

@bluesign bluesign May 9, 2023

Choose a reason for hiding this comment

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

The field is just called minimumStakeRequired, so it could include delegators if we wanted to expand the definition of the term role and fix the documentation. I'm a little torn. I'll see if I can get another opinion.

I think this is very good idea.

To explain my thinking a bit. This contract is called FlowIDTableStaking. Role in this context is FlowIDTableStaking.Role more like Staking.Role. I think making distinction between NodeRole and StakingRole can be achieved later if needed. ( NodeRole requiring a StakingRole etc )

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with @jordanschalm here. Readability is important and some more lines of code here is worth it to be correct.

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure how to resolve this. It looks like there is fairly equal support for both paths and I still am on the side of doing it how it is already done here.

Copy link
Member Author

Choose a reason for hiding this comment

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

I just pushed a new commit that uses a different field for the delegator minimums now. I think I'm convinced now.

@joshuahannan joshuahannan changed the title Add delegator staking minimum requirements FEATURES: June Epoch Upgrade PR May 9, 2023
@joshuahannan
Copy link
Member Author

I posted in the forum about the plans to upgrade: https://forum.onflow.org/t/breaking-change-coming-to-the-flow-epoch-smart-contracts-to-introduce-delegator-staking-minimums/4928

Expected dates to get the changes in effect is as follows:

CanaryNet: Monday, June 26th, 2023
Testnet: Wednesday, June 28th, 2023

And assuming that the upgrades on the canary and testnet go well,

Mainnet: Wednesday, July 5th, 2023

The upgrade plan with transactions is detailed in this service account PR: onflow/service-account#243

@joshuahannan joshuahannan merged commit de95d0c into master Jun 26, 2023
1 check passed
@joshuahannan joshuahannan deleted the delegator-min branch June 26, 2023 14:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Introduce Delegator Staking Minimums
5 participants