Pallet cannot read the origin it defined itself

[sorpaas]

In palletv2 macro code, Origin type passed to dispatchable is hard-coded to frame_system::pallet_prelude::OriginFor<T>, which is always the system origin. This causes the pallet not able to use the origin it defined itself. This needs to be changed to use the T::Origin type. Functions like pallet_collective::ensure_members won't work before we fix this.

Workaround is to use struct + trait definition and then import it in the top-level runtime, which from what I can see is how all current origin usages work.

blocks polkadot-evm/frontier#482

[sorpaas]

I'm going to temporarily use the struct + trait workaround and revisit this later, given that I'm not really familiar with the procedural macro code of this and this seems like a non-trivial work -- one has to define it similar to how Event type is done in Config, and then decide on using which Origin type in Call.

I'm planning to go back to this later, but before that, I will un-assign myself. In the mean time, if anyone feels like taking this up, please feel free!

[shawntabrizi]

@thiolliere what say you?

[gui1117]

the dispatchable declared in the pallet implements UnfilteredDispatchable with Origin associated type being the runtime origin. This is important so that we can implement Dispatchable in construct_runtime (which takes the as origin the runtime origin).

What you propose is to limit which origin can call into a pallet ? and to limit this to the origin declared by the pallet ?
I'm not against it but this is quite unusual, that means the pallet will no longer be able to declare some dispatchable which have a signed origin.

Maybe the pattern you want is indeed similar to Event declare some associated type in order to convert from the pallet origin into the runtime origin.
I'm not sure it needs macro, writing type Origin: IsType<frame_system::pallet_prelude::OriginFor<T>> + From<Origin> should work.

Just to clarify this also wasn't possible with decl_module, isn't it ? What do I miss ?

[xlc]

this is how we do it https://github.com/open-web3-stack/open-runtime-module-library/blob/37e42936c41dbdbaf0117c628c9eab0e06044844/authority/src/lib.rs#L135-L138

[sorpaas]

Just to clarify this also wasn't possible with decl_module, isn't it?

I think this was possible with decl_module, but I haven't completely tested yet. Back then you have a generic Origin type, and you can define the type constraints it satisfy, so it was possible to add constraints like Into<Result<RawOrigin, OuterOrigin>> and make that usable inside a dispatchable.

With palletv2, this Origin type is forced set to OriginFor<T> and that is <T as frame_system::Config>::Origin, which only takes the system origin's type constraints. It's now impossible to introduce any additional type constraints.

What I'm doing in Frontier is to define a new origin for signed Ethereum transactions. There are many reasons for this, particularly because we cannot treat an Ethereum transaction as a normal "signed" Substrate extrinsic, but have to have its own type. In the past we just treat them as "unsigned" extrinsics, but there were many drawbacks with that so we're working on defining a custom UncheckedExtrinsic and a custom CheckedExtrinsic. The two would handle the custom signature verification logic (among other things) and then invoke the dispatchable with that special origin just for it.

If you want to see this in practice, it's this PR (polkadot-evm/frontier#482). Related to this particular Substrate bug -- think of the following code snippets:

#[derive(PartialEq, Eq, Clone, Encode, Decode, RuntimeDebug)]
pub enum RawOrigin {
	EthereumTransaction(H160),
}

pub fn ensure_ethereum_transaction<OuterOrigin>(o: OuterOrigin) -> Result<H160, &'static str>
where
	OuterOrigin: Into<Result<RawOrigin, OuterOrigin>>,
{
	match o.into() {
		Ok(RawOrigin::EthereumTransaction(n)) => Ok(n),
		_ => Err("bad origin: expected to be an Ethereum transaction"),
	}
}

#[pallet::origin]
pub type Origin = RawOrigin;

#[pallet::config]
pub trait Config {
	type Origin: Into<Result<crate::RawOrigin, <Self as crate::Config>::Origin>>;
}

#[pallet::call]
impl<T: Config> Pallet<T> {
	fn transact(origin: OriginFor<T>, other_parameters) {
		ensure_ethereum_transaction(origin)?; // This line would fail to compile because `Origin` is `frame_system::Config::Origin` and does not implement `Into<Result<crate::RawOrigin, _>>`.
		// other code
	}
}

