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

feat: separate runtimes #46

Merged
merged 24 commits into from
Mar 27, 2024
Merged

feat: separate runtimes #46

merged 24 commits into from
Mar 27, 2024

Conversation

al3mart
Copy link
Collaborator

@al3mart al3mart commented Mar 22, 2024

This PR separates the runtime into two different ones

  • Devnet (meant to be used locally or privately)
  • Testnet (meant to be deployed on public testnets)

It configures the corresponding chain specs and its configurations depending the relay chain the network is aimed to be deployed on:

  • dev-rococo
  • dev-paseo
  • pop-rococo
  • pop-paseo

Also includes the specs generated for rococo and paseo from this code base.

I have created a pair of accounts to have a place holder in the chain spec for the testnet options, I'll be curious to know your opinions there. Aura keys might need to be changed.

A new feature has been included paseo to set the corresponding runtime values for deploying in Paseo testnet.
This feature sets the runtime for sync backing and uses monetary unit of 10 decimals.
Not activating this feature will default to set the runtime to work with async backing and a monetary unit of 12 decimals.

Copy link
Collaborator

@peterwht peterwht left a comment

Choose a reason for hiding this comment

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

cargo test build fails, especially with the integration-tests folder

node/src/chain_spec.rs Outdated Show resolved Hide resolved
node/src/command.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@evilrobot-01 evilrobot-01 left a comment

Choose a reason for hiding this comment

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

LGTM, mostly just suggestions apart from the potential issue with async-backing enabled on Paseo. We might need to switch all the items noted in https://wiki.polkadot.network/docs/maintain-guides-async-backing#phase-3---activate-async-backing off when deploying to Paseo, which could be done with an async-backing feature flag.

integration-tests/src/chains/pop_network/genesis.rs Outdated Show resolved Hide resolved
integration-tests/src/chains/rococo/genesis.rs Outdated Show resolved Hide resolved
integration-tests/src/lib.rs Outdated Show resolved Hide resolved
integration-tests/Cargo.toml Outdated Show resolved Hide resolved
node/src/chain_spec.rs Outdated Show resolved Hide resolved
runtime/testnet/Cargo.toml Outdated Show resolved Hide resolved
runtime/devnet/Cargo.toml Outdated Show resolved Hide resolved
runtime/testnet/src/extensions.rs Show resolved Hide resolved
runtime/testnet/src/lib.rs Show resolved Hide resolved
@al3mart
Copy link
Collaborator Author

al3mart commented Mar 23, 2024

Will address your comments @evilrobot-01 and maybe also move the definition of UNITS together with a feature so we can choose the precision we want to use.

I've just noticed that even if we are setting the spec correctly we still have this fixed definition in the runtime.

@evilrobot-01
Copy link
Collaborator

Will address your comments @evilrobot-01 and maybe also move the definition of UNITS together with a feature so we can choose the precision we want to use.

Probably need a better feature name than async-backing then. As Rococo is short term, maybe we just use rococo, with the defaults being for Paseo.

@Daanvdplas
Copy link
Collaborator

A suggestion is to diff the lib.rs file of both testnet & devnet, and perhaps the others just to be sure, and check for inconsistencies. I see the struct FilteredCalls missing in the devnet runtime/lib.rs now

@evilrobot-01
Copy link
Collaborator

A suggestion is to diff the lib.rs file of both testnet & devnet, and perhaps the others just to be sure, and check for inconsistencies.

Agreed, it is important that these two align, at least in terms of structure, so that they can easily be diffed to ensure certain things are not missing. They will diverge over time, but need to be able to easily diff to ensure what should be the same, is the same.

Copy link
Collaborator

@peterwht peterwht left a comment

Choose a reason for hiding this comment

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

Overall looks good to me. Agree with Frank's comments from above about using runtime alias to simplify naming.

node/src/chain_spec.rs Outdated Show resolved Hide resolved
Enum variants should be sentence-cased rather than capitalised.
Regression from main as result of runtime split
Devnet always used when resolved to default, so just uses devnet and removes the additional default variant.
@evilrobot-01 evilrobot-01 changed the title Ready Pop Network for testnets feat: separate runtimes Mar 26, 2024
Copy link
Collaborator

@evilrobot-01 evilrobot-01 left a comment

Choose a reason for hiding this comment

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

Added a PR to this one, addressing the outstanding comments. Once that is accepted and merged into this one, I am good to proceed.

@evilrobot-01 evilrobot-01 merged commit acd5892 into main Mar 27, 2024
5 checks passed
@evilrobot-01 evilrobot-01 deleted the al3mart/feat-separate-runtimes branch March 27, 2024 20:03
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