[FRAME] pallet::dynamic_parameter attribute

[muharem]

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.

[ggwpez]

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

[bkchr]

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]

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)),
		}
	}
}

[muharem]

@bkchr @xlc @kianenigma @ggwpez any feedback for the proposals above?

[kianenigma]

I strongly think that if something is not static, it should not be in the metadata.

In that spirit, I like the idea of mimicking the opaque parameter as a storage value better, but I cannot resonate a lot with the solution. It is elegant and well-thought-out, but I think it is tackling the wrong problem.

Looking at the linked issues, you can see that I have always been against #[pallet::constant] in the way that it is. In that, the fact that it puts something into the metadata, but it has no check to ensure it remains static (despite its misleading name) was always wrong to me.

The underlying problem there is that you are letting a pallet decide if something is static or not, while it physically has no way to do that. The runtime knows what sits on the other side of :Get<_> is dynamic or not, and therefore the runtime should decide if something should make it to the metadata or not.

---

Looking at the situation from first principles, I argue that the most important reason to put something into the metadata is to help (offline) signers create correct transactions.

1. This involves foremost transactions, and their parameter types.
2. Then, we also added the layout of the chain storage into the metadata. This doesn't ever change in the lifecycle of the runtime, and therefore it is a great optimization to put it in the metadata, allowing a client to query the metadata once and not go back and forth between client and runtime.
3. But then, at some point, we started adding some "parameters" of pallets/runtime to the metadata as well, such as your example of MinBid. This is when api.consts in PJS was introduced.
4. And then we messed it all up by not mandating these parameters to actually be static :))

This is the point we went wrong. I would go back and fix that, instead of fixing the status quo.

The truth is, a "parameter that is not static" is meaningless in the metadata, because you cannot get it from the metadata anyways. You have to make another round trip to the runtime, which means it should have been a separate runtime API to begin with.

Storage is the right example of a dynamic value. Why don't we put storage values in the metadata too (excluding the size limits)? Because it is dynamic. What we do instead is we put the directory of what storage items exist in the metadata, but to read the value you have to make another API call.

Same should be applied to the parameters that are not 100% known to be static: The directory/list of parameters can be in the metadata, but the values should not be.

So all in all, my thoughts for now are along the lines of:

1. #[pallet::constant] is meant for ONLY parameters that are actually constant. So this issue is invalid, and it is up to the developers to ensure whatever is linked to #[pallet::constant] is actually constant. This is a bad design because of what I said above (how can a pallet know if a :Get<_> is constant?), but it is what it is for now.
2. The list of dynamic params should be discover-able in the metadata, but the values don't matter.
3. For dynamic params, ack that another round trip is needed to the runtime, and build new runtime APIs.

Mocking dynamic parameters as storage is the closest to this.

TLDR; Metadata is being misused; we should put fewer things in the metadata, and this issue would also be non-existent.

Since PAPI is not stable yet, it is a good time to revamp the paradigm here, if we agree with my current opinion that the current design is flawed.

[jsdw]

Just responding from a metadata consumers perspective:

#[pallet::constant] is meant for ONLY parameters that are actually constant. So this issue is invalid, and it is up to the developers to ensure whatever is linked to #[pallet::constant] is actually constant. This is a bad design because of what I said above (how can a pallet know if a :Get<_> is constant?), but it is what it is for now.

Agreed; constants are written into the metadata ATM and so we have always assumed that they cannot change between runtime updates.

(I would probably not be against constants being removed as a concept and having to go via runtime APIs to access these values where needed, in the general spirit of Runtime APIs providing some abstraction over getting values)

Mocking dynamic parameters as storage is the closest to this.

From what I understand, this would be my preference*. It feels wrong from a client pov to add another abstraction that sits alongside storage, constants and runtime APIs for accessing values that might change.

(* But equally, I would be happy if we focused on adding runtime APIs to expose whatever parameters we wanted to give people access to, again in the spirit of Runtime APIs abstracting over getting values)

TLDR; Metadata is being misused; we should put fewer things in the metadata, and this issue would also be non-existent.

From my POV, metadata is mostly about providing two things:

1. Information needed to construct transactions to set the chain state (enough to codegen the relevant interfaces)
2. Information needed to get the chain state (enough to codegen the relevant interfaces)

I could see a future where we deprecate storage accesses (and maybe constants), and then metadata contains transaction stuff for setting and runtime APIs or similar for getting.

Finally; if we want anything new in metadata, the current tracking issue for V16 metadata is here, and in an ideal world we'll have it stabilised by the end of the year.

[xlc]

I am a bit confused about the whole discussion thread. If something is no longer constant, we just remove the constant attribute and that's it?

[muharem]

@kianenigma thank you for the perspective, I agree with it. Can you please explain what you mean with -

Mocking dynamic parameters as storage is the closest to this.

[muharem]

I am a bit confused about the whole discussion thread. If something is no longer constant, we just remove the constant attribute and that's it?

The idea was to introduce an alternative attribute for the constant to use it for dynamic parameters and include the information into the metadata. But after Kian's comment I am rethinking it.

[xlc]

