Filter calls in utility (#6131)

* Filter calls.

* Remove old proxy code

* Docs and repot

* Update frame/utility/src/tests.rs

Co-authored-by: Marcio Diaz <marcio.diaz@gmail.com>

* fix test

* Grumble

* Bump runtime version

* fix

* Attempt general fix

Co-authored-by: Marcio Diaz <marcio.diaz@gmail.com>
Co-authored-by: NikVolf <nikvolf@gmail.com>

bin/node/cli/tests/build_spec_works.rs

@@ -25,7 +25,7 @@ fn build_spec_works() {
 	let base_path = tempdir().expect("could not create a temp dir");
 
 	let output = Command::new(cargo_bin("substrate"))
-		.args(&["build-spec", "--rc1", "-d"])
+		.args(&["build-spec", "--dev", "-d"])
 		.arg(base_path.path())
 		.output()
 		.unwrap();

bin/node/cli/tests/check_block_works.rs

@@ -31,7 +31,7 @@ fn check_block_works() {
 	common::run_dev_node_for_a_while(base_path.path());
 
 	let status = Command::new(cargo_bin("substrate"))
-		.args(&["check-block", "--rc1", "--pruning", "archive", "-d"])
+		.args(&["check-block", "--dev", "--pruning", "archive", "-d"])
 		.arg(base_path.path())
 		.arg("1")
 		.status()

bin/node/cli/tests/common.rs

@@ -51,7 +51,7 @@ pub fn run_dev_node_for_a_while(base_path: &Path) {
 	let mut cmd = Command::new(cargo_bin("substrate"));
 
 	let mut cmd = cmd
-		.args(&["--rc1"])
+		.args(&["--dev"])
 		.arg("-d")
 		.arg(base_path)
 		.spawn()

bin/node/cli/tests/export_import_flow.rs

@@ -82,8 +82,8 @@ impl<'a> ExportImportRevertExecutor<'a> {
 		let sub_command_str = sub_command.to_string();
 		// Adding "--binary" if need be.
 		let arguments: Vec<&str> = match format_opt {
-			FormatOpt::Binary => vec![&sub_command_str, "--rc1", "--pruning", "archive", "--binary", "-d"],
-			FormatOpt::Json => vec![&sub_command_str, "--rc1", "--pruning", "archive", "-d"],
+			FormatOpt::Binary => vec![&sub_command_str, "--dev", "--pruning", "archive", "--binary", "-d"],
+			FormatOpt::Json => vec![&sub_command_str, "--dev", "--pruning", "archive", "-d"],
 		};
 
 		let tmp: TempDir;
@@ -136,7 +136,7 @@ impl<'a> ExportImportRevertExecutor<'a> {
 		let _ = fs::remove_dir_all(&self.db_path);
 	}
 
-	/// Runs the `import-blocks` command, asserting that an error was found or 
+	/// Runs the `import-blocks` command, asserting that an error was found or
 	/// not depending on `expected_to_fail`.
 	fn run_import(&mut self, fmt_opt: FormatOpt, expected_to_fail: bool) {
 		let log = self.run_block_command(SubCommand::ImportBlocks, fmt_opt, expected_to_fail);
@@ -166,7 +166,7 @@ impl<'a> ExportImportRevertExecutor<'a> {
 	/// Runs the `revert` command.
 	fn run_revert(&self) {
 		let output = Command::new(cargo_bin("substrate"))
-			.args(&["revert", "--rc1", "--pruning", "archive", "-d"])
+			.args(&["revert", "--dev", "--pruning", "archive", "-d"])
 			.arg(&self.base_path.path())
 			.output()
 			.unwrap();

bin/node/cli/tests/inspect_works.rs

@@ -31,7 +31,7 @@ fn inspect_works() {
 	common::run_dev_node_for_a_while(base_path.path());
 
 	let status = Command::new(cargo_bin("substrate"))
-		.args(&["inspect", "--rc1", "--pruning", "archive", "-d"])
+		.args(&["inspect", "--dev", "--pruning", "archive", "-d"])
 		.arg(base_path.path())
 		.args(&["block", "1"])
 		.status()

bin/node/cli/tests/purge_chain_works.rs

@@ -30,7 +30,7 @@ fn purge_chain_works() {
 	common::run_dev_node_for_a_while(base_path.path());
 
 	let status = Command::new(cargo_bin("substrate"))
-		.args(&["purge-chain", "--rc1", "-d"])
+		.args(&["purge-chain", "--dev", "-d"])
 		.arg(base_path.path())
 		.arg("-y")
 		.status()

bin/node/cli/tests/running_the_node_and_interrupt.rs

@@ -31,7 +31,7 @@ fn running_the_node_works_and_can_be_interrupted() {
 	fn run_command_and_kill(signal: Signal) {
 		let base_path = tempdir().expect("could not create a temp dir");
 		let mut cmd = Command::new(cargo_bin("substrate"))
-			.args(&["--rc1", "-d"])
+			.args(&["--dev", "-d"])
 			.arg(base_path.path())
 			.spawn()
 			.unwrap();

bin/node/runtime/src/lib.rs

@@ -93,8 +93,8 @@ pub const VERSION: RuntimeVersion = RuntimeVersion {
 	// and set impl_version to 0. If only runtime
 	// implementation changes and behavior does not, then leave spec_version as
 	// is and increment impl_version.
-	spec_version: 250,
-	impl_version: 2,
+	spec_version: 251,
+	impl_version: 0,
 	apis: RUNTIME_API_VERSIONS,
 	transaction_version: 1,
 };
@@ -179,6 +179,7 @@ impl pallet_utility::Trait for Runtime {
 	type MultisigDepositBase = MultisigDepositBase;
 	type MultisigDepositFactor = MultisigDepositFactor;
 	type MaxSignatories = MaxSignatories;
+	type IsCallable = ();
 }
 
 parameter_types! {

frame/support/src/traits.rs

@@ -33,6 +33,16 @@ use crate::storage::StorageMap;
 use crate::weights::Weight;
 use impl_trait_for_tuples::impl_for_tuples;
 
+/// Simple trait for providing a filter over a reference to some type.
+pub trait Filter<T> {
+	/// Determine if a given value should be allowed through the filter (returns `true`) or not.
+	fn filter(_: &T) -> bool;
+}
+
+impl<T> Filter<T> for () {
+	fn filter(_: &T) -> bool { true }
+}
+
 /// An abstraction of a value stored within storage, but possibly as part of a larger composite
 /// item.
 pub trait StoredMap<K, T> {

frame/utility/src/lib.rs

@@ -67,11 +67,11 @@ 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, RuntimeDebug};
-use frame_support::{traits::{Get, ReservableCurrency, Currency},
+use frame_support::{traits::{Get, ReservableCurrency, Currency, Filter},
 	weights::{Weight, GetDispatchInfo, DispatchClass, FunctionOf, Pays},
 	dispatch::{DispatchResultWithPostInfo, DispatchErrorWithPostInfo, PostDispatchInfo},
 };
-use frame_system::{self as system, ensure_signed};
+use frame_system::{self as system, ensure_signed, ensure_root};
 use sp_runtime::{DispatchError, DispatchResult, traits::Dispatchable};
 
 mod tests;
@@ -85,7 +85,8 @@ pub trait Trait: frame_system::Trait {
 	type Event: From<Event<Self>> + Into<<Self as frame_system::Trait>::Event>;
 
 	/// The overarching call type.
-	type Call: Parameter + Dispatchable<Origin=Self::Origin, PostInfo=PostDispatchInfo> + GetDispatchInfo + From<frame_system::Call<Self>>;
+	type Call: Parameter + Dispatchable<Origin=Self::Origin, PostInfo=PostDispatchInfo>
+		+ GetDispatchInfo + From<frame_system::Call<Self>>;
 
 	/// The currency mechanism.
 	type Currency: ReservableCurrency<Self::AccountId>;
@@ -103,6 +104,9 @@ pub trait Trait: frame_system::Trait {
 
 	/// The maximum amount of signatories allowed in the multisig.
 	type MaxSignatories: Get<u16>;
+
+	/// Is a given call compatible with the proxying subsystem?
+	type IsCallable: Filter<<Self as Trait>::Call>;
 }
 
 /// A global extrinsic index, formed as the extrinsic index within a block, together with that
@@ -164,6 +168,8 @@ decl_error! {
 		WrongTimepoint,
 		/// A timepoint was given, yet no multisig operation is underway.
 		UnexpectedTimepoint,
+		/// A call with a `false` IsCallable filter was attempted.
+		Uncallable,
 	}
 }
 
@@ -191,6 +197,8 @@ decl_event! {
 		/// A multisig operation has been cancelled. First param is the account that is
 		/// cancelling, third is the multisig account, fourth is hash of the call.
 		MultisigCancelled(AccountId, Timepoint<BlockNumber>, AccountId, CallHash),
+		/// A call with a `false` IsCallable filter was attempted.
+		Uncallable(u32),
 	}
 }
 
@@ -230,7 +238,8 @@ decl_module! {
 
 		/// Send a batch of dispatch calls.
 		///
-		/// This will execute until the first one fails and then stop.
+		/// 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.
 		///
@@ -266,7 +275,12 @@ decl_module! {
 			Pays::Yes,
 		)]
 		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::<T>::Uncallable(index as u32));
+					return Ok(())
+				}
 				let result = call.dispatch(origin.clone());
 				if let Err(e) = result {
 					Self::deposit_event(Event::<T>::BatchInterrupted(index as u32, e.error));
@@ -278,6 +292,8 @@ decl_module! {
 
 		/// Send a call through an indexed pseudonym of the sender.
 		///
+		/// Calls must each fulfil the `IsCallable` filter.
+		///
 		/// The dispatch origin for this call must be _Signed_.
 		///
 		/// # <weight>
@@ -293,6 +309,7 @@ decl_module! {
 		)]
 		fn as_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 pseudonym = Self::sub_account_id(who, index);
 			call.dispatch(frame_system::RawOrigin::Signed(pseudonym).into())
 				.map(|_| ()).map_err(|e| e.error)
@@ -301,7 +318,8 @@ decl_module! {
 		/// Register approval for a dispatch to be made from a deterministic composite account if
 		/// approved by a total of `threshold - 1` of `other_signatories`.
 		///
-		/// If there are enough, then dispatch the call.
+		/// If there are enough, then dispatch the call. Calls must each fulfil the `IsCallable`
+		/// filter.
 		///
 		/// Payment: `MultisigDepositBase` will be reserved if this is the first approval, plus
 		/// `threshold` times `MultisigDepositFactor`. It is returned once this dispatch happens or
@@ -364,6 +382,7 @@ decl_module! {
 			call: Box<<T as Trait>::Call>,
 		) -> DispatchResultWithPostInfo {
 			let who = ensure_signed(origin)?;
+			ensure!(T::IsCallable::filter(call.as_ref()), Error::<T>::Uncallable);
 			ensure!(threshold >= 1, Error::<T>::ZeroThreshold);
 			let max_sigs = T::MaxSignatories::get() as usize;
 			ensure!(!other_signatories.is_empty(), Error::<T>::TooFewSignatories);

frame/utility/src/tests.rs

@@ -99,13 +99,24 @@ parameter_types! {
 	pub const MultisigDepositFactor: u64 = 1;
 	pub const MaxSignatories: u16 = 3;
 }
+
+pub struct TestIsCallable;
+impl Filter<Call> for TestIsCallable {
+	fn filter(c: &Call) -> bool {
+		match *c {
+			Call::Balances(pallet_balances::Call::transfer(..)) => true,
+			_ => false,
+		}
+	}
+}
 impl Trait for Test {
 	type Event = TestEvent;
 	type Call = Call;
 	type Currency = Balances;
 	type MultisigDepositBase = MultisigDepositBase;
 	type MultisigDepositFactor = MultisigDepositFactor;
 	type MaxSignatories = MaxSignatories;
+	type IsCallable = TestIsCallable;
 }
 type System = frame_system::Module<Test>;
 type Balances = pallet_balances::Module<Test>;
@@ -379,6 +390,17 @@ fn multisig_1_of_3_works() {
 	});
 }
 
+#[test]
+fn multisig_filters() {
+	new_test_ext().execute_with(|| {
+		let call = Box::new(Call::System(frame_system::Call::remark(vec![])));
+		assert_noop!(
+			Utility::as_multi(Origin::signed(1), 1, vec![], None, call.clone()),
+			Error::<Test>::Uncallable,
+		);
+	});
+}
+
 #[test]
 fn as_sub_works() {
 	new_test_ext().execute_with(|| {
@@ -399,6 +421,17 @@ fn as_sub_works() {
 	});
 }
 
+#[test]
+fn as_sub_filters() {
+	new_test_ext().execute_with(|| {
+		assert_noop!(Utility::as_sub(
+			Origin::signed(1),
+			1,
+			Box::new(Call::System(frame_system::Call::remark(vec![]))),
+		), Error::<Test>::Uncallable);
+	});
+}
+
 #[test]
 fn batch_with_root_works() {
 	new_test_ext().execute_with(|| {
@@ -429,6 +462,18 @@ fn batch_with_signed_works() {
 	});
 }
 
+#[test]
+fn batch_with_signed_filters() {
+	new_test_ext().execute_with(|| {
+		assert_ok!(
+			Utility::batch(Origin::signed(1), vec![
+				Call::System(frame_system::Call::remark(vec![]))
+			]),
+		);
+		expect_event(RawEvent::Uncallable(0));
+	});
+}
+
 #[test]
 fn batch_early_exit_works() {
 	new_test_ext().execute_with(|| {