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

[tracking] Migrate pallets to umbrella crate #6504

Open
5 of 91 tasks
re-gius opened this issue Nov 15, 2024 · 42 comments
Open
5 of 91 tasks

[tracking] Migrate pallets to umbrella crate #6504

re-gius opened this issue Nov 15, 2024 · 42 comments
Labels
C2-good-first-issue A task for a first time contributor to become familiar with the Polkadot-SDK. D0-easy Can be fixed primarily by duplicating and adapting code by an intermediate coder. I4-refactor Code needs refactoring. R0-silent Changes should not be mentioned in any release notes T1-FRAME This PR/Issue is related to core FRAME, the framework.

Comments

@re-gius
Copy link
Contributor

re-gius commented Nov 15, 2024

Migrate all pallets to polkadot-sdk-frame by adding types and preludes to it when necessary. As a reference, you can look into #5995.

The guidelines of the umbrella crate are as follows:

//! ## Maintenance Note
//!
//! > Notes for the maintainers of this crate, describing how the re-exports and preludes should
//! > work.
//!
//! * Preludes should be extensive. The goal of this pallet is to be ONLY used with the preludes.
//! The domain-specific modules are just a backup, aiming to keep things organized. Don't hesitate
//! in adding more items to the main prelude.
//! * The only non-module, non-prelude items exported from the top level crate is the `pallet`
//! macro, such that we can have the `#[frame::pallet] mod pallet { .. }` syntax working.
//! * In most cases, you might want to create a domain-specific module, but also add it to the
//! preludes, such as `hashing`.
//! * The only items that should NOT be in preludes are those that have been placed in
//! `frame-support`/`sp-runtime`, but in truth are related to just one pallet.
//! * The currency related traits are kept out of the preludes to encourage a deliberate choice of
//! one over the other.
//! * `runtime::apis` should expose all common runtime APIs that all FRAME-based runtimes need.

✅ Checklist:

  • Add polkadot-sdk-frame to the Cargo.toml
  • Remove all imports from frame_support, sp_runtime and similar, and replace with the appropriate prelude::*
  • For items that are now not in scope, use the above guideline to decide if you should add them to prelude, or reside to using deps
  • Update Cargo.toml to remove all unneeded dependencies.

👉 If you want to work on this: Please check that there is not already a merge request for the pallet that you want to work on (here or in the comments below). Please pick only one or a small set of pallets - not all at once. This keeps review times low. Add a comment below to announce the pallets you're working on.

Pallet list

@re-gius re-gius added R0-silent Changes should not be mentioned in any release notes I4-refactor Code needs refactoring. T1-FRAME This PR/Issue is related to core FRAME, the framework. D0-easy Can be fixed primarily by duplicating and adapting code by an intermediate coder. C2-good-first-issue A task for a first time contributor to become familiar with the Polkadot-SDK. labels Nov 15, 2024
@bennethxyz
Copy link

bennethxyz commented Nov 15, 2024

Can I pick this up @re-gius

Thinking this should be a tracking issue as it involves more than one crate but happy to do it!

@re-gius
Copy link
Contributor Author

re-gius commented Nov 15, 2024

Can I pick this up @re-gius

Thinking this should be a tracking issue as it involves more than one crate but happy to do it!

Yes, it's going to be a tracking issue. You are more than welcome to start working at some pallets/crates, ideally one or two per PR.

@Krayt78
Copy link
Contributor

Krayt78 commented Nov 27, 2024

Are there still some pallets to work on or are you doing all of them?

@Krayt78
Copy link
Contributor

Krayt78 commented Dec 17, 2024

Taking pallet-paged-list and pallet-paged-list-fuzzer since they go together

@UtkarshBhardwaj007
Copy link
Contributor

Hi, I'd like to work on this. I will pickup pallet-mixnet to start with.

@seemantaggarwal
Copy link

Hi, I'd like to work on this. I will pickup pallet-child-bounties to start with.

@aurexav
Copy link
Contributor

aurexav commented Dec 23, 2024

Pick pallet-salary.

@FasmaHouse
Copy link

taking pallet-exampl-task & pallet-example-offchain-worker

@Nathy-bajo
Copy link

taking pallet-sudo & pallet-statement

@tifecool
Copy link

taking pallet-paraameter & pallet-preimage

@runcomet
Copy link
Contributor

Just a quick note from my recent changes if it helps anyone save time because running cargo check takes time.

Overall I think it best to add systems in the prelude as there are needed in the particular pallet one is migrating and also if there exists in more than one pallet.

For example I considered adding frame_support::traits::tokens::*; in the top prelude but ended up causing an error with multiple Balance types in docs::sdk::packages::guides::first-pallet::src::lib.rs.

@aurexav
Copy link
Contributor

aurexav commented Dec 30, 2024

//! * The currency related traits are kept out of the preludes to encourage a deliberate choice of
//! one over the other.

Due to the same reason. I didnot add tokens to the prelude in my PR. I'm not pretty sure how to deal with that.

@runcomet
Copy link
Contributor

//! * The currency related traits are kept out of the preludes to encourage a deliberate choice of
//! one over the other.

Due to the same reason. I didnot add tokens to the prelude in my PR. I'm not pretty sure how to deal with that.

Which one of them? @AurevoirXavier

@re-gius
Copy link
Contributor Author

re-gius commented Jan 2, 2025

As in #6202 , we will reward each contributor with a small tip (20 DOT) once every two pallets (or one big pallet).

@bkchr
Copy link
Member

bkchr commented Jan 2, 2025

Why are we migrating to the frame crate and I see that we need to enable an experimental feature? This makes absolutely no sense. Either the crate is experimental or not. By pulling in one of these crates, the experimental feature is enabled in the entire build tree, which renders this feature completely useless.

@re-gius
Copy link
Contributor Author

re-gius commented Jan 3, 2025

Why are we migrating to the frame crate and I see that we need to enable an experimental feature? This makes absolutely no sense. Either the crate is experimental or not. By pulling in one of these crates, the experimental feature is enabled in the entire build tree, which renders this feature completely useless.

I think that we will remove the experimental feature flag after a few merged PRs like this, when the umbrella crate will be more stable. What do you think @kianenigma ?

@re-gius
Copy link
Contributor Author

re-gius commented Jan 3, 2025

For those of you that have open PRs, please notice that as we start merging PRs that add items to the umbrella crate preludes, you need to be careful with merging your changes since there may be overlaps.

@UtkarshBhardwaj007
Copy link
Contributor

taking pallet-node-authorization

@Krayt78
Copy link
Contributor

Krayt78 commented Jan 6, 2025

Opened an issue to convert the devensive! macro to be compatible with umbrella crate here #7054

@Krayt78
Copy link
Contributor

Krayt78 commented Jan 7, 2025

Opened an issue to convert the devensive! macro to be compatible with umbrella crate here #7054

this issue is ready to be merged, should be usefull for all pallets using the defensive! macro to get rid of the frame_support dep

programskillforverification pushed a commit to programskillforverification/polkadot-sdk that referenced this issue Jan 7, 2025
)

# Description

Migrate pallet-node-authorization to use umbrella crate. Part of paritytech#6504 

## Review Notes

* This PR migrates pallet-node-authorization to use the umbrella crate.
* Some imports like below have not been added to any prelude as they
have very limited usage across the various pallets.
```rust
use sp_core::OpaquePeerId as PeerId;
```
* Added a commonly used runtime trait for testing in the
`testing_prelude` in `substrate/frame/src/lib.rs`:
```rust
pub use sp_runtime::traits::BadOrigin;
```
* `weights.rs` uses the `weights_prelude` like:
```rust
use frame::weights_prelude::*;
```
* `tests.rs` and `mock.rs` use the `testing_prelude`:
```rust
use frame::testing_prelude::*;
```
* `lib.rs` uses the main `prelude` like:
```rust
use frame::prelude::*;
```
* For testing: Checked that local build works and tests run
successfully.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C2-good-first-issue A task for a first time contributor to become familiar with the Polkadot-SDK. D0-easy Can be fixed primarily by duplicating and adapting code by an intermediate coder. I4-refactor Code needs refactoring. R0-silent Changes should not be mentioned in any release notes T1-FRAME This PR/Issue is related to core FRAME, the framework.
Projects
None yet
Development

No branches or pull requests