diff --git a/crates/e2e/tests/e2e/limit_orders.rs b/crates/e2e/tests/e2e/limit_orders.rs index 0160dc6922..f1e23c012f 100644 --- a/crates/e2e/tests/e2e/limit_orders.rs +++ b/crates/e2e/tests/e2e/limit_orders.rs @@ -139,7 +139,7 @@ async fn single_limit_order_test(web3: Web3) { let services = Services::new(onchain.contracts()).await; services.start_protocol(solver).await; - let order = OrderCreation { + OrderCreation { sell_token: token_a.address(), sell_amount: to_wei(10), buy_token: token_b.address(), @@ -153,9 +153,6 @@ async fn single_limit_order_test(web3: Web3) { &onchain.contracts().domain_separator, SecretKeyRef::from(&SecretKey::from_slice(trader_a.private_key()).unwrap()), ); - let order_id = services.create_order(&order).await.unwrap(); - let limit_order = services.get_order(&order_id).await.unwrap(); - assert_eq!(limit_order.metadata.class, OrderClass::Limit); // Drive solution tracing::info!("Waiting for trade."); @@ -237,7 +234,7 @@ async fn two_limit_orders_test(web3: Web3) { let services = Services::new(onchain.contracts()).await; services.start_protocol(solver).await; - let order_a = OrderCreation { + OrderCreation { sell_token: token_a.address(), sell_amount: to_wei(10), buy_token: token_b.address(), @@ -251,12 +248,8 @@ async fn two_limit_orders_test(web3: Web3) { &onchain.contracts().domain_separator, SecretKeyRef::from(&SecretKey::from_slice(trader_a.private_key()).unwrap()), ); - let order_id = services.create_order(&order_a).await.unwrap(); - - let limit_order = services.get_order(&order_id).await.unwrap(); - assert!(limit_order.metadata.class.is_limit()); - let order_b = OrderCreation { + OrderCreation { sell_token: token_b.address(), sell_amount: to_wei(5), buy_token: token_a.address(), @@ -270,10 +263,6 @@ async fn two_limit_orders_test(web3: Web3) { &onchain.contracts().domain_separator, SecretKeyRef::from(&SecretKey::from_slice(trader_b.private_key()).unwrap()), ); - let order_id = services.create_order(&order_b).await.unwrap(); - - let limit_order = services.get_order(&order_id).await.unwrap(); - assert!(limit_order.metadata.class.is_limit()); wait_for_condition(TIMEOUT, || async { services.solvable_orders().await == 2 }) .await diff --git a/crates/orderbook/src/database/orders.rs b/crates/orderbook/src/database/orders.rs index 9972191210..809872c549 100644 --- a/crates/orderbook/src/database/orders.rs +++ b/crates/orderbook/src/database/orders.rs @@ -44,9 +44,8 @@ use { signing_scheme_from, signing_scheme_into, }, - fee::FeeParameters, order_quoting::Quote, - order_validation::{is_order_outside_market_price, Amounts, LimitOrderCounting}, + order_validation::LimitOrderCounting, }, sqlx::{types::BigDecimal, Connection, PgConnection}, std::convert::TryInto, @@ -63,7 +62,7 @@ pub trait OrderStoring: Send + Sync { &self, old_order: &OrderUid, new_order: &Order, - new_quote: Option, + new_quote: &Quote, ) -> Result<(), InsertionError>; async fn orders_for_tx(&self, tx_hash: &H256) -> Result>; async fn single_order(&self, uid: &OrderUid) -> Result>; @@ -272,7 +271,7 @@ impl OrderStoring for Postgres { &self, old_order: &model::order::OrderUid, new_order: &model::order::Order, - new_quote: Option, + new_quote: &Quote, ) -> anyhow::Result<(), super::orders::InsertionError> { let _timer = super::Metrics::get() .database_queries @@ -281,6 +280,7 @@ impl OrderStoring for Postgres { let old_order = *old_order; let new_order = new_order.clone(); + let new_quote = new_quote.clone(); let mut connection = self.pool.acquire().await?; connection .transaction(move |ex| { @@ -292,11 +292,8 @@ impl OrderStoring for Postgres { ) .await?; insert_order(&new_order, ex).await?; - if let Some(quote) = new_quote { - insert_quote(&new_order.metadata.uid, "e, ex).await?; - } + insert_quote(&new_order.metadata.uid, &new_quote, ex).await?; Self::insert_order_app_data(&new_order, ex).await?; - Ok(()) } .boxed() @@ -367,33 +364,12 @@ impl LimitOrderCounting for Postgres { .start_timer(); let mut ex = self.pool.acquire().await?; - Ok(database::orders::user_orders_with_quote( + Ok(database::orders::count_limit_orders_by_owner( &mut ex, now_in_epoch_seconds().into(), &ByteArray(owner.0), ) .await? - .into_iter() - .filter(|order_with_quote| { - is_order_outside_market_price( - &Amounts { - sell: big_decimal_to_u256(&order_with_quote.order_sell_amount).unwrap(), - buy: big_decimal_to_u256(&order_with_quote.order_buy_amount).unwrap(), - fee: big_decimal_to_u256(&order_with_quote.order_fee_amount).unwrap(), - }, - &Amounts { - sell: big_decimal_to_u256(&order_with_quote.quote_sell_amount).unwrap(), - buy: big_decimal_to_u256(&order_with_quote.quote_buy_amount).unwrap(), - fee: FeeParameters { - gas_amount: order_with_quote.quote_gas_amount, - gas_price: order_with_quote.quote_gas_price, - sell_token_price: order_with_quote.quote_sell_token_price, - } - .fee(), - }, - ) - }) - .count() .try_into() .unwrap()) } @@ -797,7 +773,8 @@ mod tests { }, ..Default::default() }; - db.replace_order(&old_order.metadata.uid, &new_order, None) + let quote = Quote::default(); + db.replace_order(&old_order.metadata.uid, &new_order, "e) .await .unwrap(); @@ -858,8 +835,9 @@ mod tests { db.insert_order(&new_order, None).await.unwrap(); // Attempt to replace an old order with one that already exists should fail. + let quote = Quote::default(); let err = db - .replace_order(&old_order.metadata.uid, &new_order, None) + .replace_order(&old_order.metadata.uid, &new_order, "e) .await .unwrap_err(); assert!(matches!(err, InsertionError::DuplicatedRecord)); diff --git a/crates/orderbook/src/orderbook.rs b/crates/orderbook/src/orderbook.rs index 3b1f44f555..218db0a5b5 100644 --- a/crates/orderbook/src/orderbook.rs +++ b/crates/orderbook/src/orderbook.rs @@ -216,10 +216,10 @@ impl Orderbook { if let Some(old_order) = replaced_order { self.replace_order(order, old_order, quote).await } else { - let quote_id = quote.as_ref().and_then(|quote| quote.id); + let quote_id = quote.id; self.database - .insert_order(&order, quote) + .insert_order(&order, Some(quote)) .await .map_err(|err| AddOrderError::from_insertion(err, &order))?; Metrics::on_order_operation(&order, OrderOperation::Created); @@ -346,7 +346,7 @@ impl Orderbook { &self, validated_new_order: Order, old_order: Order, - quote: Option, + quote: Quote, ) -> Result<(OrderUid, Option), AddOrderError> { // Replacement order signatures need to be validated meaning we cannot // accept `PreSign` orders, otherwise anyone can cancel a user order by @@ -363,10 +363,10 @@ impl Orderbook { return Err(AddOrderError::InvalidReplacement); } - let quote_id = quote.as_ref().and_then(|quote| quote.id); + let quote_id = quote.id; self.database - .replace_order(&old_order.metadata.uid, &validated_new_order, quote) + .replace_order(&old_order.metadata.uid, &validated_new_order, "e) .await .map_err(|err| AddOrderError::from_insertion(err, &validated_new_order))?; Metrics::on_order_operation(&old_order, OrderOperation::Cancelled); diff --git a/crates/shared/src/order_quoting.rs b/crates/shared/src/order_quoting.rs index 7644026f25..bac128fd4d 100644 --- a/crates/shared/src/order_quoting.rs +++ b/crates/shared/src/order_quoting.rs @@ -8,7 +8,7 @@ use { crate::{ db_order_conversions::order_kind_from, fee::FeeParameters, - order_validation::PreOrderData, + order_validation::{PreOrderClass, PreOrderData}, price_estimation::Verification, }, anyhow::{Context, Result}, @@ -18,7 +18,7 @@ use { futures::TryFutureExt as _, gas_estimation::GasPriceEstimating, model::{ - order::{OrderClass, OrderKind}, + order::OrderKind, quote::{OrderQuoteRequest, OrderQuoteSide, QuoteId, QuoteSigningScheme, SellAmount}, }, number::conversions::big_decimal_to_u256, @@ -543,7 +543,7 @@ impl From<&OrderQuoteRequest> for PreOrderData { buy_token_balance: quote_request.buy_token_balance, sell_token_balance: quote_request.sell_token_balance, signing_scheme: quote_request.signing_scheme.into(), - class: OrderClass::Market, + class: PreOrderClass::Market, } } } diff --git a/crates/shared/src/order_validation.rs b/crates/shared/src/order_validation.rs index f7939b8b93..7d5d160490 100644 --- a/crates/shared/src/order_validation.rs +++ b/crates/shared/src/order_validation.rs @@ -100,7 +100,7 @@ pub trait OrderValidating: Send + Sync { domain_separator: &DomainSeparator, settlement_contract: H160, full_app_data_override: Option, - ) -> Result<(Order, Option), ValidationError>; + ) -> Result<(Order, Quote), ValidationError>; } #[derive(Debug)] @@ -268,7 +268,14 @@ pub struct PreOrderData { pub buy_token_balance: BuyTokenDestination, pub sell_token_balance: SellTokenSource, pub signing_scheme: SigningScheme, - pub class: OrderClass, + pub class: PreOrderClass, +} + +#[derive(Debug, Eq, PartialEq, Default)] +pub enum PreOrderClass { + #[default] + Market, + Limit, } fn actual_receiver(owner: H160, order: &OrderData) -> H160 { @@ -297,8 +304,8 @@ impl PreOrderData { sell_token_balance: order.sell_token_balance, signing_scheme, class: match order.fee_amount.is_zero() { - true => OrderClass::Limit, - false => OrderClass::Market, + true => PreOrderClass::Limit, + false => PreOrderClass::Market, }, } } @@ -412,7 +419,7 @@ impl OrderValidating for OrderValidator { return Err(PartialValidationError::Forbidden); } - if order.class == OrderClass::Market && order.partially_fillable { + if order.class == PreOrderClass::Market && order.partially_fillable { return Err(PartialValidationError::UnsupportedOrderType); } @@ -512,7 +519,7 @@ impl OrderValidating for OrderValidator { domain_separator: &DomainSeparator, settlement_contract: H160, full_app_data_override: Option, - ) -> Result<(Order, Option), ValidationError> { + ) -> Result<(Order, Quote), ValidationError> { // Happens before signature verification because a miscalculated app data hash // by the API user would lead to being unable to validate the signature below. let app_data = self.validate_app_data(&order.app_data, &full_app_data_override)?; @@ -558,7 +565,6 @@ impl OrderValidating for OrderValidator { } let pre_order = PreOrderData::from_order_creation(owner, &data, signing_scheme); - let class = pre_order.class; self.partial_validate(pre_order) .await .map_err(ValidationError::Partial)?; @@ -639,93 +645,38 @@ impl OrderValidating for OrderValidator { // Check if we need to re-classify the market order if it is outside the market // price. We consider out-of-price orders as liquidity orders. See // . - let (class, quote) = match class { - // This has to be here in order to keep the previous behaviour - OrderClass::Market => { - let quote = get_quote_and_check_fee( - &*self.quoter, - "e_parameters, - order.quote_id, - Some(data.fee_amount), - self.market_orders_deprecation_date, - ) - .await?; - tracing::debug!( - ?uid, - ?order, - ?quote, - "checking if order is outside market price" - ); - if is_order_outside_market_price( - &Amounts { - sell: data.sell_amount, - buy: data.buy_amount, - fee: data.fee_amount, - }, - &Amounts { - sell: quote.sell_amount, - buy: quote.buy_amount, - fee: quote.fee_amount, - }, - ) { - tracing::debug!(%uid, ?owner, ?class, "order being flagged as outside market price"); - (OrderClass::Limit, Some(quote)) - } else { - (class, Some(quote)) - } - } - OrderClass::Limit => { - let quote = get_quote_and_check_fee( - &*self.quoter, - "e_parameters, - order.quote_id, - None, - self.market_orders_deprecation_date, - ) - .await?; - // If the order is not "In-Market", check for the limit orders - if is_order_outside_market_price( - &Amounts { - sell: data.sell_amount, - buy: data.buy_amount, - fee: data.fee_amount, - }, - &Amounts { - sell: quote.sell_amount, - buy: quote.buy_amount, - fee: quote.fee_amount, - }, - ) { - self.check_max_limit_orders(owner).await?; - } - (class, Some(quote)) - } - OrderClass::Liquidity => { - let quote = get_quote_and_check_fee( - &*self.quoter, - "e_parameters, - order.quote_id, - None, - self.market_orders_deprecation_date, - ) - .await?; - // If the order is not "In-Market", check for the limit orders - if is_order_outside_market_price( - &Amounts { - sell: data.sell_amount, - buy: data.buy_amount, - fee: data.fee_amount, - }, - &Amounts { - sell: quote.sell_amount, - buy: quote.buy_amount, - fee: quote.fee_amount, - }, - ) { - self.check_max_limit_orders(owner).await?; - } - (OrderClass::Limit, None) - } + let fee_amount = (!data.fee_amount.is_zero()).then_some(data.fee_amount); + let quote = get_quote_and_check_fee( + &*self.quoter, + "e_parameters, + order.quote_id, + fee_amount, + self.market_orders_deprecation_date, + ) + .await?; + tracing::debug!( + ?uid, + ?order, + ?quote, + "checking if order is outside market price" + ); + let class = if is_order_outside_market_price( + &Amounts { + sell: data.sell_amount, + buy: data.buy_amount, + fee: data.fee_amount, + }, + &Amounts { + sell: quote.sell_amount, + buy: quote.buy_amount, + fee: quote.fee_amount, + }, + ) { + tracing::debug!(%uid, ?owner, "order being flagged as outside market price"); + self.check_max_limit_orders(owner).await?; + OrderClass::Limit + } else { + OrderClass::Market }; let order = Order { @@ -791,11 +742,9 @@ impl OrderValidPeriodConfiguration { if order.signing_scheme == SigningScheme::PreSign { return Duration::MAX; } - match order.class { - OrderClass::Market => self.max_market, - OrderClass::Limit => self.max_limit, - OrderClass::Liquidity => Duration::MAX, + PreOrderClass::Market => self.max_market, + PreOrderClass::Limit => self.max_limit, } } } @@ -1142,7 +1091,7 @@ mod tests { valid_to: legit_valid_to + validity_configuration.max_limit.as_secs() as u32 + 1, - class: OrderClass::Limit, + class: PreOrderClass::Limit, ..Default::default() }) .await, @@ -1229,7 +1178,7 @@ mod tests { .is_ok()); assert!(validator .partial_validate(PreOrderData { - class: OrderClass::Limit, + class: PreOrderClass::Limit, owner: H160::from_low_u64_be(0x42), valid_to: time::now_in_epoch_seconds() + validity_configuration.max_market.as_secs() as u32 @@ -1238,16 +1187,6 @@ mod tests { }) .await .is_ok()); - assert!(validator - .partial_validate(PreOrderData { - partially_fillable: true, - class: OrderClass::Liquidity, - owner: H160::from_low_u64_be(0x42), - valid_to: u32::MAX, - ..order() - }) - .await - .is_ok()); } #[tokio::test] @@ -1424,12 +1363,10 @@ mod tests { fee_amount: U256::zero(), ..creation.clone() }; - let (order, quote) = validator + validator .validate_and_construct_order(creation_, &domain_separator, Default::default(), None) .await .unwrap(); - assert!(quote.is_some()); - assert!(order.metadata.class.is_limit()); let creation_ = OrderCreation { fee_amount: U256::zero(), @@ -1439,12 +1376,10 @@ mod tests { }, ..creation }; - let (order, quote) = validator + validator .validate_and_construct_order(creation_, &domain_separator, Default::default(), None) .await .unwrap(); - assert!(quote.is_some()); - assert!(order.metadata.class.is_limit()); } #[tokio::test] @@ -1647,70 +1582,6 @@ mod tests { assert!(matches!(result, Err(ValidationError::ZeroAmount))); } - #[tokio::test] - async fn post_out_of_market_orders_when_limit_orders_disabled() { - let expected_buy_amount = U256::from(100); - - let mut order_quoter = MockOrderQuoting::new(); - let mut bad_token_detector = MockBadTokenDetecting::new(); - let mut balance_fetcher = MockBalanceFetching::new(); - order_quoter.expect_find_quote().returning(move |_, _| { - Ok(Quote { - buy_amount: expected_buy_amount, - sell_amount: U256::from(1), - fee_amount: U256::from(1), - ..Default::default() - }) - }); - bad_token_detector - .expect_detect() - .returning(|_| Ok(TokenQuality::Good)); - balance_fetcher - .expect_can_transfer() - .returning(|_, _| Ok(())); - let mut limit_order_counter = MockLimitOrderCounting::new(); - limit_order_counter.expect_count().returning(|_| Ok(0u64)); - let validator = OrderValidator::new( - dummy_contract!(WETH9, [0xef; 20]), - Arc::new(order_validation::banned::Users::none()), - OrderValidPeriodConfiguration::any(), - false, - Arc::new(bad_token_detector), - dummy_contract!(HooksTrampoline, [0xcf; 20]), - Arc::new(order_quoter), - Arc::new(balance_fetcher), - Arc::new(MockSignatureValidating::new()), - Arc::new(limit_order_counter), - 0, - Arc::new(MockCodeFetching::new()), - Default::default(), - None, - ); - let order = OrderCreation { - valid_to: time::now_in_epoch_seconds() + 2, - sell_token: H160::from_low_u64_be(1), - buy_token: H160::from_low_u64_be(2), - buy_amount: expected_buy_amount + 1, // buy more than expected - sell_amount: U256::from(1), - fee_amount: U256::from(1), - kind: OrderKind::Sell, - signature: Signature::Eip712(EcdsaSignature::non_zero()), - app_data: OrderCreationAppData::Full { - full: "{}".to_string(), - }, - ..Default::default() - }; - let (order, quote) = validator - .validate_and_construct_order(order, &Default::default(), Default::default(), None) - .await - .unwrap(); - - // Out-of-price orders are intentionally marked as liquidity - // orders! - assert_eq!(order.metadata.class, OrderClass::Limit); - assert!(quote.is_some()); - } - #[tokio::test] async fn post_validate_err_wrong_owner() { let mut order_quoter = MockOrderQuoting::new();