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

Update collator candidate self-bond in Moonriver #2139

Merged
merged 3 commits into from
Mar 15, 2023

Conversation

Agusrodri
Copy link
Contributor

@Agusrodri Agusrodri commented Feb 28, 2023

What does it do?

According to this referenda https://moonriver.polkassembly.network/referenda/1, the change proposed in this PR is the one that refers to update the bond to 10,000 MOVR in order the collator to be considered eligible and become a collator candidate. The current bond is 500 MOVR.

⚠️ Breaking Changes ⚠️

  • All changes are moonriver only.
  • Changed both MinCandidateStk and MinCollatorStk constants to the value of 10,000 MOVR.

Notes

  • MinCandidateStk refers to the minimum stake required to be reserved to be a candidate. Located in the moonriver runtime Config (lib file).
  • MinCollatorStk refers to the minimum stake required to become a collator. Located in the moonriver runtime Config (lib file).

@Agusrodri Agusrodri added the D1-runtime-migration PR introduces code that might require downstream chains to run a runtime upgrade. label Feb 28, 2023
@notlesh notlesh added B7-runtimenoteworthy Changes should be noted in any runtime-upgrade release notes D5-nicetohaveaudit⚠️ PR contains trivial changes to logic that should be properly reviewed. D10-breaksdocs Changes to code that require changes in documentation breaking Needs to be mentioned in breaking changes and removed D1-runtime-migration PR introduces code that might require downstream chains to run a runtime upgrade. labels Feb 28, 2023
Copy link
Contributor

@notlesh notlesh 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 so far. We will want to document the changes a bit more in this PR description so that others know exactly what's changing. I also think (hope) some tests will break because of this :)

@@ -674,7 +674,7 @@ impl pallet_parachain_staking::Config for Runtime {
/// Minimum stake required to become a collator
type MinCollatorStk = ConstU128<{ 1000 * currency::MOVR * currency::SUPPLY_FACTOR }>;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Wasn't this discussed to be changed too ?

Copy link
Contributor Author

@Agusrodri Agusrodri Feb 28, 2023

Choose a reason for hiding this comment

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

Yeah, I realised that I should change this constant too. That´s what I did a few minutes ago and now I'm finishing fixing some tests.

Copy link
Contributor

@notlesh notlesh Feb 28, 2023

Choose a reason for hiding this comment

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

Shouldn't this imply that MinCandidateStk <= MinCollatorStk? That wasn't the case before this PR, but it seems to be implied:

        /// Minimum stake required for any candidate to be in `SelectedCandidates` for the round
        #[pallet::constant]
        type MinCollatorStk: Get<BalanceOf<Self>>;
        /// Minimum stake required for any account to be a collator candidate
        #[pallet::constant]
        type MinCandidateStk: Get<BalanceOf<Self>>;

CC @4meta5

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 also be careful bumping MinCollatorStk because it could prevent a lot (theoretically all) candidates from being selected for future rounds

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 agree. For now I put the same value in both constants (your bond must be 10000 MOVR to both be selected for the round and be a collator candidate). We should discuss this in more detail and then I change the values and fix tests again if necessary.

@Agusrodri
Copy link
Contributor Author

Looks good so far. We will want to document the changes a bit more in this PR description so that others know exactly what's changing. I also think (hope) some tests will break because of this :)

Yes! I will definitely add a better description. About tests, I'm finishing fixing some of the integration ones and others within the moonriver runtime that had broken.

@crystalin
Copy link
Collaborator

@Agusrodri Please verify the vote was approved and merge if that is the case

@Agusrodri Agusrodri merged commit 093b04a into master Mar 15, 2023
@Agusrodri Agusrodri deleted the agustin-collator-minimum-bond-discussion branch March 15, 2023 13:36
@notlesh notlesh added D1-audited👍 PR contains changes to fund-managing logic that has been properly reviewed and externally audited and removed D5-nicetohaveaudit⚠️ PR contains trivial changes to logic that should be properly reviewed. labels Apr 26, 2023
imstar15 pushed a commit to AvaProtocol/moonbeam that referenced this pull request May 16, 2023
…#2139)

* change MinCandidateStk constant in moonriver runtime

* fix moonriver runtime and integration tests

* update chain_spec
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B7-runtimenoteworthy Changes should be noted in any runtime-upgrade release notes breaking Needs to be mentioned in breaking changes D1-audited👍 PR contains changes to fund-managing logic that has been properly reviewed and externally audited D10-breaksdocs Changes to code that require changes in documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants