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

update solochain to use #[runtime] #5772

Merged
merged 16 commits into from
Sep 23, 2024
Merged

update solochain to use #[runtime] #5772

merged 16 commits into from
Sep 23, 2024

Conversation

Jan-Jan
Copy link
Contributor

@Jan-Jan Jan-Jan commented Sep 19, 2024

Description

  • This is part of issue 5242, specifically getting solochain to use #[frame::runtime]
  • Furthermore, reinforced the convention of Template instead of TemplateModule

Integration

  • Should be integrated into the solochain template and documentation

Review Notes

  • Refactored solochain template from construct_runtime! to #[runtime].
  • AFAIU Template is our new convention, and preferred over TemplateModule.

Out of scope

  • The #[runtime] documentation is still very rudimentary, and should ideally be expanded to explain the macro, both what it does and the input options.
  • Furthermore, suggest update #[runtime] documentation to replace #[crate::runtime] with #[frame_support::runtime]

@Jan-Jan Jan-Jan added C1-mentor A task where a mentor is available. Please indicate in the issue who the mentor could be. I4-refactor Code needs refactoring. labels Sep 19, 2024
@Jan-Jan Jan-Jan requested a review from gupnik September 19, 2024 13:32
@Jan-Jan Jan-Jan changed the title [trivial] update solochain to use #[runtime] Sep 19, 2024
@Jan-Jan Jan-Jan added the R0-silent Changes should not be mentioned in any release notes label Sep 19, 2024
Comment on lines +13 to +22
RuntimeCall,
RuntimeEvent,
RuntimeError,
RuntimeOrigin,
RuntimeFreezeReason,
RuntimeHoldReason,
RuntimeSlashReason,
RuntimeLockId,
RuntimeTask
)]
Copy link
Member

Choose a reason for hiding this comment

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

not sure the philosophy we want to follow, but the template does not need all these derives. i could see an argument to use it either way

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm pro reducing this, but I cannot find the documentation explaining what each does. My personal preference would be to exclude them, and explain in the template why we are excluding them.

Copy link
Member

Choose a reason for hiding this comment

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

if the type isnt being used anywhere in the file, you can pretty safely delete it.

Copy link
Contributor

Choose a reason for hiding this comment

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

@Jan-Jan seems like the PR got merged without this being addressed. Looks like a good follow up. You can also look at the mock.rs in the parachain template and fix this there too.

@shawntabrizi
Copy link
Member

bot fmt

@command-bot
Copy link

command-bot bot commented Sep 23, 2024

@shawntabrizi https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/7415672 was started for your command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh". Check out https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/pipelines?page=1&scope=all&username=group_605_bot to know what else is being executed currently.

Comment bot cancel 4-e5fa40d6-1806-430e-b2c6-0e140b61303a to cancel this command or bot cancel to cancel all commands in this pull request.

@command-bot
Copy link

command-bot bot commented Sep 23, 2024

@shawntabrizi Command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh" has finished. Result: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/7415672 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/7415672/artifacts/download.

@bkchr bkchr added this pull request to the merge queue Sep 23, 2024
Merged via the queue into master with commit 08498f5 Sep 23, 2024
207 of 209 checks passed
@bkchr bkchr deleted the runtime-macro branch September 23, 2024 23:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C1-mentor A task where a mentor is available. Please indicate in the issue who the mentor could be. I4-refactor Code needs refactoring. R0-silent Changes should not be mentioned in any release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants