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

Make the project capable of handling multiple runtimes #721

Merged
merged 81 commits into from
Aug 11, 2022

Conversation

sea212
Copy link
Member

@sea212 sea212 commented Jul 26, 2022

Closes #493

This PR splits the previous runtime crate into three different crates: runtime/common, runtime/battery-station and runtime/zeitgeist. runtime/common contains common types, tests, API implementations and the common runtime. The runtime/battery-station and runtime/zeitgeist crate utilize those through wrapping macros. Those macros copy the common parts over and in some cases also allow to extend those, for example to add new pallets to the runtime, extend the configuration or add new APIs. This might not be the cleanest approach, but the other approach to use one common create for everything, which will spare us copying from common to another runtime, does not work because it introduces circular dependencies and requires the node to build the same crate with different features, which is prohibited.

In addition to that, this PR adjusts the node crate to be able to handle multiple runtimes. A new abstract client implementation is offered, which abstracts the client over the generic RuntimeAPI and Executor (which is used to dispatch calls). The node can be built with with-battery-station-runtime and/or with-zeitgeist-runtime features to include the respective runtimes. By default, both runtimes are used. When creating a new chainspec (staging network), the respective runtime used to generate the chainspec must be included in the build. Currently the dev chain option uses the battery station runtime with a development staging chainspec. The rationale behind this is that battery station should always be the most advanced network, hence be used for development work. However, this can be extended to zeitgeist-dev and battery-station-dev if required.

@sea212 sea212 added the s:in-progress The pull requests is currently being worked on label Jul 26, 2022
@sea212 sea212 added this to the v0.3.5 milestone Jul 26, 2022
@sea212 sea212 self-assigned this Jul 26, 2022
@sea212 sea212 added s:review-needed The pull request requires reviews and removed s:in-progress The pull requests is currently being worked on labels Aug 10, 2022
@sea212 sea212 added s:in-progress The pull requests is currently being worked on and removed s:review-needed The pull request requires reviews labels Aug 10, 2022
@sea212 sea212 added s:review-needed The pull request requires reviews and removed s:in-progress The pull requests is currently being worked on labels Aug 10, 2022
Copy link
Member

@maltekliemann maltekliemann 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 to me! 👍

Some comments (in addition to the remars in the code):

  • I think the README.md should be updated to reflect these changes. We should also update the documentation repository regarding compiling for Battery Station.
  • When I run cargo build --release --features=with-zeitgeist-runtime, both Zeitgeist and Battery Station get compiled. Is this as expected?
  • The standalone node seems to default to Battery Station, so even after building specifically with with-zeitgeist-runtime, running target/release/zeitgeist starts a Battery Station client. Is this as expected?

Edit. What worries me about the macro usage is that we cannot auto-format the code, and, that error messages may be become cryptic.

runtime/zeitgeist/src/parameters.rs Outdated Show resolved Hide resolved
runtime/common/src/lib.rs Show resolved Hide resolved
node/src/chain_spec/battery_station.rs Show resolved Hide resolved
node/src/chain_spec/battery_station.rs Outdated Show resolved Hide resolved
@sea212
Copy link
Member Author

sea212 commented Aug 10, 2022

The standalone node seems to default to Battery Station, so even after building specifically with with-zeitgeist-runtime, running target/release/zeitgeist starts a Battery Station client. Is this as expected?

This is expected behavior. When we enter no chain or "dev", the node has to select which runtime it will execute the development (staging) configuration on. I assumed that Battery Station is the more advanced network (and it has sudo, which is useful in the development network), so it defaults to Battery Station.

@sea212
Copy link
Member Author

sea212 commented Aug 10, 2022

When I run cargo build --release --features=with-zeitgeist-runtime, both Zeitgeist and Battery Station get compiled. Is this as expected?

Yes. By default, both runtimes are built (default feature in Cargo.toml). If you want to build the node with only one runtime, you have to execute this command: cargo build --no-default-features --features=with-zeitgeist-runtime from within the node crate. If you execute that command from the top level directory, it will build both runtimes anyways because they are members of the complete workspace.
With regard to that fact (auto build both runtimes), do you still think a readme and docs update is necessary? All the docs are still up to date and lead to fully functional clients.

@sea212 sea212 added s:in-progress The pull requests is currently being worked on and removed s:review-needed The pull request requires reviews labels Aug 10, 2022
@sea212 sea212 added s:review-needed The pull request requires reviews and removed s:in-progress The pull requests is currently being worked on labels Aug 10, 2022
@maltekliemann
Copy link
Member

With regard to that fact (auto build both runtimes), do you still think a readme and docs update is necessary? All the docs are still up to date and lead to fully functional clients.

Ok, somehow I assumed that setting one of the features was necessary. Yeah, didn't think this through. We can leave the docs the way they are. 👍

Copy link
Member

@maltekliemann maltekliemann left a comment

Choose a reason for hiding this comment

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

👍

@vivekvpandya vivekvpandya added s:accepted This pull request is ready for merge and removed s:review-needed The pull request requires reviews labels Aug 11, 2022
@sea212
Copy link
Member Author

sea212 commented Aug 11, 2022

Thanks for reviewing this monster of a PR guys!

@sea212 sea212 merged commit a6ec392 into main Aug 11, 2022
@sea212 sea212 deleted the sea212-separate-runtime branch August 11, 2022 10:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
s:accepted This pull request is ready for merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Runtime] Create one runtime configuration per network
3 participants