Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Change the Config of the MaxRococoNum Slot from a Constant to a Storage function #7217

Merged
merged 61 commits into from
Aug 15, 2023
Merged

Change the Config of the MaxRococoNum Slot from a Constant to a Storage function #7217

merged 61 commits into from
Aug 15, 2023

Conversation

AlexD10S
Copy link
Contributor

From the PR: Change the Config of the MaxRococoNum Slot from a Constant to a Storage function

Revamp the pallet to get MaxPermanentSlots and MaxTemporarySlots out of the Config trait of assigned_slots pallet.
And add two extrinsics so Root can modify these values without the need of a runtime upgrade.

@AlexD10S AlexD10S requested a review from al3mart May 12, 2023 09:53
al3mart
al3mart reviewed May 12, 2023
View reviewed changes
Copy link
Contributor

@al3mart al3mart left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Left a comment, overall looking good.

runtime/common/src/assigned_slots/mod.rs Outdated
Comment on lines 208 to 210
fn on_runtime_upgrade() -> frame_support::weights::Weight {
migration::migrate_to_v2::<T>()
}
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 add the migration in the runtime, like these

Just as a good practice to follow, so it is easier to spot which migrations will get executed in the next upgrade.
This would also work, but the discoverability of this migration happening for other devs is worse.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

@rphmeier
Copy link
Contributor

FYI, parameter_types! also supports declaring values in storage.

ggwpez
ggwpez reviewed May 14, 2023
View reviewed changes
Copy link
Member

@ggwpez ggwpez left a comment •
edited
Loading

Choose a reason for hiding this comment

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

You can also just change this

pub const MaxPermanentSlots: u32 = 100;
pub const MaxTemporarySlots: u32 = 100;

to this

pub storage MaxPermanentSlots: u32 = 100;
pub storage MaxTemporarySlots: u32 = 100;

since you require root any way. It is a two line change. Root can then use System::set_storage to poke them.
But i guess your approach is less hacky.

ggwpez
ggwpez reviewed May 14, 2023
View reviewed changes
runtime/common/src/assigned_slots/mod.rs Outdated Show resolved Hide resolved
runtime/common/src/assigned_slots/mod.rs Outdated Show resolved Hide resolved
runtime/common/src/assigned_slots/mod.rs Outdated Show resolved Hide resolved
runtime/common/src/assigned_slots/mod.rs Outdated Show resolved Hide resolved
runtime/common/src/assigned_slots/mod.rs Outdated Show resolved Hide resolved
@ggwpez ggwpez added A3-in_progress Pull request is in progress. No review needed at this stage. T1-runtime This PR/Issue is related to the topic “runtime”. B1-note_worthy Changes should be noted in the release notes labels May 14, 2023
@AlexD10S
Copy link
Contributor Author

You can also just change this

pub const MaxPermanentSlots: u32 = 100;
pub const MaxTemporarySlots: u32 = 100;

to this

pub storage MaxPermanentSlots: u32 = 100;
pub storage MaxTemporarySlots: u32 = 100;

since you require root any way. It is a two line change. Root can then use System::set_storage to poke them. But i guess your approach is less hacky.

Thanks for the feedback. Is great to know this approach, but as you said it looks more hacky. I believe it can be a bit confusing to see the 100 as a magic number in the Config if it can be changed in the storage with the System::set_storage

@AlexD10S AlexD10S requested review from ggwpez and al3mart May 17, 2023 12:13
@al3mart al3mart added D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit. C1-low PR touches the given topic and has a low impact on builders. labels May 18, 2023
@al3mart al3mart self-requested a review May 19, 2023 11:26
@command-bot
Copy link

command-bot bot commented Aug 7, 2023

@AlexD10S Command "$PIPELINE_SCRIPTS_DIR/commands/bench/bench.sh" --subcommand=runtime --runtime=westend --target_dir=polkadot --pallet=runtime_common::assigned_slots has finished. Result: https://gitlab.parity.io/parity/mirrors/polkadot/-/jobs/3340029 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/polkadot/-/jobs/3340029/artifacts/download.

ggwpez
ggwpez approved these changes Aug 8, 2023
View reviewed changes
Copy link
Member

@ggwpez ggwpez left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

runtime/common/src/assigned_slots/benchmarking.rs Outdated Show resolved Hide resolved
liamaharon
liamaharon suggested changes Aug 10, 2023
View reviewed changes
Copy link
Contributor

@liamaharon liamaharon left a comment

Choose a reason for hiding this comment

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

Looking good! Just need to tweak the migration and I left some other minor comments on things you may choose to address.

runtime/common/src/assigned_slots/benchmarking.rs Outdated Show resolved Hide resolved
runtime/common/src/assigned_slots/migration.rs Outdated Show resolved Hide resolved
runtime/common/src/assigned_slots/mod.rs Outdated Show resolved Hide resolved
runtime/common/src/assigned_slots/mod.rs Outdated Show resolved Hide resolved
runtime/common/src/assigned_slots/mod.rs Outdated Show resolved Hide resolved
runtime/common/src/assigned_slots/mod.rs Outdated Show resolved Hide resolved
runtime/common/src/assigned_slots/mod.rs Outdated Show resolved Hide resolved
runtime/common/src/assigned_slots/mod.rs Outdated Show resolved Hide resolved
runtime/common/src/assigned_slots/mod.rs Outdated Show resolved Hide resolved
@AlexD10S AlexD10S added A0-please_review Pull request needs code review. and removed A0-please_review Pull request needs code review. labels Aug 14, 2023
gilescope
gilescope approved these changes Aug 15, 2023
View reviewed changes
Copy link
Contributor

@gilescope gilescope left a comment

Choose a reason for hiding this comment

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

LGTM

@AlexD10S
Copy link
Contributor Author

bot merge

@paritytech-processbot paritytech-processbot bot merged commit 1bddab1 into master Aug 15, 2023
4 checks passed
@paritytech-processbot paritytech-processbot bot deleted the alexd10s/revamp_assigned_slots_rococo branch August 15, 2023 13:17
@AlexD10S AlexD10S mentioned this pull request Aug 31, 2023
Closed
@crystalin crystalin mentioned this pull request Oct 20, 2023
Closed
@JesseAbram JesseAbram mentioned this pull request Oct 26, 2023
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@Daanvdplas Daanvdplas Daanvdplas left review comments

@liamaharon liamaharon liamaharon approved these changes

@al3mart al3mart al3mart approved these changes

@ggwpez ggwpez ggwpez approved these changes

@gilescope gilescope gilescope approved these changes

Assignees
No one assigned
Labels
A0-please_review Pull request needs code review. A3-in_progress Pull request is in progress. No review needed at this stage. B1-note_worthy Changes should be noted in the release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit. E0-runtime_migration PR introduces code that might require downstream chains to run a runtime upgrade. T1-runtime This PR/Issue is related to the topic “runtime”.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@AlexD10S @rphmeier @gilescope @ggwpez @al3mart @liamaharon @Daanvdplas