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

added mode attribute tag #11597

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
00c3874
added mode attribute tag
Szegoo Jun 5, 2022
d3f6b6c
fmt
Szegoo Jun 5, 2022
814c76d
Merge branch 'paritytech:master' into without-transactional
Szegoo Jun 27, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions frame/support/procedural/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -434,6 +434,12 @@ pub fn transactional(attr: TokenStream, input: TokenStream) -> TokenStream {
transactional::transactional(attr, input).unwrap_or_else(|e| e.to_compile_error().into())
}

#[proc_macro_attribute]
pub fn without_transactional(attr: TokenStream, input: TokenStream) -> TokenStream {
transactional::without_transactional(attr, input)
.unwrap_or_else(|e| e.to_compile_error().into())
}

#[proc_macro_attribute]
pub fn require_transactional(attr: TokenStream, input: TokenStream) -> TokenStream {
transactional::require_transactional(attr, input)
Expand Down
16 changes: 16 additions & 0 deletions frame/support/procedural/src/transactional.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,22 @@ pub fn transactional(_attr: TokenStream, input: TokenStream) -> Result<TokenStre
Ok(output.into())
}

pub fn without_transactional(_attr: TokenStream, input: TokenStream) -> Result<TokenStream> {
let ItemFn { attrs, vis, sig, block } = syn::parse(input)?;

let crate_ = generate_crate_access_2018("frame-support")?;
let output = quote! {
#(#attrs)*
#vis #sig {
use #crate_::storage::set_transactional_mode;
set_transactional_mode();
#block
}
};

Ok(output.into())
}

pub fn require_transactional(_attr: TokenStream, input: TokenStream) -> Result<TokenStream> {
let ItemFn { attrs, vis, sig, block } = syn::parse(input)?;

Expand Down
3 changes: 2 additions & 1 deletion frame/support/src/storage/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,8 @@ use sp_std::{marker::PhantomData, prelude::*};

pub use self::{
transactional::{
in_storage_layer, with_storage_layer, with_transaction, with_transaction_unchecked,
in_storage_layer, set_transactional_mode, with_storage_layer, with_transaction,
with_transaction_unchecked,
},
types::StorageEntryMetadataBuilder,
};
Expand Down
37 changes: 36 additions & 1 deletion frame/support/src/storage/transactional.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,8 @@ pub type Layer = u32;
pub const TRANSACTION_LEVEL_KEY: &[u8] = b":transaction_level:";
/// The maximum number of nested layers.
pub const TRANSACTIONAL_LIMIT: Layer = 255;
/// The key that holds if transactional is being used.
pub const TRANSACTIONAL_MODE_KEY: &[u8] = b":transactional_mode:";

/// Returns the current number of nested transactional layers.
fn get_transaction_level() -> Layer {
Expand Down Expand Up @@ -89,7 +91,17 @@ impl Drop for StorageLayerGuard {

/// Check if the current call is within a transactional layer.
pub fn is_transactional() -> bool {
get_transaction_level() > 0
get_transaction_level() > 0 && get_transactional_mode()
}

/// 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);
}
Comment on lines +97 to 105
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.


/// Execute the supplied function in a new storage transaction.
Expand Down Expand Up @@ -298,4 +310,27 @@ mod tests {
assert_noop!(res, "epic fail");
});
}

#[test]
fn transactional_mode_should_be_true_by_default() {
TestExternalities::default().execute_with(|| {
assert_eq!(get_transactional_mode(), true);
});
}

#[test]
fn set_transactional_mode_should_work() {
TestExternalities::default().execute_with(|| {
set_transactional_mode(false);
assert_eq!(get_transactional_mode(), false);
});
}

#[test]
fn is_transactional_should_be_false_when_mode_false() {
TestExternalities::default().execute_with(|| {
set_transactional_mode(false);
assert_eq!(is_transactional(), false);
});
}
}