Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

frame: GenesisBuild::build allowed in no_std #14107

Merged
merged 17 commits into from
May 25, 2023

Conversation

michalkucharczyk
Copy link
Contributor

@michalkucharczyk michalkucharczyk commented May 9, 2023

GenesisBuild::build function will be required for no_std in no native runtime world.

GenesisBuild::build macro generated function allows to build the runtime GenesisConfig assembled from all pallets' GenesisConfigs. build for runtime will be implemented in follow-up.

Step towards: paritytech/polkadot-sdk#25

polkadot companion: paritytech/polkadot#7271
cumulus companion: paritytech/cumulus#2624

i`GenesisBuild::build` function will be required for no_std in no native
runtime world.

`GenesisBuild::build` macro generated function allows to build the runtime
GenesisConfig assembled from all pallets' GenesisConfigs.
@michalkucharczyk michalkucharczyk requested a review from a team May 9, 2023 15:29
@michalkucharczyk michalkucharczyk added B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit labels May 9, 2023
@paritytech paritytech deleted a comment from paritytech-cicd-pr May 9, 2023
@paritytech paritytech deleted a comment from command-bot bot May 9, 2023
@paritytech paritytech deleted a comment from command-bot bot May 9, 2023
Copy link
Member

@bkchr bkchr left a comment

Choose a reason for hiding this comment

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

I'm not sure we should already do this? I mean we can probably get rid off this trait entirely when we just write all genesis values to the state and on the node side, after calling the runtime api, taking the OverlayedChanges to fetch all the state changes. Then we basically achieve the same as currently is being achieved by this trait.

@ggwpez
Copy link
Member

ggwpez commented May 9, 2023

Get rid of the trait?!

I know that we normally use it in tests to write a genesis config to storage.

@bkchr
Copy link
Member

bkchr commented May 9, 2023

I know that we normally use it in tests to write a genesis config to storage.

Good point, I forgot about tests! Still doesn't mean we need it in no_std :D

@michalkucharczyk
Copy link
Contributor Author

michalkucharczyk commented May 9, 2023

I'm not sure we should already do this? I mean we can probably get rid off this trait entirely when we just write all genesis values to the state and on the node side, after calling the runtime api, taking the OverlayedChanges to fetch all the state changes. Then we basically achieve the same as currently is being achieved by this trait.

We need a function that calls the build function of all the pallets in runtime, which in turn put the values into storage (see: https://github.com/paritytech/substrate/blob/c014d4a876b1b9db9affabfc19ba1707831c8c3f/frame/system/src/lib.rs#LL647C15-L647C15). And we need it in no_std.

So the idea is to use this generated function from new GenesisBuilder Runtime API.

The storage part can be removed later.

Copy link
Member

@bkchr bkchr left a comment

Choose a reason for hiding this comment

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

Good points @michalkucharczyk!

- #[cfg(feature = "std")] is not longer added to GenesisBuild implementation.
@michalkucharczyk
Copy link
Contributor Author

GenesisConfig::build function expects AccountId to be Display:

Ok(_) => {
panic!("Duplicate member in elections-phragmen genesis: {}", member)
},

What would be your preference to solve this?

Changing bounds of AccountId to Display seems to heavy for me:

type AccountId: Parameter
+ Member
+ MaybeSerializeDeserialize
+ Debug
+ MaybeDisplay
+ Ord
+ MaxEncodedLen;

@KiChjang
Copy link
Contributor

Huh, so MaybeDisplay is not working because we now are making sure that GenesisBuild is always Display regardless of whether the std feature is enabled or not. I think it is fine to replace MaybeDisplay with Display, since most AccountId types would most certainly have a Display impl (otherwise I don't know how they attempt to show an account ID via the logs).

@michalkucharczyk
Copy link
Contributor Author

michalkucharczyk commented May 10, 2023

Huh, so MaybeDisplay is not working because we now are making sure that GenesisBuild is always Display

Actually it is not about the GenesisBuild trait. It is only one, stand-alone case where panic! is used in GenesisBuild::build function.

Adding Display bound is ok, but it will pull all the (maybe unneccesarily) display staff into wasm runtime.

@bkchr
Copy link
Member

bkchr commented May 10, 2023

GenesisConfig::build function expects AccountId to be Display:

Ok(_) => {
panic!("Duplicate member in elections-phragmen genesis: {}", member)
},

What would be your preference to solve this?

Changing bounds of AccountId to Display seems to heavy for me:

type AccountId: Parameter
+ Member
+ MaybeSerializeDeserialize
+ Debug
+ MaybeDisplay
+ Ord
+ MaxEncodedLen;

Just use debug printing?

@KiChjang
Copy link
Contributor

Ah ok, yes, the panic! macro should really be using debug printing anyway.

@michalkucharczyk michalkucharczyk requested a review from a team May 10, 2023 13:56
@Polkadot-Forum
Copy link

This pull request has been mentioned on Polkadot Forum. There might be relevant details there:

https://forum.polkadot.network/t/genesisconfig-in-wasm-runtime-a-step-towards-a-non-native-future/2887/1

@KiChjang
Copy link
Contributor

As soon as CI is green, I believe this is ready to merge.

@michalkucharczyk
Copy link
Contributor Author

bot rebase

@paritytech-processbot
Copy link

Rebased

@michalkucharczyk
Copy link
Contributor Author

bot rebase

@paritytech-processbot
Copy link

Rebased

@michalkucharczyk
Copy link
Contributor Author

michalkucharczyk commented May 23, 2023

I've added requried companions, some pallets in cumulus and polkadot didn't implement Default in no_std.

@michalkucharczyk
Copy link
Contributor Author

bot rebase

@paritytech-processbot
Copy link

Rebased

@michalkucharczyk
Copy link
Contributor Author

bot rebase

@paritytech-processbot
Copy link

Rebased

@michalkucharczyk
Copy link
Contributor Author

bot merge

@paritytech-processbot paritytech-processbot bot merged commit f4cb21c into master May 25, 2023
@paritytech-processbot paritytech-processbot bot deleted the mku-frame-build-genesis-for-no-std branch May 25, 2023 21:49
Ank4n pushed a commit that referenced this pull request Jul 8, 2023
* frame: GenesisBuild::build allowed in no_std

i`GenesisBuild::build` function will be required for no_std in no native
runtime world.

`GenesisBuild::build` macro generated function allows to build the runtime
GenesisConfig assembled from all pallets' GenesisConfigs.

* fixes

* GenesisBuild::build avaiable in no-std

- #[cfg(feature = "std")] is not longer added to GenesisBuild implementation.

* system: hash69 available for no-std

* elections-phragmen: panic message fixed for no_std

* frame::suport: doc updated

* test-runtime: default for GenesisConfig

* frame::test-pallet: serde/std added to std feature deps

* Cargo.toml: deps sorted

* Cargo.lock update

cargo update -p frame-support-test-pallet -p frame-support-test

* frame ui tests: cleanup

---------

Co-authored-by: parity-processbot <>
nathanwhit pushed a commit to nathanwhit/substrate that referenced this pull request Jul 19, 2023
* frame: GenesisBuild::build allowed in no_std

i`GenesisBuild::build` function will be required for no_std in no native
runtime world.

`GenesisBuild::build` macro generated function allows to build the runtime
GenesisConfig assembled from all pallets' GenesisConfigs.

* fixes

* GenesisBuild::build avaiable in no-std

- #[cfg(feature = "std")] is not longer added to GenesisBuild implementation.

* system: hash69 available for no-std

* elections-phragmen: panic message fixed for no_std

* frame::suport: doc updated

* test-runtime: default for GenesisConfig

* frame::test-pallet: serde/std added to std feature deps

* Cargo.toml: deps sorted

* Cargo.lock update

cargo update -p frame-support-test-pallet -p frame-support-test

* frame ui tests: cleanup

---------

Co-authored-by: parity-processbot <>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants