Better error for when origin filter prevent the call to be dispatched (#10134)

* better error

* Apply suggestions from code review

Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>

* remove unused

* fix error

* fmt

* fix tests

* fmt

* Update frame/contracts/src/exec.rs

Co-authored-by: Alexander Theißen <alex.theissen@me.com>

* fix typo

Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>
Co-authored-by: Alexander Theißen <alex.theissen@me.com>

substrate/frame/contracts/src/exec.rs

@@ -1094,10 +1094,7 @@ mod tests {
 	use pallet_contracts_primitives::ReturnFlags;
 	use pretty_assertions::assert_eq;
 	use sp_core::Bytes;
-	use sp_runtime::{
+	use sp_runtime::{traits::Hash, DispatchError};
-		traits::{BadOrigin, Hash},
-		DispatchError,
-	};
 	use std::{cell::RefCell, collections::HashMap, rc::Rc};
 
 	type System = frame_system::Pallet<Test>;
@@ -2114,7 +2111,10 @@ mod tests {
 			let forbidden_call = Call::Balances(BalanceCall::transfer { dest: CHARLIE, value: 22 });
 
 			// simple cases: direct call
-			assert_err!(ctx.ext.call_runtime(forbidden_call.clone()), BadOrigin);
+			assert_err!(
+				ctx.ext.call_runtime(forbidden_call.clone()),
+				frame_system::Error::<Test>::CallFiltered
+			);
 
 			// as part of a patch: return is OK (but it interrupted the batch)
 			assert_ok!(ctx.ext.call_runtime(Call::Utility(UtilCall::batch {
@@ -2159,7 +2159,7 @@ mod tests {
 						phase: Phase::Initialization,
 						event: MetaEvent::Utility(pallet_utility::Event::BatchInterrupted(
 							1,
-							BadOrigin.into()
+							frame_system::Error::<Test>::CallFiltered.into()
 						),),
 						topics: vec![],
 					},

substrate/frame/multisig/src/tests.rs

@@ -846,7 +846,7 @@ fn multisig_filters() {
 		let call = Box::new(Call::System(frame_system::Call::set_code { code: vec![] }));
 		assert_noop!(
 			Multisig::as_multi_threshold_1(Origin::signed(1), vec![2], call.clone()),
-			DispatchError::BadOrigin,
+			DispatchError::from(frame_system::Error::<Test>::CallFiltered),
 		);
 	});
 }

substrate/frame/proxy/src/tests.rs

@@ -174,6 +174,8 @@ use frame_system::Call as SystemCall;
 use pallet_balances::{Call as BalancesCall, Error as BalancesError, Event as BalancesEvent};
 use pallet_utility::{Call as UtilityCall, Event as UtilityEvent};
 
+type SystemError = frame_system::Error<Test>;
+
 pub fn new_test_ext() -> sp_io::TestExternalities {
 	let mut t = frame_system::GenesisConfig::default().build_storage::<Test>().unwrap();
 	pallet_balances::GenesisConfig::<Test> {
@@ -333,7 +335,9 @@ fn filtering_works() {
 		assert_ok!(Proxy::proxy(Origin::signed(3), 1, None, call.clone()));
 		System::assert_last_event(ProxyEvent::ProxyExecuted(Ok(())).into());
 		assert_ok!(Proxy::proxy(Origin::signed(4), 1, None, call.clone()));
-		System::assert_last_event(ProxyEvent::ProxyExecuted(Err(DispatchError::BadOrigin)).into());
+		System::assert_last_event(
+			ProxyEvent::ProxyExecuted(Err(SystemError::CallFiltered.into())).into(),
+		);
 
 		let derivative_id = Utility::derivative_account_id(1, 0);
 		assert!(Balances::mutate_account(&derivative_id, |a| a.free = 1000).is_ok());
@@ -344,9 +348,13 @@ fn filtering_works() {
 		assert_ok!(Proxy::proxy(Origin::signed(2), 1, None, call.clone()));
 		System::assert_last_event(ProxyEvent::ProxyExecuted(Ok(())).into());
 		assert_ok!(Proxy::proxy(Origin::signed(3), 1, None, call.clone()));
-		System::assert_last_event(ProxyEvent::ProxyExecuted(Err(DispatchError::BadOrigin)).into());
+		System::assert_last_event(
+			ProxyEvent::ProxyExecuted(Err(SystemError::CallFiltered.into())).into(),
+		);
 		assert_ok!(Proxy::proxy(Origin::signed(4), 1, None, call.clone()));
-		System::assert_last_event(ProxyEvent::ProxyExecuted(Err(DispatchError::BadOrigin)).into());
+		System::assert_last_event(
+			ProxyEvent::ProxyExecuted(Err(SystemError::CallFiltered.into())).into(),
+		);
 
 		let call = Box::new(Call::Utility(UtilityCall::batch { calls: vec![*inner] }));
 		assert_ok!(Proxy::proxy(Origin::signed(2), 1, None, call.clone()));
@@ -355,10 +363,12 @@ fn filtering_works() {
 			ProxyEvent::ProxyExecuted(Ok(())).into(),
 		]);
 		assert_ok!(Proxy::proxy(Origin::signed(3), 1, None, call.clone()));
-		System::assert_last_event(ProxyEvent::ProxyExecuted(Err(DispatchError::BadOrigin)).into());
+		System::assert_last_event(
+			ProxyEvent::ProxyExecuted(Err(SystemError::CallFiltered.into())).into(),
+		);
 		assert_ok!(Proxy::proxy(Origin::signed(4), 1, None, call.clone()));
 		expect_events(vec![
-			UtilityEvent::BatchInterrupted(0, DispatchError::BadOrigin).into(),
+			UtilityEvent::BatchInterrupted(0, SystemError::CallFiltered.into()).into(),
 			ProxyEvent::ProxyExecuted(Ok(())).into(),
 		]);
 
@@ -371,18 +381,24 @@ fn filtering_works() {
 			ProxyEvent::ProxyExecuted(Ok(())).into(),
 		]);
 		assert_ok!(Proxy::proxy(Origin::signed(3), 1, None, call.clone()));
-		System::assert_last_event(ProxyEvent::ProxyExecuted(Err(DispatchError::BadOrigin)).into());
+		System::assert_last_event(
+			ProxyEvent::ProxyExecuted(Err(SystemError::CallFiltered.into())).into(),
+		);
 		assert_ok!(Proxy::proxy(Origin::signed(4), 1, None, call.clone()));
 		expect_events(vec![
-			UtilityEvent::BatchInterrupted(0, DispatchError::BadOrigin).into(),
+			UtilityEvent::BatchInterrupted(0, SystemError::CallFiltered.into()).into(),
 			ProxyEvent::ProxyExecuted(Ok(())).into(),
 		]);
 
 		let call = Box::new(Call::Proxy(ProxyCall::remove_proxies {}));
 		assert_ok!(Proxy::proxy(Origin::signed(3), 1, None, call.clone()));
-		System::assert_last_event(ProxyEvent::ProxyExecuted(Err(DispatchError::BadOrigin)).into());
+		System::assert_last_event(
+			ProxyEvent::ProxyExecuted(Err(SystemError::CallFiltered.into())).into(),
+		);
 		assert_ok!(Proxy::proxy(Origin::signed(4), 1, None, call.clone()));
-		System::assert_last_event(ProxyEvent::ProxyExecuted(Err(DispatchError::BadOrigin)).into());
+		System::assert_last_event(
+			ProxyEvent::ProxyExecuted(Err(SystemError::CallFiltered.into())).into(),
+		);
 		assert_ok!(Proxy::proxy(Origin::signed(2), 1, None, call.clone()));
 		expect_events(vec![
 			BalancesEvent::<Test>::Unreserved(1, 5).into(),
@@ -462,13 +478,17 @@ fn proxying_works() {
 
 		let call = Box::new(Call::System(SystemCall::set_code { code: vec![] }));
 		assert_ok!(Proxy::proxy(Origin::signed(3), 1, None, call.clone()));
-		System::assert_last_event(ProxyEvent::ProxyExecuted(Err(DispatchError::BadOrigin)).into());
+		System::assert_last_event(
+			ProxyEvent::ProxyExecuted(Err(SystemError::CallFiltered.into())).into(),
+		);
 
 		let call =
 			Box::new(Call::Balances(BalancesCall::transfer_keep_alive { dest: 6, value: 1 }));
 		assert_ok!(Call::Proxy(super::Call::new_call_variant_proxy(1, None, call.clone()))
 			.dispatch(Origin::signed(2)));
-		System::assert_last_event(ProxyEvent::ProxyExecuted(Err(DispatchError::BadOrigin)).into());
+		System::assert_last_event(
+			ProxyEvent::ProxyExecuted(Err(SystemError::CallFiltered.into())).into(),
+		);
 		assert_ok!(Proxy::proxy(Origin::signed(3), 1, None, call.clone()));
 		System::assert_last_event(ProxyEvent::ProxyExecuted(Ok(())).into());
 		assert_eq!(Balances::free_balance(6), 2);

substrate/frame/support/procedural/src/construct_runtime/expand/call.rs

@@ -22,6 +22,7 @@ use syn::Ident;
 
 pub fn expand_outer_dispatch(
 	runtime: &Ident,
+	system_pallet: &Pallet,
 	pallet_decls: &[Pallet],
 	scrate: &TokenStream,
 ) -> TokenStream {
@@ -29,6 +30,7 @@ pub fn expand_outer_dispatch(
 	let mut variant_patterns = Vec::new();
 	let mut query_call_part_macros = Vec::new();
 	let mut pallet_names = Vec::new();
+	let system_path = &system_pallet.path;
 
 	let pallets_with_call = pallet_decls.iter().filter(|decl| decl.exists_part("Call"));
 
@@ -106,7 +108,9 @@ pub fn expand_outer_dispatch(
 			type PostInfo = #scrate::weights::PostDispatchInfo;
 			fn dispatch(self, origin: Origin) -> #scrate::dispatch::DispatchResultWithPostInfo {
 				if !<Self::Origin as #scrate::traits::OriginTrait>::filter_call(&origin, &self) {
-					return #scrate::sp_std::result::Result::Err(#scrate::dispatch::DispatchError::BadOrigin.into());
+					return #scrate::sp_std::result::Result::Err(
+						#system_path::Error::<#runtime>::CallFiltered.into()
+					);
 				}
 
 				#scrate::traits::UnfilteredDispatchable::dispatch_bypass_filter(self, origin)

substrate/frame/support/procedural/src/construct_runtime/expand/origin.rs

@@ -18,23 +18,14 @@
 use crate::construct_runtime::{Pallet, SYSTEM_PALLET_NAME};
 use proc_macro2::TokenStream;
 use quote::quote;
-use syn::{token, Generics, Ident};
+use syn::{Generics, Ident};
 
 pub fn expand_outer_origin(
 	runtime: &Ident,
+	system_pallet: &Pallet,
 	pallets: &[Pallet],
-	pallets_token: token::Brace,
 	scrate: &TokenStream,
 ) -> syn::Result<TokenStream> {
-	let system_pallet =
-		pallets.iter().find(|decl| decl.name == SYSTEM_PALLET_NAME).ok_or_else(|| {
-			syn::Error::new(
-				pallets_token.span,
-				"`System` pallet declaration is missing. \
-			 Please add this line: `System: frame_system::{Pallet, Call, Storage, Config, Event<T>},`",
-			)
-		})?;
-
 	let mut caller_variants = TokenStream::new();
 	let mut pallet_conversions = TokenStream::new();
 	let mut query_origin_part_macros = Vec::new();

substrate/frame/support/procedural/src/construct_runtime/mod.rs

@@ -214,17 +214,26 @@ fn construct_runtime_final_expansion(
 		pallets_token,
 	} = definition;
 
+	let system_pallet =
+		pallets.iter().find(|decl| decl.name == SYSTEM_PALLET_NAME).ok_or_else(|| {
+			syn::Error::new(
+				pallets_token.span,
+				"`System` pallet declaration is missing. \
+			 Please add this line: `System: frame_system::{Pallet, Call, Storage, Config, Event<T>},`",
+			)
+		})?;
+
 	let hidden_crate_name = "construct_runtime";
 	let scrate = generate_crate_access(&hidden_crate_name, "frame-support");
 	let scrate_decl = generate_hidden_includes(&hidden_crate_name, "frame-support");
 
 	let outer_event = expand::expand_outer_event(&name, &pallets, &scrate)?;
 
-	let outer_origin = expand::expand_outer_origin(&name, &pallets, pallets_token, &scrate)?;
+	let outer_origin = expand::expand_outer_origin(&name, &system_pallet, &pallets, &scrate)?;
 	let all_pallets = decl_all_pallets(&name, pallets.iter());
 	let pallet_to_index = decl_pallet_runtime_setup(&name, &pallets, &scrate);
 
-	let dispatch = expand::expand_outer_dispatch(&name, &pallets, &scrate);
+	let dispatch = expand::expand_outer_dispatch(&name, &system_pallet, &pallets, &scrate);
 	let metadata = expand::expand_runtime_metadata(&name, &pallets, &scrate, &unchecked_extrinsic);
 	let outer_config = expand::expand_outer_config(&name, &pallets, &scrate);
 	let inherent =

substrate/frame/support/test/tests/system.rs

@@ -63,7 +63,9 @@ frame_support::decl_error! {
 		TestError,
 		/// Error documentation
 		/// with multiple lines
-		AnotherError
+		AnotherError,
+		// Required by construct_runtime
+		CallFiltered,
 	}
 }
 

substrate/frame/system/src/lib.rs

@@ -560,6 +560,8 @@ pub mod pallet {
 		NonDefaultComposite,
 		/// There is a non-zero reference count preventing the account from being purged.
 		NonZeroRefCount,
+		/// The origin filter prevent the call to be dispatched.
+		CallFiltered,
 	}
 
 	/// Exposed trait-generic origin type.

substrate/frame/utility/src/tests.rs

@@ -288,7 +288,7 @@ fn as_derivative_filters() {
 					value: 1
 				})),
 			),
-			DispatchError::BadOrigin
+			DispatchError::from(frame_system::Error::<Test>::CallFiltered),
 		);
 	});
 }
@@ -338,7 +338,8 @@ fn batch_with_signed_filters() {
 			vec![Call::Balances(pallet_balances::Call::transfer_keep_alive { dest: 2, value: 1 })]
 		),);
 		System::assert_last_event(
-			utility::Event::BatchInterrupted(0, DispatchError::BadOrigin).into(),
+			utility::Event::BatchInterrupted(0, frame_system::Error::<Test>::CallFiltered.into())
+				.into(),
 		);
 	});
 }
@@ -573,7 +574,7 @@ fn batch_all_does_not_nest() {
 					actual_weight: Some(<Test as Config>::WeightInfo::batch_all(1) + info.weight),
 					pays_fee: Pays::Yes
 				},
-				error: DispatchError::BadOrigin,
+				error: frame_system::Error::<Test>::CallFiltered.into(),
 			}
 		);
 
@@ -585,7 +586,8 @@ fn batch_all_does_not_nest() {
 		// and balances.
 		assert_ok!(Utility::batch_all(Origin::signed(1), vec![batch_nested]));
 		System::assert_has_event(
-			utility::Event::BatchInterrupted(0, DispatchError::BadOrigin).into(),
+			utility::Event::BatchInterrupted(0, frame_system::Error::<Test>::CallFiltered.into())
+				.into(),
 		);
 		assert_eq!(Balances::free_balance(1), 10);
 		assert_eq!(Balances::free_balance(2), 10);