Move support macros to attribute procedural macros

[gui1117]

This issue is about moving decl_* to one simple module with attribute macro, inspired by ink macros.
goal is readability, understandability and IDE support, I test with rust-analyzer on vscode.

the idea is to have something like

#[frame_support::pallet(Example)]
pub mod pallet {
	#[pallet::trait_]
	pub trait Trait: frame_system::Trait {
		#[pallet::const_]
		type MyGetParam: Get<u32>;
	}

	#[pallet::module]
	pub struct Module<T, I>(core::marker::PhantomData<T, I>);

.....
}

Precise design should now be discussed in the PR #6877

[gui1117]

Good point from @bkchr IDE support is quite poor for rust macro for the moment and proper handling of proc-macro expansion from IDE could come.

I conclude myself that instead of optimizing syntax for having function information for storages, maybe better to optimize readibility/understandability and remove as much boilerplate as possible.

[bkchr]

goal is readability, understandability and IDE support, I test with rust-analyzer on vscode.

IDE support should be not the goal here. If the compiler accepts it, it is valid code. This means the IDE needs to improve and I don't want that we write code that makes rust-analyzer happy. If they finished their proc-macro support and something does not work, we should open an upstream issue.

(Too slow)

[bkchr]

Regarding storage, we could switch either to something like this:

#[storage]
struct Storage {
    my_item: Map,
    my_other_item: Value,
}

Which gives you access to the stuff by doing:

Storage::my_value().get();

Storage::my_other_value().set("hey");

We would return some fake instances that still write data directly to the storage as it is done currently.

Or if we want to split up the storage items, as you proposed, I would do the following:

#[storage(map)]
struct TotalBalance<T>(T::Balance);

Regarding putting everything into a module, I'm not a real fan of it. I understand that there is currently no other way, but I don't really like it :D

[kianenigma]

Pretty old but might still be related: https://hackmd.io/n_tTUbpMQiKiG7F2dURb6g?view

[gui1117]

could relate #5678

[kianenigma]

@thiolliere you probably made a mistake here? you are referencing the same issue 😄

could relate #5678

[gui1117]

@thiolliere you probably made a mistake here? you are referencing the same issue smile

could relate #5678

aah :-/ it was this one #5040

Also I have a WIP branch https://github.com/paritytech/substrate/compare/gui-macro-attribute?expand=1

I still have to figure out how we want to do storage declararion (and try to reuse current proc_macro implementation) and add some tests.

[gui1117]

EDIT: see message below

[gui1117]

this is a new list of ideas for decl_storage, then I'll go for implementation and should be quite in good shape IMHO

EDIT: people seems to have voted mainly on 2

1 - do as of now: specific and concise syntax

each storage declared as:
decl_storage!(MyStorage get(fn foo) config(): map u32 => u32)
or multiple one at once:

decl_storage!(
  MyStorage get(fn foo) config(): map u32 => u32;
  MyStorage2 get(fn foo) config(): map u32 => u32;
)

2 - ask user to implement a rust trait helped by macro:

user implement a real rust trait but some associated type are expanded by macro

the full rust trait is:

trait StorageMap {
    /// Key type to insert
    type Key: Codec;

    /// Hasher to use
    type Hasher: StoragHasher;

    /// Query type to store
    type Query: Codec;

    /// If not provided, macro will expand to Query::default().
    ///
    /// Default returned value when storage is empty
    fn default() -> Self::Query;
    // NOTE: this was before the syntax `= ..` before

    /// Automatically filled by macro: if Query is `Option<$something>` then value is only inner
    /// type, otherwise Query == Value
    ///
    /// Value stored on chain.
    type Value: Codec;

    /// Automatically filled by macro (using function default above)
    fn from_optional_value_to_query(v: Option<Self::Value>) -> Self::Query;
    /// Automatically filled by macro.
    fn from_query_to_optional_value(v: Self::Query) -> Option<Self::Value>;

    /// Automatically filled by macro (using the name of the pallet)
    fn module_prefix() -> &'static [u8];

    /// Automatically filled by macro (using the name of the storage)
    fn storage_prefix() -> &'static [u8];

