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

New #[runtime] should be real rust Rust for assigning a type #4723

Closed
shawntabrizi opened this issue Jun 7, 2024 · 3 comments · Fixed by #4769
Closed

New #[runtime] should be real rust Rust for assigning a type #4723

shawntabrizi opened this issue Jun 7, 2024 · 3 comments · Fixed by #4769
Assignees
Labels
T1-FRAME This PR/Issue is related to core FRAME, the framework.

Comments

@shawntabrizi
Copy link
Member

shawntabrizi commented Jun 7, 2024

The new #[runtime] macro is great, but the assignment of pallet types is awkward since the type is assigned to a crate name:

/// Mandatory system pallet that should always be included in a FRAME runtime.
#[runtime::pallet_index(0)]
pub type System = frame_system;

This is just not what would be valid in Rust.

Instead, we should simply update this to point to the Pallet<T> object, which is actually what is happening:

/// Mandatory system pallet that should always be included in a FRAME runtime.
#[runtime::pallet_index(0)]
pub type System = frame_system::Pallet<Runtime>;

This makes this line of code more accurate for what System or any pallet type represents, and also more in alignment with real Rust.

cc @gupnik @kianenigma

@gupnik
Copy link
Contributor

gupnik commented Jun 7, 2024

Instead, we should simply update this to point to the Pallet<T> object, which is actually what is happening:

Yeah, I had considered it but realised that this would mean an addition of ::Pallet<Runtime> to all pallets without much benefit to the user. I wasn't sure if it was worth it, but happy to realign if we all agree that this is the correct way forward.

@shawntabrizi
Copy link
Member Author

I think these extra characters are near zero overhead, but in fact having a user use the type System, in ways like System::set_code() in a test, and not realizing that type System = frame_system::Pallet<Runtime> is quite bad.

And, in the interest of having all substrate macros be as close to real rust as possible, it is also better that we just have it make sense that a struct is being assigned to a type.

Anyway, glad to know this will be a simple change.

@gupnik gupnik self-assigned this Jun 7, 2024
@gupnik gupnik added the T1-FRAME This PR/Issue is related to core FRAME, the framework. label Jun 7, 2024
@kianenigma
Copy link
Contributor

Supporting both is a good compromise to make sure it is not a breaking change for existing users.

github-merge-queue bot pushed a commit that referenced this issue Jun 25, 2024
Fixes #4723. Also,
closes #4622

As stated in the linked issue, this PR adds the ability to use a real
rust type for pallet alias in the new `runtime` macro:
```rust
#[runtime::pallet_index(0)]
pub type System = frame_system::Pallet<Runtime>;
```

Please note that the current syntax still continues to be supported.

CC: @shawntabrizi @kianenigma

---------

Co-authored-by: command-bot <>
Co-authored-by: Bastian Köcher <[email protected]>
TarekkMA pushed a commit to moonbeam-foundation/polkadot-sdk that referenced this issue Aug 2, 2024
Fixes paritytech#4723. Also,
closes paritytech#4622

As stated in the linked issue, this PR adds the ability to use a real
rust type for pallet alias in the new `runtime` macro:
```rust
#[runtime::pallet_index(0)]
pub type System = frame_system::Pallet<Runtime>;
```

Please note that the current syntax still continues to be supported.

CC: @shawntabrizi @kianenigma

---------

Co-authored-by: command-bot <>
Co-authored-by: Bastian Köcher <[email protected]>
sfffaaa pushed a commit to peaqnetwork/polkadot-sdk that referenced this issue Dec 27, 2024
Fixes paritytech#4723. Also,
closes paritytech#4622

As stated in the linked issue, this PR adds the ability to use a real
rust type for pallet alias in the new `runtime` macro:
```rust
#[runtime::pallet_index(0)]
pub type System = frame_system::Pallet<Runtime>;
```

Please note that the current syntax still continues to be supported.

CC: @shawntabrizi @kianenigma

---------

Co-authored-by: command-bot <>
Co-authored-by: Bastian Köcher <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T1-FRAME This PR/Issue is related to core FRAME, the framework.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants