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>

Author: gui1117

src/lib.rs

@@ -41,8 +41,8 @@ use sp_runtime::{DispatchResult, traits::{Dispatchable, Zero}};
 use sp_runtime::traits::Member;
 use frame_support::{
 	decl_module, decl_event, decl_error, decl_storage, Parameter, ensure, traits::{
-		Get, ReservableCurrency, Currency, Filter, FilterStack, FilterStackGuard,
-		ClearFilterGuard, InstanceFilter
+		Get, ReservableCurrency, Currency, InstanceFilter,
+		OriginTrait, IsType,
 	}, weights::{GetDispatchInfo, constants::{WEIGHT_PER_MICROS, WEIGHT_PER_NANOS}},
 	dispatch::{PostDispatchInfo, IsSubType},
 };
@@ -60,14 +60,12 @@ 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>> + IsSubType<Module<Self>, Self>;
+		+ GetDispatchInfo + From<frame_system::Call<Self>> + IsSubType<Module<Self>, Self>
+		+ IsType<<Self as frame_system::Trait>::Call>;
 
 	/// The currency mechanism.
 	type Currency: ReservableCurrency<Self::AccountId>;
 
-	/// Is a given call compatible with the proxying subsystem?
-	type IsCallable: FilterStack<<Self as Trait>::Call>;
-
 	/// A kind of proxy; specified with the proxy and passed in to the `IsProxyable` fitler.
 	/// The instance filter determines whether a given call may be proxied under this type.
 	type ProxyType: Parameter + Member + Ord + PartialOrd + InstanceFilter<<Self as Trait>::Call>
@@ -105,8 +103,6 @@ decl_error! {
 		NotFound,
 		/// Sender is not a proxy of the account to be proxied.
 		NotProxy,
-		/// A call with a `false` `IsCallable` filter was attempted.
-		Uncallable,
 		/// A call which is incompatible with the proxy type's filter was attempted.
 		Unproxyable,
 		/// Account is already a proxy.
@@ -171,19 +167,17 @@ decl_module! {
 				.find(|x| &x.0 == &who && force_proxy_type.as_ref().map_or(true, |y| &x.1 == y))
 				.ok_or(Error::<T>::NotProxy)?;
 
-			// We're now executing as a freshly authenticated new account, so the previous call
-			// restrictions no longer apply.
-			let _clear_guard = ClearFilterGuard::<T::IsCallable, <T as Trait>::Call>::new();
-			let _filter_guard = FilterStackGuard::<T::IsCallable, <T as Trait>::Call>::new(
-				move |c| match c.is_sub_type() {
+			// This is a freshly authenticated new account, the origin restrictions doesn't apply.
+			let mut origin: T::Origin = frame_system::RawOrigin::Signed(real).into();
+			origin.add_filter(move |c: &<T as frame_system::Trait>::Call| {
+				let c = <T as Trait>::Call::from_ref(c);
+				match c.is_sub_type() {
 					Some(Call::add_proxy(_, ref pt)) | Some(Call::remove_proxy(_, ref pt))
 						if !proxy_type.is_superset(&pt) => false,
-					_ => proxy_type.filter(&c)
+					_ => proxy_type.filter(c)
 				}
-			);
-			ensure!(T::IsCallable::filter(&call), Error::<T>::Uncallable);
-
-			let e = call.dispatch(frame_system::RawOrigin::Signed(real).into());
+			});
+			let e = call.dispatch(origin);
 			Self::deposit_event(RawEvent::ProxyExecuted(e.map(|_| ()).map_err(|e| e.error)));
 		}
 

src/tests.rs

@@ -23,7 +23,7 @@ use super::*;
 
 use frame_support::{
 	assert_ok, assert_noop, impl_outer_origin, parameter_types, impl_outer_dispatch,
-	impl_filter_stack, weights::Weight, impl_outer_event, RuntimeDebug, dispatch::DispatchError
+	weights::Weight, impl_outer_event, RuntimeDebug, dispatch::DispatchError, traits::Filter,
 };
 use codec::{Encode, Decode};
 use sp_core::H256;
@@ -62,6 +62,7 @@ parameter_types! {
 	pub const AvailableBlockRatio: Perbill = Perbill::one();
 }
 impl frame_system::Trait for Test {
+	type BaseCallFilter = BaseFilter;
 	type Origin = Origin;
 	type Index = u64;
 	type BlockNumber = u64;
@@ -99,15 +100,12 @@ impl pallet_balances::Trait for Test {
 impl pallet_utility::Trait for Test {
 	type Event = TestEvent;
 	type Call = Call;
-	type IsCallable = IsCallable;
 }
 parameter_types! {
 	pub const ProxyDepositBase: u64 = 1;
 	pub const ProxyDepositFactor: u64 = 1;
 	pub const MaxProxies: u16 = 4;
 }
-pub struct IsCallable;
-impl_filter_stack!(crate::tests::IsCallable, crate::tests::BaseFilter, crate::tests::Call, is_callable);
 #[derive(Copy, Clone, Eq, PartialEq, Ord, PartialOrd, Encode, Decode, RuntimeDebug)]
 pub enum ProxyType {
 	Any,
@@ -142,7 +140,6 @@ impl Trait for Test {
 	type Event = TestEvent;
 	type Call = Call;
 	type Currency = Balances;
-	type IsCallable = IsCallable;
 	type ProxyType = ProxyType;
 	type ProxyDepositBase = ProxyDepositBase;
 	type ProxyDepositFactor = ProxyDepositFactor;
@@ -158,7 +155,6 @@ use frame_system::Call as SystemCall;
 use pallet_balances::Call as BalancesCall;
 use pallet_balances::Error as BalancesError;
 use pallet_utility::Call as UtilityCall;
-use pallet_utility::Error as UtilityError;
 use pallet_utility::Event as UtilityEvent;
 use super::Call as ProxyCall;
 
@@ -201,7 +197,8 @@ fn filtering_works() {
 		expect_event(RawEvent::ProxyExecuted(Ok(())));
 		assert_ok!(Proxy::proxy(Origin::signed(3), 1, None, call.clone()));
 		expect_event(RawEvent::ProxyExecuted(Ok(())));
-		assert_noop!(Proxy::proxy(Origin::signed(4), 1, None, call.clone()), Error::<Test>::Uncallable);
+		assert_ok!(Proxy::proxy(Origin::signed(4), 1, None, call.clone()));
+		expect_event(RawEvent::ProxyExecuted(Err(DispatchError::BadOrigin)));
 
 		let sub_id = Utility::sub_account_id(1, 0);
 		Balances::mutate_account(&sub_id, |a| a.free = 1000);
@@ -210,32 +207,41 @@ fn filtering_works() {
 		let call = Box::new(Call::Utility(UtilityCall::as_sub(0, inner.clone())));
 		assert_ok!(Proxy::proxy(Origin::signed(2), 1, None, call.clone()));
 		expect_event(RawEvent::ProxyExecuted(Ok(())));
-		assert_noop!(Proxy::proxy(Origin::signed(3), 1, None, call.clone()), Error::<Test>::Uncallable);
+		assert_ok!(Proxy::proxy(Origin::signed(3), 1, None, call.clone()));
+		expect_event(RawEvent::ProxyExecuted(Err(DispatchError::BadOrigin)));
 		assert_ok!(Proxy::proxy(Origin::signed(4), 1, None, call.clone()));
 		expect_event(RawEvent::ProxyExecuted(Ok(())));
 
 		let call = Box::new(Call::Utility(UtilityCall::as_limited_sub(0, inner.clone())));
 		assert_ok!(Proxy::proxy(Origin::signed(2), 1, None, call.clone()));
 		expect_event(RawEvent::ProxyExecuted(Ok(())));
-		assert_noop!(Proxy::proxy(Origin::signed(3), 1, None, call.clone()), Error::<Test>::Uncallable);
+		assert_ok!(Proxy::proxy(Origin::signed(3), 1, None, call.clone()));
+		expect_event(RawEvent::ProxyExecuted(Err(DispatchError::BadOrigin)));
 		assert_ok!(Proxy::proxy(Origin::signed(4), 1, None, call.clone()));
-		let de = DispatchError::from(UtilityError::<Test>::Uncallable).stripped();
-		expect_event(RawEvent::ProxyExecuted(Err(de)));
+		expect_event(RawEvent::ProxyExecuted(Err(DispatchError::BadOrigin)));
 
 		let call = Box::new(Call::Utility(UtilityCall::batch(vec![*inner])));
 		assert_ok!(Proxy::proxy(Origin::signed(2), 1, None, call.clone()));
 		expect_events(vec![UtilityEvent::BatchCompleted.into(), RawEvent::ProxyExecuted(Ok(())).into()]);
-		assert_noop!(Proxy::proxy(Origin::signed(3), 1, None, call.clone()), Error::<Test>::Uncallable);
+		assert_ok!(Proxy::proxy(Origin::signed(3), 1, None, call.clone()));
+		expect_event(RawEvent::ProxyExecuted(Err(DispatchError::BadOrigin)));
 		assert_ok!(Proxy::proxy(Origin::signed(4), 1, None, call.clone()));
-		expect_events(vec![UtilityEvent::Uncallable(0).into(), RawEvent::ProxyExecuted(Ok(())).into()]);
+		expect_events(vec![
+			UtilityEvent::BatchInterrupted(0, DispatchError::BadOrigin).into(),
+			RawEvent::ProxyExecuted(Ok(())).into(),
+		]);
 
 		let inner = Box::new(Call::Proxy(ProxyCall::add_proxy(5, ProxyType::Any)));
 		let call = Box::new(Call::Utility(UtilityCall::batch(vec![*inner])));
 		assert_ok!(Proxy::proxy(Origin::signed(2), 1, None, call.clone()));
 		expect_events(vec![UtilityEvent::BatchCompleted.into(), RawEvent::ProxyExecuted(Ok(())).into()]);
-		assert_noop!(Proxy::proxy(Origin::signed(3), 1, None, call.clone()), Error::<Test>::Uncallable);
+		assert_ok!(Proxy::proxy(Origin::signed(3), 1, None, call.clone()));
+		expect_event(RawEvent::ProxyExecuted(Err(DispatchError::BadOrigin)));
 		assert_ok!(Proxy::proxy(Origin::signed(4), 1, None, call.clone()));
-		expect_events(vec![UtilityEvent::Uncallable(0).into(), RawEvent::ProxyExecuted(Ok(())).into()]);
+		expect_events(vec![
+			UtilityEvent::BatchInterrupted(0, DispatchError::BadOrigin).into(),
+			RawEvent::ProxyExecuted(Ok(())).into(),
+		]);
 	});
 }
 
@@ -293,10 +299,12 @@ fn proxying_works() {
 		assert_eq!(Balances::free_balance(6), 1);
 
 		let call = Box::new(Call::System(SystemCall::set_code(vec![])));
-		assert_noop!(Proxy::proxy(Origin::signed(3), 1, None, call.clone()), Error::<Test>::Uncallable);
+		assert_ok!(Proxy::proxy(Origin::signed(3), 1, None, call.clone()));
+		expect_event(RawEvent::ProxyExecuted(Err(DispatchError::BadOrigin)));
 
 		let call = Box::new(Call::Balances(BalancesCall::transfer_keep_alive(6, 1)));
-		assert_noop!(Proxy::proxy(Origin::signed(2), 1, None, call.clone()), Error::<Test>::Uncallable);
+		assert_ok!(Call::Proxy(super::Call::proxy(1, None, call.clone())).dispatch(Origin::signed(2)));
+		expect_event(RawEvent::ProxyExecuted(Err(DispatchError::BadOrigin)));
 		assert_ok!(Proxy::proxy(Origin::signed(3), 1, None, call.clone()));
 		expect_event(RawEvent::ProxyExecuted(Ok(())));
 		assert_eq!(Balances::free_balance(6), 2);