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

[FRAME] pallet::dynamic_parameter attribute #3238

Open
Tracked by #4520
muharem opened this issue Feb 7, 2024 · 3 comments
Open
Tracked by #4520

[FRAME] pallet::dynamic_parameter attribute #3238

muharem opened this issue Feb 7, 2024 · 3 comments
Assignees

Comments

@muharem
Copy link
Contributor

muharem commented Feb 7, 2024

Problem

The application of the new dynamic parameters feature to pallet's config parameter will make it non-constant. Consequently, the #[pallet::constant] attribute will generate an incorrect metadata.

Possible Solution

Introduce a new attribute #[pallet::dynamic_parameter] (or #[pallet::dynamic_param] to match the dynamic_params module name). This attribute would include metadata containing a default value for the key and its location (storage key) in the storage.

@muharem muharem self-assigned this Feb 7, 2024
@ggwpez
Copy link
Member

ggwpez commented Feb 7, 2024

There are a few related issues to this #244 and #1139.
I still think #[pallet::variable] makes sense.

@bkchr
Copy link
Member

bkchr commented Feb 13, 2024

This attribute would include metadata containing a default value for the key and its location (storage key) in the storage.

This will require a new metadata version, but it is the correct solution. Maybe for an initial implementation we could use the custom metadata and then later put this into a new metadata version.

@muharem
Copy link
Contributor Author

muharem commented Jun 13, 2024

I have looked into the issue more closely, and below mu thoughts about it. Please share your thoughts.

Problem:

1. Pallet's Variable Parameter

The pallet's type/value parameter can be defined at the runtime level as either a constant value or dynamic. With the given configuration definition below, the metadata for such a parameter is correct in the first case. In the second case, it's incorrect because the value of the type can be either a static default value or overridden by its value in the storage (via parameters.set_parameter call).

// pallet_nis
trait Config {
  #[pallet::constant]
  type MinBid: Get<Balance>;
}

2. Runtime's Variable Parameter

In the example below, there is no way for the client to look up the value of BaseDeposit and ByteDeposit. They might carry the static default value, or they could be overridden by the value in the storage.

impl pallet_preimage::Config for Runtime {
  // ...     
  type Consideration = HoldConsideration<
    AccountId,
    Balances,
    PreimageHoldReason,
    LinearStoragePrice<
	dynamic_params::preimage::BaseDeposit,
	dynamic_params::preimage::ByteDeposit,
	Balance,
    >,
  >;
}

Possible Solutions

Before reading the following proposal, please check the appendix below to see the current types definitions and layout of the runtime's metadata.

Solution Without Changes to the Parameter Pallet

We can take the constant metadata as a base for the variable metadata layout and add an additional storage item as in the example below. The key and value types are known from the storage definition.

The problem with this solution is that a client will get the value of the RuntimeParametersValue type and will need a way to extract its inner value (Balance of MinBid as in the example below). This can only be done in a static way.

# Variable parameter item within Nis pallet metadata object
# formatted.pallets[i].variables[j]
{
  "name": "MinBid",
  "type": "6",
  "value": "0x00407a10f35a00000000000000000000",
  "storage": {
    "pallet_index": 6, # Parameters pallet index
    "name": "Parameters", # Storage item name within Parameters pallet instance
    "key": "...", # Encoded key of RuntimeParametersKey, known from the storage definition 
  }
  "docs": [
    " The minimum amount of funds that may be placed in a bid. Note that this",
  ]
},

Such a variable type parameter has to implement a trait to provide the storage information.

trait Config {
  #[pallet::variable]
  type MinBid: Get<Balance> + MaybeStorageRef;
}

The none implementation of the new trait can be provided by the parameter_types macro and some implementation by the dynamic_params macro.

Problem (2) can be solved with the same variables metadata at the root/runtime level.

Solution With Opaque Storage Value

The main problem with the above solution is that the inner value of the RuntimeParametersValue cannot be obtained dynamically; the clients must know the RuntimeParametersValue type and how to extract a particular value from it.

This can be solved by dropping the RuntimeParametersValue type and storing the values as a byte array. The storage key type may include information about the actual stored type. The RuntimeParameters can remain as is to maintain a type based API for the set_parameter call.

In this case the metadata object can have only additional key hash.

# Variable parameter item within Nis pallet metadata object
# formatted.pallets[i].variables[j]
{
  "name": "MinBid",
  "type": "6",
  "value": "0x00407a10f35a00000000000000000000",
  "key": "0xKey_Hash", # hash of the full key path including the pallet prefix, storage prefix, and the key of the value.
  "docs": [
    " The minimum amount of funds that may be placed in a bid. Note that this",
  ]
},