[sorpaas]

Maybe the pattern you want is indeed similar to Event declare some associated type in order to convert from the pallet origin into the runtime origin.
I'm not sure it needs macro, writing type Origin: IsType<frame_system::pallet_prelude::OriginFor> + From should work.

I don't think that works. I checked the code, we have special handle logic only for type Event, but we do not have any for type Origin. The origin type right now in the dispatchable is always fixed.

[gui1117]

I think this was possible with decl_module, but I haven't completely tested yet. Back then you have a generic Origin type, and you can define the type constraints it satisfy, so it was possible to add constraints like Into<Result<RawOrigin, OuterOrigin>> and make that usable inside a dispatchable.

Considering that construct_runtime will implement:

		impl #scrate::traits::UnfilteredDispatchable for Call {
			type Origin = Origin;
			fn dispatch_bypass_filter(self, origin: Origin) -> #scrate::dispatch::DispatchResultWithPostInfo {
				match self {
					#(
						#variant_patterns =>
							#scrate::traits::UnfilteredDispatchable::dispatch_bypass_filter(call, origin),
					)*
				}
			}
		}

where Origin is the runtime origin, I don't know how a pallet could work in construct_runtime if it doesn't implement UnfilteredDispatchable for the runtime origin.

---

The way I would solve your example code would be:

Solution 1: use the associated type to convert back and forth to the runtime origin

diff --git a/snipet.rs b/snipet.rs
index 7ff671e..a73f972 100644
--- a/snipet.rs
+++ b/snipet.rs
@@ -17,14 +17,17 @@ where
 pub type Origin = RawOrigin;
 
 #[pallet::config]
-pub trait Config {
-       type Origin: Into<Result<crate::RawOrigin, <Self as crate::Config>::Origin>>;
+pub trait Config: frame_system::Config  {
+       /// This type is same as frame_system::Config but we use another associated type here to be
+       /// able to have more bounds without having to put a where clause everywhere.
+       type Origin: IsType<<Self as frame_system::Config>::Origin> + Into<Result<crate::RawOrigin, <Self as crate::Config>::Origin>>;
 }
 
 #[pallet::call]
 impl<T: Config> Pallet<T> {
        fn transact(origin: OriginFor<T>, other_parameters) {
-               ensure_ethereum_transaction(origin)?; // This line would fail to compile because `Origin` is `frame_system::Config::Origin` and does not implement `Into<Result<crate::RawOrigin, _>>`.
+               let converted_origin = <T as Config>::Origin::from(origin);
+               ensure_ethereum_transaction(converted_origin)?;
                // other code
        }
 }

Solution 2: use a where clause

diff --git a/snipet.rs b/snipet.rs
index 7ff671e..0385ec8 100644
--- a/snipet.rs
+++ b/snipet.rs
@@ -17,14 +17,13 @@ where
 pub type Origin = RawOrigin;
 
 #[pallet::config]
-pub trait Config {
-       type Origin: Into<Result<crate::RawOrigin, <Self as crate::Config>::Origin>>;
+pub trait Config where Self::Origin: Into<Result<RawOrigin, OuterOrigin> {
 }
 
 #[pallet::call]
-impl<T: Config> Pallet<T> {
+impl<T: Config> Pallet<T> where T::Origin: Into<Result<RawOrigin, OuterOrigin> {
        fn transact(origin: OriginFor<T>, other_parameters) {
-               ensure_ethereum_transaction(origin)?; // This line would fail to compile because `Origin` is `frame_system::Config::Origin` and does not implement `Into<Result<crate::RawOrigin, _>>`.
+               ensure_ethereum_transaction(origin)?;
                // other code
        }
 }

[sorpaas]

@thiolliere Solution 2

Just a note on this. Setting where clause in Config doesn't seem to work. It looks like that'll end up needing where clauses in all places including storage definitions, event definitions (basically everywhere where T: Config is defined). Just setting the where clause in pallet::call seems to be enough.