-
Notifications
You must be signed in to change notification settings - Fork 336
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
Conversation
… filtering to get SelectedCandidates
@@ -115,6 +115,7 @@ impl crate::pallet::Config for Test { | |||
type MaxCollatorsPerNominator = MaxCollatorsPerNominator; | |||
type MaxFee = MaxFee; | |||
type MinCollatorStk = MinCollatorStk; | |||
type MinCollatorCandidateStk = MinCollatorStk; |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this 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.
This will require updating the documentation and terms (@albertov19 ) |
What does it do?
parachain-staking
swap_nominations
nominate_new
andjoin_nominators
into singlenominate
runtime methodValidator
toCollator
. TheValidators
storage item should be renamedSelectedCandidates
MinCollatorCandidateStk
bond, as an associated type for Config trait and only useMinCollatorStk
constant when selecting theSelectedCandidates
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.
BlocksPerRound
a storage item and add runtime method for root to update it (using > instead of % for determining round transitions)Get<Perbill>
forConfig
), 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