Introduce in-origin filtering (#6318)

* impl filter in origin

* remove IsCallable usage. Breaking: utility::batch(root, calls) no longer bypass BasicCallFilter

* rename BasicCallFilter -> BaseCallFilter

* refactor code

* Apply suggestions from code review

Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com>

* remove forgotten temporar comment

* better add suggestion in another PR

* refactor: use Clone instead of mem::replace

* fix tests

* fix tests

* fix tests

* fix benchmarks

* Make root bypass filter in utility::batch

* fix unused imports

Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com>

substrate/frame/utility/src/lib.rs

@@ -54,8 +54,9 @@ use sp_std::prelude::*;
 use codec::{Encode, Decode};
 use sp_core::TypeId;
 use sp_io::hashing::blake2_256;
-use frame_support::{decl_module, decl_event, decl_error, decl_storage, Parameter, ensure};
-use frame_support::{traits::{Filter, FilterStack, ClearFilterGuard},
+use frame_support::{decl_module, decl_event, decl_storage, Parameter};
+use frame_support::{
+	traits::{OriginTrait, UnfilteredDispatchable},
 	weights::{Weight, GetDispatchInfo, DispatchClass}, dispatch::PostDispatchInfo,
 };
 use frame_system::{self as system, ensure_signed, ensure_root};
@@ -71,23 +72,14 @@ pub trait Trait: frame_system::Trait {
 
 	/// The overarching call type.
 	type Call: Parameter + Dispatchable<Origin=Self::Origin, PostInfo=PostDispatchInfo>
-		+ GetDispatchInfo + From<frame_system::Call<Self>>;
-
-	/// Is a given call compatible with the proxying subsystem?
-	type IsCallable: FilterStack<<Self as Trait>::Call>;
+		+ GetDispatchInfo + From<frame_system::Call<Self>>
+		+ UnfilteredDispatchable<Origin=Self::Origin>;
 }
 
 decl_storage! {
 	trait Store for Module<T: Trait> as Utility {}
 }
 
-decl_error! {
-	pub enum Error for Module<T: Trait> {
-		/// A call with a `false` `IsCallable` filter was attempted.
-		Uncallable,
-	}
-}
-
 decl_event! {
 	/// Events type.
 	pub enum Event {
@@ -96,8 +88,6 @@ decl_event! {
 		BatchInterrupted(u32, DispatchError),
 		/// Batch of dispatches completed fully with no error.
 		BatchCompleted,
-		/// A call with a `false` IsCallable filter was attempted.
-		Uncallable(u32),
 	}
 }
 
@@ -111,20 +101,18 @@ impl TypeId for IndexedUtilityModuleId {
 
 decl_module! {
 	pub struct Module<T: Trait> for enum Call where origin: T::Origin {
-		type Error = Error<T>;
-
 		/// Deposit one of this module's events by using the default implementation.
 		fn deposit_event() = default;
 
 		/// Send a batch of dispatch calls.
 		///
-		/// This will execute until the first one fails and then stop. Calls must fulfil the
-		/// `IsCallable` filter unless the origin is `Root`.
-		///
 		/// May be called from any origin.
 		///
 		/// - `calls`: The calls to be dispatched from the same origin.
 		///
+		/// If origin is root then call are dispatch without checking origin filter. (This includes
+		/// bypassing `frame_system::Trait::BaseCallFilter`).
+		///
 		/// # <weight>
 		/// - Base weight: 14.39 + .987 * c µs
 		/// - Plus the sum of the weights of the `calls`.
@@ -154,11 +142,11 @@ decl_module! {
 		fn batch(origin, calls: Vec<<T as Trait>::Call>) {
 			let is_root = ensure_root(origin.clone()).is_ok();
 			for (index, call) in calls.into_iter().enumerate() {
-				if !is_root && !T::IsCallable::filter(&call) {
-					Self::deposit_event(Event::Uncallable(index as u32));
-					return Ok(())
-				}
-				let result = call.dispatch(origin.clone());
+				let result = if is_root {
+					call.dispatch_bypass_filter(origin.clone())
+				} else {
+					call.dispatch(origin.clone())
+				};
 				if let Err(e) = result {
 					Self::deposit_event(Event::BatchInterrupted(index as u32, e.error));
 					return Ok(());
@@ -169,9 +157,6 @@ decl_module! {
 
 		/// Send a call through an indexed pseudonym of the sender.
 		///
-		/// The call must fulfil only the pre-cleared `IsCallable` filter (i.e. only the level of
-		/// filtering that remains after calling `take()`).
-		///
 		/// NOTE: If you need to ensure that any account-based filtering is honored (i.e. because
 		/// you expect `proxy` to have been used prior in the call stack and you want it to apply to
 		/// any sub-accounts), then use `as_limited_sub` instead.
@@ -188,10 +173,8 @@ decl_module! {
 		)]
 		fn as_sub(origin, index: u16, call: Box<<T as Trait>::Call>) -> DispatchResult {
 			let who = ensure_signed(origin)?;
-			// We're now executing as a freshly authenticated new account, so the previous call
-			// restrictions no longer apply.
-			let _guard = ClearFilterGuard::<T::IsCallable, <T as Trait>::Call>::new();
-			ensure!(T::IsCallable::filter(&call), Error::<T>::Uncallable);
+
+			// This is a freshly authenticated new account, the origin restrictions doesn't apply.
 			let pseudonym = Self::sub_account_id(who, index);
 			call.dispatch(frame_system::RawOrigin::Signed(pseudonym).into())
 				.map(|_| ()).map_err(|e| e.error)
@@ -199,7 +182,8 @@ decl_module! {
 
 		/// Send a call through an indexed pseudonym of the sender.
 		///
-		/// Calls must each fulfil the `IsCallable` filter; it is not cleared before.
+		/// Filter from origin are passed along. The call will be dispatched with an origin which
+		/// use the same filter as the origin of this call.
 		///
 		/// NOTE: If you need to ensure that any account-based filtering is not honored (i.e.
 		/// because you expect `proxy` to have been used prior in the call stack and you do not want
@@ -216,11 +200,11 @@ decl_module! {
 			call.get_dispatch_info().class,
 		)]
 		fn as_limited_sub(origin, index: u16, call: Box<<T as Trait>::Call>) -> DispatchResult {
-			let who = ensure_signed(origin)?;
-			ensure!(T::IsCallable::filter(&call), Error::<T>::Uncallable);
+			let mut origin = origin;
+			let who = ensure_signed(origin.clone())?;
 			let pseudonym = Self::sub_account_id(who, index);
-			call.dispatch(frame_system::RawOrigin::Signed(pseudonym).into())
-				.map(|_| ()).map_err(|e| e.error)
+			origin.set_caller_from(frame_system::RawOrigin::Signed(pseudonym));
+			call.dispatch(origin).map(|_| ()).map_err(|e| e.error)
 		}
 	}
 }

substrate/frame/utility/src/tests.rs

@@ -23,7 +23,7 @@ use super::*;
 
 use frame_support::{
 	assert_ok, assert_noop, impl_outer_origin, parameter_types, impl_outer_dispatch,
-	weights::Weight, impl_outer_event
+	weights::Weight, impl_outer_event, dispatch::DispatchError, traits::Filter, storage,
 };
 use sp_core::H256;
 use sp_runtime::{Perbill, traits::{BlakeTwo256, IdentityLookup}, testing::Header};
@@ -59,6 +59,7 @@ parameter_types! {
 	pub const AvailableBlockRatio: Perbill = Perbill::one();
 }
 impl frame_system::Trait for Test {
+	type BaseCallFilter = TestBaseCallFilter;
 	type Origin = Origin;
 	type Index = u64;
 	type BlockNumber = u64;
@@ -98,8 +99,8 @@ parameter_types! {
 	pub const MultisigDepositFactor: u64 = 1;
 	pub const MaxSignatories: u16 = 3;
 }
-pub struct TestIsCallable;
-impl Filter<Call> for TestIsCallable {
+pub struct TestBaseCallFilter;
+impl Filter<Call> for TestBaseCallFilter {
 	fn filter(c: &Call) -> bool {
 		match *c {
 			Call::Balances(_) => true,
@@ -107,17 +108,9 @@ impl Filter<Call> for TestIsCallable {
 		}
 	}
 }
-impl FilterStack<Call> for TestIsCallable {
-	type Stack = ();
-	fn push(_: impl Fn(&Call) -> bool + 'static) {}
-	fn pop() {}
-	fn take() -> Self::Stack { () }
-	fn restore(_: Self::Stack) {}
-}
 impl Trait for Test {
 	type Event = TestEvent;
 	type Call = Call;
-	type IsCallable = TestIsCallable;
 }
 type System = frame_system::Module<Test>;
 type Balances = pallet_balances::Module<Test>;
@@ -171,21 +164,26 @@ fn as_sub_filters() {
 			Origin::signed(1),
 			1,
 			Box::new(Call::System(frame_system::Call::remark(vec![]))),
-		), Error::<Test>::Uncallable);
+		), DispatchError::BadOrigin);
 	});
 }
 
 #[test]
 fn batch_with_root_works() {
 	new_test_ext().execute_with(|| {
+		let k = b"a".to_vec();
+		let call = Call::System(frame_system::Call::set_storage(vec![(k.clone(), k.clone())]));
+		assert!(!TestBaseCallFilter::filter(&call));
 		assert_eq!(Balances::free_balance(1), 10);
 		assert_eq!(Balances::free_balance(2), 10);
-		assert_ok!(Utility::batch(Origin::ROOT, vec![
+		assert_ok!(Utility::batch(Origin::root(), vec![
 			Call::Balances(BalancesCall::force_transfer(1, 2, 5)),
-			Call::Balances(BalancesCall::force_transfer(1, 2, 5))
+			Call::Balances(BalancesCall::force_transfer(1, 2, 5)),
+			call, // Check filters are correctly bypassed
 		]));
 		assert_eq!(Balances::free_balance(1), 0);
 		assert_eq!(Balances::free_balance(2), 20);
+		assert_eq!(storage::unhashed::get_raw(&k), Some(k));
 	});
 }
 
@@ -213,7 +211,7 @@ fn batch_with_signed_filters() {
 				Call::System(frame_system::Call::remark(vec![]))
 			]),
 		);
-		expect_event(Event::Uncallable(0));
+		expect_event(Event::BatchInterrupted(0, DispatchError::BadOrigin));
 	});
 }