    // All operation on storage are automatically implemented in rust not by macro but at trait
    // definition as currently the case in substrate.

    fn get(..) -> .. { ..}
    fn remove(..) -> .. { ..}
    fn mutate(..) -> .. { ..}
    ...
}

user use it like this:

#[pallet::storagedef] // Expand phantom data automatically
pub struct MyStorage<T>;
#[pallet::storageimpl] // Expand some associated type in the trait automatically
impl<T: Trait> StorageMap for MyStorage<T> {
    type Key = u32;
    type Hasher = Blake2_128;
    type Query =  u32;
    fn default() -> Self::Value { 3u8 }
    .. // Here macro expand prefix and other stuff
}

or this:

#[pallet::storagedef] // Expand phantom data automatically
pub struct MyStorage<T>;
#[pallet::storageimpl] // Expand some associated type in the trait automatically
impl<T: Trait> StorageMap for MyStorage<T> {
    type Key = u32;
    type Hasher = Blake2_128;
    type Query =  Option<u32>;
    .. // Here macro expand default and other stuff
}

3 - same as 2 but with slightly less boilerplate

#[pallet::storage] // create struct, expand some associated type in the trait automatically
impl<T: Trait> StorageMap for MyStorage<T> {
    type Key = u32;
    type Hasher = Blake2_128;
    type Query =  u32;
    fn default() -> Self::Value { 3u8 }
    .. // Here macro expand prefix and other stuff
}

or this:

#[pallet::storage] // create struct, expand some associated type in the trait automatically
impl<T: Trait> StorageMap for MyStorage<T> {
    type Key = u32;
    type Hasher = Blake2_128;
    type Query =  Option<u32>;
    .. // Here macro expand default and other stuff
}

4 - ask user to implement a real rust trait with no help from macro.

user implement a real rust trait, macro use this to implement the full storage map trait.

/// value stored is Value, value queried (with get and put) is Option<Value>
trait StorageMapImplOption: StorageIdentifier {
    type Key: Codec;
    type Hasher: StorageHasher;
    type Value;
}

/// value stored is Value, value queried (with get and put) is Value (or default if None)
trait StorageMapImplUnwrapDefault: StorageIdentifier {
    type Key: Codec;
    type Hasher: StorageHasher;
    type Value;
    fn default() -> Value;
}

/// Implemented by macro using Instance and/or pallet name
trait StorageIdentifier {
    fn module_prefix () -> ...
    fn storage_prefix () -> ...
}

implemented like this:

#[pallet::storagedef] // Expand phantom data automatically, and implement StorageIdentfier.
pub struct MyStorage<T>;
#[pallet::storageimpl] // Expand metadata and implement StorageMap trait which contains method
impl<T: Trait> StorageMapImplImplUnwrapDefault for MyStorage<T> {
    type Key = u32;
    type Hasher = Blake2_128;
    type Value =  u32;
    fn default() -> Self::Value { 3u8 }
}

5 - same as 4 but no need to declare struct

user implement a real rust trait, macro use this to implement the full storage map trait.

// Expand metadata and implement StorageMap trait which contains method
// Expand storage struct and implement StorageIdentifier on it.
#[pallet::storage]
impl<T: Trait> StorageMapImplImplUnwrapDefault for MyStorage<T> {
    type Key = u32;
    type Hasher = Blake2_128;
    type Value =  u32;
    fn default() -> Self::Value { 3u8 }
}

[kianenigma]

Honestly, I am somewhat in favour of 1 from the list of ideas above because all the rest will force each storage to be declared within multiple lines, and I think that will not be desirable.

Indeed not an important factor, but something to consider. All the other proposals will make the code section related to storage at least 5x. This is also slightly bad for readability.

[bkchr]

I also think that 1 is the best option for now.

[gui1117]

thanks to @tomusdrw for giving idea, we can also do something similar to HashMap type:

user would have to write:

#[pallet::storage]
type MyStorageWithOptionQuery = StorageValue<u32, MyStorageWithOptionQueryPrefix>;

#[pallet::storage]
type MyStorageWithDefaultValue = StorageValue<u32, MyStorageWithDefaultValuePrefix, u32>;

