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 11 commits into
base: master
Choose a base branch
from
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))
}
}
32 changes: 32 additions & 0 deletions prdoc/pr_6321.prdoc
Original file line number Diff line number Diff line change
@@ -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
rainbow-promise marked this conversation as resolved.
Show resolved Hide resolved
- name: pallet-utility
bump: minor
rainbow-promise marked this conversation as resolved.
Show resolved Hide resolved
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

Choose a reason for hiding this comment

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

So adjusting with a failed main call will give the proper weights as it enters the else branch? will update this.

Thanks for the review


assert_last_event::<T>(Event::IfElseCompleted { call: Which::Main }.into());
}

impl_benchmark_test_suite! {
Pallet,
tests::new_test_ext(),
Expand Down
76 changes: 76 additions & 0 deletions substrate/frame/utility/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -64,13 +64,20 @@ 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};
pub use weights::WeightInfo;

pub use pallet::*;

#[derive(PartialEq, Eq, Clone, Copy, Encode, Decode, Debug, TypeInfo)]
pub enum Which {
rainbow-promise marked this conversation as resolved.
Show resolved Hide resolved
Main,
Fallback,
}

#[frame_support::pallet]
pub mod pallet {
use super::*;
Expand Down Expand Up @@ -120,6 +127,8 @@ pub mod pallet {
ItemFailed { error: DispatchError },
/// A call was dispatched.
DispatchedAs { result: DispatchResult },
/// if_else completed.
IfElseCompleted { call: Which },
}

// Align the call size to 1KB. As we are currently compiling the runtime for native/wasm
Expand Down Expand Up @@ -159,6 +168,8 @@ pub mod pallet {
pub enum Error<T> {
/// Too many calls batched.
TooManyCalls,
/// Multiple call failure.
InvalidCalls,
}

#[pallet::call]
Expand Down Expand Up @@ -454,6 +465,71 @@ 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)]
rainbow-promise marked this conversation as resolved.
Show resolved Hide resolved
#[pallet::weight({
let main_dispatch_info = main.get_dispatch_info();
let fallback_dispatch_info = fallback.get_dispatch_info();
(
T::WeightInfo::if_else()
.saturating_add(main_dispatch_info.call_weight)
.saturating_add(fallback_dispatch_info.call_weight),
main_dispatch_info.class,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should take the lowest class between main and fallback.

Otherwise somebody can do a call with a failing operational main call. And a fallback with a batch of normal call.

Copy link
Author

Choose a reason for hiding this comment

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

can this be compared?

or I need to implement ord here?

)
})]
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 = Weight::zero();
rainbow-promise marked this conversation as resolved.
Show resolved Hide resolved

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

// 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.
return Err(Error::<T>::InvalidCalls.into())
rainbow-promise marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

We can also do the refund in case of failure.

Suggested change
return Err(Error::<T>::InvalidCalls.into())
return Err(DispatchErrorWithPostInfo { error: Error::<T>::InvalidCalls.into(), post_info: 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())
}
}

impl<T: Config> Pallet<T> {
Expand Down
40 changes: 40 additions & 0 deletions substrate/frame/utility/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
});
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe 2 more tests:

  • successful main call.
  • failing else call.

17 changes: 17 additions & 0 deletions substrate/frame/utility/src/weights.rs

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading