-
Notifications
You must be signed in to change notification settings - Fork 695
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
base: master
Are you sure you want to change the base?
pallet-utility: if_else
#6321
Changes from all commits
b83a8e8
1b01dc9
430a791
2caf95e
4183c6b
e70a607
a93b142
cc79306
007d1c3
7f2c600
f2bd937
ad9e68b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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: 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 |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -120,6 +120,12 @@ pub mod pallet { | |
ItemFailed { error: DispatchError }, | ||
/// A call was dispatched. | ||
DispatchedAs { result: DispatchResult }, | ||
/// 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 | ||
|
@@ -159,6 +165,8 @@ pub mod pallet { | |
pub enum Error<T> { | ||
/// Too many calls batched. | ||
TooManyCalls, | ||
/// Multiple call failure. | ||
InvalidCalls, | ||
} | ||
|
||
#[pallet::call] | ||
|
@@ -454,6 +462,98 @@ 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. | ||
/// | ||
/// 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)] | ||
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, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can this be compared? or I need to implement ord There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We don't need to handle |
||
) | ||
})] | ||
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 { | ||
// 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 faild. | ||
Self::deposit_event(Event::IfElseBothFailure { main_error: main_error.error, fallback_error: fallback_error.error }); | ||
return Err(sp_runtime::DispatchErrorWithPostInfo { error: Error::<T>::InvalidCalls.into(), post_info: Some(weight).into() }) | ||
} | ||
// Fallback succeeded. | ||
Self::deposit_event(Event::IfElseFallbackSuccess { 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> { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -914,3 +914,79 @@ 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); | ||
|
||
System::assert_last_event(utility::Event::IfElseFallbackSuccess { main_error: TokenError::FundsUnavailable.into() }.into()); | ||
}); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe 2 more tests:
|
||
|
||
#[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_err_ignore_postinfo!(Utility::if_else( | ||
RuntimeOrigin::signed(1), | ||
Box::new(call_transfer(2, 11)), | ||
Box::new(call_transfer(2, 11)) | ||
), utility::Error::<Test>::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()); | ||
}) | ||
} |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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