From ca87290e5590f683de78b30a8afc22fdee616c7b Mon Sep 17 00:00:00 2001 From: Bryan Chen Date: Mon, 5 Oct 2020 17:29:26 +1300 Subject: [PATCH 01/11] Implements require_transactional --- frame/support/procedural/src/lib.rs | 5 ++ frame/support/procedural/src/transactional.rs | 15 ++++ frame/support/src/lib.rs | 2 +- frame/support/src/storage/mod.rs | 69 ++++++++++++++++++- .../support/test/tests/storage_transaction.rs | 34 ++++++++- 5 files changed, 122 insertions(+), 3 deletions(-) diff --git a/frame/support/procedural/src/lib.rs b/frame/support/procedural/src/lib.rs index bf87bd552fd06..701ede60b064b 100644 --- a/frame/support/procedural/src/lib.rs +++ b/frame/support/procedural/src/lib.rs @@ -325,3 +325,8 @@ pub fn construct_runtime(input: TokenStream) -> TokenStream { 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 require_transactional(attr: TokenStream, input: TokenStream) -> TokenStream { + transactional::require_transactional(attr, input).unwrap_or_else(|e| e.to_compile_error().into()) +} diff --git a/frame/support/procedural/src/transactional.rs b/frame/support/procedural/src/transactional.rs index fbd0c9ca0b3c4..688d8cfa067f3 100644 --- a/frame/support/procedural/src/transactional.rs +++ b/frame/support/procedural/src/transactional.rs @@ -41,3 +41,18 @@ pub fn transactional(_attr: TokenStream, input: TokenStream) -> Result Result { + let ItemFn { attrs, vis, sig, block } = syn::parse(input)?; + + let crate_ = generate_crate_access_2018()?; + let output = quote! { + #(#attrs)* + #vis #sig { + #crate_::storage::require_transaction(); + #block + } + }; + + Ok(output.into()) +} diff --git a/frame/support/src/lib.rs b/frame/support/src/lib.rs index bdbdfc04a31f9..9b650c2e2887a 100644 --- a/frame/support/src/lib.rs +++ b/frame/support/src/lib.rs @@ -267,7 +267,7 @@ macro_rules! ord_parameter_types { } #[doc(inline)] -pub use frame_support_procedural::{decl_storage, construct_runtime, transactional}; +pub use frame_support_procedural::{decl_storage, construct_runtime, transactional, require_transactional}; /// Return Err of the expression: `return Err($expression);`. /// diff --git a/frame/support/src/storage/mod.rs b/frame/support/src/storage/mod.rs index 5ee144c79c4db..b85fa16b4ef28 100644 --- a/frame/support/src/storage/mod.rs +++ b/frame/support/src/storage/mod.rs @@ -30,6 +30,41 @@ pub mod child; pub mod generator; pub mod migration; +#[cfg(not(build_type = "release"))] +mod debug_helper { + use sp_std::cell::RefCell; + + thread_local! { + static TRANSACTION_LEVEL: RefCell = RefCell::new(0); + } + + pub fn require_transaction() { + let level = TRANSACTION_LEVEL.with(|v| *v.borrow()); + if level == 0 { + panic!("Require transaction not called within with_transaction"); + } + } + + pub fn inc_transaction_level() { + TRANSACTION_LEVEL.with(|v| { + let mut val = v.borrow_mut(); + *val += 1; + if *val > 10 { + crate::debug::warn!("Detected with_transaction with nest level {}. Nested usage of with_transaction is not recommended.", *val); + } + }); + } + + pub fn dec_transaction_level() { + TRANSACTION_LEVEL.with(|v| *v.borrow_mut() -= 1); + } +} + +pub fn require_transaction() { + #[cfg(not(build_type = "release"))] + debug_helper::require_transaction(); +} + /// Execute the supplied function in a new storage transaction. /// /// All changes to storage performed by the supplied function are discarded if the returned @@ -43,7 +78,16 @@ pub fn with_transaction(f: impl FnOnce() -> TransactionOutcome) -> R { use TransactionOutcome::*; start_transaction(); - match f() { + + #[cfg(not(build_type = "release"))] + debug_helper::inc_transaction_level(); + + let res = f(); + + #[cfg(not(build_type = "release"))] + debug_helper::dec_transaction_level(); + + match res { Commit(res) => { commit_transaction(); res }, Rollback(res) => { rollback_transaction(); res }, } @@ -732,4 +776,27 @@ mod test { assert_eq!(Digest::decode(&mut &value[..]).unwrap(), expected); }); } + + #[test] + #[should_panic(expected = "Require transaction not called within with_transaction")] + fn require_transaction_should_panic() { + TestExternalities::default().execute_with(|| { + require_transaction(); + }); + } + + #[test] + fn require_transaction_should_not_panic_in_with_transaction() { + TestExternalities::default().execute_with(|| { + with_transaction(|| { + require_transaction(); + TransactionOutcome::Commit(()) + }); + + with_transaction(|| { + require_transaction(); + TransactionOutcome::Rollback(()) + }); + }); + } } diff --git a/frame/support/test/tests/storage_transaction.rs b/frame/support/test/tests/storage_transaction.rs index a7e4a75c27fcb..72ad909d1c2c6 100644 --- a/frame/support/test/tests/storage_transaction.rs +++ b/frame/support/test/tests/storage_transaction.rs @@ -17,7 +17,8 @@ use codec::{Encode, Decode, EncodeLike}; use frame_support::{ - assert_ok, assert_noop, dispatch::{DispatchError, DispatchResult}, transactional, StorageMap, StorageValue, + assert_ok, assert_noop, dispatch::{DispatchError, DispatchResult}, transactional, require_transactional, + StorageMap, StorageValue, storage::{with_transaction, TransactionOutcome::*}, }; use sp_io::TestExternalities; @@ -52,6 +53,22 @@ frame_support::decl_storage!{ } } +impl Module { + #[require_transactional] + fn mutate() { + } + + #[transactional] + fn safe() -> DispatchResult { + Self::mutate(); + Ok(()) + } + + fn not_safe() { + Self::mutate(); + } +} + struct Runtime; impl Trait for Runtime { type Origin = u32; @@ -210,3 +227,18 @@ fn transactional_annotation_in_decl_module() { assert_noop!(>::value_rollbacks(origin, 3), "nah"); }); } + +#[test] +#[should_panic(expected = "Require transaction not called within with_transaction")] +fn require_transactional_panic() { + TestExternalities::default().execute_with(|| { + >::not_safe(); + }); +} + +#[test] +fn require_transactional_not_panic() { + TestExternalities::default().execute_with(|| { + >::safe().unwrap(); + }); +} From 3aa19de1626369b2624aea166339e563416cbe7b Mon Sep 17 00:00:00 2001 From: Bryan Chen Date: Mon, 5 Oct 2020 17:42:23 +1300 Subject: [PATCH 02/11] support wasm --- frame/support/src/storage/mod.rs | 72 +++++++++++++++++++++++++------- 1 file changed, 57 insertions(+), 15 deletions(-) diff --git a/frame/support/src/storage/mod.rs b/frame/support/src/storage/mod.rs index b85fa16b4ef28..cd9679fb1eca1 100644 --- a/frame/support/src/storage/mod.rs +++ b/frame/support/src/storage/mod.rs @@ -32,32 +32,74 @@ pub mod migration; #[cfg(not(build_type = "release"))] mod debug_helper { - use sp_std::cell::RefCell; + #[cfg(feature = "std")] + mod with_std { + use sp_std::cell::RefCell; - thread_local! { - static TRANSACTION_LEVEL: RefCell = RefCell::new(0); - } + thread_local! { + static TRANSACTION_LEVEL: RefCell = RefCell::new(0); + } - pub fn require_transaction() { - let level = TRANSACTION_LEVEL.with(|v| *v.borrow()); - if level == 0 { - panic!("Require transaction not called within with_transaction"); + pub fn require_transaction() { + let level = TRANSACTION_LEVEL.with(|v| *v.borrow()); + if level == 0 { + panic!("Require transaction not called within with_transaction"); + } + } + + pub fn inc_transaction_level() { + TRANSACTION_LEVEL.with(|v| { + let mut val = v.borrow_mut(); + *val += 1; + if *val > 10 { + crate::debug::warn!("Detected with_transaction with nest level {}. Nested usage of with_transaction is not recommended.", *val); + } + }); + } + + pub fn dec_transaction_level() { + TRANSACTION_LEVEL.with(|v| *v.borrow_mut() -= 1); } } - pub fn inc_transaction_level() { - TRANSACTION_LEVEL.with(|v| { - let mut val = v.borrow_mut(); + #[cfg(feature = "std")] + pub use with_std::*; + + #[cfg(not(feature = "std"))] + mod without_std { + use sp_std::cell::RefCell; + + struct TransactionLevel(RefCell); + + // NOTE: Safe only in wasm (guarded above) because there's only one thread. + unsafe impl Send for TransactionLevel {} + unsafe impl Sync for TransactionLevel {} + + static TRANSACTION_LEVEL: TransactionLevel = TransactionLevel(RefCell::new(0)); + + pub fn require_transaction() { + let level = TRANSACTION_LEVEL.0.borrow(); + if *level == 0 { + panic!("Require transaction not called within with_transaction"); + } + } + + pub fn inc_transaction_level() { + let mut val = TRANSACTION_LEVEL.0.borrow_mut(); *val += 1; if *val > 10 { crate::debug::warn!("Detected with_transaction with nest level {}. Nested usage of with_transaction is not recommended.", *val); } - }); - } + } - pub fn dec_transaction_level() { - TRANSACTION_LEVEL.with(|v| *v.borrow_mut() -= 1); + pub fn dec_transaction_level() { + let mut val = TRANSACTION_LEVEL.0.borrow_mut(); + *val -= 1; + } } + + #[cfg(not(feature = "std"))] + pub use without_std::*; } pub fn require_transaction() { From 7dd0f105b09271d0bf8e25cf1f54dae2432124a2 Mon Sep 17 00:00:00 2001 From: Bryan Chen Date: Mon, 5 Oct 2020 21:48:49 +1300 Subject: [PATCH 03/11] only enable for debug build --- frame/support/src/storage/mod.rs | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/frame/support/src/storage/mod.rs b/frame/support/src/storage/mod.rs index cd9679fb1eca1..e765383de0227 100644 --- a/frame/support/src/storage/mod.rs +++ b/frame/support/src/storage/mod.rs @@ -30,7 +30,7 @@ pub mod child; pub mod generator; pub mod migration; -#[cfg(not(build_type = "release"))] +#[cfg(debug_assertions)] mod debug_helper { #[cfg(feature = "std")] mod with_std { @@ -88,7 +88,10 @@ mod debug_helper { let mut val = TRANSACTION_LEVEL.0.borrow_mut(); *val += 1; if *val > 10 { - crate::debug::warn!("Detected with_transaction with nest level {}. Nested usage of with_transaction is not recommended.", *val); + crate::debug::warn!( + "Detected with_transaction with nest level {}. Nested usage of with_transaction is not recommended.", + *val + ); } } @@ -103,7 +106,7 @@ mod debug_helper { } pub fn require_transaction() { - #[cfg(not(build_type = "release"))] + #[cfg(debug_assertions)] debug_helper::require_transaction(); } @@ -121,12 +124,12 @@ pub fn with_transaction(f: impl FnOnce() -> TransactionOutcome) -> R { start_transaction(); - #[cfg(not(build_type = "release"))] + #[cfg(debug_assertions)] debug_helper::inc_transaction_level(); let res = f(); - #[cfg(not(build_type = "release"))] + #[cfg(debug_assertions)] debug_helper::dec_transaction_level(); match res { From 1f4fadaabb74bf76067710dd1a94ebe6ea377652 Mon Sep 17 00:00:00 2001 From: Bryan Chen Date: Tue, 6 Oct 2020 11:43:55 +1300 Subject: [PATCH 04/11] remove wasm support and add feature flag --- frame/support/Cargo.toml | 1 + frame/support/procedural/src/lib.rs | 27 ++++++ frame/support/src/lib.rs | 4 +- frame/support/src/storage/mod.rs | 85 ++++++------------- .../support/test/tests/storage_transaction.rs | 3 +- 5 files changed, 57 insertions(+), 63 deletions(-) diff --git a/frame/support/Cargo.toml b/frame/support/Cargo.toml index d082b718262b8..56ec7d11f934e 100644 --- a/frame/support/Cargo.toml +++ b/frame/support/Cargo.toml @@ -56,3 +56,4 @@ std = [ nightly = [] strict = [] runtime-benchmarks = [] +assert-require-transaction = [] diff --git a/frame/support/procedural/src/lib.rs b/frame/support/procedural/src/lib.rs index 701ede60b064b..6bb6163700214 100644 --- a/frame/support/procedural/src/lib.rs +++ b/frame/support/procedural/src/lib.rs @@ -326,6 +326,33 @@ pub fn transactional(attr: TokenStream, input: TokenStream) -> TokenStream { transactional::transactional(attr, input).unwrap_or_else(|e| e.to_compile_error().into()) } +/// Assert the annotated function is executed within a storage transaction. +/// +/// The assertion is enabled for debug build native execution only. +/// Use feature `assert-require-transaction` to enable it for release build native execution. +/// +/// # Example +/// +/// ```nocompile +/// +/// #[require_transactional] +/// fn update_all(acc1: T::AccountId, acc2: T::AccountId, v: u32) -> DispatchResult { +/// Value::insert(acc, v); +/// Value::insert(acc2, v); +/// } +/// +/// #[transactional] +/// fn safe_update(acc1: T::AccountId, acc2: T::AccountId, v: u32) -> DispatchResult { +/// // This is safe +/// Self::update_all(acc1, acc2, v)? +/// } +/// +/// fn unsafe_update(acc1: T::AccountId, acc2: T::AccountId, v: u32) -> DispatchResult { +/// // this may panic if unsafe_update is not called within a storage transaction +/// // in DEBUG native execution +/// Self::update_all(acc1, acc2, v)? +/// } +/// ``` #[proc_macro_attribute] pub fn require_transactional(attr: TokenStream, input: TokenStream) -> TokenStream { transactional::require_transactional(attr, input).unwrap_or_else(|e| e.to_compile_error().into()) diff --git a/frame/support/src/lib.rs b/frame/support/src/lib.rs index 9b650c2e2887a..ee5aef0362a6b 100644 --- a/frame/support/src/lib.rs +++ b/frame/support/src/lib.rs @@ -267,7 +267,9 @@ macro_rules! ord_parameter_types { } #[doc(inline)] -pub use frame_support_procedural::{decl_storage, construct_runtime, transactional, require_transactional}; +pub use frame_support_procedural::{ + decl_storage, construct_runtime, transactional, require_transactional +}; /// Return Err of the expression: `return Err($expression);`. /// diff --git a/frame/support/src/storage/mod.rs b/frame/support/src/storage/mod.rs index e765383de0227..b448bae725d71 100644 --- a/frame/support/src/storage/mod.rs +++ b/frame/support/src/storage/mod.rs @@ -30,62 +30,24 @@ pub mod child; pub mod generator; pub mod migration; -#[cfg(debug_assertions)] +#[cfg(all(feature = "std", any(debug_assertions, feature = "assert-require-transaction")))] mod debug_helper { - #[cfg(feature = "std")] - mod with_std { - use sp_std::cell::RefCell; + use std::cell::RefCell; - thread_local! { - static TRANSACTION_LEVEL: RefCell = RefCell::new(0); - } - - pub fn require_transaction() { - let level = TRANSACTION_LEVEL.with(|v| *v.borrow()); - if level == 0 { - panic!("Require transaction not called within with_transaction"); - } - } - - pub fn inc_transaction_level() { - TRANSACTION_LEVEL.with(|v| { - let mut val = v.borrow_mut(); - *val += 1; - if *val > 10 { - crate::debug::warn!("Detected with_transaction with nest level {}. Nested usage of with_transaction is not recommended.", *val); - } - }); - } - - pub fn dec_transaction_level() { - TRANSACTION_LEVEL.with(|v| *v.borrow_mut() -= 1); - } + thread_local! { + static TRANSACTION_LEVEL: RefCell = RefCell::new(0); } - #[cfg(feature = "std")] - pub use with_std::*; - - #[cfg(not(feature = "std"))] - mod without_std { - use sp_std::cell::RefCell; - - struct TransactionLevel(RefCell); - - // NOTE: Safe only in wasm (guarded above) because there's only one thread. - unsafe impl Send for TransactionLevel {} - unsafe impl Sync for TransactionLevel {} - - static TRANSACTION_LEVEL: TransactionLevel = TransactionLevel(RefCell::new(0)); - - pub fn require_transaction() { - let level = TRANSACTION_LEVEL.0.borrow(); - if *level == 0 { - panic!("Require transaction not called within with_transaction"); - } + pub fn require_transaction() { + let level = TRANSACTION_LEVEL.with(|v| *v.borrow()); + if level == 0 { + panic!("Require transaction not called within with_transaction"); } + } - pub fn inc_transaction_level() { - let mut val = TRANSACTION_LEVEL.0.borrow_mut(); + pub fn inc_transaction_level() { + TRANSACTION_LEVEL.with(|v| { + let mut val = v.borrow_mut(); *val += 1; if *val > 10 { crate::debug::warn!( @@ -93,20 +55,21 @@ mod debug_helper { *val ); } - } - - pub fn dec_transaction_level() { - let mut val = TRANSACTION_LEVEL.0.borrow_mut(); - *val -= 1; - } + }); } - #[cfg(not(feature = "std"))] - pub use without_std::*; + pub fn dec_transaction_level() { + TRANSACTION_LEVEL.with(|v| *v.borrow_mut() -= 1); + } } +/// Assert this method is called within a storage transaction. +/// This will **panic** if is not called within a storage transaction. +/// +/// This assertion is enabled for native execution and DEBUG build only. +/// Use feature `assert-require-transaction` to enable it for RELEASE builds. pub fn require_transaction() { - #[cfg(debug_assertions)] + #[cfg(all(feature = "std", any(debug_assertions, feature = "assert-require-transaction")))] debug_helper::require_transaction(); } @@ -124,12 +87,12 @@ pub fn with_transaction(f: impl FnOnce() -> TransactionOutcome) -> R { start_transaction(); - #[cfg(debug_assertions)] + #[cfg(all(feature = "std", any(debug_assertions, feature = "assert-require-transaction")))] debug_helper::inc_transaction_level(); let res = f(); - #[cfg(debug_assertions)] + #[cfg(all(feature = "std", any(debug_assertions, feature = "assert-require-transaction")))] debug_helper::dec_transaction_level(); match res { diff --git a/frame/support/test/tests/storage_transaction.rs b/frame/support/test/tests/storage_transaction.rs index 72ad909d1c2c6..2d3747366bf66 100644 --- a/frame/support/test/tests/storage_transaction.rs +++ b/frame/support/test/tests/storage_transaction.rs @@ -17,8 +17,9 @@ use codec::{Encode, Decode, EncodeLike}; use frame_support::{ - assert_ok, assert_noop, dispatch::{DispatchError, DispatchResult}, transactional, require_transactional, + assert_ok, assert_noop, transactional, require_transactional, StorageMap, StorageValue, + dispatch::{DispatchError, DispatchResult}, storage::{with_transaction, TransactionOutcome::*}, }; use sp_io::TestExternalities; From ac0064c95736363981126dac70872e5da4691f36 Mon Sep 17 00:00:00 2001 From: Xiliang Chen Date: Wed, 7 Oct 2020 10:49:46 +1300 Subject: [PATCH 05/11] Apply suggestions from code review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Bastian Köcher --- frame/support/procedural/src/lib.rs | 3 +-- frame/support/src/storage/mod.rs | 2 +- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/frame/support/procedural/src/lib.rs b/frame/support/procedural/src/lib.rs index 6bb6163700214..817a00c48674e 100644 --- a/frame/support/procedural/src/lib.rs +++ b/frame/support/procedural/src/lib.rs @@ -328,8 +328,7 @@ pub fn transactional(attr: TokenStream, input: TokenStream) -> TokenStream { /// Assert the annotated function is executed within a storage transaction. /// -/// The assertion is enabled for debug build native execution only. -/// Use feature `assert-require-transaction` to enable it for release build native execution. +/// The assertion is enabled for native execution and when `debug_assertions` are enabled. /// /// # Example /// diff --git a/frame/support/src/storage/mod.rs b/frame/support/src/storage/mod.rs index b448bae725d71..0588cbbdf1a63 100644 --- a/frame/support/src/storage/mod.rs +++ b/frame/support/src/storage/mod.rs @@ -66,7 +66,7 @@ mod debug_helper { /// Assert this method is called within a storage transaction. /// This will **panic** if is not called within a storage transaction. /// -/// This assertion is enabled for native execution and DEBUG build only. +/// This assertion is enabled for native execution and when `debug_assertions` are enabled. /// Use feature `assert-require-transaction` to enable it for RELEASE builds. pub fn require_transaction() { #[cfg(all(feature = "std", any(debug_assertions, feature = "assert-require-transaction")))] From 7b206944217fa61acff88369570e4b7b4400722d Mon Sep 17 00:00:00 2001 From: Bryan Chen Date: Wed, 7 Oct 2020 10:54:02 +1300 Subject: [PATCH 06/11] only use check for debug_assertions --- frame/support/Cargo.toml | 1 - frame/support/src/storage/mod.rs | 9 ++++----- 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/frame/support/Cargo.toml b/frame/support/Cargo.toml index 56ec7d11f934e..d082b718262b8 100644 --- a/frame/support/Cargo.toml +++ b/frame/support/Cargo.toml @@ -56,4 +56,3 @@ std = [ nightly = [] strict = [] runtime-benchmarks = [] -assert-require-transaction = [] diff --git a/frame/support/src/storage/mod.rs b/frame/support/src/storage/mod.rs index 0588cbbdf1a63..fa7514b691d67 100644 --- a/frame/support/src/storage/mod.rs +++ b/frame/support/src/storage/mod.rs @@ -30,7 +30,7 @@ pub mod child; pub mod generator; pub mod migration; -#[cfg(all(feature = "std", any(debug_assertions, feature = "assert-require-transaction")))] +#[cfg(all(feature = "std", debug_assertions))] mod debug_helper { use std::cell::RefCell; @@ -67,9 +67,8 @@ mod debug_helper { /// This will **panic** if is not called within a storage transaction. /// /// This assertion is enabled for native execution and when `debug_assertions` are enabled. -/// Use feature `assert-require-transaction` to enable it for RELEASE builds. pub fn require_transaction() { - #[cfg(all(feature = "std", any(debug_assertions, feature = "assert-require-transaction")))] + #[cfg(all(feature = "std", debug_assertions))] debug_helper::require_transaction(); } @@ -87,12 +86,12 @@ pub fn with_transaction(f: impl FnOnce() -> TransactionOutcome) -> R { start_transaction(); - #[cfg(all(feature = "std", any(debug_assertions, feature = "assert-require-transaction")))] + #[cfg(all(feature = "std", debug_assertions))] debug_helper::inc_transaction_level(); let res = f(); - #[cfg(all(feature = "std", any(debug_assertions, feature = "assert-require-transaction")))] + #[cfg(all(feature = "std", debug_assertions))] debug_helper::dec_transaction_level(); match res { From 0b7c6b05757a302fd1a4e8ed6b57ae4c91f345a4 Mon Sep 17 00:00:00 2001 From: Bryan Chen Date: Sun, 11 Oct 2020 21:39:48 +1300 Subject: [PATCH 07/11] update per review --- frame/support/src/storage/mod.rs | 29 ++++++++++--------- .../support/test/tests/storage_transaction.rs | 3 ++ 2 files changed, 18 insertions(+), 14 deletions(-) diff --git a/frame/support/src/storage/mod.rs b/frame/support/src/storage/mod.rs index fa7514b691d67..96ae148bee46b 100644 --- a/frame/support/src/storage/mod.rs +++ b/frame/support/src/storage/mod.rs @@ -30,7 +30,7 @@ pub mod child; pub mod generator; pub mod migration; -#[cfg(all(feature = "std", debug_assertions))] +#[cfg(all(feature = "std", any(test, debug_assertions)))] mod debug_helper { use std::cell::RefCell; @@ -45,7 +45,15 @@ mod debug_helper { } } - pub fn inc_transaction_level() { + pub struct TransactionLevelGuard; + + impl Drop for TransactionLevelGuard { + fn drop(&mut self) { + TRANSACTION_LEVEL.with(|v| *v.borrow_mut() -= 1); + } + } + + pub fn inc_transaction_level() -> TransactionLevelGuard { TRANSACTION_LEVEL.with(|v| { let mut val = v.borrow_mut(); *val += 1; @@ -56,10 +64,8 @@ mod debug_helper { ); } }); - } - pub fn dec_transaction_level() { - TRANSACTION_LEVEL.with(|v| *v.borrow_mut() -= 1); + TransactionLevelGuard } } @@ -68,7 +74,7 @@ mod debug_helper { /// /// This assertion is enabled for native execution and when `debug_assertions` are enabled. pub fn require_transaction() { - #[cfg(all(feature = "std", debug_assertions))] + #[cfg(all(feature = "std", any(test, debug_assertions)))] debug_helper::require_transaction(); } @@ -86,15 +92,10 @@ pub fn with_transaction(f: impl FnOnce() -> TransactionOutcome) -> R { start_transaction(); - #[cfg(all(feature = "std", debug_assertions))] - debug_helper::inc_transaction_level(); - - let res = f(); - - #[cfg(all(feature = "std", debug_assertions))] - debug_helper::dec_transaction_level(); + #[cfg(all(feature = "std", any(test, debug_assertions)))] + let _guard = debug_helper::inc_transaction_level(); - match res { + match f() { Commit(res) => { commit_transaction(); res }, Rollback(res) => { rollback_transaction(); res }, } diff --git a/frame/support/test/tests/storage_transaction.rs b/frame/support/test/tests/storage_transaction.rs index 2d3747366bf66..d5e1265e911ae 100644 --- a/frame/support/test/tests/storage_transaction.rs +++ b/frame/support/test/tests/storage_transaction.rs @@ -65,6 +65,8 @@ impl Module { Ok(()) } + // only used when debug_assertions is enabled + #[allow(dead_code)] fn not_safe() { Self::mutate(); } @@ -229,6 +231,7 @@ fn transactional_annotation_in_decl_module() { }); } +#[cfg(debug_assertions)] #[test] #[should_panic(expected = "Require transaction not called within with_transaction")] fn require_transactional_panic() { From 115434bc3b1f95a4b93dec13b391e9581970d90e Mon Sep 17 00:00:00 2001 From: Bryan Chen Date: Sun, 11 Oct 2020 21:57:25 +1300 Subject: [PATCH 08/11] update docs --- frame/support/procedural/src/lib.rs | 26 ----------------------- frame/support/src/lib.rs | 33 ++++++++++++++++++++++++++++- 2 files changed, 32 insertions(+), 27 deletions(-) diff --git a/frame/support/procedural/src/lib.rs b/frame/support/procedural/src/lib.rs index 817a00c48674e..701ede60b064b 100644 --- a/frame/support/procedural/src/lib.rs +++ b/frame/support/procedural/src/lib.rs @@ -326,32 +326,6 @@ pub fn transactional(attr: TokenStream, input: TokenStream) -> TokenStream { transactional::transactional(attr, input).unwrap_or_else(|e| e.to_compile_error().into()) } -/// Assert the annotated function is executed within a storage transaction. -/// -/// The assertion is enabled for native execution and when `debug_assertions` are enabled. -/// -/// # Example -/// -/// ```nocompile -/// -/// #[require_transactional] -/// fn update_all(acc1: T::AccountId, acc2: T::AccountId, v: u32) -> DispatchResult { -/// Value::insert(acc, v); -/// Value::insert(acc2, v); -/// } -/// -/// #[transactional] -/// fn safe_update(acc1: T::AccountId, acc2: T::AccountId, v: u32) -> DispatchResult { -/// // This is safe -/// Self::update_all(acc1, acc2, v)? -/// } -/// -/// fn unsafe_update(acc1: T::AccountId, acc2: T::AccountId, v: u32) -> DispatchResult { -/// // this may panic if unsafe_update is not called within a storage transaction -/// // in DEBUG native execution -/// Self::update_all(acc1, acc2, v)? -/// } -/// ``` #[proc_macro_attribute] pub fn require_transactional(attr: TokenStream, input: TokenStream) -> TokenStream { transactional::require_transactional(attr, input).unwrap_or_else(|e| e.to_compile_error().into()) diff --git a/frame/support/src/lib.rs b/frame/support/src/lib.rs index ee5aef0362a6b..fcb6cf0f7b00c 100644 --- a/frame/support/src/lib.rs +++ b/frame/support/src/lib.rs @@ -268,9 +268,40 @@ macro_rules! ord_parameter_types { #[doc(inline)] pub use frame_support_procedural::{ - decl_storage, construct_runtime, transactional, require_transactional + decl_storage, construct_runtime, transactional }; +/// Assert the annotated function is executed within a storage transaction. +/// +/// The assertion is enabled for native execution and when `debug_assertions` are enabled. +/// +/// # Example +/// +/// ``` +/// use frame_support::{ +/// require_transactional, transactional, dispatch::DispatchResult +/// }; +/// +/// #[require_transactional] +/// fn update_all(value: u32) -> DispatchResult { +/// // Update multiple storages. +/// // Return `Err` to indicate should revert. +/// Ok(()) +/// } +/// +/// #[transactional] +/// fn safe_update(value: u32) -> DispatchResult { +/// // This is safe +/// update_all(value) +/// } +/// +/// fn unsafe_update(value: u32) -> DispatchResult { +/// // this may panic if unsafe_update is not called within a storage transaction +/// update_all(value) +/// } +/// ``` +pub use frame_support_procedural::require_transactional; + /// Return Err of the expression: `return Err($expression);`. /// /// Used as `fail!(expression)`. From 9fc1560a218d19609c9554ee030a2b981ce1d812 Mon Sep 17 00:00:00 2001 From: Xiliang Chen Date: Mon, 12 Oct 2020 11:25:36 +1300 Subject: [PATCH 09/11] Apply suggestions from code review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Bastian Köcher --- frame/support/src/lib.rs | 6 +++--- frame/support/src/storage/mod.rs | 3 +++ 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/frame/support/src/lib.rs b/frame/support/src/lib.rs index fcb6cf0f7b00c..889ff15066ba5 100644 --- a/frame/support/src/lib.rs +++ b/frame/support/src/lib.rs @@ -278,9 +278,9 @@ pub use frame_support_procedural::{ /// # Example /// /// ``` -/// use frame_support::{ -/// require_transactional, transactional, dispatch::DispatchResult -/// }; +/// # use frame_support::{ +/// # require_transactional, transactional, dispatch::DispatchResult +/// # }; /// /// #[require_transactional] /// fn update_all(value: u32) -> DispatchResult { diff --git a/frame/support/src/storage/mod.rs b/frame/support/src/storage/mod.rs index 96ae148bee46b..97c1eabe6d39d 100644 --- a/frame/support/src/storage/mod.rs +++ b/frame/support/src/storage/mod.rs @@ -53,6 +53,9 @@ mod debug_helper { } } + /// Increments the transaction level. + /// + /// Returns a guard that when dropped decrements the transaction level automatically. pub fn inc_transaction_level() -> TransactionLevelGuard { TRANSACTION_LEVEL.with(|v| { let mut val = v.borrow_mut(); From 5c926a4475d0d20e3cf74d93f921ac8a0a5b70f2 Mon Sep 17 00:00:00 2001 From: Bryan Chen Date: Mon, 12 Oct 2020 11:26:40 +1300 Subject: [PATCH 10/11] remove duplicated tests --- .../support/test/tests/storage_transaction.rs | 34 ------------------- 1 file changed, 34 deletions(-) diff --git a/frame/support/test/tests/storage_transaction.rs b/frame/support/test/tests/storage_transaction.rs index d5e1265e911ae..2f6e251c3e99a 100644 --- a/frame/support/test/tests/storage_transaction.rs +++ b/frame/support/test/tests/storage_transaction.rs @@ -54,24 +54,6 @@ frame_support::decl_storage!{ } } -impl Module { - #[require_transactional] - fn mutate() { - } - - #[transactional] - fn safe() -> DispatchResult { - Self::mutate(); - Ok(()) - } - - // only used when debug_assertions is enabled - #[allow(dead_code)] - fn not_safe() { - Self::mutate(); - } -} - struct Runtime; impl Trait for Runtime { type Origin = u32; @@ -230,19 +212,3 @@ fn transactional_annotation_in_decl_module() { assert_noop!(>::value_rollbacks(origin, 3), "nah"); }); } - -#[cfg(debug_assertions)] -#[test] -#[should_panic(expected = "Require transaction not called within with_transaction")] -fn require_transactional_panic() { - TestExternalities::default().execute_with(|| { - >::not_safe(); - }); -} - -#[test] -fn require_transactional_not_panic() { - TestExternalities::default().execute_with(|| { - >::safe().unwrap(); - }); -} From 770ed56980d2aeadb3f7e4d9fbb1c4b9cd2042ab Mon Sep 17 00:00:00 2001 From: Bryan Chen Date: Mon, 12 Oct 2020 18:09:40 +1300 Subject: [PATCH 11/11] fix test --- frame/support/test/tests/storage_transaction.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/support/test/tests/storage_transaction.rs b/frame/support/test/tests/storage_transaction.rs index 2f6e251c3e99a..e305940ee68e2 100644 --- a/frame/support/test/tests/storage_transaction.rs +++ b/frame/support/test/tests/storage_transaction.rs @@ -17,7 +17,7 @@ use codec::{Encode, Decode, EncodeLike}; use frame_support::{ - assert_ok, assert_noop, transactional, require_transactional, + assert_ok, assert_noop, transactional, StorageMap, StorageValue, dispatch::{DispatchError, DispatchResult}, storage::{with_transaction, TransactionOutcome::*},