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

asset-hub-rococo: genesis config presets added #3996

Merged
merged 14 commits into from
Aug 30, 2024

Conversation

michalkucharczyk
Copy link
Contributor

Gensis config presets moved from polkadot-parachain binary into asset-hub-rococo runtime.

relates to: #3944

@michalkucharczyk michalkucharczyk added R0-silent Changes should not be mentioned in any release notes T14-system_parachains This PR/Issue is related to system parachains. labels Apr 5, 2024
@michalkucharczyk michalkucharczyk marked this pull request as ready for review June 8, 2024 13:42
@michalkucharczyk
Copy link
Contributor Author

Do you think we can merge it?

@michalkucharczyk michalkucharczyk requested a review from a team June 8, 2024 13:45
@bkontur
Copy link
Contributor

bkontur commented Jun 10, 2024

cool, I would need this for other testnet runtimes also: #4405

@michalkucharczyk michalkucharczyk requested a review from bkontur June 10, 2024 09:48
Copy link
Contributor

@seadanda seadanda left a comment

Choose a reason for hiding this comment

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

Just one query about naming, but looks like a nice change.

pub mod preset_names {
pub const PRESET_DEVELOPMENT: &str = "development";
pub const PRESET_LOCAL: &str = "local";
pub const PRESET_GENESIS: &str = "genesis";
Copy link
Contributor

Choose a reason for hiding this comment

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

This has previously been called "Live", any reason for the change to genesis?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@seadanda Thanks for spotting this. I took the name from here (and I know it is far from being perfect) :

"asset-hub-rococo-genesis" =>
Box::new(chain_spec::asset_hubs::asset_hub_rococo_genesis_config()),

But I am open to any other name. Should I call it "live"?
I am not sure what is purpose of this particular config.

ps. I did not notice Live name anywhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
pub const PRESET_GENESIS: &str = "genesis";
pub const PRESET_GENESIS: &str = "live";

?

Copy link
Member

Choose a reason for hiding this comment

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

Not sure we need this at all? live and genesis are really not that expressive names...

Copy link
Member

Choose a reason for hiding this comment

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

I mean I assume it is for generating the gensis of the live networks, but yeah we probably don't need it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Live is what I had seen along with Local and Development for the ChainTypes

I also am not too sure what that specific config is for. Is that the only place this preset will be used? Maybe my reasoning for live is actually wrong then here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually preset names here are simply reflecting the names of predefined chain-specs exported by polkadot-parachain binary which are:

"asset-hub-rococo-dev"
"asset-hub-rococo-local"
"asset-hub-rococo-genesis"

see code here:
https://github.com/paritytech/polkadot-sdk/blob/09035a7d5d14fc3f2df3db304cd0fcc8fc9ed27b/cumulus/polkadot-parachain/src/chain_spec/mod.rs#L117C1-L120C90

This one: "asset-hub-rococo-genesis", actually is a ChainType::Live, so I can rename it. But it is also not very informative name. Not sure what is the purpose of each chain-spec flavor.

Copy link
Contributor Author

@michalkucharczyk michalkucharczyk Aug 30, 2024

Choose a reason for hiding this comment

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

For now I propose to keep it for compatibility reasons, we can always remove them in future.
I also would prefer the keep "genesis" name to allow easy mapping to corresponding chain-spec name,

At some point we will be removing predefined (e.g. "asset-hub-rococo-dev|local|genesis") chain spec flavors from the omni-node. We could remove unused presets at the same time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kianenigma what are your thoughts on this?

cumulus/parachains/common/src/genesis_config_helpers.rs Outdated Show resolved Hide resolved
Co-authored-by: Dónal Murray <donal.murray@parity.io>
cumulus/parachains/common/src/genesis_config_helpers.rs Outdated Show resolved Hide resolved
pub mod preset_names {
pub const PRESET_DEVELOPMENT: &str = "development";
pub const PRESET_LOCAL: &str = "local";
pub const PRESET_GENESIS: &str = "genesis";
Copy link
Member

Choose a reason for hiding this comment

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

Not sure we need this at all? live and genesis are really not that expressive names...

Comment on lines +42 to +43
serde_json::json!({
"balances": crate::BalancesConfig {
Copy link
Member

Choose a reason for hiding this comment

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

Why are we not using the concrete GenesisConfig type here? And then transform this into a json?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A preset is meant to be patch. This is the only reason.

If GenesisConfig struct would be used here, then the preset would contain all the fields of the genesis config. That would be less convenient / a little harder to read when displaying presets with some external tools (e.g. chain-spec-builder).

pub mod preset_names {
pub const PRESET_DEVELOPMENT: &str = "development";
pub const PRESET_LOCAL: &str = "local";
pub const PRESET_GENESIS: &str = "genesis";
Copy link
Member

Choose a reason for hiding this comment

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

I mean I assume it is for generating the gensis of the live networks, but yeah we probably don't need it.

@bkchr
Copy link
Member

bkchr commented Jun 19, 2024

Left some nitpicks. Generally looking good. Good work 👍

@paritytech-cicd-pr
Copy link

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

Copy link
Contributor

@bkontur bkontur left a comment

Choose a reason for hiding this comment

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

looks good, just one nit,
so I will adjust and continue with others here: #5327


/// Generate the session keys from individual elements.
///
/// The input must be a tuple of individual keys (a single arg for now since we have just one key).
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure about this comment and naming, why exactly will this change to a tuple?

Copy link
Contributor Author

@michalkucharczyk michalkucharczyk Aug 30, 2024

Choose a reason for hiding this comment

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

@michalkucharczyk michalkucharczyk added this pull request to the merge queue Aug 30, 2024
Merged via the queue into master with commit 95f3977 Aug 30, 2024
184 of 186 checks passed
@michalkucharczyk michalkucharczyk deleted the mku-asset-hub-rococo-preset branch August 30, 2024 16:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
R0-silent Changes should not be mentioned in any release notes T14-system_parachains This PR/Issue is related to system parachains.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants