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

parachain-template: genesis config presets added #4739

Merged
merged 10 commits into from
Aug 30, 2024

Conversation

michalkucharczyk
Copy link
Contributor

Gensis config presets moved from parachain-template-node binary into parachain-template-runtime runtime.

cc: @PierreBesson

@michalkucharczyk michalkucharczyk requested review from kianenigma and a team June 8, 2024 09:36
@michalkucharczyk michalkucharczyk added R0-silent Changes should not be mentioned in any release notes T11-documentation This PR/Issue is related to documentation. labels Jun 8, 2024
Copy link
Contributor

@kianenigma kianenigma left a comment

Choose a reason for hiding this comment

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

Would we make different choices if we know we want this code in all 3 runtimes? maybe reusing parts of it?

We need indeed want this code to also be in the solochain and minimal runtime as well :)

pub const PRESET_LOCAL_TESTNET: &str = "local_testnet";

/// Preset configuration name for a development environment.
pub const PRESET_DEVELOPMENT: &str = "development";
Copy link
Contributor

Choose a reason for hiding this comment

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

One annoying little detail about this is that if you don't put chian_type: Dev in the chain-spec, PJS apps won't show the accounts to you. There is a CLI flag for that two, but seem annoying to repeat development many times.

./chain-spec-builder --chain-type development --preset development etc etc 

We either create a ticket in PJS apps to change this (prob better), or we think about how we can improve it on our side. Maybe a chain-spec-builder --dev that does both? combined with running the chain in --dev it sounds like a good dev ex to me.

# compile wasm
# ./chain-spec-builder --runtime wasm --dev > spec.json
# ./omni-node --chain spec.json --dev

cc @gupnik @shawntabrizi

Copy link
Contributor Author

@michalkucharczyk michalkucharczyk Jun 10, 2024

Choose a reason for hiding this comment

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

chain-spec-builder --dev

This would assume that presets' naming is somehow standardized. Every runtime shall provide something in order to make flags in some other tool work correctly. This is probably OK, question is we want to go in that direction.

Copy link
Contributor

Choose a reason for hiding this comment

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

To me it sounds reasonable that these are separate flags. You could have multiple runtime presets for development, all of them with chainType "Dev".

I was thinking how one could do the pjs route. I think currently pjs calls the system_chainType RPC which gets its data directly from the chain spec. If we don't want to use the chain-spec property for this, we would need another easy way to tell that we are a dev chain.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can keep separate flags, --dev can be convenient shortcut.

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, let's keep all things configurable in the super-user mode, but one --dev that makes some assumptions is a good nice to have.

@michalkucharczyk
Copy link
Contributor Author

michalkucharczyk commented Jun 10, 2024

Would we make different choices if we know we want this code in all 3 runtimes? maybe reusing parts of it?

We need indeed want this code to also be in the solochain and minimal runtime as well :)

To which part are you referring to?
I think this is the part that will be duplicated:

const SAFE_XCM_VERSION: u32 = xcm::prelude::XCM_VERSION;
pub fn get_from_seed<TPublic: Public>(seed: &str) -> <TPublic::Pair as Pair>::Public {
pub fn get_collator_keys_from_seed(seed: &str) -> AuraId {
pub fn get_account_id_from_seed<TPublic: Public>(seed: &str) -> AccountId
pub fn template_session_keys(keys: AuraId) -> SessionKeys {

This could be moved to common crate, but at the moment template runtimes are not having common base (e.g. every runtime type is different). solochain does not need collator.
Maybe those functions are not really needed and all the key generation could be simplified. I did not look at this yet.

This could be done in some follow-up step.

@PierreBesson
Copy link

We are eager to have this merged to improve the parachain developer experience. This will allow customizing chainspecs for launching networks in a generic and easy way without requiring the node binary just the chain-spec-builder + runtime file.

@michalkucharczyk
Copy link
Contributor Author

@skunert @kianenigma what is your opinion - could we merge it and improve it further in follow-up PRs?

@paritytech-cicd-pr
Copy link

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: cargo-clippy
Logs: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/6796229

Copy link
Contributor

@skunert skunert left a comment

Choose a reason for hiding this comment

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

From my side looks good, I think we can merge this.

@kianenigma
Copy link
Contributor

Oh, I also did something similar to this as a part of #5117, and in fact wanted to extract it into a separate PR!

But great to see it already exist, review will follow now.

),
],
vec![
get_account_id_from_seed::<sr25519::Public>("Alice"),
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah i have seen this crap in a lot of places being copy pasted 🙈

Similarly, I have what I think is a better way in #5117:

  1. sp_keyring::Keyring::iter()
  2. re-export it from frame as well for better discover-ability.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like it. Not sure however, if Keyring actually shall go to frame?
Let's do this in the followup - there will following work for solochain and minimal.

Some common parts need to be extracted, probably the accounts in presets can be unified somehow.
(also see #4739 (comment) which needs to be addressed).

Copy link
Contributor

Choose a reason for hiding this comment

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

@iulianbarbu this is a pretty easy one for you to pick up :)

Copy link
Contributor

Choose a reason for hiding this comment

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

@kianenigma I am a bit late to the party. Based on the context on this thread I get that we want to do something like sp_keyring::Keyring::Alice.to_account_id() for the specific line where the thread started, and in general, for creating vec's we want to create them based on sp_keyring::Keyring::iter().

I do not get the context around re-export it from 'frame' as well for better discover-ability. Maybe we can sync offline (to not pollute anymore this merged PR) and maybe open an issue (if there isn't one) which clarifies next steps.

Copy link
Contributor Author

@michalkucharczyk michalkucharczyk Sep 13, 2024

Choose a reason for hiding this comment

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

It is about exporting that from the frame main library:

pub use sp_keyring::AccountKeyring;

Which was initially proposed by @kianenigma in this (not merged) #5117 PR.

Copy link
Contributor Author

@michalkucharczyk michalkucharczyk Sep 13, 2024

Choose a reason for hiding this comment

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

re-export it from 'frame' as well for better discover-ability basically means that you don't need extra use / crate dep (sp_keyring) when defining presets, it is just imported by frame::runtime.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see now. Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

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

I am trying re-export sp-keyring from frame in #5899, @kianenigma are you doing the similar work?

Copy link
Contributor

Choose a reason for hiding this comment

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

No, I am not doing a similar thing.

Copy link
Contributor

@kianenigma kianenigma 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 have a few suggestions

I think a cool and bold next step in the omni direction would be to actually feature gate the entire node in our templates, and if you run it, show you a message that would show you instead how you should run your chain with chopsticks + polkadot-parachain + chain-spec-builder + frame-omni-bencher.

I've been meaning to write some kind of a guide about how to mix the above 4 into a project and use them, but haven't managed to get it together. Perhaps it is finally time.

@kianenigma kianenigma added the T17-Templates This PR/Issue is related to templates label Aug 5, 2024
@michalkucharczyk michalkucharczyk added this pull request to the merge queue Aug 30, 2024
Merged via the queue into master with commit 562870d Aug 30, 2024
184 of 185 checks passed
@michalkucharczyk michalkucharczyk deleted the mku-presets-for-templates-parachain branch August 30, 2024 20:18
github-merge-queue bot pushed a commit that referenced this pull request Oct 5, 2024
# Description

Closes [#5790](#5790).
Useful for starting nodes based on minimal/solochain when doing
development or for testing omni node with less happy code paths. It is
reusing the presets defined for the nodes chain specs.

## Integration

Specifically useful for development/testing if generating chain-specs
for `minimal` or `solochain` runtimes from `templates` directories.

## Review Notes

Added `genesis_config_presets` modules for both minimal/solochain. I
reused the presets defined in each node `chain_spec` module
correspondingly.

### PRDOC

Not sure who uses templates, maybe node devs and runtime devs at start
of their learning journey, but happy to get some guidance on how to
write the prdoc if needed.

### Thinking out loud

I saw concerns around sharing functionality for such genesis config
presets between the template chains. I think there might be a case for
doing that, on the lines of this comment:
#4739 (comment).
I would add that `parachains-common::genesis_config_heleper` contains a
few methods from those mentioned, but I am unsure if using it as a
dependency for templates is correct. Feels like the comment suggests
there should be a `commons` crate concerning just `templates`, which I
agree with to some degree, if we assume `cumulus` needs might be driven
in certain directions that are not relevant to `templates` and vice
versa. However I am not so certain about this, so would welcome some
thoughts, since I am seeing `parachains-common` being used already in a
few runtime implementations:
https://crates.io/crates/parachains-common/reverse_dependencies?page=3,
so might be a good candidate already for the `common` logic.

---------

Signed-off-by: Iulian Barbu <iulian.barbu@parity.io>
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 T11-documentation This PR/Issue is related to documentation. T17-Templates This PR/Issue is related to templates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants