From b7d3e0827fc958f84936351317299fa2cdfe1427 Mon Sep 17 00:00:00 2001 From: Chralt98 Date: Thu, 28 Sep 2023 14:56:25 +0200 Subject: [PATCH] apply review comments --- zrml/orderbook-v1/src/benchmarks.rs | 28 ++++++++--------- zrml/orderbook-v1/src/lib.rs | 43 +++++++++---------------- zrml/orderbook-v1/src/tests.rs | 49 +++++++++++------------------ zrml/orderbook-v1/src/types.rs | 1 - 4 files changed, 47 insertions(+), 74 deletions(-) diff --git a/zrml/orderbook-v1/src/benchmarks.rs b/zrml/orderbook-v1/src/benchmarks.rs index 35af0770f..8f51b5d23 100644 --- a/zrml/orderbook-v1/src/benchmarks.rs +++ b/zrml/orderbook-v1/src/benchmarks.rs @@ -24,7 +24,9 @@ #![allow(clippy::type_complexity)] use super::*; -use crate::{utils::market_mock, Pallet as OrderBook}; +use crate::utils::market_mock; +#[cfg(test)] +use crate::Pallet as OrderBook; use frame_benchmarking::{account, benchmarks, whitelisted_caller}; use frame_support::dispatch::UnfilteredDispatchable; use frame_system::RawOrigin; @@ -63,11 +65,11 @@ fn order_common_parameters( // Creates an order of type `order_type`. `seed` specifies the account seed, // None will return a whitelisted account -// Returns `account`, `asset` and `order_hash` +// Returns `account`, `asset`, `order_id` fn place_order( order_type: OrderSide, seed: Option, -) -> Result<(T::AccountId, MarketIdOf, T::Hash), &'static str> { +) -> Result<(T::AccountId, MarketIdOf, OrderId), &'static str> { let (acc, outcome_asset, outcome_asset_amount, base_asset_amount, market_id) = order_common_parameters::(seed)?; @@ -81,29 +83,27 @@ fn place_order( } .dispatch_bypass_filter(RawOrigin::Signed(acc.clone()).into())?; - let order_hash = OrderBook::::order_hash(&acc, order_id); - - Ok((acc, market_id, order_hash)) + Ok((acc, market_id, order_id)) } benchmarks! { remove_order_ask { - let (caller, _, order_hash) = place_order::(OrderSide::Ask, None)?; - }: remove_order(RawOrigin::Signed(caller), order_hash) + let (caller, _, order_id) = place_order::(OrderSide::Ask, None)?; + }: remove_order(RawOrigin::Signed(caller), order_id) remove_order_bid { - let (caller, _, order_hash) = place_order::(OrderSide::Bid, None)?; - }: remove_order(RawOrigin::Signed(caller), order_hash) + let (caller, _, order_id) = place_order::(OrderSide::Bid, None)?; + }: remove_order(RawOrigin::Signed(caller), order_id) fill_order_ask { let caller = generate_funded_account::(None)?; - let (_, _, order_hash) = place_order::(OrderSide::Ask, Some(0))?; - }: fill_order(RawOrigin::Signed(caller), order_hash, None) + let (_, _, order_id) = place_order::(OrderSide::Ask, Some(0))?; + }: fill_order(RawOrigin::Signed(caller), order_id, None) fill_order_bid { let caller = generate_funded_account::(None)?; - let (_, _, order_hash) = place_order::(OrderSide::Bid, Some(0))?; - }: fill_order(RawOrigin::Signed(caller), order_hash, None) + let (_, _, order_id) = place_order::(OrderSide::Bid, Some(0))?; + }: fill_order(RawOrigin::Signed(caller), order_id, None) place_order_ask { let (caller, outcome_asset, outcome_asset_amount, base_asset_amount, market_id) = order_common_parameters::(None)?; diff --git a/zrml/orderbook-v1/src/lib.rs b/zrml/orderbook-v1/src/lib.rs index 8a4ac10c5..946896e37 100644 --- a/zrml/orderbook-v1/src/lib.rs +++ b/zrml/orderbook-v1/src/lib.rs @@ -34,9 +34,8 @@ use frame_support::{ use frame_system::{ensure_signed, pallet_prelude::OriginFor}; use orml_traits::{BalanceStatus, MultiCurrency, NamedMultiReservableCurrency}; pub use pallet::*; -use parity_scale_codec::Encode; use sp_runtime::{ - traits::{CheckedSub, Get, Hash, Zero}, + traits::{CheckedSub, Get, Zero}, ArithmeticError, Perquintill, SaturatedConversion, Saturating, }; use zeitgeist_primitives::{ @@ -85,7 +84,6 @@ mod pallet { pub(crate) type MarketIdOf = <::MarketCommons as MarketCommonsPalletApi>::MarketId; pub(crate) type AccountIdOf = ::AccountId; - pub(crate) type HashOf = ::Hash; pub(crate) type OrderOf = Order, BalanceOf, MarketIdOf>; pub(crate) type MomentOf = <::MarketCommons as MarketCommonsPalletApi>::Moment; pub(crate) type MarketCommonsBalanceOf = @@ -108,7 +106,7 @@ mod pallet { pub type NextOrderId = StorageValue<_, OrderId, ValueQuery>; #[pallet::storage] - pub type Orders = StorageMap<_, Twox64Concat, T::Hash, OrderOf, OptionQuery>; + pub type Orders = StorageMap<_, Twox64Concat, OrderId, OrderOf, OptionQuery>; #[pallet::event] #[pallet::generate_deposit(fn deposit_event)] @@ -117,7 +115,7 @@ mod pallet { T: Config, { OrderFilled { - order_hash: HashOf, + order_id: OrderId, maker: AccountIdOf, taker: AccountIdOf, filled: BalanceOf, @@ -125,12 +123,11 @@ mod pallet { unfilled_base_asset_amount: BalanceOf, }, OrderPlaced { - order_hash: HashOf, order_id: OrderId, order: OrderOf, }, OrderRemoved { - order_hash: HashOf, + order_id: OrderId, maker: T::AccountId, }, } @@ -167,13 +164,10 @@ mod pallet { T::WeightInfo::remove_order_ask().max(T::WeightInfo::remove_order_bid()) )] #[transactional] - pub fn remove_order( - origin: OriginFor, - order_hash: T::Hash, - ) -> DispatchResultWithPostInfo { + pub fn remove_order(origin: OriginFor, order_id: OrderId) -> DispatchResultWithPostInfo { let sender = ensure_signed(origin)?; - let order_data = >::get(order_hash).ok_or(Error::::OrderDoesNotExist)?; + let order_data = >::get(order_id).ok_or(Error::::OrderDoesNotExist)?; let maker = &order_data.maker; ensure!(sender == *maker, Error::::NotOrderCreator); @@ -197,9 +191,9 @@ mod pallet { } } - >::remove(order_hash); + >::remove(order_id); - Self::deposit_event(Event::OrderRemoved { order_hash, maker: maker.clone() }); + Self::deposit_event(Event::OrderRemoved { order_id, maker: maker.clone() }); match order_data.side { OrderSide::Bid => Ok(Some(T::WeightInfo::remove_order_bid()).into()), @@ -222,13 +216,12 @@ mod pallet { #[transactional] pub fn fill_order( origin: OriginFor, - order_hash: T::Hash, + order_id: OrderId, maker_partial_fill: Option>, ) -> DispatchResultWithPostInfo { let taker = ensure_signed(origin)?; - let mut order_data = - >::get(order_hash).ok_or(Error::::OrderDoesNotExist)?; + let mut order_data = >::get(order_id).ok_or(Error::::OrderDoesNotExist)?; let market = T::MarketCommons::market(&order_data.market_id)?; ensure!(market.scoring_rule == ScoringRule::Orderbook, Error::::InvalidScoringRule); ensure!(market.status == MarketStatus::Active, Error::::MarketIsNotActive); @@ -340,13 +333,13 @@ mod pallet { unfilled_outcome_asset_amount.saturating_add(unfilled_base_asset_amount); if total_unfilled.is_zero() { - >::remove(order_hash); + >::remove(order_id); } else { - >::insert(order_hash, order_data.clone()); + >::insert(order_id, order_data.clone()); } Self::deposit_event(Event::OrderFilled { - order_hash, + order_id, maker, taker: taker.clone(), filled: maker_fill, @@ -393,10 +386,8 @@ mod pallet { let order_id = >::get(); let next_order_id = order_id.checked_add(1).ok_or(ArithmeticError::Overflow)?; - let order_hash = Self::order_hash(&who, order_id); let order = Order { market_id, - order_id, side: side.clone(), maker: who.clone(), outcome_asset, @@ -424,9 +415,9 @@ mod pallet { } } - >::insert(order_hash, order.clone()); + >::insert(order_id, order.clone()); >::put(next_order_id); - Self::deposit_event(Event::OrderPlaced { order_hash, order_id, order }); + Self::deposit_event(Event::OrderPlaced { order_id, order }); match side { OrderSide::Bid => Ok(Some(T::WeightInfo::place_order_bid()).into()), @@ -442,10 +433,6 @@ mod pallet { T::PalletId::get().0 } - pub fn order_hash(creator: &T::AccountId, order_id: OrderId) -> T::Hash { - (creator, order_id).using_encoded(T::Hashing::hash) - } - pub fn outcome_assets( market_id: MarketIdOf, market: &MarketOf, diff --git a/zrml/orderbook-v1/src/tests.rs b/zrml/orderbook-v1/src/tests.rs index 71a441432..d522347e1 100644 --- a/zrml/orderbook-v1/src/tests.rs +++ b/zrml/orderbook-v1/src/tests.rs @@ -64,7 +64,6 @@ fn fill_order_fails_with_wrong_scoring_rule(scoring_rule: ScoringRule) { Markets::::insert(market_id, market); let order_id = 0u128; - let order_hash = Orderbook::order_hash(&ALICE, order_id); assert_ok!(Orderbook::place_order( RuntimeOrigin::signed(ALICE), @@ -81,7 +80,7 @@ fn fill_order_fails_with_wrong_scoring_rule(scoring_rule: ScoringRule) { })); assert_noop!( - Orderbook::fill_order(RuntimeOrigin::signed(ALICE), order_hash, None), + Orderbook::fill_order(RuntimeOrigin::signed(ALICE), order_id, None), Error::::InvalidScoringRule ); }); @@ -91,14 +90,13 @@ fn fill_order_fails_with_wrong_scoring_rule(scoring_rule: ScoringRule) { fn it_fails_order_does_not_exist() { ExtBuilder::default().build().execute_with(|| { let order_id = 0u128; - let order_hash = Orderbook::order_hash(&ALICE, order_id); assert_noop!( - Orderbook::fill_order(RuntimeOrigin::signed(ALICE), order_hash, None), + Orderbook::fill_order(RuntimeOrigin::signed(ALICE), order_id, None), Error::::OrderDoesNotExist, ); assert_noop!( - Orderbook::remove_order(RuntimeOrigin::signed(ALICE), order_hash), + Orderbook::remove_order(RuntimeOrigin::signed(ALICE), order_id), Error::::OrderDoesNotExist, ); }); @@ -168,15 +166,14 @@ fn it_fills_ask_orders_fully() { assert_eq!(reserved_bob, 10); let order_id = 0u128; - let order_hash = Orderbook::order_hash(&BOB, order_id); - assert_ok!(Orderbook::fill_order(RuntimeOrigin::signed(ALICE), order_hash, None)); + assert_ok!(Orderbook::fill_order(RuntimeOrigin::signed(ALICE), order_id, None)); let reserved_bob = Tokens::reserved_balance(outcome_asset, &BOB); assert_eq!(reserved_bob, 0); System::assert_last_event( Event::::OrderFilled { - order_hash, + order_id, maker: BOB, taker: ALICE, filled: 50, @@ -221,16 +218,15 @@ fn it_fills_bid_orders_fully() { assert_eq!(reserved_bob, 50); let order_id = 0u128; - let order_hash = Orderbook::order_hash(&BOB, order_id); assert_ok!(Tokens::deposit(outcome_asset, &ALICE, 10)); - assert_ok!(Orderbook::fill_order(RuntimeOrigin::signed(ALICE), order_hash, None)); + assert_ok!(Orderbook::fill_order(RuntimeOrigin::signed(ALICE), order_id, None)); let reserved_bob = Tokens::reserved_balance(outcome_asset, &BOB); assert_eq!(reserved_bob, 0); System::assert_last_event( Event::::OrderFilled { - order_hash, + order_id, maker: BOB, taker: ALICE, filled: 10, @@ -275,19 +271,17 @@ fn it_fills_bid_orders_partially() { assert_eq!(reserved_bob, 5000); let order_id = 0u128; - let order_hash = Orderbook::order_hash(&BOB, order_id); assert_ok!(Tokens::deposit(outcome_asset, &ALICE, 1000)); // instead of selling 1000 shares, Alice sells 700 shares let portion = Some(700); - assert_ok!(Orderbook::fill_order(RuntimeOrigin::signed(ALICE), order_hash, portion,)); + assert_ok!(Orderbook::fill_order(RuntimeOrigin::signed(ALICE), order_id, portion,)); - let order = >::get(order_hash).unwrap(); + let order = >::get(order_id).unwrap(); assert_eq!( order, Order { market_id, - order_id, side: OrderSide::Bid, maker: BOB, outcome_asset, @@ -304,7 +298,7 @@ fn it_fills_bid_orders_partially() { System::assert_last_event( Event::::OrderFilled { - order_hash, + order_id, maker: BOB, taker: ALICE, filled: 700, @@ -355,19 +349,17 @@ fn it_fills_ask_orders_partially() { assert_eq!(reserved_bob, 1000); let order_id = 0u128; - let order_hash = Orderbook::order_hash(&BOB, order_id); assert_ok!(Tokens::deposit(market.base_asset, &ALICE, 5000)); // instead of buying 5000 of the base asset, Alice buys 700 shares let portion = Some(700); - assert_ok!(Orderbook::fill_order(RuntimeOrigin::signed(ALICE), order_hash, portion,)); + assert_ok!(Orderbook::fill_order(RuntimeOrigin::signed(ALICE), order_id, portion,)); - let order = >::get(order_hash).unwrap(); + let order = >::get(order_id).unwrap(); assert_eq!( order, Order { market_id, - order_id, side: OrderSide::Ask, maker: BOB, outcome_asset, @@ -384,7 +376,7 @@ fn it_fills_ask_orders_partially() { System::assert_last_event( Event::::OrderFilled { - order_hash, + order_id, maker: BOB, taker: ALICE, filled: 700, @@ -429,15 +421,11 @@ fn it_removes_orders() { )); let order_id = 0u128; - let order_hash = Orderbook::order_hash(&ALICE, order_id); - System::assert_last_event( Event::::OrderPlaced { - order_hash, order_id: 0, order: Order { market_id, - order_id, side: OrderSide::Bid, maker: ALICE, outcome_asset: share_id, @@ -449,12 +437,11 @@ fn it_removes_orders() { .into(), ); - let order = >::get(order_hash).unwrap(); + let order = >::get(order_id).unwrap(); assert_eq!( order, Order { market_id, - order_id, side: OrderSide::Bid, maker: ALICE, outcome_asset: share_id, @@ -465,7 +452,7 @@ fn it_removes_orders() { ); assert_noop!( - Orderbook::remove_order(RuntimeOrigin::signed(BOB), order_hash), + Orderbook::remove_order(RuntimeOrigin::signed(BOB), order_id), Error::::NotOrderCreator, ); @@ -473,16 +460,16 @@ fn it_removes_orders() { >::reserved_balance(&ALICE); assert_eq!(reserved_funds, 10); - assert_ok!(Orderbook::remove_order(RuntimeOrigin::signed(ALICE), order_hash)); + assert_ok!(Orderbook::remove_order(RuntimeOrigin::signed(ALICE), order_id)); let reserved_funds = >::reserved_balance(&ALICE); assert_eq!(reserved_funds, 0); - assert!(>::get(order_hash).is_none()); + assert!(>::get(order_id).is_none()); System::assert_last_event( - Event::::OrderRemoved { order_hash, maker: ALICE }.into(), + Event::::OrderRemoved { order_id, maker: ALICE }.into(), ); }); } diff --git a/zrml/orderbook-v1/src/types.rs b/zrml/orderbook-v1/src/types.rs index 9737a044c..b0e1d2b73 100644 --- a/zrml/orderbook-v1/src/types.rs +++ b/zrml/orderbook-v1/src/types.rs @@ -31,7 +31,6 @@ pub enum OrderSide { #[derive(Clone, Encode, Eq, Decode, MaxEncodedLen, PartialEq, RuntimeDebug, TypeInfo)] pub struct Order { pub market_id: MarketId, - pub order_id: OrderId, pub side: OrderSide, pub maker: AccountId, pub outcome_asset: Asset,