[FRAME] Parameters pallet

[ggwpez]

Closes #169

Fork of the orml-parameters-pallet as introduced by open-web3-stack/open-runtime-module-library#927 (cc @xlc)
It greatly changes how the macros work, but keeps the pallet the same. The downside of my code is now that it does only support constant keys in the form of types, not value-bearing keys.
I think this is an acceptable trade off, give that it can be used by any pallet without any changes.

The pallet allows to dynamically set parameters that can be used in pallet configs while also restricting the updating on a per-key basis. The rust-docs contains a complete example.

Changes:

• Add parameters-pallet
• Use in the kitchensink as demonstration
• Add experimental attribute to define dynamic params in the runtime.
• Adding a bunch of traits to frame_support::traits::dynamic_params that can be re-used by the ORML macros

Example

First to define the parameters in the runtime file. The syntax is very explicit about the codec index and errors if there is no.

#[dynamic_params(RuntimeParameters, pallet_parameters::Parameters::<Runtime>))]
pub mod dynamic_params {
	use super::*;

	#[dynamic_pallet_params]
	#[codec(index = 0)]
	pub mod storage {
		/// Configures the base deposit of storing some data.
		#[codec(index = 0)]
		pub static BaseDeposit: Balance = 1 * DOLLARS;

		/// Configures the per-byte deposit of storing some data.
		#[codec(index = 1)]
		pub static ByteDeposit: Balance = 1 * CENTS;
	}

	#[dynamic_pallet_params]
	#[codec(index = 1)]
	pub mod contracts {
		#[codec(index = 0)]
		pub static DepositPerItem: Balance = deposit(1, 0);

		#[codec(index = 1)]
		pub static DepositPerByte: Balance = deposit(0, 1);
	}
}

Then the pallet is configured with the aggregate:

impl pallet_parameters::Config for Runtime {
	type AggregratedKeyValue = RuntimeParameters;
	type AdminOrigin = EnsureRootWithSuccess<AccountId, ConstBool<true>>;
	...
}

And then the parameters can be used in a pallet config:

impl pallet_preimage::Config for Runtime {
	type DepositBase = dynamic_params::storage::DepositBase;
}

A custom origin an be defined like this:

pub struct DynamicParametersManagerOrigin;

impl EnsureOriginWithArg<RuntimeOrigin, RuntimeParametersKey> for DynamicParametersManagerOrigin {
	type Success = ();

	fn try_origin(
		origin: RuntimeOrigin,
		key: &RuntimeParametersKey,
	) -> Result<Self::Success, RuntimeOrigin> {
		match key {
			RuntimeParametersKey::Storage(_) => {
				frame_system::ensure_root(origin.clone()).map_err(|_| origin)?;
				return Ok(())
			},
			RuntimeParametersKey::Contract(_) => {
				frame_system::ensure_root(origin.clone()).map_err(|_| origin)?;
				return Ok(())
			},
		}
	}

	#[cfg(feature = "runtime-benchmarks")]
	fn try_successful_origin(_key: &RuntimeParametersKey) -> Result<RuntimeOrigin, ()> {
		Ok(RuntimeOrigin::Root)
	}
}

[kianenigma]

I may have asked this before, but don't recall the answer? can we define one EnsureOrigin per param in this?

[ggwpez]

I may have asked this before, but don't recall the answer? can we define one EnsureOrigin per param in this?

Yes. I will extend docs for this.
It is using EnsureOriginWithArg<Self::RuntimeOrigin, KeyOf<Self>>, which can inspect the key to ensure that the permission is legit on a per-key basis. The call looks like T::AdminOrigin::ensure_origin(origin, &key).

[gupnik]

Should this be done under the experimental flag?

The downside of my cod is now that it does only support constant keys in the form of types, not value-bearing keys.

Could you add a UI Test to demonstrate the current behaviour in this case?

[xlc]

With my original code, it is simple to implement a struct GetXXX and impl Get<KeyType> for GetXXX to be compatible with existing pallets. You could have some macro to generate those as well.

[ggwpez]

With my original code, it is simple to implement a struct GetXXX and impl Get<KeyType> for GetXXX to be compatible with existing pallets. You could have some macro to generate those as well.

I am not sure if i get what you mean. The approach in the MR is compatible with old pallets, since the define_parameters macro now also does impl $crate::traits::__private::sp_core::Get<$value_type> for $key_name {.
What would impl Get<KeyType> for GetXXX enable us to do that is not possible with this MR?

[xlc]

What I am trying to say is you don’t need to rewrite orml-parameters to be compatible with existing pallets. I want to avoid two similar but incompatible pallet which will create a lot of maintenance overhead for no good reasons.

[ggwpez]

I looked at this and saw the change to the earnings module: https://github.com/AcalaNetwork/Acala/pull/2605/files#diff-0ce7b7c74aa0bd77a6df25d58b44ba9aa3f943e9f697ad9bba59651911935002

It got a new config param type ParameterStore: ParameterStore<Parameters>; which lead me to the assumption that pallets need to be retro-fitted for this. How can it be used without this @xlc ?

[xlc]

I will suggest updating pallets to fully utilize the features of parameters pallet such as parametrized keys but that's not necessary for simple use cases.

I already described how to do it in my previous comment. But let me elaborate it further using my PR as an example

• Move define_parameters from earning pallet to runtime
• Add a new struct implements Get<Option<Permill>> in the runtime

struct GetInstantUnstakeFee;
impl Get<Option<Permill>> for GetInstantUnstakeFee {
  fn get() -> Option<Permill> {
   ParameterStoreAdapter<Parameters, EarningParameters>::get(InstantUnstakeFee)
  }
}

• And type InstantUnstakeFee = GetInstantUnstakeFee;

It is easy to write a macro to generate the struct GetInstantUnstakeFee. Something similar to parameter_types

[xlc]

A piece of good code are like onions. They have many layers and we build new abstractions on top of each layer. If the most top layer is not suitable, we pick the lower layer instead. We identify the right level of abstraction for our use cases and reuse the code.

But please don't just throw it into a blender, and some spices that you want and give me a mash. It may work for you but unlikely for everyone.

[gupnik]

Here is my diff that adds support for the following syntax:

#[dynamic_params(RuntimeParameters)]
pub mod dynamic_params {
	use super::*;

	#[dynamic_pallet_params(crate::Parameters::<Runtime>, Parameters)]
	pub mod pallet1 {
		#[codec(index = 0)]
		pub static Key1: u64 = 0;
		#[codec(index = 1)]
		pub static Key2: u32 = 1;
		#[codec(index = 2)]
		pub static Key3: u128 = 2;
	}

	#[dynamic_pallet_params(crate::Parameters::<Runtime>, Parameters)]
	pub mod pallet2 {
		#[codec(index = 0)]
		pub static Key1: u64 = 0;
		#[codec(index = 1)]
		pub static Key2: u32 = 2;
		#[codec(index = 2)]
		pub static Key3: u128 = 4;
	}
}
pub use dynamic_params::*;

[xlc]

I figured that you guys have decided to ignore my suggestions and just going to do whatever you want?

[ggwpez]

I figured that you guys have decided to ignore my suggestions and just going to do whatever you want?

Pretty much. Your ideas and our need for a simple pallet seemed to not be compatible.

[xlc]

I already told you it is compatible 💁‍♂️ #2061 (comment)

[paritytech-cicd-pr]

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: cargo-clippy
Logs: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/5140095