Do not count "in-market" orders for the order limit #2433

crates/e2e/tests/e2e/limit_orders.rs

@@ -1,5 +1,6 @@
 use {
     contracts::ERC20,
+    driver::domain::eth::NonZeroU256,
     e2e::{nodes::forked_node::ForkedNodeApi, setup::*, tx},
     ethcontract::{prelude::U256, H160},
     model::{
@@ -30,6 +31,12 @@ async fn local_node_too_many_limit_orders() {
     run_test(too_many_limit_orders_test).await;
 }
 
+#[tokio::test]
+#[ignore]
+async fn local_node_limit_does_not_apply_to_in_market_orders_test() {
+    run_test(limit_does_not_apply_to_in_market_orders_test).await;
+}
+
 #[tokio::test]
 #[ignore]
 async fn local_node_mixed_limit_and_market_orders() {
@@ -483,11 +490,96 @@ async fn too_many_limit_orders_test(web3: Web3) {
         &onchain.contracts().domain_separator,
         SecretKeyRef::from(&SecretKey::from_slice(trader.private_key()).unwrap()),
     );
+
     let (status, body) = services.create_order(&order).await.unwrap_err();
     assert_eq!(status, 400);
     assert!(body.contains("TooManyLimitOrders"));
 }
 
+async fn limit_does_not_apply_to_in_market_orders_test(web3: Web3) {
+    let mut onchain = OnchainComponents::deploy(web3.clone()).await;
+
+    let [solver] = onchain.make_solvers(to_wei(1)).await;
+    let [trader] = onchain.make_accounts(to_wei(1)).await;
+    let [token_a] = onchain
+        .deploy_tokens_with_weth_uni_v2_pools(to_wei(1_000), to_wei(1_000))
+        .await;
+    token_a.mint(trader.address(), to_wei(100)).await;
+
+    // Approve GPv2 for trading
+    tx!(
+        trader.account(),
+        token_a.approve(onchain.contracts().allowance, to_wei(101))
+    );
+
+    // Place Orders
+    let services = Services::new(onchain.contracts()).await;
+    let solver_endpoint =
+        colocation::start_baseline_solver(onchain.contracts().weth.address()).await;
+    colocation::start_driver(
+        onchain.contracts(),
+        vec![colocation::SolverEngine {
+            name: "test_solver".into(),
+            account: solver,
+            endpoint: solver_endpoint,
+        }],
+    );
+    services
+        .start_api(vec![
+            "--max-limit-orders-per-user=1".into(),
+            "--price-estimation-drivers=test_quoter|http://localhost:11088/test_solver".to_string(),
+        ])
+        .await;
+
+    let quote_request = OrderQuoteRequest {
+        from: trader.address(),
+        sell_token: token_a.address(),
+        buy_token: onchain.contracts().weth.address(),
+        side: OrderQuoteSide::Sell {
+            sell_amount: SellAmount::BeforeFee {
+                value: NonZeroU256::try_from(to_wei(5)).unwrap(),
+            },
+        },
+        ..Default::default()
+    };
+    let quote = services.submit_quote(&quote_request).await.unwrap();
+
+    let order = OrderCreation {
+        sell_token: token_a.address(),
+        sell_amount: quote.quote.sell_amount,
+        buy_token: onchain.contracts().weth.address(),
+        buy_amount: quote.quote.buy_amount.saturating_sub(to_wei(1)),
+        valid_to: model::time::now_in_epoch_seconds() + 300,
+        kind: OrderKind::Sell,
+        ..Default::default()
+    }
+    .sign(
+        EcdsaSigningScheme::Eip712,
+        &onchain.contracts().domain_separator,
+        SecretKeyRef::from(&SecretKey::from_slice(trader.private_key()).unwrap()),
+    );
+    let order_id = services.create_order(&order).await.unwrap();
+    let limit_order = services.get_order(&order_id).await.unwrap();
+    assert!(limit_order.metadata.class.is_limit());
+
+    // Place another "in-market" order in order to check it is not limited
+    let order = OrderCreation {
+        sell_token: token_a.address(),
+        sell_amount: quote.quote.sell_amount,
+        buy_token: onchain.contracts().weth.address(),
+        buy_amount: quote.quote.buy_amount.saturating_sub(to_wei(2)),
+        valid_to: model::time::now_in_epoch_seconds() + 300,
+        kind: OrderKind::Sell,
+        ..Default::default()
+    }
+    .sign(
+        EcdsaSigningScheme::Eip712,
+        &onchain.contracts().domain_separator,
+        SecretKeyRef::from(&SecretKey::from_slice(trader.private_key()).unwrap()),
+    );
+    assert!(services.create_order(&order).await.is_ok());
+}
+
 async fn forked_mainnet_single_limit_order_test(web3: Web3) {
     let mut onchain = OnchainComponents::deployed(web3.clone()).await;
     let forked_node_api = web3.api::<ForkedNodeApi<_>>();

crates/shared/src/order_validation.rs

@@ -351,20 +351,14 @@ impl OrderValidator {
         self
     }
 
-    async fn check_max_limit_orders(
-        &self,
-        owner: H160,
-        class: &OrderClass,
-    ) -> Result<(), ValidationError> {
-        if class.is_limit() {
-            let num_limit_orders = self
-                .limit_order_counter
-                .count(owner)
-                .await
-                .map_err(ValidationError::Other)?;
-            if num_limit_orders >= self.max_limit_orders_per_user {
-                return Err(ValidationError::TooManyLimitOrders);
-            }
+    async fn check_max_limit_orders(&self, owner: H160) -> Result<(), ValidationError> {
+        let num_limit_orders = self
+            .limit_order_counter
+            .count(owner)
+            .await
+            .map_err(ValidationError::Other)?;
+        if num_limit_orders >= self.max_limit_orders_per_user {
+            return Err(ValidationError::TooManyLimitOrders);
         }
         Ok(())
     }
@@ -593,32 +587,6 @@ impl OrderValidating for OrderValidator {
             additional_gas: app_data.inner.protocol.hooks.gas_limit(),
             verification,
         };
-        let quote = match class {
-            OrderClass::Market => {
-                let fee = Some(data.fee_amount);
-                let quote = get_quote_and_check_fee(
-                    &*self.quoter,
-                    &quote_parameters,
-                    order.quote_id,
-                    fee,
-                    self.market_orders_deprecation_date,
-                )
-                .await?;
-                Some(quote)
-            }
-            OrderClass::Limit => {
-                let quote = get_quote_and_check_fee(
-                    &*self.quoter,
-                    &quote_parameters,
-                    order.quote_id,
-                    None,
-                    self.market_orders_deprecation_date,
-                )
-                .await?;
-                Some(quote)
-            }
-            OrderClass::Liquidity => None,
-        };
 
         let min_balance = minimum_balance(&data).ok_or(ValidationError::SellAmountOverflow)?;
 
@@ -668,17 +636,26 @@ impl OrderValidating for OrderValidator {
             },
         }
 
-        tracing::debug!(
-            ?uid,
-            ?order,
-            ?quote,
-            "checking if order is outside market price"
-        );
         // 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
         // <https://github.com/cowprotocol/services/pull/301>.
-        let class = match (class, &quote) {
-            (OrderClass::Market, Some(quote))
+        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,
+                    &quote_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,
@@ -690,16 +667,42 @@ impl OrderValidating for OrderValidator {
                         buy: quote.buy_amount,
                         fee: quote.fee_amount,
                     },
-                ) =>
-            {
-                tracing::debug!(%uid, ?owner, ?class, "order being flagged as outside market price");
-                OrderClass::Liquidity
+                ) {
+                    tracing::debug!(%uid, ?owner, ?class, "order being flagged as outside market price");
+                    (OrderClass::Liquidity, Some(quote))
+                } else {
+                    (class, Some(quote))
+                }
             }
-            (_, _) => class,
+            OrderClass::Limit => {
+                let quote = get_quote_and_check_fee(
+                    &*self.quoter,
+                    &quote_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 => (class, None),
         };
 
-        self.check_max_limit_orders(owner, &class).await?;
-
         let order = Order {
             metadata: OrderMetadata {
                 owner,
@@ -1424,9 +1427,15 @@ mod tests {
         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(|_, _| Ok(Default::default()));
+        order_quoter.expect_find_quote().returning(|_, _| {
+            Ok(Quote {
+                id: None,
+                data: Default::default(),
+                sell_amount: U256::from(1),
+                buy_amount: U256::from(1),
+                fee_amount: Default::default(),
+            })
+        });
         bad_token_detector
             .expect_detect()
             .returning(|_| Ok(TokenQuality::Good));
@@ -1449,7 +1458,7 @@ mod tests {
 
         let validator = OrderValidator::new(
             dummy_contract!(WETH9, [0xef; 20]),
-            Arc::new(order_validation::banned::Users::none()),
+            hashset!(),
             OrderValidPeriodConfiguration::any(),
             false,
             Arc::new(bad_token_detector),
@@ -1461,14 +1470,13 @@ mod tests {
             MAX_LIMIT_ORDERS_PER_USER,
             Arc::new(MockCodeFetching::new()),
             Default::default(),
-            None,
         );
 
         let creation = OrderCreation {
             valid_to: model::time::now_in_epoch_seconds() + 2,
             sell_token: H160::from_low_u64_be(1),
             buy_token: H160::from_low_u64_be(2),
-            buy_amount: U256::from(1),
+            buy_amount: U256::from(10),
             sell_amount: U256::from(1),
             signature: Signature::Eip712(EcdsaSignature::non_zero()),
             app_data: OrderCreationAppData::Full {
@@ -1490,6 +1498,74 @@ mod tests {
         );
     }
 
+    #[tokio::test]
+    async fn post_limit_does_not_apply_to_in_market_orders() {
+        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(|_, _| Ok(Default::default()));
+        bad_token_detector
+            .expect_detect()
+            .returning(|_| Ok(TokenQuality::Good));
+        balance_fetcher
+            .expect_can_transfer()
+            .returning(|_, _| Ok(()));
+
+        let mut signature_validating = MockSignatureValidating::new();
+        signature_validating
+            .expect_validate_signature_and_get_additional_gas()
+            .never();
+        let signature_validating = Arc::new(signature_validating);
+
+        const MAX_LIMIT_ORDERS_PER_USER: u64 = 2;
+
+        let mut limit_order_counter = MockLimitOrderCounting::new();
+        limit_order_counter
+            .expect_count()
+            .returning(|_| Ok(MAX_LIMIT_ORDERS_PER_USER));
+
+        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),
+            signature_validating,
+            Arc::new(limit_order_counter),
+            MAX_LIMIT_ORDERS_PER_USER,
+            Arc::new(MockCodeFetching::new()),
+            Default::default(),
+            None,
+        );
+
+        let creation = OrderCreation {
+            valid_to: model::time::now_in_epoch_seconds() + 2,
+            sell_token: H160::from_low_u64_be(1),
+            buy_token: H160::from_low_u64_be(2),
+            buy_amount: U256::from(1),
+            sell_amount: U256::from(1),
+            signature: Signature::Eip712(EcdsaSignature::non_zero()),
+            app_data: OrderCreationAppData::Full {
+                full: "{}".to_string(),
+            },
+            ..Default::default()
+        };
+        assert!(validator
+            .validate_and_construct_order(
+                creation.clone(),
+                &Default::default(),
+                Default::default(),
+                None,
+            )
+            .await
+            .is_ok());
+    }
+
     #[tokio::test]
     async fn post_validate_err_zero_amount() {
         let mut order_quoter = MockOrderQuoting::new();