Yeah you are basically describing storages. I don't think we have a real problem to address here.

The way I designed the parameter pallet is that having the Key type to be an aggregated type and with the metadata type system, the SDK can just read the possible key variants from the type and query the storages. No need to have any more metadata magic as everything is already encoded in the types.

[kianenigma]

@kianenigma thank you for the perspective, I agree with it. Can you please explain what you mean with -

Mocking dynamic parameters as storage is the closest to this.

As in a dynamic parameter is very similar to storage: you just need enough information in the metadata to read the possibilities (aka possible key prefixes), and never put individual keys/values for those possibilities in the metadata as it would be too much + it would change.

I am glad we re-learned again: Anything going into the metadata MUST BE CONSTANT, period.

In that spirit, the only action items I see @muharem:

1. note the above in https://paritytech.github.io/polkadot-sdk/master/polkadot_sdk_docs/reference_docs/metadata/index.html
2. ask the question: is enough data already in the metadata for an app to read possible parameter keys (not values)? if yes, the close this, if no, provide exactly that and no more.

[muharem]

There should be enough data for a client to iterate over the parameters storage item and read all parameters in the storage, and even decode them. But while the parameter is not in the storage and only has it's statically defined default value, client can discover it only in the runtime code base. I wanna still look into how to address it, but cannot plan it now.

[xlc]

the keys are encoded as enum and all the info are included in metadata. there is no additional work needed

[muharem]

@xlc how a client get a default/initial value of a parameter? a value that is not stored in the storage

[xlc]

someone need to double check but I think the metadata already includes a encoded default value
besides, if a more complicated logic is needed for default values, we should use runtime API or something else and that doesn’t have anything to do with metadata

[muharem]

@xlc I will double check

[jsdw]

^ Where storage entries have default values, those values are indeed encoded in the metadata (see https://github.com/paritytech/frame-metadata/blob/f81f8477af8d764ea114dc8a61690c7d84eb5662/frame-metadata/src/v14.rs#L218)

[muharem]

^ Where storage entries have default values, those values are indeed encoded in the metadata (see https://github.com/paritytech/frame-metadata/blob/f81f8477af8d764ea114dc8a61690c7d84eb5662/frame-metadata/src/v14.rs#L218)

This is a default value for the storage item. For Parameters storage item it has to be Optional since we cannot have a single default value for different key values. The regular types do not have default values in metadata.

We could introduce a default values for all general types and have a wrapper type for parameter value to have it's default value. Clients will still have to check the storage to make sure the default value is not overloaded. This looks a bit complex to me.

We can introduce a simple general runtime API.

// returns default value or the value from the storage if some
trait Parameters { 
  fn get_parameter(key: RuntimeParametersKey) -> RuntimeParametersValue;
  fn get_parameters() -> Vec<(RuntimeParametersKey, RuntimeParametersValue);
}

These types might be not nice to work with, since they are quite complex composite types. But clients will have enough data to work with them.

It would be nice if they also will return a expire_at so the clients can cache the values. The Parameters pallet will have to support it. This is nice to have.

trait Parameters { 
  fn get_parameter(key: RuntimeParametersKey) -> (expire_at, RuntimeParametersValue);
  fn get_parameters() -> (expire_at, Vec<(RuntimeParametersKey, RuntimeParametersValue));
}

Refer to the Appendix of this comment to see the types: #3238 (comment)

[xlc]

Maybe we need a way to specify default values (not 100% convinced as it is never a requirement at Acala).

For expire at, I don't really think it is needed. What are the use cases? The client should subscript to the storage anyway and get notified when it changes. The only place it can be helpful is to persist data across restart/session is it really something people needs? The UI should be able to batch fetch storage so the overhead is minimal to fetch few more parameters.

[muharem]

Maybe we need a way to specify default values (not 100% convinced as it is never a requirement at Acala).

how do you see it? in metadata? since ParametersValue is a variant (e.g. ParametersValue::MinBid(Balance)), we will need an additional wrapper type around types like Balance and Perquintill (e.g. ParametersValue::MinBid(MinBidValue)). Plus all types in metadata get a new default attribute.

[xlc]

I haven't paid too much attention to the implementation of the scale-info but in theory, it can represent type like GetU32<1000> so we can encode default values in type system. That may or may not be enough but someone need to provide some solid use cases for people to evaluate.

[muharem]

@xlc right, the GetU32<1000> would be a wrapper type in such a case, with some unique name like GetU32_Default1000. But that wont be enough, we will still need an additional wrapper types for things like Perquintill (e.g. Perquintill_Default80 getter).

// Pallet namespace storage value 
pub enum ParametersValue {
	Target(Perquintill_Default80),
	// `Balance` is the actual value we care about
	MinBid(Balance_Default_10000),
}

We need to have some conclusion here. I think we have two options now, (1) runtime API and (2) default property for types in metadata + default wrapper types.

@xlc @kianenigma @jsdw what do you think?

[xlc]

I will say (2) requires less work and more isolated (build a generic default value via type system) which can be useful in other places (e.g. replace the current default value mechanism in storage metadata).