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

Sys 4010 make autogrowth mechanism configurable #422

Merged
merged 16 commits into from
Jun 26, 2024

Conversation

micaelffrancoAV
Copy link
Contributor

Proposed changes

  • Mechanism to enable or disable growth.

Type of change/Merge

🚨What type of change is this PR?

Put an x in the boxes that apply

  • Release
    • Increase versions
    • Baseline tests passed
    • Release type:
      • Major release
      • Minor release
      • Patch release

Checklist

Put an x in the boxes that apply. You can also fill these out after creating the PR.

  • You describe the purpose of the PR, e.g.:
    • What does it do?
    • Highlight what important points reviewers should know about;
    • Indicates if there is something left for follow-up PRs.
  • Documentation updated
  • Business logic tested successfully
  • Verify First, Write Last: In Substrate development, it is important that you always ensure preconditions are met and return errors at the beginning. After these checks have completed, then you may begin the function's computation.

Further comments

Copy link
Contributor

@nahuseyoum nahuseyoum left a comment

Choose a reason for hiding this comment

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

The way its setup now, we need to have a storage migration script to set this value to true so it doesn't break for Aventus so we can either :

  • include a migration script or
  • invert the logic and use growth_disabled. This way, by default, Growth will be triggered and if a client wants to disable it, they can using the SUDO extrinsic.

@@ -245,6 +245,8 @@ pub mod pallet {
type AccountToBytesConvert: pallet_avn::AccountToBytesConverter<Self::AccountId>;

type BridgeInterface: pallet_avn::BridgeInterface;

type GrowthEnabled: Get<bool>;
Copy link
Contributor

Choose a reason for hiding this comment

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

if we are setting it in storage, I guess we can remove the config trait?

@@ -245,6 +245,8 @@ pub mod pallet {
type AccountToBytesConvert: pallet_avn::AccountToBytesConverter<Self::AccountId>;

type BridgeInterface: pallet_avn::BridgeInterface;

type GrowthEnabled: Get<bool>;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
type GrowthEnabled: Get<bool>;
#[pallet::constant]
type GrowthEnabled: Get<bool>;

@@ -687,6 +689,7 @@ pub mod pallet {
pub delay: EraIndex,
pub min_collator_stake: BalanceOf<T>,
pub min_total_nominator_stake: BalanceOf<T>,
pub growth_enabled: bool,
Copy link
Contributor

Choose a reason for hiding this comment

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

is this needed?

@@ -697,6 +700,7 @@ pub mod pallet {
delay: Default::default(),
min_collator_stake: Default::default(),
min_total_nominator_stake: Default::default(),
growth_enabled: T::GrowthEnabled::get(),
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

@@ -566,6 +583,7 @@ impl ExtBuilder {
delay: 2,
min_collator_stake: self.min_collator_stake,
min_total_nominator_stake: self.min_total_nominator_stake,
growth_enabled: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

}

#[test]
fn growth_info_is_not_updated() {
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@@ -439,6 +441,7 @@ impl ExtBuilder {
delay: 2,
min_collator_stake: 10,
min_total_nominator_stake: 5,
growth_enabled: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

and here

@@ -502,6 +504,7 @@ impl ExtBuilder {
delay: 2,
min_collator_stake: 10,
min_total_nominator_stake: 5,
growth_enabled: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

here too

@micaelffrancoAV micaelffrancoAV merged commit 95ee38e into main Jun 26, 2024
5 checks passed
@micaelffrancoAV micaelffrancoAV deleted the SYS-4010_Make_autogrowth_mechanism_configurable branch June 26, 2024 11:11
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.

3 participants