-
Notifications
You must be signed in to change notification settings - Fork 696
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
Conversation
User @rainbow-promise, please sign the CLA here. |
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.
I don't think you understand the requirements clearly because you have implemented batch, not if-else.
the fallback is only executed if and only if the main call failed. if main call successes, do not call fallback
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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.
Ok.
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.
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.
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.
Looks pretty good now, thanks!
I just put some comments.
Need the CI to run some checks @ggwpez 🙏 |
Utility Call Fallback
Resolves #6000