The downside is that the storage value (byte array) will need to have a maximum length.

Conclusion

The dynamic parameters solution and possible metadata solution are quite complex. The first generates complex types for the keys and values which clients must know to read the values. The metadata solution requires not only code generation for the pallet but also for the additional trait bound to parameters (within parameter_types and dynamic_params). I believe we need to simplify the solution overall as much as possible.

For problem (2), I need more input from the client developers. The problem might need a different solution, such as an API call returning the final deposit amount for a given footprint.

Other than dropping the value type for the parameters storage, I would give up the namespacing for dynamic parameters (e.g., RuntimeParameters::Nis::MinBid) and keep it flat (e.g., RuntimeParameters::NisMinBid), similar to the parameters we define with parameter_types at the runtime level (though I know this will make it incompatible with ORML, but I would suggest making this change to ORML too).

Appendix

Constant metadata from the Rococo Relay Chain

# Constant parameter item within Nis pallet metadata object
# formatted.pallets[i].constants[j]
{
  "name": "MinBid",
  "type": "6",
  "value": "0x00407a10f35a00000000000000000000",
  "docs": [
    " The minimum amount of funds that may be placed in a bid. Note that this",
  ]
},

Storage metadata from the Rococo Relay Chain

# Storage item within `Parameters` pallet metadata object
# formatted.pallets[i].storage
"storage": {
  "prefix": "Parameters",
  "items": [
    {
      "name": "Parameters",
      "modifier": "Optional",
      "type": {
        "Map": {
          "hashers": [
            "Blake2_128Concat"
          ],
          "key": "35", # RuntimeParametersKey 
          "value": "43" # RuntimeParametersValue
        }
      },
      "fallback": "0x00",
      "docs": [
        " Stored parameters."
      ]
    }
  ]
},

Dynamic parameters of Rococo Relay Runtime

// Used as a parameter to `Parameters::set_parameter` call
pub enum RuntimeParameters {
	Nis(dynamic_params::nis::Parameters),
	Preimage(dynamic_params::preimage::Parameters),
}
// Used as a storage key to store a parameter value
pub enum RuntimeParametersKey {
    Nis(dynamic_params::nis::ParametersKey),
    Preimage(dynamic_params::nis::ParametersKey),
}
// Used as a storage value wrapper type to store an actual parameter value
pub enum RuntimeParametersValue {
    Nis(dynamic_params::nis::ParametersValue),
    Preimage(dynamic_params::nis::ParametersValue),
}
// Converts the call's `RuntimeParameters` parameter to key and value persisted in the storage, 
// `RuntimeParametersKey` and the `RuntimeParametersValue`
impl frame_support::traits::dynamic_params::AggregatedKeyValue for RuntimeParameters {
	type Key = RuntimeParametersKey;
	type Value = RuntimeParametersValue;
	fn into_parts(self) -> (Self::Key, Option<Self::Value>) {
		match self {
			RuntimeParameters::Nis(parameter) => {
				let (key, value) = parameter.into_parts();
				(RuntimeParametersKey::Nis(key), value.map(RuntimeParametersValue::Nis))
			},
			RuntimeParameters::Preimage(parameter) => {
				let (key, value) = parameter.into_parts();
				(RuntimeParametersKey::Preimage(key), value.map(RuntimeParametersValue::Preimage))
			},
		}
	}
}

Dynamic parameters of nis pallet instance from Rococo Relay Runtime

// namespace dynamic_params::nis;

// Used as an inner type of the parameter to `Parameters::set_parameter` call
pub enum Parameters {
        // inner `Target` is a key, `Perquintill` is a value 
	Target(Target, Option<Perquintill>),
	MinBid(MinBid, Option<Balance>),
}
// Storage key 
pub struct MinBid;
// Pallet namespace storage key 
pub enum ParametersKey {
	Target(Target),
	MinBid(MinBid),
}
// Pallet namespace storage value 
pub enum ParametersValue {
	Target(Perquintill),
	// `Balance` is the actual value we care about
	MinBid(Balance),
}
// Converts the `Parameters` into the `ParametersKey` and the `ParametersValue`
impl frame_support::traits::dynamic_params::AggregatedKeyValue for Parameters {
	type Key = ParametersKey;
	type Value = ParametersValue;
	fn into_parts(self) -> (Self::Key, Option<Self::Value>) {
		match self {
			Parameters::Target(key, value) =>
				(ParametersKey::Target(key), value.map(ParametersValue::Target)),
			Parameters::MinBid(key, value) =>
				(ParametersKey::MinBid(key), value.map(ParametersValue::MinBid)),
		}
	}
}

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

No branches or pull requests

3 participants