parameter_type!(
  pub const MyDefaultGetter: u32 = 3u32;
);
#[pallet::storage]
type MyStorageWithMyDefaultValue = StorageValue<u32, MyStorageWithMyDefaultValuePrefix, u32, MyDefaultGetter>;

and frame-support would write this:

struct GetDefault<T>(core::marker::PhantomData<T>);
trait Getter<T> { // This trait is `Get` in frame-support currently
    fn get() -> T;
}
impl<T: Default> Getter<T> for GetDefault<T> {
    fn get() -> T {
        T::default()
    }
}
struct StorageValue<V, Prefix, Q=Option<V>, D=GetDefault<Q>>(core::marker::PhantomData::<(V, Q, D, Prefix)>);

impl<V, P, D:Getter<Option<V>>> StorageValue<V, P, Option<V>, D> {
    fn get() {}
    fn put() {}
}
impl<V, P, D:Getter<V>> StorageValue<V, P, V, D> {
    fn get() {}
    fn put() {}
}

Full compiling example here: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=4823a6b588378ab7e8de952e12ecae00

EDIT: I already implemented it in the WIP branch, looks very good IMO

[bkchr]

user would have to write:

#[pallet::storage]
type MyStorageWithOptionQuery = StorageValue<u32, MyStorageWithOptionQueryPrefix>;

#[pallet::storage]
type MyStorageWithDefaultValue = StorageValue<u32, MyStorageWithDefaultValuePrefix, u32>;

parameter_type!(
  pub const MyDefaultGetter: u32 = 3u32;
);
#[pallet::storage]
type MyStorageWithMyDefaultValue = StorageValue<u32, MyStorageWithMyDefaultValuePrefix, u32, MyDefaultGetter>;

and frame-support would write this:

Looks good :)

[apopiak]

Can we get a more concrete example?
Would the following translation be correct?
old syntax

decl_storage!{
trait Store for Module<T: Trait> as AuctionManager {
    pub TotalDebitInAuction get(fn total_debit_in_auction): Balance;
    pub DebitAuctions get(fn debit_auctions): map hasher(twox_64_concat) AuctionId =>
			Option<DebitAuctionItem<T::BlockNumber>>;
}
}

-->
new syntax

parameter_type!(
  pub const BalanceDefault: Balance = 0;
);
#[pallet::storage]
type TotalDebitInAuction = StorageValue<Balance, AuctionManager, BalanceDefault>;
#[pallet::storage]
type DebitAuctions = StorageMap<AuctionId, DevitAuctionItem<T::BlockNumber>, AuctionManager, Twox64Concat>;

[gui1117]

@apopiak I updated the WIP branch, at its current shape it would be written:

// Or you can skip this if you consider that the Default implementation of Balance returns already 0.
parameter_type!(
  pub const BalanceDefault: Balance = 0;
);
#[pallet::storage]
type TotalDebitInAuction = StorageValue<TotalDebitInAuctionPrefix, Balance, ValueQuery, BalanceDefault>;

#[pallet::storage]
type DebitAuctions = StorageMap<DebitAuctionsPrefix, Twox64Concat, AuctionId, DevitAuctionItem<T::BlockNumber>>; // By default QueryKind is OptionQuery so no need to specify.

[apopiak]

So TotalDebitInAuctionPrefix and DebitAuctionsPrefix are types injected by the macro and not defined in the pallet?

[gui1117]

So TotalDebitInAuctionPrefix and DebitAuctionsPrefix are types injected by the macro and not defined in the pallet?

indeed the attribute pallet::storage will generate this type. I feel the magic is fairly minimal: just one type is generated, ready to be used.

[apopiak]

So there will still be a (different) place where trait Store for Module<T: Trait> as AuctionManager is specified? Presumably above/in the outer scope so that the macro can know about it?

[gui1117]

yes name of the pallet is specified at the module attribute: #[frame_support::pallet(MyPalletExample)], the store trait is still generated and implemented for module, not sure how to make it obvious for users: maybe I'll add another attribute #[generate_store_trait(Store)] somewhere but as far as I see, if people find Store trait confusing they can just not use it.

