-
Notifications
You must be signed in to change notification settings - Fork 19
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
chore(genesis): move development genesis config to runtime #2152
chore(genesis): move development genesis config to runtime #2152
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅ |
adb092a
to
66145ee
Compare
7a032a9
to
28770ba
Compare
17d61a3
to
bd280f3
Compare
820ea2c
to
26952b6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Reviewed changes
- Built & ran node, verified genesis config
- Ran unit & e2e tests, passing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haven't gotten all the way through, but wanted to call out one fix. The rest looks good so far. Excited to see this happening!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good one concern is bloating the size of runtime wasm. We already know wasm has a size limit on the polkadot and if it's bigger than a certain size the upgrade won't work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Read through changes
🚢 it!
d47f870
to
b497605
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added some questions and nits.
Move Development Genesis Config to the Runtime to support decoupling of client and runtime dependencies. When running `chain-spec-builder`, the following command: ``` chain-spec-builder list-presets –-runtime-wasm-path ./target/debug/wbuild/frequency-runtime/frequency_runtime.wasm ``` outputs: ``` {“presets”:[“development”]} ``` Additionally, when running: ``` chain-spec-builder display-preset --runtime-wasm-path ./target/debug/wbuild/frequency-runtime/frequency_runtime.wasm --preset-name development ``` it outputs the development genesis config: ``` {"aura":{"authorities":[]},"auraExt":{},"balances":{ ...} ... } ``` issue-2149 issue-2142
Move the Frequency and Paseo genesis configurations to the runtime. Additionally, refactor the genesis builder for Paseo, Frequency, and Development into a single genesis_builder function.
4b8b5b2
to
00e453c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed the code and looked good.
vec![ | ||
sp_genesis_builder::PresetId::from("development"), | ||
sp_genesis_builder::PresetId::from("frequency-local"), | ||
sp_genesis_builder::PresetId::from("frequency"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: why do you need frequency
for local features?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question! This is something I mentioned during our stand-up, and I believe we agreed that having the frequency feature enabled by default was ok.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Read through changes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Read through this PR. Was mainly focused on code organization and naming since I'm not a pro in all of the rust semantics. That being said, looks good to me!
Can you please take another look? Requesting a new review because of new changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
- Ran unit tests
- Ran e2e tests
- Inspected build with
chain-spec-builder
Move Development Genesis Config to the Runtime to
support decoupling of client and runtime dependencies.
When running
chain-spec-builder
, the following command:Outputs:
With feature flags "frequency-no-relay", "frequency-local" enabled:
Additionally, when running:
it outputs the development genesis config:
With feature flags "frequency" only, mainnet presets show up:
Additionally, when running:
it outputs the frequency genesis config:
#2149
#2142