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

fix: export-genesis-state command #1521

Merged
merged 3 commits into from
Sep 17, 2023
Merged

Conversation

Moliholy
Copy link
Contributor

Description

Currently the ExportGenesisState command in polkadot parachain uses an asynchronous context to run, which seems to display some warnings. See the screenshot below:

Screenshot 2023-09-12 at 17 12 15

After the changes in this PR, which essentially runs the command in a synchronous context, the command works properly without any warning.

Screenshot 2023-09-12 at 18 23 46

The remaining runtimes were added to construct_benchmark_partials macro in order not to fail if the runtime was not included in the non-exhaustive initial list, similarly to the construct_async_run one.

For completeness: tests were made following this tutorial.

@Moliholy Moliholy added the T0-node This PR/Issue is related to the topic “node”. label Sep 12, 2023
Some(Subcommand::ExportGenesisState(cmd)) => {
let runner = cli.create_runner(cmd)?;
runner.sync_run(|config| {
construct_benchmark_partials!(config, |partials| cmd.run(&*config.chain_spec, &*partials.client))
Copy link
Member

Choose a reason for hiding this comment

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

The macro should then be renamed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What about simply construct_partials?

If not, any suggestion?

On a side note: I've seen construct_benchmark_partials in several parachains and templates. If we are now to change the name (and I definitely agree with that, since I found it misleading in all cases I saw) we should take into account it will tend to be imitated elsewhere.

Copy link
Member

Choose a reason for hiding this comment

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

What about simply construct_partials?

Sounds good.

we should take into account it will tend to be imitated elsewhere.

People copying our code/name is not really something that we should take into account here. This isn't any public interface.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good.

Done in commit 3a10545.

People copying our code/name is not really something that we should take into account here. This isn't any public interface.

Understood. Thanks for clarifying.

@Moliholy Moliholy requested a review from a team September 14, 2023 13:58
@bkchr bkchr merged commit 2ca30ed into master Sep 17, 2023
7 checks passed
@bkchr bkchr deleted the fix/export-genesis-state-cmd branch September 17, 2023 22:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T0-node This PR/Issue is related to the topic “node”.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants