Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

added mode attribute tag #11597

Closed
wants to merge 3 commits into from
Closed

Conversation

Szegoo
Copy link
Contributor

@Szegoo Szegoo commented Jun 5, 2022

Still in progress.

I have added a new attribute mode that represents whether an extrinsic is gonna have a storage layer.

When done this should fix #11533

Comment on lines +97 to 105
/// Returns the mode of transactional
pub fn get_transactional_mode() -> bool {
crate::storage::unhashed::get_or::<bool>(TRANSACTIONAL_MODE_KEY, true)
}

/// Set the value of transactional mode
pub fn set_transactional_mode(value: bool) {
crate::storage::unhashed::put::<bool>(TRANSACTIONAL_MODE_KEY, &value);
}
Copy link
Member

Choose a reason for hiding this comment

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

This is not the right thing to do.

The decision of whether we use transactional or not should be stateless, and happen at compile time.

You need to track the existence of without_transactional within the macro, and then simple use it to determine which arm of the code you want to compile

Copy link
Member

Choose a reason for hiding this comment

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

				match self {
					#(
						Self::#fn_name { #( #args_name_pattern, )* } => {
							#frame_support::sp_tracing::enter_span!(
								#frame_support::sp_tracing::trace_span!(stringify!(#fn_name))
							);
							if #without_transactional {
								<#pallet_ident<#type_use_gen>>::#fn_name(origin, #( #args_name, )* )
									.map(Into::into).map_err(Into::into)
							} else {
								// We execute all dispatchable in at least one storage layer, allowing them
								// to return an error at any point, and undoing any storage changes.
								#frame_support::storage::in_storage_layer(|| {
									<#pallet_ident<#type_use_gen>>::#fn_name(origin, #( #args_name, )* )
										.map(Into::into).map_err(Into::into)
								})
							}
						},
					)*

Copy link
Member

Choose a reason for hiding this comment

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

Actually, even this can be done a bit better, by using the procedural macro to generate one of:

<#pallet_ident<#type_use_gen>>::#fn_name(origin, #( #args_name, )* )
	.map(Into::into).map_err(Into::into)

or

// We execute all dispatchable in at least one storage layer, allowing them
// to return an error at any point, and undoing any storage changes.
#frame_support::storage::in_storage_layer(|| {
	<#pallet_ident<#type_use_gen>>::#fn_name(origin, #( #args_name, )* )
		.map(Into::into).map_err(Into::into)
})

And then this code is inserted into the same spot.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@shawntabrizi Where should this be checked? It can't be inside the transactional macro, because it is now the default and you don't have to use it. What part of the code is currently getting executed by default to make use of transactional? Sorry if I didn't understand you right.

Copy link
Contributor

Choose a reason for hiding this comment

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

If you take a look at the previous PR, you'll see that under frame/support/procedural/src/pallet/expand/call.rs, there is a new piece of code that unquestionably puts all extrinsic execution logic in a closure fed to in_storage_layer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@shawntabrizi To make the code you sent work we need access to the parsed definition of the pallet Def for the fn_name, args_name_pattern, pallet_ident and type_use_gen. Should I change the function parameters so it accepts def?

Copy link
Contributor

Choose a reason for hiding this comment

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

@Szegoo It sounds like you're looking at the wrong place. frame/support/procedural/src/pallet/expand/call.rs should contain the code where you can decide whether or not there should be storage layer.

@stale
Copy link

stale bot commented Jul 27, 2022

Hey, is anyone still working on this? Due to the inactivity this issue has been automatically marked as stale. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. label Jul 27, 2022
@Szegoo Szegoo closed this Jul 30, 2022
@Szegoo
Copy link
Contributor Author

Szegoo commented Jul 30, 2022

Closed because of: #11533 (comment)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add #[without_storage_layer] tag which explicitly allows an extrinsic to not have a storage layer
3 participants