pallet-utility: if_else
#6321
pallet-utility: if_else
#6321
Conversation
|
User @rainbow-promise, please sign the CLA here. |
|
Need to take a closer look at the extrinsic executions, even with the revisions it Is not quit there yet. |
substrate/frame/utility/src/lib.rs
Outdated
| @@ -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 }, | |||
There was a problem hiding this comment.
no string in pallets, the current implementation make it hard for sdk users to figure out the meaning of this
it could be an enum
but I don’t think the current event make sense as there are only 3 possibilities: main success, mail failed + fallback success, all failed
the all failed case should just result the whole call to be failed. ie no extra event. so we only really two event: main success, and fallback called
|
looks fine now. need proper weights |
|
I am guessing the weight signature below the pallet index will have some form of condition as well? |
|
use the worst case (i.e. main + fallback). extra weight will be refunded as the actual weight is returned in the call |
this also applies to dispatch class? |
I would say dispatch class is the lowest class of the 2. if both are operational then it is operational. if one is normal then it is normal. etc. |
how can one determine the class of a |
take a look at batch, they do similar operation. You can use |
|
Also the |
| let caller = whitelisted_caller(); | ||
|
|
||
| #[extrinsic_call] | ||
| _(RawOrigin::Signed(caller), main_call, fallback_call); |
There was a problem hiding this comment.
The err branch should be slightly more costly, but difference should be negligible so it is ok.
There was a problem hiding this comment.
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_eq!(Balances::free_balance(1), 5); | ||
| assert_eq!(Balances::free_balance(2), 15); | ||
| }); | ||
| } |
There was a problem hiding this comment.
Maybe 2 more tests:
- successful main call.
- failing else call.
|
Need the CI to run some checks @ggwpez 🙏 |
| // Both calls have faild. | ||
| Self::deposit_event(Event::IfElseBothFailure { main_error: main_error.error, fallback_error: fallback_error.error }); | ||
| return Err(Error::<T>::InvalidCalls.into()) |
There was a problem hiding this comment.
We can also do the refund in case of failure.
| return Err(Error::<T>::InvalidCalls.into()) | |
| return Err(DispatchErrorWithPostInfo { error: Error::<T>::InvalidCalls.into(), post_info: Some(weight).into() }) |
| 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.
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.
Utility Call Fallback
Resolves #6000