-
Notifications
You must be signed in to change notification settings - Fork 740
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
Use the umbrella crate for the parachain template #5991
Use the umbrella crate for the parachain template #5991
Conversation
62d9103
to
1123cd1
Compare
1123cd1
to
58869d0
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.
You can decide to do this in a separate PR, but part of the issue requirement is also to move the runtime+pallet to polkadot-sdk-frame
umbrella crate.
You can see the minimal template for how this works.
You might need to add more items to the preludes in polkadot-sdk-frame
for this. See #4782
@kianenigma I migrated also the runtime + pallet, but I had to keep a few of the old crates because of some limitations. I will create a follow-up issue for this. This PR is ready for review. PTAL ! |
But you have not migrated them to In fact, I see now that even in the original PR for the minimal template, we have not fully moved the runtime to the In short, I think for our templates, we should build the pallet+runtime ONLY with Pinging a few heads to agree with this: @ggwpez @franciscoaguirre @muharem @shawntabrizi and if agreed, I suggest not further deferring this goal. I am fixing a lot of the little shortcomings in |
Oh sorry, I didn't know that. I thought I had to use But what's the difference ? Isn't it another way of referring to the same crates in the end ? |
Similar, but the Please see this commit d253bc5 and the docs for |
We can still merge this PR as is, happy to take a step back and not be a blocker as it is a clear improvement :) But my overall intention here is to standardize: with this PR and #5995:
As for the decision tree above, I am personally in favor of using it just as I would like to make a decision about the "umbrella crate strategy", and present it in Sub0, so please chime in, and be contrarion if you want to :) |
I think there is not much difference, as long as people are using the frame crate at all (no matter where they import it from). |
Implemented the suggestions. For the pallet I used only the |
templates/parachain/node/Cargo.toml
Outdated
@@ -22,82 +19,31 @@ jsonrpsee = { features = ["server"], workspace = true } | |||
futures = { workspace = true } | |||
serde_json = { workspace = true, default-features = true } | |||
docify = { workspace = true } | |||
color-print = { workspace = true } | |||
|
|||
polkadot-sdk = { workspace = true, features = ["experimental", "node"] } |
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.
Why are we enabling a experimental feature in our templates? There is something fishy again..
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.
I would soon remove this flag from the frame umbrella crate, rest assured.
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 like it wasn't needed. Removed it.
substrate/frame/support/src/lib.rs
Outdated
@@ -915,7 +915,7 @@ pub mod pallet_prelude { | |||
pub use scale_info::TypeInfo; | |||
pub use sp_inherents::MakeFatalError; | |||
pub use sp_runtime::{ | |||
traits::{MaybeSerializeDeserialize, Member, ValidateUnsigned}, | |||
traits::{CheckedAdd, MaybeSerializeDeserialize, Member, One, ValidateUnsigned}, |
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.
What about all the other Checked*
traits and Zero
?
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 them.
d7f01a1
Use the umbrella crate for the parachain template
This covers almost all the dependencies. There are just a few exceptions for which I created a separate issue: #5993
Also related to: #4782