add serialization support for dynamic_params macro and AggregateKey #9971
Conversation
| paste = { workspace = true } | ||
| scale-info = { features = ["derive"], workspace = true } | ||
| serde = { features = ["derive"], optional = true, workspace = true, default-features = true } | ||
| serde = { optional = true, workspace = true } |
There was a problem hiding this comment.
Problem is one of the runtime has dynamic_params with types that are not serializable. So, due to cargo's feature unification we will need to disable the support for dynamic_params serialization everywhere.
Because if we enable this feature in one runtime, all runtime will have pallet_parameters with serde enabled which will result in substrate runtime not compiling.
| #[pallet::genesis_config] | ||
| pub struct GenesisConfig<T: Config> { | ||
| /// Default runtime parameters | ||
| #[cfg(feature = "serde")] |
There was a problem hiding this comment.
Right now all the other pallets also have serde enabled all the time.
There was a problem hiding this comment.
Probably the best would be to disable GenesisConfig at all when serde is not enabled. But this needs to be done for every pallet.
There was a problem hiding this comment.
@bkchr We tried that unfortunately without changing the genesis config macro that is not possible. This is the least invasive way.
| #[cfg(feature = "serde")] | ||
| /// The overarching KV type of the parameters with serde serialization implemented | ||
| /// | ||
| /// Usually created by [`frame_support::dynamic_params`] or equivalent. | ||
| type RuntimeParameters: AggregatedKeyValue + serde::Serialize + serde::de::DeserializeOwned; |
There was a problem hiding this comment.
| #[cfg(feature = "serde")] | |
| /// The overarching KV type of the parameters with serde serialization implemented | |
| /// | |
| /// Usually created by [`frame_support::dynamic_params`] or equivalent. | |
| type RuntimeParameters: AggregatedKeyValue + serde::Serialize + serde::de::DeserializeOwned; |
| /// | ||
| /// Usually created by [`frame_support::dynamic_params`] or equivalent. | ||
| #[pallet::no_default_bounds] | ||
| type RuntimeParameters: AggregatedKeyValue; |
There was a problem hiding this comment.
| type RuntimeParameters: AggregatedKeyValue; | |
| type RuntimeParameters: AggregatedKeyValue + MaybeSerializeDeserialize; |
| #[pallet::no_default_bounds] | ||
| #[cfg(not(feature = "serde"))] | ||
| /// The overarching KV type of the parameters. | ||
| /// | ||
| /// Usually created by [`frame_support::dynamic_params`] or equivalent. |
There was a problem hiding this comment.
| #[pallet::no_default_bounds] | |
| #[cfg(not(feature = "serde"))] | |
| /// The overarching KV type of the parameters. | |
| /// | |
| /// Usually created by [`frame_support::dynamic_params`] or equivalent. | |
| /// The overarching KV type of the parameters. | |
| /// | |
| /// Usually created by [`frame_support::dynamic_params`] or equivalent. | |
| #[pallet::no_default_bounds] | |
| #[cfg(not(feature = "serde"))] |
yes, planning to add test as soon as somebody from parity validates the approach. |
2 participants
Description
This PR is attempt at #9714.
Review Notes
It adds conditional serialization support for
dynamic_paramsmacro by accepting a third argument as an expressionserializable = <bool>. The parameter is made optional to ensure backward compatibility.Apart from that, I have modified
pallet-parameterswith following changes:RuntimeParametersserializable are behindserdefeature.serdefeature inpallet-parameternow is refactored as following:a)
serdecrate is still optional but without default features and derive feature.b) the default features (
std) andderivefeatures are enabled ifstdfeature ofpallet-parameters.I have not modified any runtime to enable
pallet-parameter'sserdefeature. Since due to feature unification it will be applied for every runtime and the substrate runtime contains few parameters which does not implementSerialize.TODO:
Checklist
Trequired)