Repository: paritytech/polkadot-sdk (language: rust)

Issue:
utility.dispatch_as error handling
The `utility.dispatch_as` function hides errors: https://github.com/paritytech/substrate/blob/8186c519a861ea519ed3dc4391f6e13f64300ce6/frame/utility/src/lib.rs#L392-L405

This is problematic when you have `dispatch_as` calls within `batch_all` calls - the batch will succeed despite errors. I assume that you won't want to change the behavior of existing extrinsics, so it would be really nice if a new function could be added that propagates the error. I'm struggling to come up with a suitable name to distinguish it from the extisting `dispatch_as` though..
utility.dispatch_as error handling
The `utility.dispatch_as` function hides errors: https://github.com/paritytech/substrate/blob/8186c519a861ea519ed3dc4391f6e13f64300ce6/frame/utility/src/lib.rs#L392-L405

This is problematic when you have `dispatch_as` calls within `batch_all` calls - the batch will succeed despite errors. I assume that you won't want to change the behavior of existing extrinsics, so it would be really nice if a new function could be added that propagates the error. I'm struggling to come up with a suitable name to distinguish it from the extisting `dispatch_as` though..


Discussion / hints:
Yea it looks a bit weird. That call is more of an `force_dispatch_as`, since it ignores the error.  
I think it makes sense to change, since it can still be wrapped in a `force_batch` to ignore the error - if desired.
Not saying this is desired behaviour but it is intended behaviour consistent with other similar code.

The reason is that the `disaptch_as` have successfully dispatched a call and therefore the Ok response. If for example the origin check failed, it will then respond Err. It is not good to merge errors which could cause more confusion.
> Not saying this is desired behaviour but it is intended behaviour consistent with other similar code.
> 
> The reason is that the `disaptch_as` have successfully dispatched a call and therefore the Ok response. If for example the origin check failed, it will then respond Err. It is not good to merge errors which could cause more confusion.

Yea you can certainly argue that it's working as intended. I'd be fine with leaving the existing function untouched. You can consider this a feature request rather than a bug report. Just to illustrate the need of a `dispatch_as` variant that propagates errors, let me explain how I ran into this issue.

I had a batch of ~50 calls initializing a new testnet. I wrapped the calls in `batch_all` since I wanted an all-or-nothing approach - some of the calls would only have the desired behavior if all of the previous calls succeeded. However, there was a failure hidden by some of the `dispatch_as` calls within the batch, and as a result the batch was committed, resulting in an unintended chain state that was very difficult to fix.
Any reason why the linked PR was not merged even though it was approved? https://github.com/paritytech/substrate/pull/13387

Do we still want the changes in submitted in the PR?
> Any reason why the linked PR was not merged even though it was approved? paritytech/substrate#13387
>
> Do we still want the changes in submitted in the PR?

+1
I don't see any reason for this not getting merged. If you are interested, please port it to `polkadot-sdk`. BTW I would give it a different name than `ensure_dispatch_as` :D
I can help with the porting, if the OP no responds. 
@aurexav just take it.
Yea it looks a bit weird. That call is more of an `force_dispatch_as`, since it ignores the error.  
I think it makes sense to change, since it can still be wrapped in a `force_batch` to ignore the error - if desired.
Not saying this is desired behaviour but it is intended behaviour consistent with other similar code.

The reason is that the `disaptch_as` have successfully dispatched a call and therefore the Ok response. If for example the origin check failed, it will then respond Err. It is not good to merge errors which could cause more confusion.
> Not saying this is desired behaviour but it is intended behaviour consistent with other similar code.
> 
> The reason is that the `disaptch_as` have successfully dispatched a call and therefore the Ok response. If for example the origin check failed, it will then respond Err. It is not good to merge errors which could cause more confusion.

Yea you can certainly argue that it's working as intended. I'd be fine with leaving the existing function untouched. You can consider this a feature request rather than a bug report. Just to illustrate the need of a `dispatch_as` variant that propagates errors, let me explain how I ran into this issue.

