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.

Sign up for GitHub

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

Jump to bottom

Use para id in specs instead of flag #157

Merged
merged 12 commits into from
Dec 13, 2021
Merged

Use para id in specs instead of flag #157

merged 12 commits into from
Dec 13, 2021

Conversation

joelamouche
Copy link
Contributor

@joelamouche joelamouche commented Dec 6, 2021 •
edited
Loading

  • Parachain id flag is not supported anymore and should be included in genesis specs
  • Testing sequence will now use the specs to decide on chain id
  • Added a test that isn't included in the ci with two parachains to test ids
  • remove chain option from collator spawning and genesis export

tackles #151 #156

@joelamouche
Copy link
Contributor Author

@shawntabrizi ready for review

bkchr
bkchr reviewed Dec 6, 2021
View reviewed changes
Copy link
Member

@bkchr bkchr left a comment

Choose a reason for hiding this comment

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

Imo we should entirely remove the paraid from here. Some projects may still support the cli argument or whatever. However, I'm also planing to remove the Para id by default from the chain spec.

@joelamouche
Copy link
Contributor Author

However, I'm also planing to remove the Para id by default from the chain spec

@bkchr So how does one determine their parachain id?

@bkchr
Copy link
Member

bkchr commented Dec 7, 2021

You hard code it in the runtime for example.

riusricardo
riusricardo reviewed Dec 7, 2021
View reviewed changes
src/spawn.ts
args.push("--parachain-id=" + id);
}
if (chain) {
args.push("--chain=" + chain);
Copy link

Choose a reason for hiding this comment

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

Why do we need to remove chain selection from the parachain parameters? It will cause issues when you export the genesis state.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

According to a comment, it was't supported by cumulus anymore

Copy link

Choose a reason for hiding this comment

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

I think this was a misunderstanding. The only thing that got removed was the parachain-id paritytech/cumulus#739
No need to remove the option to select a chain. It is very needed, in polkadot-collator we have several chains, the same stands for the polkadot binary.

Copy link

Choose a reason for hiding this comment

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

Indeed. How was this resolved, perhaps I'm missing something @joelamouche ?

Copy link
Contributor Author

@joelamouche joelamouche Dec 13, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

@ghzlatarev I left the --chain command. So if you specify the chain in the chain field of the parachain in hte config, it should work

Copy link

Choose a reason for hiding this comment

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

@joelamouche I don't see it in startCollator or exportGenesisWasm , is it not needed there ?
Right now if I start a westmint-local network I get some default Local Testnet

Copy link

Choose a reason for hiding this comment

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

@joelamouche i hit this code path https://github.com/paritytech/cumulus/blob/master/polkadot-parachains/src/command.rs#L119 -> https://github.com/paritytech/cumulus/blob/master/polkadot-parachains/src/chain_spec.rs#L71

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right Im adding it back

@joelamouche
Copy link
Contributor Author

You hard code it in the runtime for example.

@bkchr but what if I want to run the same runtime on 2 different ids?
In the moonbeam tests, we are running two copies of moonbase runtime. Would they automatically have different parachain ids if I removed the parachain id fro the specs?

@bkchr
Copy link
Member

bkchr commented Dec 7, 2021

You hard code it in the runtime for example.

@bkchr but what if I want to run the same runtime on 2 different ids? In the moonbeam tests, we are running two copies of moonbase runtime. Would they automatically have different parachain ids if I removed the parachain id fro the specs?

This is completely up to the parachain developer. Aka if you want to support two different parachain ids, I see no reason why that should not be doable. However, the official code doesn't need to provide this, because it only confuses people.

@joelamouche
Copy link
Contributor Author

OK that seems a pretty big refactor to let's leave it to a follow up PR @bkchr ?
I've started to try to implement that but the resolvedId is used in the addGenesisParachain function in spec.ts file and using an incremental index doesnt seem to work. Maybe you have an idea why @shawntabrizi

@nuke-web3 nuke-web3 mentioned this pull request Dec 7, 2021
Closed
2 tasks
shawntabrizi
shawntabrizi reviewed Dec 7, 2021
View reviewed changes
src/runner.ts Outdated
const { id, resolvedId, balance } = parachain;

if (resolvedId) {
await changeGenesisConfig(`${chain}.json`, {});
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 you passing an empty object to changeGenesisConfig?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh yeah I forgot to remove that :s

@shawntabrizi
Copy link
Member

@joelamouche seems the code is just wrong in the case of a resolvedId? Otherwise, not sure why it wouldn't work.

joelamouche
joelamouche commented Dec 8, 2021
View reviewed changes
test/test-utils/para-node.ts
@@ -156,7 +154,6 @@ export async function startParachainNodes(options: ParachainOptions): Promise<{
bin: BINARY_PATH,
id: (1000 * (i + 1)).toString(),
balance: "1000000000000000000000",
// chain: options.chain, this is not supported by cumulus
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@riusricardo this is the comment I was referring to. I'm going to put it back though, for collators that do need it

@joelamouche
Copy link
Contributor Author

@shawntabrizi ready for review

@jimmychu0807
Copy link

hmmm.. didn't aware you guys are working on this...

I am working on #160

@shawntabrizi shawntabrizi merged commit 1098183 into paritytech:master Dec 13, 2021
@shawntabrizi
Copy link
Member

the config.json has a chain parameter that you specify for something like that

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ghzlatarev ghzlatarev ghzlatarev left review comments

@riusricardo riusricardo riusricardo left review comments

@bkchr bkchr bkchr left review comments

@shawntabrizi shawntabrizi shawntabrizi approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@joelamouche @bkchr @shawntabrizi @jimmychu0807 @riusricardo @ghzlatarev