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

GenesisBuild split #14330

Closed
wants to merge 3 commits into from
Closed

GenesisBuild split #14330

wants to merge 3 commits into from

Conversation

michalkucharczyk
Copy link
Contributor

@michalkucharczyk michalkucharczyk commented Jun 9, 2023

This PR removes the storage building part from the GenesisBuild trait.

assimilate_storage and build_storage methods were moved to new GenesisBuildStorage trait which is implemented only for pallets GenesisConfig (and not RuntimeGenesisConfig). PalletGenesisConfig marker trait was added to facilitate this.

The rationale behind this is as follows: since #14306 GenesisBuild is implemented for RuntimeGenesisConfig. As the BuildStorage is also implemented for RuntimeGenesisConfig there was is duality now - RuntiemGenesisConfig had two bulid_storage implementations.

Trait split is implemented in 13d66fd, renaming in ad3b7df.

Step towards: paritytech/polkadot-sdk#25

cumulus companion: paritytech/cumulus#2720

@michalkucharczyk michalkucharczyk changed the title 'GenesisBuild` split GenesisBuild split Jun 9, 2023
@michalkucharczyk michalkucharczyk added A0-please_review Pull request needs code review. 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 Jun 9, 2023
@paritytech-cicd-pr
Copy link

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: test-linux-stable
Logs: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2961605

@michalkucharczyk michalkucharczyk marked this pull request as ready for review June 9, 2023 17:52
@michalkucharczyk michalkucharczyk requested review from a team June 9, 2023 17:52
@michalkucharczyk michalkucharczyk requested a review from a team June 9, 2023 17:52
#[cfg(feature = "std")]
/// A trait to define functions to build the storage for a genesis config, T and I are placeholder
/// for pallet trait and pallet instance.
pub trait GenesisBuildStorage<T, I = ()>: GenesisBuild<T, I> {
Copy link
Contributor

@gui1117 gui1117 Jun 11, 2023

Choose a reason for hiding this comment

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

I don't really understand, you but in another trait these method so you can implement GenesisBuild on RuntimeGenesisConfig ?

I'm not sure it worth it, why not create a new trait for RuntimeGenesisConfig specifically, GenesisBuild was meant for pallets and thus the signature is quite specific with <T, I>, I think it is better to keep traits and implementation of RuntimeGenesisConfig apart, no ?

Creating a new trait RuntimeGenesisBuild, maybe merged with BuildStorage should be better IMO

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry for my bad suggestion, I agree what other said in the other PR, maybe this PR improve on the situation with a unique proper GenesisBuild, we could eventually get rid of the generics once with have decl_storage removed (is it already ?) and we can keep the other method for not breaking peoples tests in another trait or another way

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. 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.

3 participants