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

Add parachain related parameters to chain-spec-builder #4889

Merged

Conversation

CrackTheCode016
Copy link
Contributor

@CrackTheCode016 CrackTheCode016 commented Jun 26, 2024

When using with polkadot-parachain, you usually need to specify the relay_chain and para_id fields in the chain spec.

With this PR it can be achieved by specifying newly added --para-id and --relay-chain command line args, e.g:

chain-spec-builder create -r _runtime.wasm  --para-id 100 --relay-chain xxx default

This was implemented by simple json blobs merging.

Additionally unit tests covering basic functionality were added.

Also adds a fix for not overwriting the chain spec with the default config each time, swallowing not standard fields is also fixed.

Fixes: #4873

@bkchr
Copy link
Member

bkchr commented Jun 26, 2024

Generally we should start with this: #4873

Maybe having like a separate command to add this could be useful. I mean I'm not a 100% fan of it, but it will make it easier for the users.

@CrackTheCode016
Copy link
Contributor Author

Yeah that makes sense. I can either include a fix for that issue in this PR (left a comment on how to potentially fix it here), or push another PR to address it

@bkchr
Copy link
Member

bkchr commented Jun 27, 2024

If you like, you can push it directly here.

@bkchr
Copy link
Member

bkchr commented Jul 3, 2024

@CrackTheCode016 tests are failing.

@kianenigma kianenigma added the T11-documentation This PR/Issue is related to documentation. label Jul 10, 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 be keen to see this complete, so that we can proceed with w3f/polkadot-wiki#5928, but conversations needs resolving @CrackTheCode016.

If in doubt, I suggest doing sth backwards compatible for now.

Also, as for w3f/polkadot-wiki#5928, we can always edit the chain-spec files manually, and it is not super hard to add two fields two it, fwiw, so I'd say tutorials can proceed, and we make small updates to them later.

@CrackTheCode016
Copy link
Contributor Author

@kianenigma Yes, looking to do that tomorrow / early next week, had a few hiccups with being sick and some other tasks that needed prioritization, but will be on it asap.

Most likely will just do create-parachain with a couple of required fields for the relay chain, para id, and an optional field for including the "properties" for a ticker and stuff, to get the bare minimum finished for now.

this way, anything currently using create will not break.

@CrackTheCode016 CrackTheCode016 requested review from a team and koute as code owners August 14, 2024 18:49
@paritytech-review-bot paritytech-review-bot bot requested a review from a team August 14, 2024 18:49
@CrackTheCode016
Copy link
Contributor Author

For now, this is what I have done. Let me know if it is OK:

  • I split up the core functions of generating a chain spec to be a bit more generic, so we can do stuff like solo/para builders without an issue.
  • I added the para fields as part of the create command, as I felt that a separate command would end up being unreasonable as it is quite repetitive. Right now, the behavior is:
    • If no parachain fields (--relay-chain and --para-id) are provided, then it produces a normal chain-spec. This means it is backward compatible with whoever is using it.
    • If both --relay-chain and --para-id are provided, then a parachain chain spec is generated, along with some properties that are common + work with the template, which is what many people would be using it for.

This tool could probably benefit from some more refactor for expansion later. Something that would be nice to see is a field-by-field edit feature, so you can edit some parameters from the CLI, which I imagine is not very difficult to do.


Solo chain spec generation:

chain-spec-builder create \
-v \
-r runtime.wasm \
patch patch.json

Parachain spec generation:

chain-spec-builder create \
-v \
--relay-chain polkadot \
--para-id 1000 \
-r runtime.wasm \
patch patch.json

@kianenigma
Copy link
Contributor

kianenigma commented Aug 15, 2024

Your formatter seems to be at a different version than ours, which made the recent commits a bit hard to review.

@alvicsam similar to the convo around the stable version used in CI tests, how should one find the exact nightly we use for FMT? I with that these values are stored somewhere that is intuitive to find :) (ref)

@CrackTheCode016
Copy link
Contributor Author

@kianenigma Ah yes, apologies for this, I had been messing with some stuff in later versions. Please do let me know the version to use for nightly and I'll reformat for ease of readability

@kianenigma
Copy link
Contributor

Thank you @michalkucharczyk!

From my side, I'd mainly like to see the discussed guideline of "it should feel like omni-bencher" fulfilled. AFAIK this is already done, so will be ready for a final review here :)

@michalkucharczyk
Copy link
Contributor

I'd say this is ready for next round of reviews.

I also fixed swallowing unknown fields, applied all the comments from previous reviews (hope I did not miss anything), and also added some testsuite for basic scenarios.

@michalkucharczyk michalkucharczyk changed the title Add extension to chain_spec_builder Add parachain related parameters to chain-spec-builder Sep 3, 2024
@michalkucharczyk michalkucharczyk changed the title Add parachain related parameters to chain-spec-builder Add parachain related parameters to chain-spec-builder Sep 3, 2024
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.

Looks really good, only some nits.

substrate/bin/utils/chain-spec-builder/tests/test.rs Outdated Show resolved Hide resolved
substrate/bin/utils/chain-spec-builder/src/lib.rs Outdated Show resolved Hide resolved
substrate/bin/utils/chain-spec-builder/src/lib.rs Outdated Show resolved Hide resolved
@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/7248082

@michalkucharczyk michalkucharczyk added this pull request to the merge queue Sep 4, 2024
Merged via the queue into paritytech:master with commit 89b41c5 Sep 4, 2024
185 of 187 checks passed
@michalkucharczyk michalkucharczyk deleted the bader-chain-spec-ext branch September 4, 2024 11:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T11-documentation This PR/Issue is related to documentation.
Projects
Status: Milestone 0
Development

Successfully merging this pull request may close these issues.

chain-spec-builder: Do not swallow unknown fields
7 participants