[gavofyork]

Looks pretty good mostly. But I really don't like the *Prefix parameter. What's the point of it? When does it ever get altered by the user? It just looks like pointless boilerplate AFACT.

[gui1117]

Looks pretty good mostly. But I really don't like the *Prefix parameter. What's the point of it? When does it ever get altered by the user? It just looks like pointless boilerplate AFACT.

Yes, but I'm not sure how to improve, if the attribute #[pallet::storage] add automatically the generic to the type, I'm afraid people will complain macro expansion is not understandable. I felt like having pallet::storage only generating a ready-to-use type was more idiomatic.

Otherwise I'm still ok to open the discussion about keeping something like decl_storage as first proposition in #5678 (comment)

[bkchr]

Yes, but I'm not sure how to improve, if the attribute #[pallet::storage] add automatically the generic to the type, I'm afraid people will complain macro expansion is not understandable. I felt like having pallet::storage only generating a ready-to-use type was more idiomatic.

What is this prefix parameter actually and what does it contains? And where is this generated?

[gavofyork]

I don't see how adding a completely undefined type into the generic helps make it more idiomatic or easier to understand. Instead it just clutters the definition with repeated information, introducing an additional papercut to code maintenance.

[gui1117]

Yes, but I'm not sure how to improve, if the attribute #[pallet::storage] add automatically the generic to the type, I'm afraid people will complain macro expansion is not understandable. I felt like having pallet::storage only generating a ready-to-use type was more idiomatic.

What is this prefix parameter actually and what does it contains? And where is this generated?

The definition of StorageMapType is at frame/support/src/storage/types.rs https://github.com/paritytech/substrate/pull/6877/files#diff-dab381e3561152742e46e2470692a960R84-R117

/// A type that implements StorageValue when generics are correctly set:
/// * Prefix must implement StorageInstance, a ready-to-use structure implementing StorageInstance
///   is generated by `#[pallet::storage]` in pallet macro.
/// * Value must implement FullCodec
/// * QueryKind must implmeent QueryKindTrait
/// * OnEmpty must implement Get<QueryKind::Query> + 'static
///
/// By default query kind is OptionQuery and OnEmpty returns Default Query (i.e. None for
/// OptionQuery or default value for ValueQuery).
pub struct StorageValueType<Prefix, Value, QueryKind=OptionQuery, OnEmpty=GetDefault>(
	core::marker::PhantomData<(Prefix, Value, QueryKind, OnEmpty)>
);

So if I define the struct with only PhantomData I need some way to get the prefix. This allow to use type MyStorage = ... as regular rust type alias, with only magic that Prefix doesn't need to be written by user.

another idea

Otherwise we could also think of having prefix defined in fields maybe:

pub struct StorageValueType<Value> {
    storage_prefix: &'static str,
    pallet_prefix: &'static str,
    phantom: core::marker::PhantomData<Value>,
}

const MyStorage: StorageValueType<u32> = StorageValueType {
    storage_prefix: "MyStorage",
    pallet_prefix: "MyPalletExample",
    phantom: core::marker::PhantomData,
};

[gui1117]

I don't see how adding a completely undefined type into the generic helps make it more idiomatic or easier to understand.

It moves the magic from the full type expansion to just the generation of one ready-to-use structure.

Instead it just clutters the definition with repeated information, introducing an additional papercut to code maintenance.

We can make the macro check that the use prefixed is the one generated, this can easily be done, so the maintenance cost should be very small.

[gavofyork]

How about this:

#[pallet::type_value] pub type BalanceDefault: Balance = 0;
// Or you can skip this if you consider that the Default implementation of Balance returns already 0.
#[pallet::storage]
type TotalDebitInAuction = StorageValue<_, Balance, ValueQuery, BalanceDefault>;

#[pallet::storage]
type DebitAuctions = StorageMap<_, Twox64Concat, AuctionId, DevitAuctionItem<T::BlockNumber>>; // By default QueryKind is OptionQuery so no need to specify.

[gui1117]

very good idea, I implemented _ for prefix placeholder.
for pallet::type_value I'm not sure yet how to make it parsable by attribute macro but I'm looking into it