Skip to content

Commit

Permalink
Do not count "in-market" orders for the order limit #2433
Browse files Browse the repository at this point in the history
  • Loading branch information
@m-lord-renkse
m-lord-renkse committed Mar 1, 2024
1 parent 1d0d6cf commit 6dc40eb
Show file tree
Hide file tree
Showing 2 changed files with 160 additions and 39 deletions.
92 changes: 92 additions & 0 deletions crates/e2e/tests/e2e/limit_orders.rs
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
use {
contracts::ERC20,
driver::domain::eth::NonZeroU256,
e2e::{nodes::forked_node::ForkedNodeApi, setup::*, tx},
ethcontract::{prelude::U256, H160},
model::{
Expand Down Expand Up @@ -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() {
Expand Down Expand Up @@ -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<_>>();
Expand Down
107 changes: 68 additions & 39 deletions crates/shared/src/order_validation.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -345,20 +345,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(())
}
Expand Down Expand Up @@ -582,22 +576,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)
.await?;
Some(quote)
}
OrderClass::Limit => {
let quote =
get_quote_and_check_fee(&*self.quoter, &quote_parameters, order.quote_id, None)
.await?;
Some(quote)
}
OrderClass::Liquidity => None,
};

let min_balance = minimum_balance(&data).ok_or(ValidationError::SellAmountOverflow)?;

Expand Down Expand Up @@ -647,6 +625,22 @@ impl OrderValidating for OrderValidator {
},
}

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)
.await?;
Some(quote)
}
OrderClass::Limit => {
let quote =
get_quote_and_check_fee(&*self.quoter, &quote_parameters, order.quote_id, None)
.await?;
Some(quote)
}
OrderClass::Liquidity => None,
};
tracing::debug!(
?uid,
?order,
Expand All @@ -656,8 +650,22 @@ 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
// <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),
)
.await?;
tracing::debug!(
?uid,
?order,
?quote,
"checking if order is outside market price"
);
if is_order_outside_market_price(
&Amounts {
sell: data.sell_amount,
Expand All @@ -669,16 +677,37 @@ 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))
}
}
OrderClass::Limit => {
let quote =
get_quote_and_check_fee(&*self.quoter, &quote_parameters, order.quote_id, None)
.await?;
// If the order is "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))
}
(_, _) => class,
OrderClass::Liquidity => (class, None),
};

self.check_max_limit_orders(owner, &class).await?;

let order = Order {
metadata: OrderMetadata {
owner,
Expand Down

0 comments on commit 6dc40eb

Please sign in to comment.