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

Conversation

rainbow-promise
Copy link

@rainbow-promise rainbow-promise commented Oct 31, 2024

Utility Call Fallback

This introduces a new extrinsic: if_else

Which first attempts to dispatch the main call(s). If the main call(s) fail, the fallback call(s) is dispatched instead. Both calls are executed with the same origin.

In the event of a fallback failure the whole call fails with the weights returned.

Use Case

Some use cases might involve submitting a batch type call in either main, fallback or both.

Resolves #6000

Polkadot Address: 1HbdqutFR8M535LpbLFT41w3j7v9ptEYGEJKmc6PKpqthZ8

@cla-bot-2021
Copy link

cla-bot-2021 bot commented Oct 31, 2024

User @rainbow-promise, please sign the CLA here.

Copy link
Contributor

@xlc xlc left a 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

substrate/frame/utility/src/lib.rs Outdated Show resolved Hide resolved
@rainbow-promise
Copy link
Author

rainbow-promise commented Nov 1, 2024

Need to take a closer look at the extrinsic executions, even with the revisions it Is not quit there yet.

@rainbow-promise rainbow-promise marked this pull request as ready for review November 6, 2024 16:48
@rainbow-promise rainbow-promise requested a review from a team as a code owner November 6, 2024 16:48
@rainbow-promise
Copy link
Author

please review @xlc @ggwpez

@xlc
Copy link
Contributor

xlc commented Nov 6, 2024

looks fine now. need proper weights

@rainbow-promise
Copy link
Author

I am guessing the weight signature below the pallet index will have some form of condition as well?

@xlc
Copy link
Contributor

xlc commented Nov 7, 2024

use the worst case (i.e. main + fallback). extra weight will be refunded as the actual weight is returned in the call

@rainbow-promise
Copy link
Author

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?

@gui1117
Copy link
Contributor

gui1117 commented Nov 7, 2024

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.

@rainbow-promise
Copy link
Author

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 RuntimeCall? if Operational or Normal?

@gui1117
Copy link
Contributor

gui1117 commented Nov 7, 2024

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 RuntimeCall? if Operational or Normal?

take a look at batch, they do similar operation. You can use get_dispatch_info on the call.

@rainbow-promise
Copy link
Author

Also the kitchensink-runtime has no genesis_config_presets.rs file, using frame-omni-bencher errors out for that runtime because of that.

@rainbow-promise
Copy link
Author

i have added weights to all runtimes containing pallet-utility please review @ggwpez @xlc @gui1117

@rainbow-promise
Copy link
Author

Also the kitchensink-runtime has no genesis_config_presets.rs file, using frame-omni-bencher errors out for that runtime because of that.

Should I raise an issue for this? @xlc @gui1117

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?

substrate/frame/utility/src/lib.rs Outdated Show resolved Hide resolved
substrate/frame/utility/src/lib.rs Outdated Show resolved Hide resolved
substrate/frame/utility/src/tests.rs Show resolved Hide resolved
Copy link
Member

@ggwpez ggwpez left a 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.

prdoc/pr_6321.prdoc Outdated Show resolved Hide resolved
prdoc/pr_6321.prdoc Outdated Show resolved Hide resolved
substrate/frame/utility/src/lib.rs Outdated Show resolved Hide resolved
substrate/frame/utility/src/lib.rs Show resolved Hide resolved
substrate/frame/utility/src/lib.rs Outdated Show resolved Hide resolved
@github-actions github-actions bot requested a review from gui1117 November 22, 2024 10:02
Copy link
Member

@ggwpez ggwpez left a 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 🙏

Just some small comments.

substrate/frame/utility/src/lib.rs Outdated Show resolved Hide resolved
substrate/frame/utility/src/tests.rs Outdated Show resolved Hide resolved
substrate/frame/utility/src/tests.rs Show resolved Hide resolved
substrate/frame/utility/src/tests.rs Show resolved Hide resolved
Copy link
Member

@shawntabrizi shawntabrizi left a comment

Choose a reason for hiding this comment

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

lgtm

@shawntabrizi
Copy link
Member

/tip medium

Copy link

@shawntabrizi A referendum for a medium (80 DOT) tip was successfully submitted for @rainbow-promise (1HbdqutFR8M535LpbLFT41w3j7v9ptEYGEJKmc6PKpqthZ8 on polkadot).

Referendum number: 1327.
tip

Copy link

The referendum has appeared on Polkassembly.

@gui1117
Copy link
Contributor

gui1117 commented Dec 3, 2024

@rainbow-promise did you sign the CLA in the first message #6321 (comment) ?

@paritytech paritytech deleted a comment from cla-bot-2021 bot Dec 3, 2024
@paritytech paritytech deleted a comment from cla-bot-2021 bot Dec 3, 2024
@paritytech paritytech deleted a comment from cla-bot-2021 bot Dec 3, 2024
@rainbow-promise
Copy link
Author

rainbow-promise commented Dec 4, 2024

@rainbow-promise did you sign the CLA in the first message #6321 (comment) ?

@gui1117 I did, any reason why it pops up on new PRs?

@gui1117
Copy link
Contributor

gui1117 commented Dec 4, 2024

@rainbow-promise did you sign the CLA in the first message #6321 (comment) ?

@gui1117 I did, any reason why it pops up on new PRs?

I have no idea, I see the CI test is failing, but it doesn't say much.

@rainbow-promise
Copy link
Author

rainbow-promise commented Dec 4, 2024

@gui1117 Still the same with a different browser.

@gui1117
Copy link
Contributor

gui1117 commented Dec 4, 2024

@gui1117 Still the same with a different browser.

Thanks I will try to see internally

@github-actions github-actions bot requested a review from gui1117 December 4, 2024 02:53
@rainbow-promise
Copy link
Author

My email address was set to private, I thought that could resolve the issue

@gui1117
Copy link
Contributor

gui1117 commented Dec 4, 2024

/cla run-cla-check

@cla-bot-2021
Copy link

cla-bot-2021 bot commented Dec 4, 2024

Queueing command execution: run-cla-check

@cla-bot-2021
Copy link

cla-bot-2021 bot commented Dec 4, 2024

@gui1117 Command execution has finished.

@mutantcornholio
Copy link
Contributor

Hey @rainbow-promise, I've checked cla's history and can't see any signatures in the database, while there're still pending ones. Are you on Matrix? If so, could you please write me at @yuri:parity.io, so we could go through the process together, with me looking at what's happening on back end?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T2-pallets This PR/Issue is related to a particular pallet.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Utility if_else call
6 participants