I had a batch of ~50 calls initializing a new testnet. I wrapped the calls in `batch_all` since I wanted an all-or-nothing approach - some of the calls would only have the desired behavior if all of the previous calls succeeded. However, there was a failure hidden by some of the `dispatch_as` calls within the batch, and as a result the batch was committed, resulting in an unintended chain state that was very difficult to fix.
Any reason why the linked PR was not merged even though it was approved? https://github.com/paritytech/substrate/pull/13387

Do we still want the changes in submitted in the PR?
> Any reason why the linked PR was not merged even though it was approved? paritytech/substrate#13387
>
> Do we still want the changes in submitted in the PR?

+1
I don't see any reason for this not getting merged. If you are interested, please port it to `polkadot-sdk`. BTW I would give it a different name than `ensure_dispatch_as` :D
I can help with the porting, if the OP no responds. 
@aurexav just take it.

Fix patch:
diff --git a/cumulus/parachains/runtimes/assets/asset-hub-rococo/src/weights/pallet_utility.rs b/cumulus/parachains/runtimes/assets/asset-hub-rococo/src/weights/pallet_utility.rs
index 80afbde1b1e07..d70c1947c4e34 100644
--- a/cumulus/parachains/runtimes/assets/asset-hub-rococo/src/weights/pallet_utility.rs
+++ b/cumulus/parachains/runtimes/assets/asset-hub-rococo/src/weights/pallet_utility.rs
@@ -99,6 +99,11 @@ impl<T: frame_system::Config> pallet_utility::WeightInfo for WeightInfo<T> {
 			// Standard Error: 1_745
 			.saturating_add(Weight::from_parts(6_562_902, 0).saturating_mul(c.into()))
 	}
