Skip to content

Commit

Permalink
Eliminate the order class in post order #2469
Browse files Browse the repository at this point in the history
  • Loading branch information
@m-lord-renkse
m-lord-renkse committed Mar 7, 2024
1 parent b22bbc7 commit c5f119c
Show file tree
Hide file tree
Showing 4 changed files with 51 additions and 144 deletions.
42 changes: 10 additions & 32 deletions crates/orderbook/src/database/orders.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -63,7 +62,7 @@ pub trait OrderStoring: Send + Sync {
&self,
old_order: &OrderUid,
new_order: &Order,
new_quote: Option<Quote>,
new_quote: &Quote,
) -> Result<(), InsertionError>;
async fn orders_for_tx(&self, tx_hash: &H256) -> Result<Vec<Order>>;
async fn single_order(&self, uid: &OrderUid) -> Result<Option<Order>>;
Expand Down Expand Up @@ -272,7 +271,7 @@ impl OrderStoring for Postgres {
&self,
old_order: &model::order::OrderUid,
new_order: &model::order::Order,
new_quote: Option<Quote>,
new_quote: &Quote,
) -> anyhow::Result<(), super::orders::InsertionError> {
let _timer = super::Metrics::get()
.database_queries
Expand All @@ -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| {
Expand All @@ -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, &quote, ex).await?;
}
insert_quote(&new_order.metadata.uid, &new_quote, ex).await?;
Self::insert_order_app_data(&new_order, ex).await?;

Ok(())
}
.boxed()
Expand Down Expand Up @@ -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())
}
Expand Down Expand Up @@ -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, &quote)
.await
.unwrap();

Expand Down Expand Up @@ -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, &quote)
.await
.unwrap_err();
assert!(matches!(err, InsertionError::DuplicatedRecord));
Expand Down
10 changes: 5 additions & 5 deletions crates/orderbook/src/orderbook.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -346,7 +346,7 @@ impl Orderbook {
&self,
validated_new_order: Order,
old_order: Order,
quote: Option<Quote>,
quote: Quote,
) -> Result<(OrderUid, Option<i64>), AddOrderError> {
// Replacement order signatures need to be validated meaning we cannot
// accept `PreSign` orders, otherwise anyone can cancel a user order by
Expand All @@ -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, &quote)
.await
.map_err(|err| AddOrderError::from_insertion(err, &validated_new_order))?;
Metrics::on_order_operation(&old_order, OrderOperation::Cancelled);
Expand Down
3 changes: 1 addition & 2 deletions crates/shared/src/order_quoting.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -543,7 +543,6 @@ 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,
}
}
}
Expand Down
140 changes: 35 additions & 105 deletions crates/shared/src/order_validation.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ pub trait OrderValidating: Send + Sync {
domain_separator: &DomainSeparator,
settlement_contract: H160,
full_app_data_override: Option<String>,
) -> Result<(Order, Option<Quote>), ValidationError>;
) -> Result<(Order, Quote), ValidationError>;
}

#[derive(Debug)]
Expand Down Expand Up @@ -268,7 +268,6 @@ pub struct PreOrderData {
pub buy_token_balance: BuyTokenDestination,
pub sell_token_balance: SellTokenSource,
pub signing_scheme: SigningScheme,
pub class: OrderClass,
}

fn actual_receiver(owner: H160, order: &OrderData) -> H160 {
Expand Down Expand Up @@ -296,10 +295,6 @@ impl PreOrderData {
buy_token_balance: order.buy_token_balance,
sell_token_balance: order.sell_token_balance,
signing_scheme,
class: match order.fee_amount.is_zero() {
true => OrderClass::Limit,
false => OrderClass::Market,
},
}
}
}
Expand Down Expand Up @@ -412,10 +407,6 @@ impl OrderValidating for OrderValidator {
return Err(PartialValidationError::Forbidden);
}

if order.class == OrderClass::Market && order.partially_fillable {
return Err(PartialValidationError::UnsupportedOrderType);
}

if order.buy_token_balance != BuyTokenDestination::Erc20 {
return Err(PartialValidationError::UnsupportedBuyTokenDestination(
order.buy_token_balance,
Expand Down Expand Up @@ -512,7 +503,7 @@ impl OrderValidating for OrderValidator {
domain_separator: &DomainSeparator,
settlement_contract: H160,
full_app_data_override: Option<String>,
) -> Result<(Order, Option<Quote>), 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)?;
Expand Down Expand Up @@ -558,7 +549,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)?;
Expand Down Expand Up @@ -639,93 +629,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
// <https://github.com/cowprotocol/services/pull/301>.
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,
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,
&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 => {
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?;
}
(OrderClass::Limit, None)
}
let fee_amount = (!data.fee_amount.is_zero()).then(|| data.fee_amount);
let quote = get_quote_and_check_fee(
&*self.quoter,
&quote_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 {
Expand Down Expand Up @@ -791,12 +726,7 @@ 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,
}
self.max_limit
}
}

Expand Down

0 comments on commit c5f119c

Please sign in to comment.