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

stake prioritized features for next release #285

Merged
merged 12 commits into from
Mar 16, 2021
Merged

stake prioritized features for next release #285

merged 12 commits into from
Mar 16, 2021

Conversation

4meta5
Copy link
Contributor

@4meta5 4meta5 commented Mar 14, 2021

What does it do?

  • use FRAMEv2 macro syntax
  • change name to parachain-staking
  • remove swap_nominations
  • merge nominate_new and join_nominators into single nominate runtime method
  • change all instances of Validator to Collator. The Validators storage item should be renamed SelectedCandidates
  • add MinCollatorCandidateStk bond, as an associated type for Config trait and only use MinCollatorStk constant when selecting the SelectedCandidates

What important points reviewers should know?

Is there something left for follow-up PRs?

I started to make these changes, but realized they need to be split into follow ups to get proper review.

  • make BlocksPerRound a storage item and add runtime method for root to update it (using > instead of % for determining round transitions)
  • make collator fee a pallet constant (associated type Get<Perbill> for Config), instead of configurable per collator (as per preferred design constraints)

What alternative implementations were considered?

Are there relevant PRs or issues in other repositories (Substrate, Polkadot, Frontier, Cumulus)?

What value does it bring to the blockchain users?

Checklist

  • Does it require a purge of the network?
  • You bumped the runtime version if there are breaking changes in the runtime ?
  • Does it require changes in documentation/tutorials ?

@@ -115,6 +115,7 @@ impl crate::pallet::Config for Test {
type MaxCollatorsPerNominator = MaxCollatorsPerNominator;
type MaxFee = MaxFee;
type MinCollatorStk = MinCollatorStk;
type MinCollatorCandidateStk = MinCollatorStk;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we want, we can set this to be a different value from MinCollatorStk. The new constant is the minimum amount reserved for an account to become a collator candidate. The MinCollatorStk is now the minimum required for a collator candidate to be chosen for the round in SelectedCandidates (which is the set of collators for the round).

Why? If we want to lower the capital requirements for becoming a collator candidate such that accounts can become a candidate with less funds, but they still need to get a certain backing in order to be selected in SelectedCandidates. This was mentioned to me by @artkaseman

Copy link
Contributor

Choose a reason for hiding this comment

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

To me this makes sense in the opposite case of what you described: We want to raise the cost of joining the candidates (because there are too many) but we don't want to boot anyone from the active set (yet).

The case where we lower the cost to become a candidate seems like it might have unintended consequences. Imagine we lower the cost to become a candidate such that it is lower than the total stake to enter the active set. I can image a lot of stakers saying "wth, I made the minimum stake requirement, I got nominators, and I'm in the top N. Why am I not active?"

That being said, if people want it I'm fine with it.

Copy link
Contributor Author

@4meta5 4meta5 Mar 15, 2021

Choose a reason for hiding this comment

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

I can image a lot of stakers saying "wth, I made the minimum stake requirement, I got nominators, and I'm in the top N. Why am I not active?"

If we monitor the chain and make sure that there are at least TotalSelectedCandidates collator candidates with the MinCollatorStk, then this will never happen because anyone in the top N will be put in SelectedCandidates for the round.

@4meta5 4meta5 mentioned this pull request Mar 15, 2021
@4meta5 4meta5 marked this pull request as ready for review March 15, 2021 06:29
Copy link
Contributor

@JoshOrndorff JoshOrndorff left a comment

Choose a reason for hiding this comment

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

Looks good. Nice cleanup and update!

I'll mention a few little notes about naming and docs that crossed my mind, but no pressure to make these changes here:

  • You've got a notion of SelectedCollators. In the nimbus consensus as well as the research that Gorka has been doing, we've been using the term "Active" where you used Selected. Obviously neither is objectively better.
  • One big thing that distinguishes this pallet from Parity's is that this pallet uses direct delegation. Nominators choose exactly who they nominate and with what share of their stake. This is different from Parity's where you approval vote and then run Phragmen. Mightbe worth mentioning in the docs.

@4meta5 4meta5 requested a review from crystalin March 15, 2021 15:26
@crystalin
Copy link
Collaborator

This will require updating the documentation and terms (@albertov19 )

@4meta5 4meta5 merged commit 084ddff into master Mar 16, 2021
@4meta5 4meta5 deleted the amar-clean-stake branch March 16, 2021 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.

None yet

3 participants