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

Multiple client and runtime support #415

Merged
merged 49 commits into from
May 19, 2021
Merged

Multiple client and runtime support #415

merged 49 commits into from
May 19, 2021

Conversation

tgmichel
Copy link
Contributor

@tgmichel tgmichel commented May 16, 2021

This PR introduces:

  • Support for moonbeam, moonbase, moonriver and moonshadow networks.
  • Executors for each network and AbstractClient as a proxy.
  • Chainspecs for each network.

General

  • Move common primitive types to core-primtives.
  • Labeling, binary names, etc..
  • Review copyrights.

Service

  • Setup moon* clients.
  • Setup moon* chainspec.
  • Review command's function load_spec logic.
  • Review chain Subcommands.
  • Review and remove all my in-code TODO-multiple-runtimes comments.
  • Deal with multiple transaction converters on each runtime.

Runtime

  • Bootstrap moonriver
  • Bootstrap moonshadow

@crystalin
Copy link
Collaborator

@tgmichel , I've seen you are doing a lot of #[cfg(feature = "with-moonriver-runtime")]
Are you sure it is the right strategy ?
I was more thinking of doing it the same way as Kusama/Polkadot/Westend... (see https://github.com/paritytech/polkadot/blob/master/node/service/src/lib.rs#L84)

@JoshOrndorff or @4meta5 who has more experience with those, what do you suggest ?

@tgmichel
Copy link
Contributor Author

I was more thinking of doing it the same way as Kusama/Polkadot/Westend...

Yeah, it's the other possible approach: no features, enum matching the client and include everything in the binary to launch any of the networks.

@crystalin
Copy link
Collaborator

Yes @tgmichel , I'm more leaning into that solution if you don't see any drawback

@JoshOrndorff
Copy link
Contributor

JoshOrndorff commented May 17, 2021

I prefer @tgmichel's current approach (using cargo features) inspired by Acala. The parity approach has a few drawbacks that both lead to longer build time.

  1. You have to build all runtimes when you build the service. This is a big developer experience bummer in cumulus (Building the Collator builds all the Polkadot runtimes. paritytech/cumulus#175) and leads to larger binaries,docker images, etc.
    2. You have more complex generics and other confusing patterns throughout the rest of your codebase. (For example: https://github.com/paritytech/cumulus/blob/master/client/consensus/relay-chain/src/lib.rs#L281-L375)

There are also advantages to Parity's approach. The big one is that there is only a single binary, single docker image, no need to remember build flags. I'm also more familiar with Parity's approach, so there may be drawbacks to the Acala's approach that I'm not yet aware of.

EDIT: My point #2 was wrong.

@tgmichel
Copy link
Contributor Author

tgmichel commented May 17, 2021

@JoshOrndorff main drawback I found overall is the AbstractClient having a shared RuntimeApiCollection. In Polkadot 0.9.1 for example they circumvent that by just bogus implementing ParachainHost runtime api in networks that don't yet support it (for example in polkadot). Honestly we may end up doing that instead all the feature witchcraftery.

Edit: I mean we can still use the feature approach for optimization, but not for the RuntimeApiCollection stuff.

Copy link
Contributor

@JoshOrndorff JoshOrndorff left a comment

Choose a reason for hiding this comment

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

Here's a preliminary review. I didn't quite get to the end.

node/cli/src/cli.rs Outdated Show resolved Hide resolved
node/cli/src/cli.rs Show resolved Hide resolved
node/cli/src/command.rs Show resolved Hide resolved
node/cli/src/command.rs Outdated Show resolved Hide resolved
node/cli/src/command.rs Outdated Show resolved Hide resolved
node/cli/src/command.rs Outdated Show resolved Hide resolved
node/cli/src/lib.rs Outdated Show resolved Hide resolved
node/service/src/client.rs Show resolved Hide resolved
node/service/src/client.rs Outdated Show resolved Hide resolved
node/service/src/client.rs Outdated Show resolved Hide resolved
@joelamouche
Copy link
Contributor

What happens if no flag is passed? We might wanna update the moonbeam-launch readme (and maybe also tests)

@crystalin
Copy link
Collaborator

@tgmichel , I've merged master, which included some changes from Joshy on Nimbus.
@JoshOrndorff I removed the author_id from the chain_spec and service. Maybe this was wrong, let me know, it is commit 9c74bcd

@crystalin
Copy link
Collaborator

@JoshOrndorff I restored the author for part of it, as it was needed for the --dev
@tgmichel , we should target to merge it asap, once we get another review, so other people can integrate quickly (as it generates a lot of conflict with other work on master)

@tgmichel tgmichel marked this pull request as ready for review May 19, 2021 14:56
@tgmichel
Copy link
Contributor Author

Everyone, thank you for your reviews. Merging now.

@tgmichel tgmichel merged commit 9c92844 into master May 19, 2021
@tgmichel tgmichel deleted the tgm-runtimes branch May 19, 2021 15:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants