Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

pallet-utility: if_else #6321

Open
wants to merge 22 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
b83a8e8
extrinsic outline
rainbow-promise Oct 31, 2024
1b01dc9
new: outline utility_if_else
rainbow-promise Nov 1, 2024
430a791
do not clone calls
rainbow-promise Nov 6, 2024
2caf95e
compiler needs to know the size, put in Box
rainbow-promise Nov 6, 2024
4183c6b
siimple test cases
rainbow-promise Nov 6, 2024
e70a607
if_else::benchmarking
rainbow-promise Nov 7, 2024
a93b142
add weights substrate
rainbow-promise Nov 7, 2024
cc79306
add weight to all runtimes
rainbow-promise Nov 7, 2024
007d1c3
add prdoc
rainbow-promise Nov 12, 2024
7f2c600
Merge branch 'master' into if-else-utility
rainbow-promise Nov 12, 2024
f2bd937
check CI event not emitted
rainbow-promise Nov 13, 2024
ad9e68b
DispatchErrorWithPostInfo
rainbow-promise Nov 14, 2024
5ef6752
conditional Dispatch class
rainbow-promise Nov 15, 2024
1d01444
nit DispatchClass
rainbow-promise Nov 15, 2024
975ac65
all calls fail return just error, events don't fire in the same block
rainbow-promise Nov 21, 2024
aa1fdc7
Merge branch 'master' into if-else-utility
rainbow-promise Nov 21, 2024
caf067b
include fallback error in DispatchErrorWithPostInfo
rainbow-promise Nov 22, 2024
cd7d48f
Merge branch 'master' into if-else-utility
rainbow-promise Nov 27, 2024
ca8c072
nested if_else test
rainbow-promise Nov 27, 2024
898b5e1
Merge branch 'master' into if-else-utility
rainbow-promise Nov 29, 2024
be2f1c2
Merge branch 'master' into if-else-utility
gui1117 Dec 3, 2024
f65f270
Merge branch 'master' into if-else-utility
rainbow-promise Dec 4, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -99,4 +99,12 @@ 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 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))
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -98,4 +98,12 @@ 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 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))
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -98,4 +98,12 @@ 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 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))
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -99,4 +99,12 @@ 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 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))
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -98,4 +98,12 @@ 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 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))
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -99,4 +99,12 @@ 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 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))
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -99,4 +99,12 @@ 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 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))
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -96,4 +96,12 @@ 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 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))
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -96,4 +96,12 @@ 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 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))
}
}
8 changes: 8 additions & 0 deletions polkadot/runtime/rococo/src/weights/pallet_utility.rs
Original file line number Diff line number Diff line change
Expand Up @@ -99,4 +99,12 @@ 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 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))
}
}
8 changes: 8 additions & 0 deletions polkadot/runtime/westend/src/weights/pallet_utility.rs
Original file line number Diff line number Diff line change
Expand Up @@ -99,4 +99,12 @@ 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 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))
}
}
33 changes: 33 additions & 0 deletions prdoc/pr_6321.prdoc
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
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.

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
12 changes: 12 additions & 0 deletions substrate/frame/utility/src/benchmarking.rs
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,18 @@ mod benchmark {
assert_last_event::<T>(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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The err branch should be slightly more costly, but difference should be negligible so it is ok.

Copy link
Author

@rainbow-promise rainbow-promise Nov 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok.

Thanks for the review

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can use a root call, it will be failing, like set_heap_page.
This would benchmark for the failing case, which is more costly

Copy link
Author

@rainbow-promise rainbow-promise Nov 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

...
        #[benchmark]
	fn if_else() {
		let main_call = Box::new(frame_system::Call::set_heap_pages { pages: 0u64 }.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::<T>(
			Event::IfElseFallbackCalled {
				main_error: sp_runtime::DispatchError::Module(sp_runtime::ModuleError {
					index: 0,
					error: [5, 0, 0, 0],
					message: None,
				}),
			}
			.into(),
		);
	}

Copy link
Author

@rainbow-promise rainbow-promise Nov 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gui1117 the event passes for cargo test -p pallet-utility --features runtime-benchmarks

but fails when building for most runtimes

Copy link
Contributor

@gui1117 gui1117 Dec 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you give an example of fail when building runtime? it seems to work for me, do you have a command to reproduce?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, best to use other pallets for this benchmark case.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand what you mean, can you say again?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mean to ask if importing other pallets and using their call enum is possible?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no I think pallet utility can't assume usage of any other pallet than system.

but fails when building for most runtimes

But what is the command you run that resulted in a runtime failing to build?


assert_last_event::<T>(Event::IfElseMainSuccess.into());
}

impl_benchmark_test_suite! {
Pallet,
tests::new_test_ext(),
Expand Down
106 changes: 105 additions & 1 deletion substrate/frame/utility/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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},
dispatch::{
extract_actual_weight,
DispatchClass::{Normal, Operational},
GetDispatchInfo, PostDispatchInfo,
},
traits::{IsSubType, OriginTrait, UnfilteredDispatchable},
};
use sp_core::TypeId;
Expand Down Expand Up @@ -120,6 +124,10 @@ pub mod pallet {
ItemFailed { error: DispatchError },
/// A call was dispatched.
DispatchedAs { result: DispatchResult },
/// Main call was dispatched.
IfElseMainSuccess,
/// The fallback call was dispatched.
IfElseFallbackCalled { main_error: DispatchError },
}

// Align the call size to 1KB. As we are currently compiling the runtime for native/wasm
Expand Down Expand Up @@ -454,6 +462,102 @@ pub mod pallet {
let res = call.dispatch_bypass_filter(frame_system::RawOrigin::Root.into());
res.map(|_| ()).map_err(|e| e.error)
}

/// 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` 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.
///
/// 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.
///
/// ## 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`.
rainbow-promise marked this conversation as resolved.
Show resolved Hide resolved
#[pallet::call_index(6)]
rainbow-promise marked this conversation as resolved.
Show resolved Hide resolved
#[pallet::weight({
let main = main.get_dispatch_info();
let fallback = fallback.get_dispatch_info();
(
T::WeightInfo::if_else()
.saturating_add(main.call_weight)
.saturating_add(fallback.call_weight),
if main.class == Operational && fallback.class == Operational { Operational } else { Normal },
)
})]
pub fn if_else(
origin: OriginFor<T>,
main: Box<<T as Config>::RuntimeCall>,
fallback: Box<<T as Config>::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();

// Track the weights
let mut weight = T::WeightInfo::if_else();

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())
};

// Add weight of the main call
weight = weight.saturating_add(extract_actual_weight(&main_result, &info));

if let Err(main_error) = main_result {
rainbow-promise marked this conversation as resolved.
Show resolved Hide resolved
// 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)
};

// 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(),
})
}
// 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())
}
}

impl<T: Config> Pallet<T> {
Expand Down
Loading
Loading