Use pallet `trait Config` also for the `Call` interface

[mustermeiszer]

Is there an existing issue?

• [X] I have searched the existing issues

Experiencing problems? Have you tried our Stack Exchange first?

• [X] This is not a support question.

Motivation

Currently, we often need to have some additional logic to happen before a call is executed.

E.g. We want to restrict the address a sender is able to send their native tokens to. In order to archive that we would need to build a wrapper pallet that mimics the pallet-balances API 1:1, and then not expose the Call in the construct_runtime-macro.

While this is definitely feasible, it is a lot of unnesessary overhead.

Request

IMO the best solution would be, if the Call interface of a pallet is part of the trait Config so that consumers of this pallet can implement their own logic or just build a logic before or after the pallets own implementation.

Solution

The following is just a draft and I am not so well versed with how the construct_runtime! macro works and everything else around frame, that this might be impossible to archive. I am guessing the generate enum Call needs a generic and be bound by trait Config too here.

The Pallet

#[pallet::config]
trait Config: frame_system::Config {
    type Balance;

    #[pallet::weight(1)]
    #[pallet::call_index(0)]
    fn transfer(origin: OriginFor<Self>, receiver: Self::AccountId, amount: Self::Balance) -> DispatchResult {
        Pallet::<Self>::transfer(origin, receiver, amount)
    }
}

the trait itself should always have a public default implementation provided by the pallet, allowing for backwards compatibility for runtimes.

The Runtime

construct_runtime!(
    pub enum Runtime where
        Block = Block,
        NodeBlock = cfg_primitives::Block,
        UncheckedExtrinsic = UncheckedExtrinsic
    {
        System: frame_system::{Pallet, Call, Config, Storage, Event<T>} = 0,
                 Balances: pallet_balances::{Pallet, Call, Config, Storage, Event<T>}   = 1.
         }
)

// ... system impl

impl pallet_balances::Config for Runtime {
    type Balance = u128;

    fn transfer(origin: OriginFor<Self>, receiver: Self::AccountId, amount: Self::Balance) -> DispatchResult {
           // Some pre-logic

          Pallet::<Self>::transfer(origin, receiver, amount)?;

           // Some post-logic
    }
}               

Are you willing to help with this request?

Yes!

[mustermeiszer]

Any response here?

[ggwpez]

I think it should be possible to do this with a signed extension that matches on the call and then filters the arguments.

[bkchr]

Yeah signed extension would work here, but only if the call is coming as part of a transaction. So, you could not filter calls that are being dispatched by any other pallet.

[mustermeiszer]

I think it should be possible to do this with a signed extension that matches on the call and then filters the arguments.

• I am curious, how would this play with external integrations? I tried using a SingedExtension but polkadot-js was not able to generate valid signatures. But I did not deeper into this. My assumption would be that every signer needs to know about these new fields that need to part of the signed payload, or can they easily derive them from the metadata?

• Could you change state in pre_dispatch and post_dispatch -- I am assuming this is an anti-pattern, as it would be hard to test

• In general, my approach was to allow to consumers to override pallet logic, while keeping api compatability.

[xlc]

SignedExtension will work. If it does not have extra signing payload, pjs will complaint there is unknown SignedExtension and ignore it but everything should work. And you can change state there.

OO (or mostly inheritance) is now generally considered less ideal design pattern and we should prefer composition. In this particular case, the solution is update pallet-balances to add various hooks to achieve your goal. In fact, it is already possible to customize some behaviour of pallet-balances and we have done that already.

[mustermeiszer]

OO (or mostly inheritance) is now generally considered less ideal design pattern and we should prefer composition. In this particular case, the solution is update pallet-balances to add various hooks to achieve your goal. In fact, it is already possible to customize some behaviour of pallet-balances and we have done that already.

Can you show me where and how you did customize that?

