From 016c20803976c045de9bfce148ffb931f3f66fa5 Mon Sep 17 00:00:00 2001 From: maltekliemann Date: Wed, 20 Apr 2022 17:30:53 +0200 Subject: [PATCH 1/6] Make extrinsics `#[transactional]` --- zrml/court/src/lib.rs | 9 +++------ zrml/liquidity-mining/src/lib.rs | 3 ++- zrml/orderbook-v1/src/lib.rs | 2 ++ zrml/prediction-markets/src/lib.rs | 11 +++++++++++ zrml/swaps/src/lib.rs | 20 ++++++++++++-------- 5 files changed, 30 insertions(+), 15 deletions(-) diff --git a/zrml/court/src/lib.rs b/zrml/court/src/lib.rs index 8756dd286..84803f23a 100644 --- a/zrml/court/src/lib.rs +++ b/zrml/court/src/lib.rs @@ -76,8 +76,7 @@ mod pallet { #[pallet::call] impl Pallet { - // `transactional` attribute is not used simply because - // `remove_juror_from_all_courts_of_all_markets` is infallible. + // MARK(non-transactional): `remove_juror_from_all_courts_of_all_markets` is infallible. #[pallet::weight(T::WeightInfo::exit_court())] pub fn exit_court(origin: OriginFor) -> DispatchResult { let who = ensure_signed(origin)?; @@ -87,8 +86,7 @@ mod pallet { Ok(()) } - // `transactional` attribute is not used here because once `reserve_named` is - // successful, `insert` won't fail. + // MARK(non-transactional): Once `reserve_named` is successful, `insert` won't fail. #[pallet::weight(T::WeightInfo::join_court())] pub fn join_court(origin: OriginFor) -> DispatchResult { let who = ensure_signed(origin)?; @@ -105,8 +103,7 @@ mod pallet { Ok(()) } - // `transactional` attribute is not used here because no fallible storage operation - // is performed. + // MARK(non-transactional): No fallible storage operation is performed. #[pallet::weight(T::WeightInfo::vote())] pub fn vote( origin: OriginFor, diff --git a/zrml/liquidity-mining/src/lib.rs b/zrml/liquidity-mining/src/lib.rs index 84146462e..c53bbec8c 100644 --- a/zrml/liquidity-mining/src/lib.rs +++ b/zrml/liquidity-mining/src/lib.rs @@ -53,7 +53,7 @@ mod pallet { traits::{ Currency, ExistenceRequirement, Get, Hooks, IsType, StorageVersion, WithdrawReasons, }, - Blake2_128Concat, PalletId, Twox64Concat, + transactional, Blake2_128Concat, PalletId, Twox64Concat, }; use frame_system::{ensure_root, pallet_prelude::OriginFor}; use sp_runtime::{ @@ -78,6 +78,7 @@ mod pallet { #[pallet::call] impl Pallet { #[pallet::weight(T::WeightInfo::set_per_block_distribution())] + #[transactional] pub fn set_per_block_distribution( origin: OriginFor, per_block_distribution: BalanceOf, diff --git a/zrml/orderbook-v1/src/lib.rs b/zrml/orderbook-v1/src/lib.rs index 64b3fe9f8..96cf697fb 100644 --- a/zrml/orderbook-v1/src/lib.rs +++ b/zrml/orderbook-v1/src/lib.rs @@ -65,6 +65,7 @@ mod pallet { #[pallet::weight( T::WeightInfo::cancel_order_ask().max(T::WeightInfo::cancel_order_bid()) )] + #[transactional] pub fn cancel_order( origin: OriginFor, asset: Asset, @@ -109,6 +110,7 @@ mod pallet { #[pallet::weight( T::WeightInfo::fill_order_ask().max(T::WeightInfo::fill_order_bid()) )] + #[transactional] pub fn fill_order(origin: OriginFor, order_hash: T::Hash) -> DispatchResultWithPostInfo { let sender = ensure_signed(origin)?; let mut bid = true; diff --git a/zrml/prediction-markets/src/lib.rs b/zrml/prediction-markets/src/lib.rs index bc6b54ca4..eaafaa9e0 100644 --- a/zrml/prediction-markets/src/lib.rs +++ b/zrml/prediction-markets/src/lib.rs @@ -131,6 +131,7 @@ mod pallet { T::MaxCategories::get().into() )) )] + #[transactional] pub fn admin_destroy_market( origin: OriginFor, market_id: MarketIdOf, @@ -191,6 +192,7 @@ mod pallet { // Within the same block, operations that interact with the activeness of the same // market will behave differently before and after this call. #[pallet::weight(T::WeightInfo::admin_move_market_to_closed())] + #[transactional] pub fn admin_move_market_to_closed( origin: OriginFor, market_id: MarketIdOf, @@ -222,6 +224,7 @@ mod pallet { T::MaxCategories::get().into() ).saturating_sub(T::WeightInfo::internal_resolve_scalar_reported()) ))] + #[transactional] pub fn admin_move_market_to_resolved( origin: OriginFor, market_id: MarketIdOf, @@ -248,6 +251,7 @@ mod pallet { /// NOTE: Can only be called by the `ApprovalOrigin`. /// #[pallet::weight(T::WeightInfo::approve_market())] + #[transactional] pub fn approve_market( origin: OriginFor, market_id: MarketIdOf, @@ -355,6 +359,7 @@ mod pallet { /// See also: Polkadot Treasury /// #[pallet::weight(T::WeightInfo::cancel_pending_market())] + #[transactional] pub fn cancel_pending_market( origin: OriginFor, market_id: MarketIdOf, @@ -376,6 +381,7 @@ mod pallet { } #[pallet::weight(T::WeightInfo::create_categorical_market())] + #[transactional] pub fn create_categorical_market( origin: OriginFor, oracle: T::AccountId, @@ -557,6 +563,7 @@ mod pallet { } #[pallet::weight(T::WeightInfo::create_scalar_market())] + #[transactional] pub fn create_scalar_market( origin: OriginFor, oracle: T::AccountId, @@ -778,6 +785,7 @@ mod pallet { /// NOTE: Requires the market to be already disputed `MaxDisputes` amount of times. /// #[pallet::weight(10_000_000)] + #[transactional] pub fn global_dispute(origin: OriginFor, market_id: MarketIdOf) -> DispatchResult { let _sender = ensure_signed(origin)?; let _market = T::MarketCommons::market(&market_id)?; @@ -790,6 +798,7 @@ mod pallet { #[pallet::weight(T::WeightInfo::redeem_shares_categorical() .max(T::WeightInfo::redeem_shares_scalar()) )] + #[transactional] pub fn redeem_shares( origin: OriginFor, market_id: MarketIdOf, @@ -919,6 +928,7 @@ mod pallet { /// Rejects a market that is waiting for approval from the advisory committee. #[pallet::weight(T::WeightInfo::reject_market())] + #[transactional] pub fn reject_market(origin: OriginFor, market_id: MarketIdOf) -> DispatchResult { T::ApprovalOrigin::ensure_origin(origin)?; @@ -1005,6 +1015,7 @@ mod pallet { #[pallet::weight( T::WeightInfo::sell_complete_set(T::MaxCategories::get().into()) )] + #[transactional] pub fn sell_complete_set( origin: OriginFor, market_id: MarketIdOf, diff --git a/zrml/swaps/src/lib.rs b/zrml/swaps/src/lib.rs index 89cfac2ba..1d080053c 100644 --- a/zrml/swaps/src/lib.rs +++ b/zrml/swaps/src/lib.rs @@ -44,7 +44,7 @@ mod pallet { pallet_prelude::{StorageDoubleMap, StorageMap, StorageValue, ValueQuery}, storage::{with_transaction, TransactionOutcome}, traits::{Get, IsType, StorageVersion}, - Blake2_128Concat, PalletId, Twox64Concat, + transactional, Blake2_128Concat, PalletId, Twox64Concat, }; use frame_system::{ensure_root, ensure_signed, pallet_prelude::OriginFor}; use orml_traits::{BalanceStatus, MultiCurrency, MultiReservableCurrency}; @@ -85,7 +85,7 @@ mod pallet { #[pallet::call] impl Pallet { #[pallet::weight(T::WeightInfo::admin_set_pool_as_stale())] - #[frame_support::transactional] + #[transactional] pub fn admin_set_pool_as_stale( origin: OriginFor, market_type: MarketType, @@ -115,7 +115,7 @@ mod pallet { /// * `min_assets_out`: List of asset lower bounds. No asset should be lower than the /// provided values. #[pallet::weight(T::WeightInfo::pool_exit(min_assets_out.len() as u32))] - #[frame_support::transactional] + #[transactional] pub fn pool_exit( origin: OriginFor, pool_id: PoolId, @@ -166,6 +166,7 @@ mod pallet { /// * `pool_id`: Unique pool identifier. /// * `amount`: The amount of base currency that should be removed from subsidy. #[pallet::weight(T::WeightInfo::pool_exit_subsidy())] + #[transactional] pub fn pool_exit_subsidy( origin: OriginFor, pool_id: PoolId, @@ -250,6 +251,7 @@ mod pallet { /// * `max_pool_amount`: The calculated amount of assets for the pool must be equal or /// greater than the given value. #[pallet::weight(T::WeightInfo::pool_exit_with_exact_asset_amount())] + // MARK(non-transactional): Immediately calls and returns a transactional. pub fn pool_exit_with_exact_asset_amount( origin: OriginFor, pool_id: PoolId, @@ -282,7 +284,7 @@ mod pallet { /// * `min_asset_amount`: The calculated amount for the asset must the equal or less /// than the given value. #[pallet::weight(T::WeightInfo::pool_exit_with_exact_pool_amount())] - #[frame_support::transactional] + #[transactional] pub fn pool_exit_with_exact_pool_amount( origin: OriginFor, pool_id: PoolId, @@ -350,7 +352,7 @@ mod pallet { /// * `max_assets_in`: List of asset upper bounds. No asset should be greater than the /// provided values. #[pallet::weight(T::WeightInfo::pool_join(max_assets_in.len() as u32))] - #[frame_support::transactional] + #[transactional] pub fn pool_join( origin: OriginFor, pool_id: PoolId, @@ -394,6 +396,7 @@ mod pallet { /// * `pool_id`: Unique pool identifier. /// * `amount`: The amount of base currency that should be added to subsidy. #[pallet::weight(T::WeightInfo::pool_join_subsidy())] + #[transactional] pub fn pool_join_subsidy( origin: OriginFor, pool_id: PoolId, @@ -445,6 +448,7 @@ mod pallet { /// * `asset_amount`: Asset amount that is entering the pool. /// * `min_pool_amount`: The calculated amount for the pool must be equal or greater /// than the given value. + // MARK(non-transactional): Immediately calls and returns a transactional. #[pallet::weight(T::WeightInfo::pool_join_with_exact_asset_amount())] pub fn pool_join_with_exact_asset_amount( origin: OriginFor, @@ -478,7 +482,7 @@ mod pallet { /// * `max_asset_amount`: The calculated amount of assets for the pool must be equal or /// less than the given value. #[pallet::weight(T::WeightInfo::pool_join_with_exact_pool_amount())] - #[frame_support::transactional] + #[transactional] pub fn pool_join_with_exact_pool_amount( origin: OriginFor, pool_id: PoolId, @@ -542,7 +546,7 @@ mod pallet { /// * `min_asset_amount_out`: Minimum asset amount that can leave the pool. /// * `max_price`: Market price must be equal or less than the provided value. #[pallet::weight(T::WeightInfo::swap_exact_amount_in_rikiddo(T::MaxAssets::get().into()))] - #[frame_support::transactional] + #[transactional] pub fn swap_exact_amount_in( origin: OriginFor, pool_id: PoolId, @@ -579,7 +583,7 @@ mod pallet { /// * `asset_amount_out`: Amount that will be transferred from the pool to the provider. /// * `max_price`: Market price must be equal or less than the provided value. #[pallet::weight(T::WeightInfo::swap_exact_amount_out_rikiddo(T::MaxAssets::get().into()))] - #[frame_support::transactional] + #[transactional] pub fn swap_exact_amount_out( origin: OriginFor, pool_id: PoolId, From cde754a16634702554803a9a313edacd56fdce80 Mon Sep 17 00:00:00 2001 From: maltekliemann Date: Wed, 20 Apr 2022 18:24:18 +0200 Subject: [PATCH 2/6] Add workaround for compiler error --- zrml/prediction-markets/src/lib.rs | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/zrml/prediction-markets/src/lib.rs b/zrml/prediction-markets/src/lib.rs index eaafaa9e0..412deecd0 100644 --- a/zrml/prediction-markets/src/lib.rs +++ b/zrml/prediction-markets/src/lib.rs @@ -402,8 +402,10 @@ mod pallet { Self::ensure_market_start_is_in_time(&period)?; } - // Require sha3-384 as multihash. - let MultiHash::Sha3_384(multihash) = metadata; + // Require sha3-384 as multihash. TODO The irrefutable `if let` is a workaround for a + // compiler error. Link an issue for this! + let multihash = + if let MultiHash::Sha3_384(multihash) = metadata { multihash } else { [0u8; 50] }; ensure!(multihash[0] == 0x15 && multihash[1] == 0x30, >::InvalidMultihash); let status: MarketStatus = match creation { @@ -583,8 +585,10 @@ mod pallet { Self::ensure_market_start_is_in_time(&period)?; } - // Require sha3-384 as multihash. - let MultiHash::Sha3_384(multihash) = metadata; + // Require sha3-384 as multihash. TODO The irrefutable `if let` is a workaround for a + // compiler error. Link an issue for this! + let multihash = + if let MultiHash::Sha3_384(multihash) = metadata { multihash } else { [0u8; 50] }; ensure!(multihash[0] == 0x15 && multihash[1] == 0x30, >::InvalidMultihash); let status: MarketStatus = match creation { From 758b079b0c8ac304d86ec2563efb6c60fef3a625 Mon Sep 17 00:00:00 2001 From: maltekliemann Date: Wed, 20 Apr 2022 20:50:04 +0200 Subject: [PATCH 3/6] Protect workaround from clippy --- zrml/prediction-markets/src/lib.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/zrml/prediction-markets/src/lib.rs b/zrml/prediction-markets/src/lib.rs index 412deecd0..0b3b78122 100644 --- a/zrml/prediction-markets/src/lib.rs +++ b/zrml/prediction-markets/src/lib.rs @@ -404,6 +404,7 @@ mod pallet { // Require sha3-384 as multihash. TODO The irrefutable `if let` is a workaround for a // compiler error. Link an issue for this! + #[allow(irrefutable_let_patterns)] let multihash = if let MultiHash::Sha3_384(multihash) = metadata { multihash } else { [0u8; 50] }; ensure!(multihash[0] == 0x15 && multihash[1] == 0x30, >::InvalidMultihash); @@ -587,6 +588,7 @@ mod pallet { // Require sha3-384 as multihash. TODO The irrefutable `if let` is a workaround for a // compiler error. Link an issue for this! + #[allow(irrefutable_let_patterns)] let multihash = if let MultiHash::Sha3_384(multihash) = metadata { multihash } else { [0u8; 50] }; ensure!(multihash[0] == 0x15 && multihash[1] == 0x30, >::InvalidMultihash); From 58a6364623c9e2bcfeb19dbebaab5ace08349d52 Mon Sep 17 00:00:00 2001 From: maltekliemann Date: Wed, 11 May 2022 12:55:50 +0200 Subject: [PATCH 4/6] Add issue to workaround --- zrml/prediction-markets/src/lib.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/zrml/prediction-markets/src/lib.rs b/zrml/prediction-markets/src/lib.rs index 4e0bd5467..0c7807b51 100644 --- a/zrml/prediction-markets/src/lib.rs +++ b/zrml/prediction-markets/src/lib.rs @@ -372,8 +372,8 @@ mod pallet { Self::ensure_market_start_is_in_time(&period)?; } - // Require sha3-384 as multihash. TODO The irrefutable `if let` is a workaround for a - // compiler error. Link an issue for this! + // Require sha3-384 as multihash. TODO(#608) The irrefutable `if let` is a workaround + // for a compiler error. Link an issue for this! #[allow(irrefutable_let_patterns)] let multihash = if let MultiHash::Sha3_384(multihash) = metadata { multihash } else { [0u8; 50] }; @@ -557,8 +557,8 @@ mod pallet { Self::ensure_market_start_is_in_time(&period)?; } - // Require sha3-384 as multihash. TODO The irrefutable `if let` is a workaround for a - // compiler error. Link an issue for this! + // Require sha3-384 as multihash. TODO(#608) The irrefutable `if let` is a workaround + // for a compiler error. Link an issue for this! #[allow(irrefutable_let_patterns)] let multihash = if let MultiHash::Sha3_384(multihash) = metadata { multihash } else { [0u8; 50] }; From b8cfc84db8d5af4e580227a34f967972905744c9 Mon Sep 17 00:00:00 2001 From: Malte Kliemann Date: Sat, 14 May 2022 12:15:07 +0200 Subject: [PATCH 5/6] Apply suggestions from code review Co-authored-by: Harald Heckmann --- zrml/liquidity-mining/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/zrml/liquidity-mining/src/lib.rs b/zrml/liquidity-mining/src/lib.rs index 783e642eb..835ff1b5d 100644 --- a/zrml/liquidity-mining/src/lib.rs +++ b/zrml/liquidity-mining/src/lib.rs @@ -78,7 +78,7 @@ mod pallet { #[pallet::call] impl Pallet { #[pallet::weight(T::WeightInfo::set_per_block_distribution())] - #[transactional] + // MARK(non-transactional): `set_per_block_distribution` is infallible. pub fn set_per_block_distribution( origin: OriginFor, #[pallet::compact] per_block_distribution: BalanceOf, From 83905a56d0d526edd5a99a32d870bade69fa8e9b Mon Sep 17 00:00:00 2001 From: maltekliemann Date: Sat, 14 May 2022 15:10:58 +0200 Subject: [PATCH 6/6] Remove unnecessary import --- zrml/liquidity-mining/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/zrml/liquidity-mining/src/lib.rs b/zrml/liquidity-mining/src/lib.rs index 835ff1b5d..3ef64d833 100644 --- a/zrml/liquidity-mining/src/lib.rs +++ b/zrml/liquidity-mining/src/lib.rs @@ -53,7 +53,7 @@ mod pallet { traits::{ Currency, ExistenceRequirement, Get, Hooks, IsType, StorageVersion, WithdrawReasons, }, - transactional, Blake2_128Concat, PalletId, Twox64Concat, + Blake2_128Concat, PalletId, Twox64Concat, }; use frame_system::{ensure_root, pallet_prelude::OriginFor}; use sp_runtime::{