+
+	fn dispatch_as_fallible() -> Weight {
+		Default::default()
+	}
+
 	fn if_else() -> Weight {
 		// Proof Size summary in bytes:
 		//  Measured:  `0`
diff --git a/cumulus/parachains/runtimes/assets/asset-hub-westend/src/weights/pallet_utility.rs b/cumulus/parachains/runtimes/assets/asset-hub-westend/src/weights/pallet_utility.rs
index ef6d9fb4ba945..240779520a0b2 100644
--- a/cumulus/parachains/runtimes/assets/asset-hub-westend/src/weights/pallet_utility.rs
+++ b/cumulus/parachains/runtimes/assets/asset-hub-westend/src/weights/pallet_utility.rs
@@ -98,6 +98,11 @@ impl<T: frame_system::Config> pallet_utility::WeightInfo for WeightInfo<T> {
 			// Standard Error: 3_765
 			.saturating_add(Weight::from_parts(6_028_416, 0).saturating_mul(c.into()))
 	}
+
+	fn dispatch_as_fallible() -> Weight {
+		Default::default()
+	}
+
 	fn if_else() -> Weight {
 		// Proof Size summary in bytes:
 		//  Measured:  `0`
diff --git a/cumulus/parachains/runtimes/bridge-hubs/bridge-hub-rococo/src/weights/pallet_utility.rs b/cumulus/parachains/runtimes/bridge-hubs/bridge-hub-rococo/src/weights/pallet_utility.rs
index 4e531593f4d53..0c5a7cf0aeb34 100644
--- a/cumulus/parachains/runtimes/bridge-hubs/bridge-hub-rococo/src/weights/pallet_utility.rs
+++ b/cumulus/parachains/runtimes/bridge-hubs/bridge-hub-rococo/src/weights/pallet_utility.rs
@@ -98,6 +98,11 @@ impl<T: frame_system::Config> pallet_utility::WeightInfo for WeightInfo<T> {
 			// Standard Error: 1_601
 			.saturating_add(Weight::from_parts(5_138_293, 0).saturating_mul(c.into()))
 	}
+
+	fn dispatch_as_fallible() -> Weight {
+		Default::default()
+	}
+
 	fn if_else() -> Weight {
 		// Proof Size summary in bytes:
 		//  Measured:  `0`
diff --git a/cumulus/parachains/runtimes/bridge-hubs/bridge-hub-westend/src/weights/pallet_utility.rs b/cumulus/parachains/runtimes/bridge-hubs/bridge-hub-westend/src/weights/pallet_utility.rs
index 5d05e89e45932..b81d217f5b010 100644
--- a/cumulus/parachains/runtimes/bridge-hubs/bridge-hub-westend/src/weights/pallet_utility.rs
+++ b/cumulus/parachains/runtimes/bridge-hubs/bridge-hub-westend/src/weights/pallet_utility.rs
@@ -99,6 +99,11 @@ impl<T: frame_system::Config> pallet_utility::WeightInfo for WeightInfo<T> {
 			// Standard Error: 1_601
 			.saturating_add(Weight::from_parts(5_138_293, 0).saturating_mul(c.into()))
 	}
+
+	fn dispatch_as_fallible() -> Weight {
+		Default::default()
+	}
+	
 	fn if_else() -> Weight {
 		// Proof Size summary in bytes:
 		//  Measured:  `0`
diff --git a/cumulus/parachains/runtimes/collectives/collectives-westend/src/weights/pallet_utility.rs b/cumulus/parachains/runtimes/collectives/collectives-westend/src/weights/pallet_utility.rs
index 6887e41099e35..d959b11649bbb 100644
--- a/cumulus/parachains/runtimes/collectives/collectives-westend/src/weights/pallet_utility.rs
+++ b/cumulus/parachains/runtimes/collectives/collectives-westend/src/weights/pallet_utility.rs
@@ -98,6 +98,11 @@ impl<T: frame_system::Config> pallet_utility::WeightInfo for WeightInfo<T> {
 			// Standard Error: 1_395
 			.saturating_add(Weight::from_parts(5_000_971, 0).saturating_mul(c.into()))
 	}
+
+	fn dispatch_as_fallible() -> Weight {
+		Default::default()
+	}
+
 	fn if_else() -> Weight {
 		// Proof Size summary in bytes:
 		//  Measured:  `0`
diff --git a/cumulus/parachains/runtimes/coretime/coretime-rococo/src/weights/pallet_utility.rs b/cumulus/parachains/runtimes/coretime/coretime-rococo/src/weights/pallet_utility.rs
index 2619a4180bab7..1bfac221c2c8f 100644
--- a/cumulus/parachains/runtimes/coretime/coretime-rococo/src/weights/pallet_utility.rs
+++ b/cumulus/parachains/runtimes/coretime/coretime-rococo/src/weights/pallet_utility.rs
@@ -99,6 +99,11 @@ impl<T: frame_system::Config> pallet_utility::WeightInfo for WeightInfo<T> {
 			// Standard Error: 1_621
 			.saturating_add(Weight::from_parts(3_312_302, 0).saturating_mul(c.into()))
 	}
+
+	fn dispatch_as_fallible() -> Weight {
+		Default::default()
+	}
+
 	fn if_else() -> Weight {
 		// Proof Size summary in bytes:
 		//  Measured:  `0`
diff --git a/cumulus/parachains/runtimes/coretime/coretime-westend/src/weights/pallet_utility.rs b/cumulus/parachains/runtimes/coretime/coretime-westend/src/weights/pallet_utility.rs
index f2c40f3371185..7ca8e00c2a7dd 100644
--- a/cumulus/parachains/runtimes/coretime/coretime-westend/src/weights/pallet_utility.rs
+++ b/cumulus/parachains/runtimes/coretime/coretime-westend/src/weights/pallet_utility.rs
@@ -99,6 +99,11 @@ impl<T: frame_system::Config> pallet_utility::WeightInfo for WeightInfo<T> {
 			// Standard Error: 740
 			.saturating_add(Weight::from_parts(2_800_888, 0).saturating_mul(c.into()))
 	}
+
+	fn dispatch_as_fallible() -> Weight {
+		Default::default()
+	}
+
 	fn if_else() -> Weight {
 		// Proof Size summary in bytes:
 		//  Measured:  `0`
diff --git a/cumulus/parachains/runtimes/people/people-rococo/src/weights/pallet_utility.rs b/cumulus/parachains/runtimes/people/people-rococo/src/weights/pallet_utility.rs
index f30f077695262..0871b257d39c3 100644
--- a/cumulus/parachains/runtimes/people/people-rococo/src/weights/pallet_utility.rs
+++ b/cumulus/parachains/runtimes/people/people-rococo/src/weights/pallet_utility.rs
@@ -96,6 +96,11 @@ impl<T: frame_system::Config> pallet_utility::WeightInfo for WeightInfo<T> {
 			// Standard Error: 3_915
 			.saturating_add(Weight::from_parts(4_372_646, 0).saturating_mul(c.into()))
 	}
+
+	fn dispatch_as_fallible() -> Weight {
+		Default::default()
+	}
+
 	fn if_else() -> Weight {
 		// Proof Size summary in bytes:
 		//  Measured:  `0`
diff --git a/cumulus/parachains/runtimes/people/people-westend/src/weights/pallet_utility.rs b/cumulus/parachains/runtimes/people/people-westend/src/weights/pallet_utility.rs
index c7f98f70fdd84..d8def37891d4d 100644
--- a/cumulus/parachains/runtimes/people/people-westend/src/weights/pallet_utility.rs
+++ b/cumulus/parachains/runtimes/people/people-westend/src/weights/pallet_utility.rs
@@ -96,6 +96,11 @@ impl<T: frame_system::Config> pallet_utility::WeightInfo for WeightInfo<T> {
 			// Standard Error: 7_605
 			.saturating_add(Weight::from_parts(4_306_193, 0).saturating_mul(c.into()))
 	}
+
+	fn dispatch_as_fallible() -> Weight {
+		Default::default()
+	}
+
 	fn if_else() -> Weight {
 		// Proof Size summary in bytes:
 		//  Measured:  `0`
diff --git a/polkadot/runtime/rococo/src/weights/pallet_utility.rs b/polkadot/runtime/rococo/src/weights/pallet_utility.rs
index 5e580de6aad54..2b1db130801a4 100644
--- a/polkadot/runtime/rococo/src/weights/pallet_utility.rs
+++ b/polkadot/runtime/rococo/src/weights/pallet_utility.rs
@@ -99,6 +99,11 @@ impl<T: frame_system::Config> pallet_utility::WeightInfo for WeightInfo<T> {
 			// Standard Error: 460
 			.saturating_add(Weight::from_parts(3_173_577, 0).saturating_mul(c.into()))
 	}
+
+	fn dispatch_as_fallible() -> Weight {
+		Default::default()
+	}
+
 	fn if_else() -> Weight {
 		// Proof Size summary in bytes:
 		//  Measured:  `0`
diff --git a/polkadot/runtime/westend/src/weights/pallet_utility.rs b/polkadot/runtime/westend/src/weights/pallet_utility.rs
index 84fa0589a5821..a13c68545526d 100644
--- a/polkadot/runtime/westend/src/weights/pallet_utility.rs
+++ b/polkadot/runtime/westend/src/weights/pallet_utility.rs
@@ -99,6 +99,11 @@ impl<T: frame_system::Config> pallet_utility::WeightInfo for WeightInfo<T> {
 			// Standard Error: 2_817
 			.saturating_add(Weight::from_parts(5_113_539, 0).saturating_mul(c.into()))
 	}
+
+	fn dispatch_as_fallible() -> Weight {
+		Default::default()
+	}
+
 	fn if_else() -> Weight {
 		// Proof Size summary in bytes:
 		//  Measured:  `0`
diff --git a/prdoc/pr_7407.prdoc b/prdoc/pr_7407.prdoc
new file mode 100644
index 0000000000000..e99e417691848
--- /dev/null
+++ b/prdoc/pr_7407.prdoc
@@ -0,0 +1,40 @@
+title: 'Fixes #219'
+doc:
+- audience: Runtime Dev
+  description: |-
+    Add a new extrinsic `dispatch_as_fallible`.
+
+    It's almost the same as `dispatch_as` but check the result of the call.
+
+    Closes #219.
+
+    And add more unit tests to cover `dispatch_as` and `dispatch_as_fallible`.
+
+    ---
+
+    Polkadot address: 156HGo9setPcU2qhFMVWLkcmtCEGySLwNqa3DaEiYSWtte4Y
+crates:
+- name: asset-hub-rococo-runtime
+  bump: minor
+- name: asset-hub-westend-runtime
+  bump: minor
+- name: bridge-hub-rococo-runtime
+  bump: minor
+- name: bridge-hub-westend-runtime
+  bump: minor
+- name: collectives-westend-runtime
+  bump: minor
+- name: coretime-rococo-runtime
+  bump: minor
+- name: coretime-westend-runtime
+  bump: minor
+- name: people-rococo-runtime
+  bump: minor
+- name: people-westend-runtime
+  bump: minor
+- name: rococo-runtime
+  bump: minor
+- name: westend-runtime
+  bump: minor
+- name: pallet-utility
+  bump: minor
diff --git a/substrate/frame/utility/src/benchmarking.rs b/substrate/frame/utility/src/benchmarking.rs
index 261d52436889a..a329815836ba6 100644
--- a/substrate/frame/utility/src/benchmarking.rs
+++ b/substrate/frame/utility/src/benchmarking.rs
@@ -92,6 +92,18 @@ mod benchmark {
 		assert_last_event::<T>(Event::BatchCompleted.into());
 	}
 
+	#[benchmark]
+	fn dispatch_as_fallible() {
+		let caller = account("caller", SEED, SEED);
+		let call = Box::new(frame_system::Call::remark { remark: vec![] }.into());
+		let origin: T::RuntimeOrigin = RawOrigin::Signed(caller).into();
+		let pallets_origin = origin.caller().clone();
+		let pallets_origin = T::PalletsOrigin::from(pallets_origin);
+
+		#[extrinsic_call]
+		_(RawOrigin::Root, Box::new(pallets_origin), call);
+	}
+
 	#[benchmark]
 	fn if_else() {
 		// Failing main call.
diff --git a/substrate/frame/utility/src/lib.rs b/substrate/frame/utility/src/lib.rs
index 63a02febb94c5..03b193052c37a 100644
--- a/substrate/frame/utility/src/lib.rs
+++ b/substrate/frame/utility/src/lib.rs
@@ -554,6 +554,34 @@ pub mod pallet {
 				post_info: Some(weight).into(),
 			})
 		}
+
+		/// Dispatches a function call with a provided origin.
+		///
+		/// Almost the same as [`Pallet::dispatch_as`] but forwards any error of the inner call.
+		///
+		/// The dispatch origin for this call must be _Root_.
+		#[pallet::call_index(7)]
+		#[pallet::weight({
+			let dispatch_info = call.get_dispatch_info();
+			(
+				T::WeightInfo::dispatch_as_fallible()
+					.saturating_add(dispatch_info.call_weight),
+				dispatch_info.class,
+			)
+		})]
+		pub fn dispatch_as_fallible(
+			origin: OriginFor<T>,
+			as_origin: Box<T::PalletsOrigin>,
+			call: Box<<T as Config>::RuntimeCall>,
+		) -> DispatchResult {
+			ensure_root(origin)?;
+
+			call.dispatch_bypass_filter((*as_origin).into()).map_err(|e| e.error)?;
+
+			Self::deposit_event(Event::DispatchedAs { result: Ok(()) });
+
+			Ok(())
+		}
 	}
 
 	impl<T: Config> Pallet<T> {
diff --git a/substrate/frame/utility/src/weights.rs b/substrate/frame/utility/src/weights.rs
index 30922bbb22d5b..ce57af72f9142 100644
--- a/substrate/frame/utility/src/weights.rs
+++ b/substrate/frame/utility/src/weights.rs
@@ -56,6 +56,7 @@ pub trait WeightInfo {
 	fn batch_all(c: u32, ) -> Weight;
 	fn dispatch_as() -> Weight;
 	fn force_batch(c: u32, ) -> Weight;
+	fn dispatch_as_fallible() -> Weight;
 	fn if_else() -> Weight;
 }
 
@@ -126,6 +127,10 @@ impl<T: frame_system::Config> WeightInfo for SubstrateWeight<T> {
 			.saturating_add(Weight::from_parts(4_570_923, 0).saturating_mul(c.into()))
 			.saturating_add(T::DbWeight::get().reads(2_u64))
 	}
+	fn dispatch_as_fallible() -> Weight {
+		Weight::MAX
+	}
+
 	fn if_else() -> Weight {
 		// Proof Size summary in bytes:
 		//  Measured:  `0`
@@ -202,6 +207,11 @@ impl WeightInfo for () {
 			.saturating_add(Weight::from_parts(4_570_923, 0).saturating_mul(c.into()))
 			.saturating_add(RocksDbWeight::get().reads(2_u64))
 	}
+
+	fn dispatch_as_fallible() -> Weight {
+		Weight::MAX
+	}
+
 	fn if_else() -> Weight {
 		// Proof Size summary in bytes:
 		//  Measured:  `0`


Test patch:
diff --git a/substrate/frame/utility/src/tests.rs b/substrate/frame/utility/src/tests.rs
index 759621907dfed..56e7e3c21ea24 100644
--- a/substrate/frame/utility/src/tests.rs
+++ b/substrate/frame/utility/src/tests.rs
@@ -262,6 +262,14 @@ fn call_foobar(err: bool, start_weight: Weight, end_weight: Option<Weight>) -> R
 	RuntimeCall::Example(ExampleCall::foobar { err, start_weight, end_weight })
 }
 
+fn utility_events() -> Vec<Event> {
+	System::events()
+		.into_iter()
+		.map(|r| r.event)
+		.filter_map(|e| if let RuntimeEvent::Utility(inner) = e { Some(inner) } else { None })
+		.collect()
+}
+
 #[test]
 fn as_derivative_works() {
 	new_test_ext().execute_with(|| {
@@ -916,6 +924,33 @@ fn with_weight_works() {
 	})
 }
 
+#[test]
+fn dispatch_as_works() {
+	new_test_ext().execute_with(|| {
+		Balances::force_set_balance(RuntimeOrigin::root(), 666, 100).unwrap();
+		assert_eq!(Balances::free_balance(666), 100);
+		assert_eq!(Balances::free_balance(777), 0);
+		assert_ok!(Utility::dispatch_as(
+			RuntimeOrigin::root(),
+			Box::new(OriginCaller::system(frame_system::RawOrigin::Signed(666))),
+			Box::new(call_transfer(777, 100))
+		));
+		assert_eq!(Balances::free_balance(666), 0);
+		assert_eq!(Balances::free_balance(777), 100);
+
+		System::reset_events();
+		assert_ok!(Utility::dispatch_as(
+			RuntimeOrigin::root(),
+			Box::new(OriginCaller::system(frame_system::RawOrigin::Signed(777))),
+			Box::new(RuntimeCall::Timestamp(TimestampCall::set { now: 0 }))
+		));
+		assert_eq!(
+			utility_events(),
+			vec![Event::DispatchedAs { result: Err(DispatchError::BadOrigin) }]
+		);
+	})
+}
+
 #[test]
 fn if_else_with_root_works() {
 	new_test_ext().execute_with(|| {
@@ -983,6 +1018,31 @@ fn if_else_successful_main_call() {
 	})
 }
 
+#[test]
+fn dispatch_as_fallible_works() {
+	new_test_ext().execute_with(|| {
+		Balances::force_set_balance(RuntimeOrigin::root(), 666, 100).unwrap();
+		assert_eq!(Balances::free_balance(666), 100);
+		assert_eq!(Balances::free_balance(777), 0);
+		assert_ok!(Utility::dispatch_as_fallible(
+			RuntimeOrigin::root(),
+			Box::new(OriginCaller::system(frame_system::RawOrigin::Signed(666))),
+			Box::new(call_transfer(777, 100))
+		));
+		assert_eq!(Balances::free_balance(666), 0);
+		assert_eq!(Balances::free_balance(777), 100);
+
+		assert_noop!(
+			Utility::dispatch_as_fallible(
+				RuntimeOrigin::root(),
+				Box::new(OriginCaller::system(frame_system::RawOrigin::Signed(777))),
+				Box::new(RuntimeCall::Timestamp(TimestampCall::set { now: 0 }))
+			),
+			DispatchError::BadOrigin,
+		);
+	})
+}
+
 #[test]
 fn if_else_failing_fallback_call() {
 	new_test_ext().execute_with(|| {