Pallet Syntax: Analysis of `T` consistency

kianenigma commented 1 year ago

In this issue, I will describe how T is used within a pallet, and we can discuss if/how it can be improved. Would be good to double check all of this. The best way is try and start writing a pallet from scratch and letting yourself be confused.

Storage Items

Storage items are valid in either of

type Foo<T> = StorageValue<Value = u32>
type Foo<T: Config> = StorageValue<Value = T::AccountId>

but this is not valid: type Foo = StorageValue<Value = u32>.

which implies that <T> is always needed, and is always expanded to <T: Config> even if you drop it.

But this is not the case and type Foo<T> = StorageValue<Value = T::AccountId> is also not valid.

`Pallet` struct

Similarly, this is also always generic over T, and can be defined as either of:

pub struct Pallet<T>(_);
pub struct Pallet<T>(sp_std::marker::PhantomData<T>); ✅

But, confusingly, neither of the below are valid, which makes this not abide by the same mental model as storage items, where <T> is auto-expanded for you to <T: Config>.

pub struct Pallet<T: Config>(_);
pub struct Pallet<T: Config>(sp_std::marker::PhantomData<T>);

`enum Event`

This one acts like a very normal enum. Unlike storage, if you need T, you define it, if not, you don't. The only quirk here is that the if you define any genric, the pallet macro will auto-generate a hidden PhamtomData field for you.

✅
enum Event {
  Foo(u32)
}

✅
enum Event<T> {
  Foo(u32)
}

✅
enum Event<T: Config> {
  Foo(T::AccountId)
}

❌
enum Even<T> {
  Foo(T::AccountId)
}

I think putting the PhantomData in there by default has been confusing and has probably led us to putting it there in many pallets without needing it.

`enum Error`

This one works similar to struct Pallet, where only T is accepted.

✅
pub enum Error<T> {}

❌
pub enum Error<T: Config> {}

❌
pub enum Error {}

The Error enum is among more involved ones, and you always need T to get pallet_index, which is needed to convert this upwards to DispatchError.

`GenesisConfig`

The usage of T in GenesisConfig seems to be exactly like one would expect from Rust. The only inconsistency I see is that unlike Event, if you use <T> or <T: Config>, a PhantomData is not auto-injected for you. Both of the following is valid:

✅
#[pallet::genesis_config]
pub struct GenesisConfig {
    pub foo: u32,
}

#[cfg(feature = "std")]
impl Default for GenesisConfig {
    fn default() -> Self {
        unimplemented!()
    }
}

#[pallet::genesis_build]
impl<T> GenesisBuild<T> for GenesisConfig {
    fn build(&self) {
        unimplemented!()
    }
}

✅
#[pallet::genesis_config]
pub struct GenesisConfig<T: Config> {
    pub foo: u32,
    pub bar: T::BlockNumber,
}

#[cfg(feature = "std")]
impl<T: Config> Default for GenesisConfig<T> {
    fn default() -> Self {
        unimplemented!()
    }
}

#[pallet::genesis_build]
impl<T: Config> GenesisBuild<T> for GenesisConfig<T> {
    fn build(&self) {
        unimplemented!()
    }
}

All in all, Just looking at this from a syntactic point of view, I see two main points of inconsistency+improvement:

In some cases, we generate a PhantomData and in some cases we don't
In some cases we are flexible about <T> being <T: Config>, but in some cases like Error and Pallet we are arguably too strict for no reason, and don't allow the latter.

gui1117 commented 1 year ago

For storage items we need the generic T in the first type parameter of StorageValue, macro should generate something along the line:

type Foo<T> = StorageValue<_GeneratedPrefixForFoo<T>, ...>

bounding Config is only necessary when using the type in order to access the different methods implemented.

It is needed to access the name of the pallet which is used to derive the key to store the value in the trie

For Error: I forbid the T: Config because, contrary to event, we never use syntax like T::Account. So it is never useful to bound it. But, all in all, bounding Config is also harmless.

For GenesisConfig I think we don't generate the phantom data because it looks a bit ugly when using the type. For Event I think the phantom data is rather harmless as it is hidden from doc.

EDIT: I agree allowing T: Config for pallet error and pallet struct could be better. The bounding is not useful, but at the same time it is harmless. So people can do as they prefer

Polkadot-Forum commented 7 months ago

This issue has been mentioned on Polkadot Forum. There might be relevant details there:

https://forum.polkadot.network/t/ink-analyzer-300-unique-vs-code-extension-installs-preparing-for-ink-v5-moving-to-retroactive-treasury-funding-and-gauging-interest-in-a-frame-equivalent/6512/2

Polkadot-Forum commented 7 months ago

This issue has been mentioned on Polkadot Forum. There might be relevant details there:

https://forum.polkadot.network/t/ink-analyzer-300-unique-vs-code-extension-installs-preparing-for-ink-v5-moving-to-retroactive-treasury-funding-and-gauging-interest-in-a-frame-equivalent/6512/3

paritytech / polkadot-sdk