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

Add pallet_parameters. #2898

Closed

Conversation

TarekkMA
Copy link
Contributor

@TarekkMA TarekkMA commented Aug 5, 2024

What does it do?

Add pallet_parameters to allow setting constants using a root origin.

What important points reviewers should know?

Is there something left for follow-up PRs?

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?

@TarekkMA TarekkMA requested a review from RomarQ August 6, 2024 14:52
@TarekkMA TarekkMA changed the title Add pallet_parameters Add pallet_parameters. Aug 6, 2024
@TarekkMA TarekkMA requested a review from librelois August 6, 2024 14:52
runtime/moonbase/src/lib.rs Outdated Show resolved Hide resolved
@librelois librelois 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. breaking Needs to be mentioned in breaking changes labels Aug 6, 2024
@TarekkMA TarekkMA requested review from RomarQ August 7, 2024 09:40
@TarekkMA TarekkMA marked this pull request as ready for review August 7, 2024 12:28
RomarQ
RomarQ previously approved these changes Aug 7, 2024
@librelois
Copy link
Collaborator

Please not merge, we need MBF feedback to know the list of constants they want to move to storage

@librelois librelois added the MBF Waiting MBF input label Aug 7, 2024
@TarekkMA TarekkMA requested a review from RomarQ August 16, 2024 05:17
@librelois librelois removed the MBF Waiting MBF input label Aug 23, 2024
pub mod runtime_config {
// for fees, 80% are burned, 20% to the treasury
#[codec(index = 0)]
pub static FeesTreasuryPercentage: Perbill = Perbill::from_percent(20);
Copy link
Contributor

Choose a reason for hiding this comment

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

This could probably be Percent instead of Perbill. But not an issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's to give ability to add more precision if needed in the future.

@@ -1124,6 +1124,10 @@ macro_rules! impl_runtime_apis_plus_common {
hex_literal::hex!( "0d715f2646c8f85767b5d2764bb27826"
"04a74d81251e398fd8a0a4d55023bb3f")
.to_vec().into(),
// Parameters Parameters
Copy link
Collaborator

Choose a reason for hiding this comment

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

Parameters Parameters is a map, not a value.
This whitelist do nothing then, as we can whitelist only full keys

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there no option to whitelist an entire storage?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think so, we have to edit manually this list each time we add a parameter

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@TarekkMA
Copy link
Contributor Author

Closed in favor of #2923. (Branch should be on moonbeam repo)

@TarekkMA TarekkMA closed this Aug 30, 2024
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 D5-nicetohaveaudit⚠️ PR contains trivial changes to logic that should be properly reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants