From b83a8e8f8be9daf5e42c76cd8b6d4dcdd99f1729 Mon Sep 17 00:00:00 2001 From: rainbow-promise Date: Thu, 31 Oct 2024 14:50:33 -0700 Subject: [PATCH 01/16] extrinsic outline --- substrate/frame/utility/src/lib.rs | 51 ++++++++++++++++++++++++++++++ 1 file changed, 51 insertions(+) diff --git a/substrate/frame/utility/src/lib.rs b/substrate/frame/utility/src/lib.rs index 26c38d1f0459..ef7f696b8766 100644 --- a/substrate/frame/utility/src/lib.rs +++ b/substrate/frame/utility/src/lib.rs @@ -120,6 +120,10 @@ pub mod pallet { ItemFailed { error: DispatchError }, /// A call was dispatched. DispatchedAs { result: DispatchResult }, + /// one or both if_else calls failed. + IfElseFailure { which: String, error: DispatchError }, + /// if_else completed. + IfElseCompleted, } // Align the call size to 1KB. As we are currently compiling the runtime for native/wasm @@ -454,6 +458,53 @@ pub mod pallet { let res = call.dispatch_bypass_filter(frame_system::RawOrigin::Root.into()); res.map(|_| ()).map_err(|e| e.error) } + + #[pallet::call_index(6)] + #[pallet::weight(10_000)] + pub fn if_else( + origin: OriginFor, + main: ::RuntimeCall, + fallback: ::RuntimeCall, + ) -> DispatchResultWithPostInfo { + // Do not allow the `None` origin. + if ensure_none(origin.clone()).is_ok() { + return Err(BadOrigin.into()) + } + + let is_root = ensure_root(origin.clone()).is_ok(); + let calls: Vec<::RuntimeCall> = vec![main.clone(), fallback.clone()]; + + // Track the weights + let mut weight = Weight::zero(); + for (index, call) in calls.into_iter().enumerate() { + let info = call.get_dispatch_info(); + // If origin is root, don't apply any dispatch filters; root can call anything. + let result = if is_root { + call.dispatch_bypass_filter(origin.clone()) + } else { + call.dispatch(origin.clone()) + }; + // Add weight of this call. + weight = weight.saturating_add(extract_actual_weight(&result, &info)); + if let Err(e) = result { + let which = if index == 0 { "main".to_string() } else { "both calls have failed".to_string() }; // Use descriptive names + Self::deposit_event(Event::IfElseFailure { + which, + error: e.error, + + }); + + // Take the weight of this function into account. + let base_weight = T::WeightInfo::batch(index.saturating_add(1) as u32); + // Return the actual used weight + base_weight of this call. + return Ok(Some(base_weight.saturating_add(weight)).into()) + } + Self::deposit_event(Event::ItemCompleted); + } + Self::deposit_event(Event::IfElseCompleted); + let base_weight = T::WeightInfo::batch(2u32); + Ok(Some(base_weight.saturating_add(weight)).into()) + } } impl Pallet { From 1b01dc9d6d875628553cf1cbb0bd8ea6d8289d4e Mon Sep 17 00:00:00 2001 From: rainbow-promise Date: Fri, 1 Nov 2024 10:13:12 -0700 Subject: [PATCH 02/16] new: outline utility_if_else --- substrate/frame/utility/src/lib.rs | 109 +++++++++++++++++------------ 1 file changed, 66 insertions(+), 43 deletions(-) diff --git a/substrate/frame/utility/src/lib.rs b/substrate/frame/utility/src/lib.rs index ef7f696b8766..84aad1bbb9d7 100644 --- a/substrate/frame/utility/src/lib.rs +++ b/substrate/frame/utility/src/lib.rs @@ -123,7 +123,7 @@ pub mod pallet { /// one or both if_else calls failed. IfElseFailure { which: String, error: DispatchError }, /// if_else completed. - IfElseCompleted, + IfElseCompleted { which: String }, } // Align the call size to 1KB. As we are currently compiling the runtime for native/wasm @@ -459,51 +459,74 @@ pub mod pallet { res.map(|_| ()).map_err(|e| e.error) } - #[pallet::call_index(6)] + #[pallet::call_index(7)] #[pallet::weight(10_000)] pub fn if_else( - origin: OriginFor, - main: ::RuntimeCall, - fallback: ::RuntimeCall, + origin: OriginFor, + main: ::RuntimeCall, + fallback: ::RuntimeCall, ) -> DispatchResultWithPostInfo { - // Do not allow the `None` origin. - if ensure_none(origin.clone()).is_ok() { - return Err(BadOrigin.into()) - } - - let is_root = ensure_root(origin.clone()).is_ok(); - let calls: Vec<::RuntimeCall> = vec![main.clone(), fallback.clone()]; - - // Track the weights - let mut weight = Weight::zero(); - for (index, call) in calls.into_iter().enumerate() { - let info = call.get_dispatch_info(); - // If origin is root, don't apply any dispatch filters; root can call anything. - let result = if is_root { - call.dispatch_bypass_filter(origin.clone()) - } else { - call.dispatch(origin.clone()) - }; - // Add weight of this call. - weight = weight.saturating_add(extract_actual_weight(&result, &info)); - if let Err(e) = result { - let which = if index == 0 { "main".to_string() } else { "both calls have failed".to_string() }; // Use descriptive names - Self::deposit_event(Event::IfElseFailure { - which, - error: e.error, - - }); - - // Take the weight of this function into account. - let base_weight = T::WeightInfo::batch(index.saturating_add(1) as u32); - // Return the actual used weight + base_weight of this call. - return Ok(Some(base_weight.saturating_add(weight)).into()) - } - Self::deposit_event(Event::ItemCompleted); - } - Self::deposit_event(Event::IfElseCompleted); - let base_weight = T::WeightInfo::batch(2u32); - Ok(Some(base_weight.saturating_add(weight)).into()) + // Do not allow the `None` origin. + if ensure_none(origin.clone()).is_ok() { + return Err(BadOrigin.into()); + } + + let is_root = ensure_root(origin.clone()).is_ok(); + + // Track the weights + let mut weight = Weight::zero(); + + // Execute the main call first + let main_result = if is_root { + main.clone().dispatch_bypass_filter(origin.clone()) + } else { + main.clone().dispatch(origin.clone()) + }; + + // Add weight of the main call + let info = main.get_dispatch_info(); + weight = weight.saturating_add(extract_actual_weight(&main_result, &info)); + + if let Err(main_call_error) = main_result { + // Both calls have failed + Self::deposit_event(Event::IfElseFailure { + which: "Main call".to_string(), + error: main_call_error.error, + }); + + // If the main call failed, execute the fallback call + let fallback_result = if is_root { + fallback.clone().dispatch_bypass_filter(origin.clone()) + } else { + fallback.clone().dispatch(origin.clone()) + }; + + // Add weight of the fallback call + let fallback_info = fallback.get_dispatch_info(); + weight = weight.saturating_add(extract_actual_weight(&fallback_result, &fallback_info)); + + if let Err(fallback_error) = fallback_result { + // Both calls have failed + Self::deposit_event(Event::IfElseFailure { + which: "Both calls have failed".to_string(), + error: fallback_error.error, + }); + + // Calculate base weight and return + let base_weight = T::WeightInfo::batch(2u32); + return Ok(Some(base_weight.saturating_add(weight)).into()); + } + + // Fallback succeeded. + Self::deposit_event(Event::IfElseCompleted { + which: "Fallback".to_string(), + }); + } + // Main call succesed. + Self::deposit_event(Event::IfElseCompleted { + which: "Main".to_string() + }); + Ok(Some(weight).into()) } } From 430a791bee29f8d760f02894f5a6f3fd69a4dc1e Mon Sep 17 00:00:00 2001 From: rainbow-promise Date: Wed, 6 Nov 2024 03:46:28 -0800 Subject: [PATCH 03/16] do not clone calls --- substrate/frame/utility/src/lib.rs | 61 +++++++++++++----------------- 1 file changed, 27 insertions(+), 34 deletions(-) diff --git a/substrate/frame/utility/src/lib.rs b/substrate/frame/utility/src/lib.rs index 84aad1bbb9d7..7b0670e25154 100644 --- a/substrate/frame/utility/src/lib.rs +++ b/substrate/frame/utility/src/lib.rs @@ -67,10 +67,17 @@ use frame_support::{ use sp_core::TypeId; use sp_io::hashing::blake2_256; use sp_runtime::traits::{BadOrigin, Dispatchable, TrailingZeroInput}; +use scale_info::TypeInfo; pub use weights::WeightInfo; pub use pallet::*; +#[derive(PartialEq, Eq, Clone, Copy, Encode, Decode, Debug, TypeInfo)] +pub enum Which { + Main, + Fallback, +} + #[frame_support::pallet] pub mod pallet { use super::*; @@ -120,10 +127,8 @@ pub mod pallet { ItemFailed { error: DispatchError }, /// A call was dispatched. DispatchedAs { result: DispatchResult }, - /// one or both if_else calls failed. - IfElseFailure { which: String, error: DispatchError }, /// if_else completed. - IfElseCompleted { which: String }, + IfElseCompleted { call: Which }, } // Align the call size to 1KB. As we are currently compiling the runtime for native/wasm @@ -163,6 +168,8 @@ pub mod pallet { pub enum Error { /// Too many calls batched. TooManyCalls, + /// Multiple call failure. + InvalidCalls } #[pallet::call] @@ -475,57 +482,43 @@ pub mod pallet { // Track the weights let mut weight = Weight::zero(); + + let info = main.get_dispatch_info(); // Execute the main call first let main_result = if is_root { - main.clone().dispatch_bypass_filter(origin.clone()) + main.dispatch_bypass_filter(origin.clone()) } else { - main.clone().dispatch(origin.clone()) + main.dispatch(origin.clone()) }; // Add weight of the main call - let info = main.get_dispatch_info(); weight = weight.saturating_add(extract_actual_weight(&main_result, &info)); - if let Err(main_call_error) = main_result { - // Both calls have failed - Self::deposit_event(Event::IfElseFailure { - which: "Main call".to_string(), - error: main_call_error.error, - }); - + if let Err(_main_call_error) = main_result { // If the main call failed, execute the fallback call + let fallback_info = fallback.get_dispatch_info(); + let fallback_result = if is_root { - fallback.clone().dispatch_bypass_filter(origin.clone()) + fallback.dispatch_bypass_filter(origin.clone()) } else { - fallback.clone().dispatch(origin.clone()) + fallback.dispatch(origin.clone()) }; // Add weight of the fallback call - let fallback_info = fallback.get_dispatch_info(); weight = weight.saturating_add(extract_actual_weight(&fallback_result, &fallback_info)); - if let Err(fallback_error) = fallback_result { - // Both calls have failed - Self::deposit_event(Event::IfElseFailure { - which: "Both calls have failed".to_string(), - error: fallback_error.error, - }); - - // Calculate base weight and return - let base_weight = T::WeightInfo::batch(2u32); - return Ok(Some(base_weight.saturating_add(weight)).into()); + if let Err(_fallback_error) = fallback_result { + // Both calls have faild. + return Err(Error::::InvalidCalls.into()) + } - // Fallback succeeded. - Self::deposit_event(Event::IfElseCompleted { - which: "Fallback".to_string(), - }); + Self::deposit_event(Event::IfElseCompleted { call: Which::Fallback }); + return Ok(Some(weight).into()); } - // Main call succesed. - Self::deposit_event(Event::IfElseCompleted { - which: "Main".to_string() - }); + // Main call succeeded. + Self::deposit_event(Event::IfElseCompleted { call: Which::Main }); Ok(Some(weight).into()) } } From 2caf95e96609ca35e63455a2e805b1e5bb4c1105 Mon Sep 17 00:00:00 2001 From: rainbow-promise Date: Wed, 6 Nov 2024 08:04:04 -0800 Subject: [PATCH 04/16] compiler needs to know the size, put in Box --- substrate/frame/utility/src/lib.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/substrate/frame/utility/src/lib.rs b/substrate/frame/utility/src/lib.rs index 7b0670e25154..ec9cb2127311 100644 --- a/substrate/frame/utility/src/lib.rs +++ b/substrate/frame/utility/src/lib.rs @@ -466,12 +466,12 @@ pub mod pallet { res.map(|_| ()).map_err(|e| e.error) } - #[pallet::call_index(7)] + #[pallet::call_index(6)] #[pallet::weight(10_000)] pub fn if_else( origin: OriginFor, - main: ::RuntimeCall, - fallback: ::RuntimeCall, + main: Box<::RuntimeCall>, + fallback: Box<::RuntimeCall>, ) -> DispatchResultWithPostInfo { // Do not allow the `None` origin. if ensure_none(origin.clone()).is_ok() { @@ -482,7 +482,7 @@ pub mod pallet { // Track the weights let mut weight = Weight::zero(); - + let info = main.get_dispatch_info(); // Execute the main call first From 4183c6bb8bb498a896a2672f5acdc7c3ea7cefd8 Mon Sep 17 00:00:00 2001 From: rainbow-promise Date: Wed, 6 Nov 2024 08:46:12 -0800 Subject: [PATCH 05/16] siimple test cases --- substrate/frame/utility/src/tests.rs | 40 ++++++++++++++++++++++++++++ 1 file changed, 40 insertions(+) diff --git a/substrate/frame/utility/src/tests.rs b/substrate/frame/utility/src/tests.rs index 274a90d77cf0..2e23dbe2b896 100644 --- a/substrate/frame/utility/src/tests.rs +++ b/substrate/frame/utility/src/tests.rs @@ -914,3 +914,43 @@ fn with_weight_works() { ); }) } + +#[test] +fn if_else_with_root_works() { + new_test_ext().execute_with(|| { + let k = b"a".to_vec(); + let call = RuntimeCall::System(frame_system::Call::set_storage { + items: vec![(k.clone(), k.clone())], + }); + assert!(!TestBaseCallFilter::contains(&call)); + assert_eq!(Balances::free_balance(1), 10); + assert_eq!(Balances::free_balance(2), 10); + assert_ok!(Utility::if_else( + RuntimeOrigin::root(), + Box::new(RuntimeCall::Balances(BalancesCall::force_transfer { + source: 1, + dest: 2, + value: 11 + })), + Box::new(call), + )); + assert_eq!(Balances::free_balance(1), 10); + assert_eq!(Balances::free_balance(2), 10); + assert_eq!(storage::unhashed::get_raw(&k), Some(k)); + }); +} + +#[test] +fn if_else_with_signed_works() { + new_test_ext().execute_with(|| { + assert_eq!(Balances::free_balance(1), 10); + assert_eq!(Balances::free_balance(2), 10); + assert_ok!(Utility::if_else( + RuntimeOrigin::signed(1), + Box::new(call_transfer(2, 11)), + Box::new(call_transfer(2, 5)) + )); + assert_eq!(Balances::free_balance(1), 5); + assert_eq!(Balances::free_balance(2), 15); + }); +} From e70a60703df6874db051aacf68787e37b40c2bf1 Mon Sep 17 00:00:00 2001 From: rainbow-promise Date: Thu, 7 Nov 2024 04:39:15 -0800 Subject: [PATCH 06/16] if_else::benchmarking --- substrate/frame/utility/src/benchmarking.rs | 12 ++++++++++++ substrate/frame/utility/src/lib.rs | 11 ++++++++++- 2 files changed, 22 insertions(+), 1 deletion(-) diff --git a/substrate/frame/utility/src/benchmarking.rs b/substrate/frame/utility/src/benchmarking.rs index 88556c05195a..808c158df18b 100644 --- a/substrate/frame/utility/src/benchmarking.rs +++ b/substrate/frame/utility/src/benchmarking.rs @@ -92,6 +92,18 @@ mod benchmark { assert_last_event::(Event::BatchCompleted.into()); } + #[benchmark] + fn if_else() { + let main_call = Box::new(frame_system::Call::remark { remark: vec![0] }.into()); + let fallback_call = Box::new(frame_system::Call::remark { remark: vec![1] }.into()); + let caller = whitelisted_caller(); + + #[extrinsic_call] + _(RawOrigin::Signed(caller), main_call, fallback_call); + + assert_last_event::(Event::IfElseCompleted { call: Which::Main }.into()); + } + impl_benchmark_test_suite! { Pallet, tests::new_test_ext(), diff --git a/substrate/frame/utility/src/lib.rs b/substrate/frame/utility/src/lib.rs index ec9cb2127311..99e9b57b8daf 100644 --- a/substrate/frame/utility/src/lib.rs +++ b/substrate/frame/utility/src/lib.rs @@ -467,7 +467,16 @@ pub mod pallet { } #[pallet::call_index(6)] - #[pallet::weight(10_000)] + #[pallet::weight({ + let main_dispatch_info = main.get_dispatch_info(); + let fallback_dispatch_info = fallback.get_dispatch_info(); + ( + T::WeightInfo::dispatch_as() + .saturating_add(main_dispatch_info.call_weight) + .saturating_add(fallback_dispatch_info.call_weight), + main_dispatch_info.class, + ) + })] pub fn if_else( origin: OriginFor, main: Box<::RuntimeCall>, From a93b142a9bc13aac3ab21b43b30dec0258c1ec24 Mon Sep 17 00:00:00 2001 From: rainbow-promise Date: Thu, 7 Nov 2024 04:49:31 -0800 Subject: [PATCH 07/16] add weights substrate --- substrate/frame/utility/src/lib.rs | 2 +- substrate/frame/utility/src/weights.rs | 17 +++++++++++++++++ 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/substrate/frame/utility/src/lib.rs b/substrate/frame/utility/src/lib.rs index 99e9b57b8daf..f549d097e12a 100644 --- a/substrate/frame/utility/src/lib.rs +++ b/substrate/frame/utility/src/lib.rs @@ -471,7 +471,7 @@ pub mod pallet { let main_dispatch_info = main.get_dispatch_info(); let fallback_dispatch_info = fallback.get_dispatch_info(); ( - T::WeightInfo::dispatch_as() + T::WeightInfo::if_else() .saturating_add(main_dispatch_info.call_weight) .saturating_add(fallback_dispatch_info.call_weight), main_dispatch_info.class, diff --git a/substrate/frame/utility/src/weights.rs b/substrate/frame/utility/src/weights.rs index 502f85a3f178..40c36a9c2e73 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 if_else() -> Weight; } /// Weights for `pallet_utility` using the Substrate node and recommended hardware. @@ -125,6 +126,14 @@ impl WeightInfo for SubstrateWeight { .saturating_add(Weight::from_parts(4_955_816, 0).saturating_mul(c.into())) .saturating_add(T::DbWeight::get().reads(2_u64)) } + fn if_else() -> Weight { + // Proof Size summary in bytes: + // Measured: `0` + // Estimated: `0` + // Minimum execution time: 6_000_000 picoseconds. + Weight::from_parts(7_000_000, 0) + .saturating_add(Weight::from_parts(0, 0)) + } } // For backwards compatibility and tests. @@ -193,4 +202,12 @@ impl WeightInfo for () { .saturating_add(Weight::from_parts(4_955_816, 0).saturating_mul(c.into())) .saturating_add(RocksDbWeight::get().reads(2_u64)) } + fn if_else() -> Weight { + // Proof Size summary in bytes: + // Measured: `0` + // Estimated: `0` + // Minimum execution time: 6_000_000 picoseconds. + Weight::from_parts(7_000_000, 0) + .saturating_add(Weight::from_parts(0, 0)) + } } From cc79306275a62d5e82fc19aa5a7948cab4b2fa75 Mon Sep 17 00:00:00 2001 From: rainbow-promise Date: Thu, 7 Nov 2024 07:32:00 -0800 Subject: [PATCH 08/16] add weight to all runtimes --- .../assets/asset-hub-rococo/src/weights/pallet_utility.rs | 8 ++++++++ .../asset-hub-westend/src/weights/pallet_utility.rs | 8 ++++++++ .../bridge-hub-rococo/src/weights/pallet_utility.rs | 8 ++++++++ .../bridge-hub-westend/src/weights/pallet_utility.rs | 8 ++++++++ .../collectives-westend/src/weights/pallet_utility.rs | 8 ++++++++ .../coretime-rococo/src/weights/pallet_utility.rs | 8 ++++++++ .../coretime-westend/src/weights/pallet_utility.rs | 8 ++++++++ .../people/people-rococo/src/weights/pallet_utility.rs | 8 ++++++++ .../people/people-westend/src/weights/pallet_utility.rs | 8 ++++++++ polkadot/runtime/rococo/src/weights/pallet_utility.rs | 8 ++++++++ polkadot/runtime/westend/src/weights/pallet_utility.rs | 8 ++++++++ 11 files changed, 88 insertions(+) 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 a82115b9d093..4f3136cead78 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,4 +99,12 @@ impl pallet_utility::WeightInfo for WeightInfo { // Standard Error: 1_745 .saturating_add(Weight::from_parts(6_562_902, 0).saturating_mul(c.into())) } + fn if_else() -> Weight { + // Proof Size summary in bytes: + // Measured: `0` + // Estimated: `0` + // Minimum execution time: 6_000_000 picoseconds. + Weight::from_parts(7_000_000, 0) + .saturating_add(Weight::from_parts(0, 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 a7952d6da00e..ef6d9fb4ba94 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,4 +98,12 @@ impl pallet_utility::WeightInfo for WeightInfo { // Standard Error: 3_765 .saturating_add(Weight::from_parts(6_028_416, 0).saturating_mul(c.into())) } + fn if_else() -> Weight { + // Proof Size summary in bytes: + // Measured: `0` + // Estimated: `0` + // Minimum execution time: 6_000_000 picoseconds. + Weight::from_parts(7_000_000, 0) + .saturating_add(Weight::from_parts(0, 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 d96b9e88840f..4e531593f4d5 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,4 +98,12 @@ impl pallet_utility::WeightInfo for WeightInfo { // Standard Error: 1_601 .saturating_add(Weight::from_parts(5_138_293, 0).saturating_mul(c.into())) } + fn if_else() -> Weight { + // Proof Size summary in bytes: + // Measured: `0` + // Estimated: `0` + // Minimum execution time: 6_000_000 picoseconds. + Weight::from_parts(7_000_000, 0) + .saturating_add(Weight::from_parts(0, 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 44cd0cf91e79..15d145a8f935 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,4 +99,12 @@ impl pallet_utility::WeightInfo for WeightInfo { // Standard Error: 1_601 .saturating_add(Weight::from_parts(5_138_293, 0).saturating_mul(c.into())) } + fn if_else() -> Weight { + // Proof Size summary in bytes: + // Measured: `0` + // Estimated: `0` + // Minimum execution time: 6_000_000 picoseconds. + Weight::from_parts(7_000_000, 0) + .saturating_add(Weight::from_parts(0, 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 c60a79d91da3..6887e41099e3 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,4 +98,12 @@ impl pallet_utility::WeightInfo for WeightInfo { // Standard Error: 1_395 .saturating_add(Weight::from_parts(5_000_971, 0).saturating_mul(c.into())) } + fn if_else() -> Weight { + // Proof Size summary in bytes: + // Measured: `0` + // Estimated: `0` + // Minimum execution time: 6_000_000 picoseconds. + Weight::from_parts(7_000_000, 0) + .saturating_add(Weight::from_parts(0, 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 84eb97838680..dfff076856f2 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,4 +99,12 @@ impl pallet_utility::WeightInfo for WeightInfo { // Standard Error: 1_621 .saturating_add(Weight::from_parts(3_312_302, 0).saturating_mul(c.into())) } + fn if_else() -> Weight { + // Proof Size summary in bytes: + // Measured: `0` + // Estimated: `0` + // Minimum execution time: 6_000_000 picoseconds. + Weight::from_parts(7_000_000, 0) + .saturating_add(Weight::from_parts(0, 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 0f5340843bd6..ec26f694a5b2 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,4 +99,12 @@ impl pallet_utility::WeightInfo for WeightInfo { // Standard Error: 740 .saturating_add(Weight::from_parts(2_800_888, 0).saturating_mul(c.into())) } + fn if_else() -> Weight { + // Proof Size summary in bytes: + // Measured: `0` + // Estimated: `0` + // Minimum execution time: 6_000_000 picoseconds. + Weight::from_parts(7_000_000, 0) + .saturating_add(Weight::from_parts(0, 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 134bd1fbbc58..f30f07769526 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,4 +96,12 @@ impl pallet_utility::WeightInfo for WeightInfo { // Standard Error: 3_915 .saturating_add(Weight::from_parts(4_372_646, 0).saturating_mul(c.into())) } + fn if_else() -> Weight { + // Proof Size summary in bytes: + // Measured: `0` + // Estimated: `0` + // Minimum execution time: 6_000_000 picoseconds. + Weight::from_parts(7_000_000, 0) + .saturating_add(Weight::from_parts(0, 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 782b0ad6de8d..c7f98f70fdd8 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,4 +96,12 @@ impl pallet_utility::WeightInfo for WeightInfo { // Standard Error: 7_605 .saturating_add(Weight::from_parts(4_306_193, 0).saturating_mul(c.into())) } + fn if_else() -> Weight { + // Proof Size summary in bytes: + // Measured: `0` + // Estimated: `0` + // Minimum execution time: 6_000_000 picoseconds. + Weight::from_parts(7_000_000, 0) + .saturating_add(Weight::from_parts(0, 0)) + } } diff --git a/polkadot/runtime/rococo/src/weights/pallet_utility.rs b/polkadot/runtime/rococo/src/weights/pallet_utility.rs index 6f2a374247f8..541414a807a1 100644 --- a/polkadot/runtime/rococo/src/weights/pallet_utility.rs +++ b/polkadot/runtime/rococo/src/weights/pallet_utility.rs @@ -99,4 +99,12 @@ impl pallet_utility::WeightInfo for WeightInfo { // Standard Error: 460 .saturating_add(Weight::from_parts(3_173_577, 0).saturating_mul(c.into())) } + fn if_else() -> Weight { + // Proof Size summary in bytes: + // Measured: `0` + // Estimated: `0` + // Minimum execution time: 6_000_000 picoseconds. + Weight::from_parts(6_000_000, 0) + .saturating_add(Weight::from_parts(0, 0)) + } } diff --git a/polkadot/runtime/westend/src/weights/pallet_utility.rs b/polkadot/runtime/westend/src/weights/pallet_utility.rs index f8238e9351dc..84fa0589a582 100644 --- a/polkadot/runtime/westend/src/weights/pallet_utility.rs +++ b/polkadot/runtime/westend/src/weights/pallet_utility.rs @@ -99,4 +99,12 @@ impl pallet_utility::WeightInfo for WeightInfo { // Standard Error: 2_817 .saturating_add(Weight::from_parts(5_113_539, 0).saturating_mul(c.into())) } + fn if_else() -> Weight { + // Proof Size summary in bytes: + // Measured: `0` + // Estimated: `0` + // Minimum execution time: 6_000_000 picoseconds. + Weight::from_parts(7_000_000, 0) + .saturating_add(Weight::from_parts(0, 0)) + } } From 007d1c37edaa6e405227e82c3f14ef3c06ec936e Mon Sep 17 00:00:00 2001 From: rainbow-promise Date: Tue, 12 Nov 2024 14:07:02 -0800 Subject: [PATCH 09/16] add prdoc --- .../rococo/src/weights/pallet_utility.rs | 2 +- prdoc/pr_6321.prdoc | 32 ++++++++ substrate/frame/utility/src/lib.rs | 78 +++++++++---------- substrate/frame/utility/src/tests.rs | 2 +- 4 files changed, 73 insertions(+), 41 deletions(-) create mode 100644 prdoc/pr_6321.prdoc diff --git a/polkadot/runtime/rococo/src/weights/pallet_utility.rs b/polkadot/runtime/rococo/src/weights/pallet_utility.rs index 541414a807a1..5e580de6aad5 100644 --- a/polkadot/runtime/rococo/src/weights/pallet_utility.rs +++ b/polkadot/runtime/rococo/src/weights/pallet_utility.rs @@ -104,7 +104,7 @@ impl pallet_utility::WeightInfo for WeightInfo { // Measured: `0` // Estimated: `0` // Minimum execution time: 6_000_000 picoseconds. - Weight::from_parts(6_000_000, 0) + Weight::from_parts(7_000_000, 0) .saturating_add(Weight::from_parts(0, 0)) } } diff --git a/prdoc/pr_6321.prdoc b/prdoc/pr_6321.prdoc new file mode 100644 index 000000000000..7f8cc1d21371 --- /dev/null +++ b/prdoc/pr_6321.prdoc @@ -0,0 +1,32 @@ +title: pallet-utility: if_else + +doc: + - audience: Runtime Dev + description: This PR adds the `if_else` call to `pallet-utility` + enabling an error fallback when the main call is unsuccessful. + +crates: + - name: asset-hub-rococo-runtime + bump: patch + - name: asset-hub-westend-runtime + bump: patch + - name: bridge-hub-rococo-runtime + bump: patch + - name: bridge-hub-westend-runtime + bump: patch + - name: collectives-westend-runtime + bump: patch + - name: coretime-rococo-runtime + bump: patch + - name: coretime-westend-runtime + bump: patch + - name: people-rococo-runtime + bump: patch + - name: people-westend-runtime + bump: patch + - name: rococo-runtime + bump: patch + - name: westend-runtime + bump: patch + - name: pallet-utility + bump: minor \ No newline at end of file diff --git a/substrate/frame/utility/src/lib.rs b/substrate/frame/utility/src/lib.rs index f549d097e12a..fafd6fce1c9a 100644 --- a/substrate/frame/utility/src/lib.rs +++ b/substrate/frame/utility/src/lib.rs @@ -64,10 +64,10 @@ use frame_support::{ dispatch::{extract_actual_weight, GetDispatchInfo, PostDispatchInfo}, traits::{IsSubType, OriginTrait, UnfilteredDispatchable}, }; +use scale_info::TypeInfo; use sp_core::TypeId; use sp_io::hashing::blake2_256; use sp_runtime::traits::{BadOrigin, Dispatchable, TrailingZeroInput}; -use scale_info::TypeInfo; pub use weights::WeightInfo; pub use pallet::*; @@ -169,7 +169,7 @@ pub mod pallet { /// Too many calls batched. TooManyCalls, /// Multiple call failure. - InvalidCalls + InvalidCalls, } #[pallet::call] @@ -478,57 +478,57 @@ pub mod pallet { ) })] pub fn if_else( - origin: OriginFor, - main: Box<::RuntimeCall>, - fallback: Box<::RuntimeCall>, + origin: OriginFor, + main: Box<::RuntimeCall>, + fallback: Box<::RuntimeCall>, ) -> DispatchResultWithPostInfo { - // Do not allow the `None` origin. - if ensure_none(origin.clone()).is_ok() { - return Err(BadOrigin.into()); - } + // Do not allow the `None` origin. + if ensure_none(origin.clone()).is_ok() { + return Err(BadOrigin.into()); + } - let is_root = ensure_root(origin.clone()).is_ok(); + let is_root = ensure_root(origin.clone()).is_ok(); - // Track the weights - let mut weight = Weight::zero(); + // Track the weights + let mut weight = Weight::zero(); let info = main.get_dispatch_info(); - // Execute the main call first - let main_result = if is_root { - main.dispatch_bypass_filter(origin.clone()) - } else { - main.dispatch(origin.clone()) - }; + // Execute the main call first + let main_result = if is_root { + main.dispatch_bypass_filter(origin.clone()) + } else { + main.dispatch(origin.clone()) + }; - // Add weight of the main call - weight = weight.saturating_add(extract_actual_weight(&main_result, &info)); + // Add weight of the main call + weight = weight.saturating_add(extract_actual_weight(&main_result, &info)); - if let Err(_main_call_error) = main_result { - // If the main call failed, execute the fallback call + if let Err(_main_call_error) = main_result { + // If the main call failed, execute the fallback call let fallback_info = fallback.get_dispatch_info(); - let fallback_result = if is_root { - fallback.dispatch_bypass_filter(origin.clone()) - } else { - fallback.dispatch(origin.clone()) - }; + let fallback_result = if is_root { + fallback.dispatch_bypass_filter(origin.clone()) + } else { + fallback.dispatch(origin.clone()) + }; - // Add weight of the fallback call - weight = weight.saturating_add(extract_actual_weight(&fallback_result, &fallback_info)); + // Add weight of the fallback call + weight = + weight.saturating_add(extract_actual_weight(&fallback_result, &fallback_info)); - if let Err(_fallback_error) = fallback_result { - // Both calls have faild. + if let Err(_fallback_error) = fallback_result { + // Both calls have faild. return Err(Error::::InvalidCalls.into()) - - } - // Fallback succeeded. - Self::deposit_event(Event::IfElseCompleted { call: Which::Fallback }); - return Ok(Some(weight).into()); - } + } + // Fallback succeeded. + Self::deposit_event(Event::IfElseCompleted { call: Which::Fallback }); + return Ok(Some(weight).into()); + } // Main call succeeded. - Self::deposit_event(Event::IfElseCompleted { call: Which::Main }); - Ok(Some(weight).into()) + Self::deposit_event(Event::IfElseCompleted { call: Which::Main }); + Ok(Some(weight).into()) } } diff --git a/substrate/frame/utility/src/tests.rs b/substrate/frame/utility/src/tests.rs index 2e23dbe2b896..b6c1e70c299e 100644 --- a/substrate/frame/utility/src/tests.rs +++ b/substrate/frame/utility/src/tests.rs @@ -947,7 +947,7 @@ fn if_else_with_signed_works() { assert_eq!(Balances::free_balance(2), 10); assert_ok!(Utility::if_else( RuntimeOrigin::signed(1), - Box::new(call_transfer(2, 11)), + Box::new(call_transfer(2, 11)), Box::new(call_transfer(2, 5)) )); assert_eq!(Balances::free_balance(1), 5); From f2bd9379d70ee6df85c65af33ad5a8c012885c76 Mon Sep 17 00:00:00 2001 From: rainbow-promise Date: Wed, 13 Nov 2024 06:19:36 -0800 Subject: [PATCH 10/16] check CI event not emitted --- prdoc/pr_6321.prdoc | 24 +++++----- substrate/frame/utility/src/benchmarking.rs | 2 +- substrate/frame/utility/src/lib.rs | 53 +++++++++++++++------ substrate/frame/utility/src/tests.rs | 36 ++++++++++++++ 4 files changed, 88 insertions(+), 27 deletions(-) diff --git a/prdoc/pr_6321.prdoc b/prdoc/pr_6321.prdoc index 7f8cc1d21371..011b51384dd1 100644 --- a/prdoc/pr_6321.prdoc +++ b/prdoc/pr_6321.prdoc @@ -7,26 +7,26 @@ doc: crates: - name: asset-hub-rococo-runtime - bump: patch + bump: major - name: asset-hub-westend-runtime - bump: patch + bump: major - name: bridge-hub-rococo-runtime - bump: patch + bump: major - name: bridge-hub-westend-runtime - bump: patch + bump: major - name: collectives-westend-runtime - bump: patch + bump: major - name: coretime-rococo-runtime - bump: patch + bump: major - name: coretime-westend-runtime - bump: patch + bump: major - name: people-rococo-runtime - bump: patch + bump: major - name: people-westend-runtime - bump: patch + bump: major - name: rococo-runtime - bump: patch + bump: major - name: westend-runtime - bump: patch + bump: major - name: pallet-utility - bump: minor \ No newline at end of file + bump: major \ No newline at end of file diff --git a/substrate/frame/utility/src/benchmarking.rs b/substrate/frame/utility/src/benchmarking.rs index 808c158df18b..392f1eb016e9 100644 --- a/substrate/frame/utility/src/benchmarking.rs +++ b/substrate/frame/utility/src/benchmarking.rs @@ -101,7 +101,7 @@ mod benchmark { #[extrinsic_call] _(RawOrigin::Signed(caller), main_call, fallback_call); - assert_last_event::(Event::IfElseCompleted { call: Which::Main }.into()); + assert_last_event::(Event::IfElseMainSuccess.into()); } impl_benchmark_test_suite! { diff --git a/substrate/frame/utility/src/lib.rs b/substrate/frame/utility/src/lib.rs index fafd6fce1c9a..a333ee9f60db 100644 --- a/substrate/frame/utility/src/lib.rs +++ b/substrate/frame/utility/src/lib.rs @@ -72,12 +72,6 @@ pub use weights::WeightInfo; pub use pallet::*; -#[derive(PartialEq, Eq, Clone, Copy, Encode, Decode, Debug, TypeInfo)] -pub enum Which { - Main, - Fallback, -} - #[frame_support::pallet] pub mod pallet { use super::*; @@ -127,8 +121,12 @@ pub mod pallet { ItemFailed { error: DispatchError }, /// A call was dispatched. DispatchedAs { result: DispatchResult }, - /// if_else completed. - IfElseCompleted { call: Which }, + /// Main call was dispatched. + IfElseMainSuccess, + /// The fallback call was dispatched. + IfElseFallbackSuccess { main_error: DispatchError }, + /// Both calls failed. + IfElseBothFailure { main_error: DispatchError, fallback_error: DispatchError }, } // Align the call size to 1KB. As we are currently compiling the runtime for native/wasm @@ -466,6 +464,32 @@ pub mod pallet { res.map(|_| ()).map_err(|e| e.error) } + /// Dispatch a fallback call in the event the main call fails to execute. + /// + /// This function first attempts to dispatch the `main` call. If the `main` call fails, the `fallback` call + /// is dispatched instead. Both calls are executed with the same origin, and the weight of both calls + /// is accumulated and returned. The success or failure of the main and fallback calls is tracked and + /// appropriate events are deposited. + /// + /// May be called from any origin except `None`. + /// + /// - `main`: The main call to be dispatched. This is the primary action to execute. + /// - `fallback`: The fallback call to be dispatched in case the `main` call fails. + /// + /// ## Dispatch Logic + /// - If the origin is `root`, both the main and fallback calls are executed without applying any origin filters. + /// - If the origin is not `root`, the origin filter is applied to both the `main` and `fallback` calls. + /// + /// ## Complexity + /// - O(1) for the origin check and dispatching the main call. + /// - O(1) for the origin check and dispatching the fallback call (if needed). + /// - Overall complexity is O(1) since we only dispatch at most two calls. + /// + /// ## Weight + /// The weight of this call is calculated as the sum of: + /// - The weight of the `main` call (if it is executed), + /// - The weight of the `fallback` call (if the `main` call fails and the `fallback` is executed), + /// - A base weight (`WeightInfo::if_else()`), which accounts for the logic involved in dispatching and handling both calls. #[pallet::call_index(6)] #[pallet::weight({ let main_dispatch_info = main.get_dispatch_info(); @@ -490,7 +514,7 @@ pub mod pallet { let is_root = ensure_root(origin.clone()).is_ok(); // Track the weights - let mut weight = Weight::zero(); + let mut weight = T::WeightInfo::if_else(); let info = main.get_dispatch_info(); @@ -504,30 +528,31 @@ pub mod pallet { // Add weight of the main call weight = weight.saturating_add(extract_actual_weight(&main_result, &info)); - if let Err(_main_call_error) = main_result { + if let Err(main_error) = main_result { // If the main call failed, execute the fallback call let fallback_info = fallback.get_dispatch_info(); let fallback_result = if is_root { fallback.dispatch_bypass_filter(origin.clone()) } else { - fallback.dispatch(origin.clone()) + fallback.dispatch(origin) }; // Add weight of the fallback call weight = weight.saturating_add(extract_actual_weight(&fallback_result, &fallback_info)); - if let Err(_fallback_error) = fallback_result { + if let Err(fallback_error) = fallback_result { // Both calls have faild. + Self::deposit_event(Event::IfElseBothFailure { main_error: main_error.error, fallback_error: fallback_error.error }); return Err(Error::::InvalidCalls.into()) } // Fallback succeeded. - Self::deposit_event(Event::IfElseCompleted { call: Which::Fallback }); + Self::deposit_event(Event::IfElseFallbackSuccess { main_error: main_error.error }); return Ok(Some(weight).into()); } // Main call succeeded. - Self::deposit_event(Event::IfElseCompleted { call: Which::Main }); + Self::deposit_event(Event::IfElseMainSuccess); Ok(Some(weight).into()) } } diff --git a/substrate/frame/utility/src/tests.rs b/substrate/frame/utility/src/tests.rs index b6c1e70c299e..02f007507ece 100644 --- a/substrate/frame/utility/src/tests.rs +++ b/substrate/frame/utility/src/tests.rs @@ -952,5 +952,41 @@ fn if_else_with_signed_works() { )); assert_eq!(Balances::free_balance(1), 5); assert_eq!(Balances::free_balance(2), 15); + + System::assert_last_event(utility::Event::IfElseFallbackSuccess { main_error: TokenError::FundsUnavailable.into() }.into()); }); } + +#[test] +fn if_else_successful_main_call() { + new_test_ext().execute_with(|| { + assert_eq!(Balances::free_balance(1), 10); + assert_eq!(Balances::free_balance(2), 10); + assert_ok!(Utility::if_else( + RuntimeOrigin::signed(1), + Box::new(call_transfer(2, 9)), + Box::new(call_transfer(2, 1)) + )); + assert_eq!(Balances::free_balance(1), 1); + assert_eq!(Balances::free_balance(2), 19); + + System::assert_last_event(utility::Event::IfElseMainSuccess.into()); + }) +} + +#[test] +fn if_else_failing_else_call() { + new_test_ext().execute_with(|| { + assert_eq!(Balances::free_balance(1), 10); + assert_eq!(Balances::free_balance(2), 10); + assert_noop!(Utility::if_else( + RuntimeOrigin::signed(1), + Box::new(call_transfer(2, 11)), + Box::new(call_transfer(2, 11)) + ), utility::Error::::InvalidCalls); + assert_eq!(Balances::free_balance(1), 10); + assert_eq!(Balances::free_balance(2), 10); + + System::assert_last_event(utility::Event::IfElseBothFailure { main_error: TokenError::FundsUnavailable.into(), fallback_error: TokenError::FundsUnavailable.into()}.into()); + }) +} From ad9e68be99a948415a608c0920e549920605a102 Mon Sep 17 00:00:00 2001 From: rainbow-promise Date: Thu, 14 Nov 2024 12:18:13 -0800 Subject: [PATCH 11/16] DispatchErrorWithPostInfo --- substrate/frame/utility/src/lib.rs | 3 +-- substrate/frame/utility/src/tests.rs | 2 +- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/substrate/frame/utility/src/lib.rs b/substrate/frame/utility/src/lib.rs index a333ee9f60db..4d782bf215be 100644 --- a/substrate/frame/utility/src/lib.rs +++ b/substrate/frame/utility/src/lib.rs @@ -64,7 +64,6 @@ use frame_support::{ dispatch::{extract_actual_weight, GetDispatchInfo, PostDispatchInfo}, traits::{IsSubType, OriginTrait, UnfilteredDispatchable}, }; -use scale_info::TypeInfo; use sp_core::TypeId; use sp_io::hashing::blake2_256; use sp_runtime::traits::{BadOrigin, Dispatchable, TrailingZeroInput}; @@ -545,7 +544,7 @@ pub mod pallet { if let Err(fallback_error) = fallback_result { // Both calls have faild. Self::deposit_event(Event::IfElseBothFailure { main_error: main_error.error, fallback_error: fallback_error.error }); - return Err(Error::::InvalidCalls.into()) + return Err(sp_runtime::DispatchErrorWithPostInfo { error: Error::::InvalidCalls.into(), post_info: Some(weight).into() }) } // Fallback succeeded. Self::deposit_event(Event::IfElseFallbackSuccess { main_error: main_error.error }); diff --git a/substrate/frame/utility/src/tests.rs b/substrate/frame/utility/src/tests.rs index 02f007507ece..596101da80d2 100644 --- a/substrate/frame/utility/src/tests.rs +++ b/substrate/frame/utility/src/tests.rs @@ -979,7 +979,7 @@ fn if_else_failing_else_call() { new_test_ext().execute_with(|| { assert_eq!(Balances::free_balance(1), 10); assert_eq!(Balances::free_balance(2), 10); - assert_noop!(Utility::if_else( + assert_err_ignore_postinfo!(Utility::if_else( RuntimeOrigin::signed(1), Box::new(call_transfer(2, 11)), Box::new(call_transfer(2, 11)) From 5ef6752dba921e52e2d614c0c057d9b9a0e758bd Mon Sep 17 00:00:00 2001 From: rainbow-promise Date: Fri, 15 Nov 2024 06:07:32 -0800 Subject: [PATCH 12/16] conditional Dispatch class --- substrate/frame/utility/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/substrate/frame/utility/src/lib.rs b/substrate/frame/utility/src/lib.rs index 4d782bf215be..fbbe9385baa9 100644 --- a/substrate/frame/utility/src/lib.rs +++ b/substrate/frame/utility/src/lib.rs @@ -497,7 +497,7 @@ pub mod pallet { T::WeightInfo::if_else() .saturating_add(main_dispatch_info.call_weight) .saturating_add(fallback_dispatch_info.call_weight), - main_dispatch_info.class, + if main.class == Operational && fallback.class == Operational { Operational } else { Normal }, ) })] pub fn if_else( From 1d01444478cb9cefdc910e04f5f355cb21c57fd2 Mon Sep 17 00:00:00 2001 From: rainbow-promise Date: Fri, 15 Nov 2024 07:05:27 -0800 Subject: [PATCH 13/16] nit DispatchClass --- substrate/frame/utility/src/lib.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/substrate/frame/utility/src/lib.rs b/substrate/frame/utility/src/lib.rs index fbbe9385baa9..bf1baae6172f 100644 --- a/substrate/frame/utility/src/lib.rs +++ b/substrate/frame/utility/src/lib.rs @@ -61,7 +61,7 @@ extern crate alloc; use alloc::{boxed::Box, vec::Vec}; use codec::{Decode, Encode}; use frame_support::{ - dispatch::{extract_actual_weight, GetDispatchInfo, PostDispatchInfo}, + dispatch::{extract_actual_weight, GetDispatchInfo, PostDispatchInfo, DispatchClass::{Operational, Normal}}, traits::{IsSubType, OriginTrait, UnfilteredDispatchable}, }; use sp_core::TypeId; @@ -491,12 +491,12 @@ pub mod pallet { /// - A base weight (`WeightInfo::if_else()`), which accounts for the logic involved in dispatching and handling both calls. #[pallet::call_index(6)] #[pallet::weight({ - let main_dispatch_info = main.get_dispatch_info(); - let fallback_dispatch_info = fallback.get_dispatch_info(); + let main = main.get_dispatch_info(); + let fallback = fallback.get_dispatch_info(); ( T::WeightInfo::if_else() - .saturating_add(main_dispatch_info.call_weight) - .saturating_add(fallback_dispatch_info.call_weight), + .saturating_add(main.call_weight) + .saturating_add(fallback.call_weight), if main.class == Operational && fallback.class == Operational { Operational } else { Normal }, ) })] From 975ac654a8dc443e4ac83876e79cc5efa5424c6a Mon Sep 17 00:00:00 2001 From: rainbow-promise Date: Thu, 21 Nov 2024 13:33:50 -0800 Subject: [PATCH 14/16] all calls fail return just error, events don't fire in the same block --- prdoc/pr_6321.prdoc | 2 +- substrate/frame/utility/src/lib.rs | 54 +++++++++++++++------------- substrate/frame/utility/src/tests.rs | 22 +++++++----- 3 files changed, 45 insertions(+), 33 deletions(-) diff --git a/prdoc/pr_6321.prdoc b/prdoc/pr_6321.prdoc index 011b51384dd1..87157a93dd2c 100644 --- a/prdoc/pr_6321.prdoc +++ b/prdoc/pr_6321.prdoc @@ -1,4 +1,4 @@ -title: pallet-utility: if_else +title: Utility call fallback doc: - audience: Runtime Dev diff --git a/substrate/frame/utility/src/lib.rs b/substrate/frame/utility/src/lib.rs index bf1baae6172f..c0ad84518a25 100644 --- a/substrate/frame/utility/src/lib.rs +++ b/substrate/frame/utility/src/lib.rs @@ -61,7 +61,11 @@ extern crate alloc; use alloc::{boxed::Box, vec::Vec}; use codec::{Decode, Encode}; use frame_support::{ - dispatch::{extract_actual_weight, GetDispatchInfo, PostDispatchInfo, DispatchClass::{Operational, Normal}}, + dispatch::{ + extract_actual_weight, + DispatchClass::{Normal, Operational}, + GetDispatchInfo, PostDispatchInfo, + }, traits::{IsSubType, OriginTrait, UnfilteredDispatchable}, }; use sp_core::TypeId; @@ -123,9 +127,7 @@ pub mod pallet { /// Main call was dispatched. IfElseMainSuccess, /// The fallback call was dispatched. - IfElseFallbackSuccess { main_error: DispatchError }, - /// Both calls failed. - IfElseBothFailure { main_error: DispatchError, fallback_error: DispatchError }, + IfElseFallbackCalled { main_error: DispatchError }, } // Align the call size to 1KB. As we are currently compiling the runtime for native/wasm @@ -165,8 +167,8 @@ pub mod pallet { pub enum Error { /// Too many calls batched. TooManyCalls, - /// Multiple call failure. - InvalidCalls, + /// Main and fallback call failure. + IfElseBothFailure, } #[pallet::call] @@ -464,31 +466,33 @@ pub mod pallet { } /// Dispatch a fallback call in the event the main call fails to execute. + /// May be called from any origin except `None`. /// - /// This function first attempts to dispatch the `main` call. If the `main` call fails, the `fallback` call - /// is dispatched instead. Both calls are executed with the same origin, and the weight of both calls - /// is accumulated and returned. The success or failure of the main and fallback calls is tracked and - /// appropriate events are deposited. + /// This function first attempts to dispatch the `main` call. + /// If the `main` call fails, the `fallback` is attemted. + /// if the fallback is successfully dispatched, the weights of both calls + /// are accumulated and an event containing the main call error is deposited. /// - /// May be called from any origin except `None`. + /// In the event of a fallback failure the whole call fails + /// with the weights returned. /// /// - `main`: The main call to be dispatched. This is the primary action to execute. /// - `fallback`: The fallback call to be dispatched in case the `main` call fails. /// /// ## Dispatch Logic - /// - If the origin is `root`, both the main and fallback calls are executed without applying any origin filters. - /// - If the origin is not `root`, the origin filter is applied to both the `main` and `fallback` calls. + /// - If the origin is `root`, both the main and fallback calls are executed without + /// applying any origin filters. + /// - If the origin is not `root`, the origin filter is applied to both the `main` and + /// `fallback` calls. /// - /// ## Complexity - /// - O(1) for the origin check and dispatching the main call. - /// - O(1) for the origin check and dispatching the fallback call (if needed). - /// - Overall complexity is O(1) since we only dispatch at most two calls. + /// ## Use Case + /// - Some use cases might involve submitting a `batch` type call in either main, fallback + /// or both. /// /// ## Weight /// The weight of this call is calculated as the sum of: - /// - The weight of the `main` call (if it is executed), - /// - The weight of the `fallback` call (if the `main` call fails and the `fallback` is executed), - /// - A base weight (`WeightInfo::if_else()`), which accounts for the logic involved in dispatching and handling both calls. + /// `WeightInfo::if_else()` + `main` | + /// `WeightInfo::if_else()` + `main` + `fallback`. #[pallet::call_index(6)] #[pallet::weight({ let main = main.get_dispatch_info(); @@ -541,13 +545,15 @@ pub mod pallet { weight = weight.saturating_add(extract_actual_weight(&fallback_result, &fallback_info)); - if let Err(fallback_error) = fallback_result { + if let Err(_fallback_error) = fallback_result { // Both calls have faild. - Self::deposit_event(Event::IfElseBothFailure { main_error: main_error.error, fallback_error: fallback_error.error }); - return Err(sp_runtime::DispatchErrorWithPostInfo { error: Error::::InvalidCalls.into(), post_info: Some(weight).into() }) + return Err(sp_runtime::DispatchErrorWithPostInfo { + error: Error::::IfElseBothFailure.into(), + post_info: Some(weight).into(), + }) } // Fallback succeeded. - Self::deposit_event(Event::IfElseFallbackSuccess { main_error: main_error.error }); + Self::deposit_event(Event::IfElseFallbackCalled { main_error: main_error.error }); return Ok(Some(weight).into()); } // Main call succeeded. diff --git a/substrate/frame/utility/src/tests.rs b/substrate/frame/utility/src/tests.rs index 596101da80d2..99b9e6ceaa46 100644 --- a/substrate/frame/utility/src/tests.rs +++ b/substrate/frame/utility/src/tests.rs @@ -953,7 +953,12 @@ fn if_else_with_signed_works() { assert_eq!(Balances::free_balance(1), 5); assert_eq!(Balances::free_balance(2), 15); - System::assert_last_event(utility::Event::IfElseFallbackSuccess { main_error: TokenError::FundsUnavailable.into() }.into()); + System::assert_last_event( + utility::Event::IfElseFallbackCalled { + main_error: TokenError::FundsUnavailable.into(), + } + .into(), + ); }); } @@ -979,14 +984,15 @@ fn if_else_failing_else_call() { new_test_ext().execute_with(|| { assert_eq!(Balances::free_balance(1), 10); assert_eq!(Balances::free_balance(2), 10); - assert_err_ignore_postinfo!(Utility::if_else( - RuntimeOrigin::signed(1), - Box::new(call_transfer(2, 11)), - Box::new(call_transfer(2, 11)) - ), utility::Error::::InvalidCalls); + assert_err_ignore_postinfo!( + Utility::if_else( + RuntimeOrigin::signed(1), + Box::new(call_transfer(2, 11)), + Box::new(call_transfer(2, 11)) + ), + utility::Error::::IfElseBothFailure + ); assert_eq!(Balances::free_balance(1), 10); assert_eq!(Balances::free_balance(2), 10); - - System::assert_last_event(utility::Event::IfElseBothFailure { main_error: TokenError::FundsUnavailable.into(), fallback_error: TokenError::FundsUnavailable.into()}.into()); }) } From caf067bed387b13fb386a165bd456d6052ca0b0a Mon Sep 17 00:00:00 2001 From: rainbow-promise Date: Fri, 22 Nov 2024 02:00:19 -0800 Subject: [PATCH 15/16] include fallback error in DispatchErrorWithPostInfo --- prdoc/pr_6321.prdoc | 53 ++++++++++++++-------------- substrate/frame/utility/src/lib.rs | 8 ++--- substrate/frame/utility/src/tests.rs | 2 +- 3 files changed, 31 insertions(+), 32 deletions(-) diff --git a/prdoc/pr_6321.prdoc b/prdoc/pr_6321.prdoc index 87157a93dd2c..9f70a1b2ee69 100644 --- a/prdoc/pr_6321.prdoc +++ b/prdoc/pr_6321.prdoc @@ -2,31 +2,32 @@ title: Utility call fallback doc: - audience: Runtime Dev - description: This PR adds the `if_else` call to `pallet-utility` - enabling an error fallback when the main call is unsuccessful. + description: | + This PR adds the `if_else` call to `pallet-utility` + enabling an error fallback when the main call is unsuccessful. crates: - - name: asset-hub-rococo-runtime - bump: major - - name: asset-hub-westend-runtime - bump: major - - name: bridge-hub-rococo-runtime - bump: major - - name: bridge-hub-westend-runtime - bump: major - - name: collectives-westend-runtime - bump: major - - name: coretime-rococo-runtime - bump: major - - name: coretime-westend-runtime - bump: major - - name: people-rococo-runtime - bump: major - - name: people-westend-runtime - bump: major - - name: rococo-runtime - bump: major - - name: westend-runtime - bump: major - - name: pallet-utility - bump: major \ No newline at end of file + - name: asset-hub-rococo-runtime + bump: major + - name: asset-hub-westend-runtime + bump: major + - name: bridge-hub-rococo-runtime + bump: major + - name: bridge-hub-westend-runtime + bump: major + - name: collectives-westend-runtime + bump: major + - name: coretime-rococo-runtime + bump: major + - name: coretime-westend-runtime + bump: major + - name: people-rococo-runtime + bump: major + - name: people-westend-runtime + bump: major + - name: rococo-runtime + bump: major + - name: westend-runtime + bump: major + - name: pallet-utility + bump: major \ No newline at end of file diff --git a/substrate/frame/utility/src/lib.rs b/substrate/frame/utility/src/lib.rs index c0ad84518a25..cf66605f9a25 100644 --- a/substrate/frame/utility/src/lib.rs +++ b/substrate/frame/utility/src/lib.rs @@ -167,8 +167,6 @@ pub mod pallet { pub enum Error { /// Too many calls batched. TooManyCalls, - /// Main and fallback call failure. - IfElseBothFailure, } #[pallet::call] @@ -545,10 +543,10 @@ pub mod pallet { weight = weight.saturating_add(extract_actual_weight(&fallback_result, &fallback_info)); - if let Err(_fallback_error) = fallback_result { - // Both calls have faild. + if let Err(fallback_error) = fallback_result { + // Both calls have failed, return fallback error return Err(sp_runtime::DispatchErrorWithPostInfo { - error: Error::::IfElseBothFailure.into(), + error: fallback_error.error, post_info: Some(weight).into(), }) } diff --git a/substrate/frame/utility/src/tests.rs b/substrate/frame/utility/src/tests.rs index 99b9e6ceaa46..db49ee3ff51a 100644 --- a/substrate/frame/utility/src/tests.rs +++ b/substrate/frame/utility/src/tests.rs @@ -990,7 +990,7 @@ fn if_else_failing_else_call() { Box::new(call_transfer(2, 11)), Box::new(call_transfer(2, 11)) ), - utility::Error::::IfElseBothFailure + TokenError::FundsUnavailable ); assert_eq!(Balances::free_balance(1), 10); assert_eq!(Balances::free_balance(2), 10); From ca8c072d167c8fdd6246e4b4748ed9da248d8007 Mon Sep 17 00:00:00 2001 From: rainbow-promise Date: Wed, 27 Nov 2024 08:04:10 -0800 Subject: [PATCH 16/16] nested if_else test --- substrate/frame/utility/src/benchmarking.rs | 5 +- substrate/frame/utility/src/lib.rs | 54 +++++++++---------- substrate/frame/utility/src/tests.rs | 58 ++++++++++++++++----- 3 files changed, 72 insertions(+), 45 deletions(-) diff --git a/substrate/frame/utility/src/benchmarking.rs b/substrate/frame/utility/src/benchmarking.rs index 392f1eb016e9..261d52436889 100644 --- a/substrate/frame/utility/src/benchmarking.rs +++ b/substrate/frame/utility/src/benchmarking.rs @@ -94,14 +94,13 @@ mod benchmark { #[benchmark] fn if_else() { - let main_call = Box::new(frame_system::Call::remark { remark: vec![0] }.into()); + // Failing main call. + let main_call = Box::new(frame_system::Call::set_code { code: vec![1] }.into()); let fallback_call = Box::new(frame_system::Call::remark { remark: vec![1] }.into()); let caller = whitelisted_caller(); #[extrinsic_call] _(RawOrigin::Signed(caller), main_call, fallback_call); - - assert_last_event::(Event::IfElseMainSuccess.into()); } impl_benchmark_test_suite! { diff --git a/substrate/frame/utility/src/lib.rs b/substrate/frame/utility/src/lib.rs index cf66605f9a25..63a02febb94c 100644 --- a/substrate/frame/utility/src/lib.rs +++ b/substrate/frame/utility/src/lib.rs @@ -486,11 +486,6 @@ pub mod pallet { /// ## Use Case /// - Some use cases might involve submitting a `batch` type call in either main, fallback /// or both. - /// - /// ## Weight - /// The weight of this call is calculated as the sum of: - /// `WeightInfo::if_else()` + `main` | - /// `WeightInfo::if_else()` + `main` + `fallback`. #[pallet::call_index(6)] #[pallet::weight({ let main = main.get_dispatch_info(); @@ -517,7 +512,7 @@ pub mod pallet { // Track the weights let mut weight = T::WeightInfo::if_else(); - let info = main.get_dispatch_info(); + let main_info = main.get_dispatch_info(); // Execute the main call first let main_result = if is_root { @@ -527,36 +522,37 @@ pub mod pallet { }; // Add weight of the main call - weight = weight.saturating_add(extract_actual_weight(&main_result, &info)); + weight = weight.saturating_add(extract_actual_weight(&main_result, &main_info)); + + let Err(main_error) = main_result else { + // If the main result is Ok, we skip the fallback logic entirely + Self::deposit_event(Event::IfElseMainSuccess); + return Ok(Some(weight).into()); + }; - if let Err(main_error) = main_result { - // If the main call failed, execute the fallback call - let fallback_info = fallback.get_dispatch_info(); + // If the main call failed, execute the fallback call + let fallback_info = fallback.get_dispatch_info(); - let fallback_result = if is_root { - fallback.dispatch_bypass_filter(origin.clone()) - } else { - fallback.dispatch(origin) - }; + let fallback_result = if is_root { + fallback.dispatch_bypass_filter(origin.clone()) + } else { + fallback.dispatch(origin) + }; - // Add weight of the fallback call - weight = - weight.saturating_add(extract_actual_weight(&fallback_result, &fallback_info)); + // Add weight of the fallback call + weight = weight.saturating_add(extract_actual_weight(&fallback_result, &fallback_info)); - if let Err(fallback_error) = fallback_result { - // Both calls have failed, return fallback error - return Err(sp_runtime::DispatchErrorWithPostInfo { - error: fallback_error.error, - post_info: Some(weight).into(), - }) - } + let Err(fallback_error) = fallback_result else { // Fallback succeeded. Self::deposit_event(Event::IfElseFallbackCalled { main_error: main_error.error }); return Ok(Some(weight).into()); - } - // Main call succeeded. - Self::deposit_event(Event::IfElseMainSuccess); - Ok(Some(weight).into()) + }; + + // Both calls have failed, return fallback error + Err(sp_runtime::DispatchErrorWithPostInfo { + error: fallback_error.error, + post_info: Some(weight).into(), + }) } } diff --git a/substrate/frame/utility/src/tests.rs b/substrate/frame/utility/src/tests.rs index db49ee3ff51a..64ad12e9c594 100644 --- a/substrate/frame/utility/src/tests.rs +++ b/substrate/frame/utility/src/tests.rs @@ -927,16 +927,19 @@ fn if_else_with_root_works() { assert_eq!(Balances::free_balance(2), 10); assert_ok!(Utility::if_else( RuntimeOrigin::root(), - Box::new(RuntimeCall::Balances(BalancesCall::force_transfer { - source: 1, - dest: 2, - value: 11 - })), - Box::new(call), + RuntimeCall::Balances(BalancesCall::force_transfer { source: 1, dest: 2, value: 11 }) + .into(), + call.into(), )); assert_eq!(Balances::free_balance(1), 10); assert_eq!(Balances::free_balance(2), 10); assert_eq!(storage::unhashed::get_raw(&k), Some(k)); + System::assert_last_event( + utility::Event::IfElseFallbackCalled { + main_error: TokenError::FundsUnavailable.into(), + } + .into(), + ); }); } @@ -947,8 +950,8 @@ fn if_else_with_signed_works() { assert_eq!(Balances::free_balance(2), 10); assert_ok!(Utility::if_else( RuntimeOrigin::signed(1), - Box::new(call_transfer(2, 11)), - Box::new(call_transfer(2, 5)) + call_transfer(2, 11).into(), + call_transfer(2, 5).into() )); assert_eq!(Balances::free_balance(1), 5); assert_eq!(Balances::free_balance(2), 15); @@ -969,8 +972,8 @@ fn if_else_successful_main_call() { assert_eq!(Balances::free_balance(2), 10); assert_ok!(Utility::if_else( RuntimeOrigin::signed(1), - Box::new(call_transfer(2, 9)), - Box::new(call_transfer(2, 1)) + call_transfer(2, 9).into(), + call_transfer(2, 1).into() )); assert_eq!(Balances::free_balance(1), 1); assert_eq!(Balances::free_balance(2), 19); @@ -980,15 +983,15 @@ fn if_else_successful_main_call() { } #[test] -fn if_else_failing_else_call() { +fn if_else_failing_fallback_call() { new_test_ext().execute_with(|| { assert_eq!(Balances::free_balance(1), 10); assert_eq!(Balances::free_balance(2), 10); assert_err_ignore_postinfo!( Utility::if_else( RuntimeOrigin::signed(1), - Box::new(call_transfer(2, 11)), - Box::new(call_transfer(2, 11)) + call_transfer(2, 11).into(), + call_transfer(2, 11).into() ), TokenError::FundsUnavailable ); @@ -996,3 +999,32 @@ fn if_else_failing_else_call() { assert_eq!(Balances::free_balance(2), 10); }) } + +#[test] +fn if_else_with_nested_if_else_works() { + new_test_ext().execute_with(|| { + assert_eq!(Balances::free_balance(1), 10); + assert_eq!(Balances::free_balance(2), 10); + + let main_call = call_transfer(2, 11).into(); + let fallback_call = call_transfer(2, 5).into(); + + let nested_if_else_call = + RuntimeCall::Utility(UtilityCall::if_else { main: main_call, fallback: fallback_call }) + .into(); + + // Nested `if_else` call. + assert_ok!(Utility::if_else( + RuntimeOrigin::signed(1), + nested_if_else_call, + call_transfer(2, 7).into() + )); + + // inner if_else fallback is executed. + assert_eq!(Balances::free_balance(1), 5); + assert_eq!(Balances::free_balance(2), 15); + + // Ensure the correct event was triggered for the main call(nested if_else). + System::assert_last_event(utility::Event::IfElseMainSuccess.into()); + }); +}