SignedExtension will work. If it does not have extra signing payload, pjs will complaint there is unknown SignedExtension and ignore it but everything should work. And you can change state there.

So, () is assumed as no additional payload?

[mustermeiszer]

And error resolution will be horrible, right? Does the transaction even make it on-chain?

[xlc]

https://github.com/AcalaNetwork/Acala/blob/6559c643f219eebd0b32eeca5e5be303c65beb4e/runtime/karura/src/lib.rs#L371 https://github.com/AcalaNetwork/Acala/blob/6559c643f219eebd0b32eeca5e5be303c65beb4e/modules/support/src/lib.rs#L62 https://github.com/AcalaNetwork/Acala/blob/6559c643f219eebd0b32eeca5e5be303c65beb4e/modules/evm/src/lib.rs#L2094

[mustermeiszer]

Thanks @xlc!

But I disagree, that using a SignedExtension is the right approach, neither do I think that the approach of providing a default implementation for traits can be seen as an OO approach.

The error handling will be insane when having multiple pre_dispatch()/post_dispatch() validation logics. XCM already has the same issues, that errors are badly propagated, how should a non-dev user know why its transaction failed, when it is only a TransactionValidity error that is returned.

My approach would allow to extend existing and integrated apis, on a per call logic, where as, with SignedExtension, there would just be a huge waterfall of unnecessary checks, hard to track what is actually happening in the codebase, and not really reducing complexity.

[mustermeiszer]

Looking at the checks I needed to do in this PR https://github.com/centrifuge/centrifuge-chain/pull/1702/files#diff-b31f5b8861490f29f23b29b470ee104a5cbabdc00278638095b9bbfd8c375354 I even more strongly think the SignedExtension approach is not the right way.

It is so hard to actually recognize breaking changes with this, as the IsSubType logic can never spot a missing match arm. Even if you could match all enum variants, this would look horrible, especially as you would need to match __Ignore also...

In general, I feel that SignedExtension are the wrong approach to have specific checks on specific calls. Its a great, check the general extrinsic approach, but for the rest it is not really usable IMO.

[bkchr]

One thing that I could imagine would be something like this:

#[pallet::config]
trait Config {
    type Sometype;

   /// The macro above actually also adds another method `on_dispatch`.
   /// By default this always implements `Ok(())`.
   fn on_dispatch(origin: &T::Origin, call: &Call) -> Result<(), DispatchError> { Ok()) };
}   

#[pallet::call]
impl<T: Config> Pallet<T> {
    fn some_call() {}
}

pallet::call would then expand to the following code:

impl<T: Config> Dispatchable for Call<T> {
      fn dispatch(self) -> Result<(), DispatchError> {
           T::on_dispatch(&origin, &self)?;

           // dispatch it...
     }
}

This way we could add this without breaking almost no code for anyone. (Would break some code that already has a on_dispatch.) In the runtime you could then implement on_dispatch for every pallet that you like. One problem of this solution is that if someone calls Pallet::some_call aka using the normal function interface, this would not go through on_dispatch. However, this is probably fine and some special case any way.

WDYT @mustermeiszer?

[mustermeiszer]

I like that a lot! That would make quite a few wrapper pallets redundant on our side and we could run a lot of pre-checks automatically upfront!

[mustermeiszer]

Do you think somebody on your side can own this change?

[bkchr]

I had hoped/thought that you would do this. :D

[mustermeiszer]

For sure, I will see when I have time to code again. ^^ Will ping you then.

[AurevoirXavier]

I'm interested in working on this. Where should I start?

[mustermeiszer]

Here: https://github.com/paritytech/polkadot-sdk/issues/2563#issuecomment-1915408620

Best would be start in the macros here IIRC: https://github.com/paritytech/polkadot-sdk/blob/e434176e0867d17336301388b46a6796b366a976/substrate/frame/support/procedural/src/lib.rs#L197-L199 The section where the call attribute is expanded and the trait Dispatchable is implemented.