From 1ac2e964a51eb0d8ed679ea55a4b2ea46c4bf5c3 Mon Sep 17 00:00:00 2001 From: Xavier Lau <x@acg.box> Date: Tue, 18 Feb 2025 21:10:17 +0800 Subject: [PATCH] Fixes #219 (#7407) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add a new extrinsic `dispatch_as_fallible`. It's almost the same as [`Pallet::dispatch_as`] but forwards any error of the inner call. Closes #219. And add more unit tests to cover `dispatch_as` and `dispatch_as_fallible`. --- Polkadot address: 156HGo9setPcU2qhFMVWLkcmtCEGySLwNqa3DaEiYSWtte4Y --------- Signed-off-by: Xavier Lau <x@acg.box> Co-authored-by: Bastian Köcher <git@kchr.de> Co-authored-by: Bastian Köcher <info@kchr.de> Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com> --- .../src/weights/pallet_utility.rs | 5 ++ .../src/weights/pallet_utility.rs | 5 ++ .../src/weights/pallet_utility.rs | 5 ++ .../src/weights/pallet_utility.rs | 5 ++ .../src/weights/pallet_utility.rs | 5 ++ .../src/weights/pallet_utility.rs | 5 ++ .../src/weights/pallet_utility.rs | 5 ++ .../src/weights/pallet_utility.rs | 5 ++ .../src/weights/pallet_utility.rs | 5 ++ .../rococo/src/weights/pallet_utility.rs | 5 ++ .../westend/src/weights/pallet_utility.rs | 5 ++ prdoc/pr_7407.prdoc | 40 +++++++++++++ substrate/frame/utility/src/benchmarking.rs | 12 ++++ substrate/frame/utility/src/lib.rs | 28 +++++++++ substrate/frame/utility/src/tests.rs | 60 +++++++++++++++++++ substrate/frame/utility/src/weights.rs | 10 ++++ 16 files changed, 205 insertions(+) create mode 100644 prdoc/pr_7407.prdoc 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 80afbde1b1e..d70c1947c4e 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 ef6d9fb4ba9..240779520a0 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 4e531593f4d..0c5a7cf0aeb 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 5d05e89e459..b81d217f5b0 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 6887e41099e..d959b11649b 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 2619a4180ba..1bfac221c2c 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 f2c40f33711..7ca8e00c2a7 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 f30f0776952..0871b257d39 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 c7f98f70fdd..d8def37891d 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 5e580de6aad..2b1db130801 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 84fa0589a58..a13c6854552 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 00000000000..e99e4176918 --- /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 261d5243688..a329815836b 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 63a02febb94..03b193052c3 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/tests.rs b/substrate/frame/utility/src/tests.rs index 759621907df..56e7e3c21ea 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(|| { diff --git a/substrate/frame/utility/src/weights.rs b/substrate/frame/utility/src/weights.rs index 30922bbb22d..ce57af72f91 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` -- GitLab