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

refactor: parachain template #4684

Merged
merged 12 commits into from
Jun 18, 2024

Conversation

Daanvdplas
Copy link
Contributor

@Daanvdplas Daanvdplas commented Jun 3, 2024

Update parachain template pallet based on polkadot sdk docs

@kianenigma

Copy link
Contributor

@kianenigma kianenigma left a comment

Choose a reason for hiding this comment

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

Great!

CC @gupnik @rzadp let's agree on all changes here, then mirror it in all 3 pallet templates?

{
System: frame_system::{Pallet, Call, Config<T>, Storage, Event<T>},
TemplateModule: crate::{Pallet, Call, Storage, Event<T>},
pub enum Test {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
pub enum Test {
pub struct Test {

Copy link
Contributor

Choose a reason for hiding this comment

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

@gupnik we can move all of this to the new syntax as well right?

Copy link
Contributor

Choose a reason for hiding this comment

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

@gupnik we can move all of this to the new syntax as well right?

Yes! @Daanvdplas Do you want to give that a try? #1378 should help.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will finish this weekend

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It also compiled without experimental feature, didn't run a local network with it though, added it because I saw it being used in other places.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, runtime macro is out of experimental now.

@kianenigma kianenigma added the T11-documentation This PR/Issue is related to documentation. label Jun 7, 2024
@Daanvdplas Daanvdplas force-pushed the daan/refactor-parachain_template branch from a642271 to 768af06 Compare June 15, 2024 16:11
@Daanvdplas
Copy link
Contributor Author

Daanvdplas commented Jun 15, 2024

CC @gupnik @rzadp let's agree on all changes here, then mirror it in all 3 pallet templates?

FYI, with Pop CLI we are working on a feature pop new pallet & pop add pallet. These commands create a new pallet from the pallet template and add the pallet to the runtime, respectively. This could effectively mean that the templates offered in the polkadot sdk (i.e. minimal, solo & parachain) won't need the pallet template included. In stead, a separate pallet crate can be created that can be used by all three.

templates/parachain/runtime/src/lib.rs Show resolved Hide resolved
templates/parachain/runtime/src/lib.rs Show resolved Hide resolved
@@ -22,7 +22,7 @@ scale-info = { version = "2.11.1", default-features = false, features = [

# frame deps
frame-benchmarking = { path = "../../../../substrate/frame/benchmarking", default-features = false, optional = true }
frame-support = { path = "../../../../substrate/frame/support", default-features = false }
frame-support = { path = "../../../../substrate/frame/support", default-features = false, features = ["experimental"] }
Copy link
Member

Choose a reason for hiding this comment

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

We should not add experimental stuff to the template. The point of it being that it is still experimental and subject to change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So revert the changes regarding construct runtime v2? @gupnik

Copy link
Contributor

Choose a reason for hiding this comment

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

So revert the changes regarding construct runtime v2? @gupnik

It's not experimental anymore. You should be able to use it without that flag @Daanvdplas.

Copy link
Member

Choose a reason for hiding this comment

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

Then we should not enable the experimental feature :P

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Apologies, I thought seeing it being used in the kitchen sink runtime but I think I had a bad dream. Thanks Kian

@Daanvdplas
Copy link
Contributor Author

Should be good to go now

Copy link
Contributor

@skunert skunert left a comment

Choose a reason for hiding this comment

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

Looks nice!

@gupnik gupnik added this pull request to the merge queue Jun 18, 2024
Merged via the queue into paritytech:master with commit 7c847f8 Jun 18, 2024
156 of 159 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T11-documentation This PR/Issue is related to documentation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants