Prevent storage item reads/writes at compile time by types that are not whitelisted

[gpestana]

It would be useful to limit at compile time the types that can access (read and/or write) a pallet storage item as a way to ensure safety and encapsulation. This can probably be achieved by augmenting the current outer macro to statically check if the storage item is only accessed by the whitelisted structs.

For example:

#[pallet::storage(allow_only(Ticker))]
pub type Count<T: Config> = StorageValue<_, u32, ValueQuery>;

impl<T: Config> Pallet<T> {
 fn add_count() {
  Count::<T>::mutate(|c| *c + 1) // compile time error because `Count` is accessed outside of the `Ticker` impl.
 }
}

// snip..

pub struct Ticker<T> {
 // snip..
}

impl <T: Config> Ticker<T> {
 fn add_count() {
    // set locks, do checks/ something else that is hard to enforce outside this impl..

    // and then..
    Count::<T>::mutate(|c| *c + 1) // OK, whitelisted
  }
}

This would be very useful to ensure that there is a strict control on how and when a storage item is mutated and/or read. An example of this is paritytech/substrate#14582, where we want to encapsulate all the logic to update the staking ledger (locks, etc) in a struct impl.

The annotation syntax is open for discussion. I think it would be useful to have some degree of granularity and whitelist only reads, writes or reads and write through the annotation.

[sam0x17]

This could probably be accomplished with a syn visitor pattern during the parse phase of #[pallet] 👍🏻

[xlc]

Can we stop adding more macro magics and try to figure out some mechanism using standard Rust concepts? Well, we cannot avoid macro magics but please try to restraint yourself from doing anything crazy with it.

In the example provided, an alternative implementation could be

// the macro already generated `struct Ticker` so we can just extend it
#[pallet::storage(getter = false, setter = false)] // accessors are not public
pub type Ticker<T: Config> = StorageValue<_, u32, ValueQuery>;

impl <T: Config> Ticker<T> {
 fn add_count() {
    // set locks, do checks/ something else that is hard to enforce outside this impl..

    // and then..
    Self::<T>::mutate(|c| *c + 1) // able to access private accessors
  }
}

[DamianStraszak]

How would it interact with migrations though?

[gpestana]

How would it interact with migrations though?

With the visitor pattern approach it should be possible to whitelist migrations as well by default. If the setters and getters are not public, I think that's going to be less straightforward.

[sam0x17]

If we're opposed to building this into the macro system, this could become a lint. Does clippy add the ability to define repo-specific custom syntax checkers?

[bkchr]

The idea from @xlc looks reasonable!