Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added QuoteMetadata to /orders/uid request #3222

Open
wants to merge 25 commits into
base: main
Choose a base branch
from
Open

Added QuoteMetadata to /orders/uid request #3222

Show file tree
Hide file tree
Changes from 20 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
1ccbf34
Implementation
mstrug Jan 8, 2025
e6206ca
Moved quote metadata to order metadata
mstrug Jan 16, 2025
c7efc2d
Merge branch 'main' into api/quote-metadata-in-orders
mstrug Jan 16, 2025
a843053
Updated tests
mstrug Jan 17, 2025
10171e0
Merge branch 'main' into api/quote-metadata-in-orders
mstrug Jan 17, 2025
8808793
Merge branch 'main' into api/quote-metadata-in-orders
mstrug Jan 22, 2025
a7d3844
Changed QuoteMetadata type to json value
mstrug Jan 23, 2025
6b3840e
Fixed returning jit orders by uid
mstrug Jan 23, 2025
2f7915e
Small refactoring
mstrug Jan 23, 2025
c028e13
Merge branch 'main' into api/quote-metadata-in-orders
mstrug Feb 4, 2025
4094f95
New implementation
mstrug Feb 7, 2025
09f8f65
Merge branch 'main' into api/quote-metadata-in-orders
mstrug Feb 7, 2025
7424988
Fixed compilation
mstrug Feb 11, 2025
603401a
Removed OrderWithQuote
mstrug Feb 14, 2025
2c511f4
Merge branch 'main' into api/quote-metadata-in-orders
mstrug Feb 14, 2025
0e93b31
Update
mstrug Feb 17, 2025
89928c0
Moved function used only in tests
mstrug Feb 17, 2025
dc548ad
Fixed clippy warning
mstrug Feb 17, 2025
61de451
Removed set_order_quote() function
mstrug Feb 17, 2025
6930802
Renamed function
mstrug Feb 18, 2025
baeeed7
Replaced function in tests
mstrug Feb 18, 2025
77bdeec
Small refactoring
mstrug Feb 18, 2025
81c3029
Readability improvement
mstrug Feb 19, 2025
12e8c87
Merge branch 'main' into api/quote-metadata-in-orders
mstrug Feb 19, 2025
126a670
Update crates/model/src/order.rs
mstrug Feb 20, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 25 additions & 25 deletions crates/database/src/orders.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -574,19 +574,6 @@ COALESCE((SELECT executed_fee_token FROM order_execution oe WHERE oe.order_uid =

pub const FROM: &str = "orders o";

pub async fn single_full_order(
ex: &mut PgConnection,
uid: &OrderUid,
) -> Result<Option<FullOrder>, sqlx::Error> {
#[rustfmt::skip]
const QUERY: &str = const_format::concatcp!(
"SELECT ", SELECT,
" FROM ", FROM,
" WHERE o.uid = $1 ",
);
sqlx::query_as(QUERY).bind(uid).fetch_optional(ex).await
}

pub async fn single_full_order_with_quote(
ex: &mut PgConnection,
uid: &OrderUid,
Expand Down Expand Up @@ -847,6 +834,19 @@ mod tests {
.await
}

async fn single_full_order_without_quote(
ex: &mut PgConnection,
uid: &OrderUid,
) -> Result<Option<FullOrder>, sqlx::Error> {
#[rustfmt::skip]
const QUERY: &str = const_format::concatcp!(
"SELECT ", SELECT,
" FROM ", FROM,
" WHERE o.uid = $1 ",
);
sqlx::query_as(QUERY).bind(uid).fetch_optional(ex).await
}

#[tokio::test]
#[ignore]
async fn postgres_order_roundtrip() {
Expand All @@ -861,7 +861,7 @@ mod tests {
let order_ = read_order(&mut db, &order.uid).await.unwrap().unwrap();
assert_eq!(order, order_);

let full_order = single_full_order(&mut db, &order.uid)
let full_order = single_full_order_without_quote(&mut db, &order.uid)
.await
.unwrap()
.unwrap();
Expand Down Expand Up @@ -923,7 +923,7 @@ mod tests {
.await
.unwrap();
insert_order(&mut db, &order).await.unwrap();
let order_ = single_full_order(&mut db, &order.uid)
let order_ = single_full_order_without_quote(&mut db, &order.uid)
.await
.unwrap()
.unwrap();
Expand Down Expand Up @@ -958,7 +958,7 @@ mod tests {
)
.await
.unwrap();
let order_ = single_full_order(&mut db, &order.uid)
let order_ = single_full_order_without_quote(&mut db, &order.uid)
.await
.unwrap()
.unwrap();
Expand Down Expand Up @@ -994,7 +994,7 @@ mod tests {
insert_or_overwrite_interaction(&mut db, &post_interaction_1, &order.uid)
.await
.unwrap();
let order_ = single_full_order(&mut db, &order.uid)
let order_ = single_full_order_without_quote(&mut db, &order.uid)
.await
.unwrap()
.unwrap();
Expand Down Expand Up @@ -1084,7 +1084,7 @@ mod tests {
insert_or_overwrite_interaction(&mut db, &pre_interaction_1, &order.uid)
.await
.unwrap();
let order_ = single_full_order(&mut db, &order.uid)
let order_ = single_full_order_without_quote(&mut db, &order.uid)
.await
.unwrap()
.unwrap();
Expand Down Expand Up @@ -1458,7 +1458,7 @@ mod tests {
.unwrap();
}

let order = single_full_order(&mut db, &order.uid)
let order = single_full_order_without_quote(&mut db, &order.uid)
.await
.unwrap()
.unwrap();
Expand Down Expand Up @@ -1558,15 +1558,15 @@ mod tests {
..Default::default()
};
insert_order(&mut db, &order).await.unwrap();
let result = single_full_order(&mut db, &order.uid)
let result = single_full_order_without_quote(&mut db, &order.uid)
.await
.unwrap()
.unwrap();
assert!(!result.invalidated);
insert_onchain_invalidation(&mut db, &EventIndex::default(), &order.uid)
.await
.unwrap();
let result = single_full_order(&mut db, &order.uid)
let result = single_full_order_without_quote(&mut db, &order.uid)
.await
.unwrap()
.unwrap();
Expand Down Expand Up @@ -1907,7 +1907,7 @@ mod tests {
.unwrap();

let fee: BigDecimal = 0.into();
let order = single_full_order(&mut db, &order_uid)
let order = single_full_order_without_quote(&mut db, &order_uid)
.await
.unwrap()
.unwrap();
Expand All @@ -1928,7 +1928,7 @@ mod tests {
.await
.unwrap();

let order = single_full_order(&mut db, &order_uid)
let order = single_full_order_without_quote(&mut db, &order_uid)
.await
.unwrap()
.unwrap();
Expand All @@ -1946,7 +1946,7 @@ mod tests {
..Default::default()
};
insert_order(&mut db, &order).await.unwrap();
let full_order = single_full_order(&mut db, &order.uid)
let full_order = single_full_order_without_quote(&mut db, &order.uid)
.await
.unwrap()
.unwrap();
Expand All @@ -1955,7 +1955,7 @@ mod tests {
crate::app_data::insert(&mut db, &order.app_data, &full_app_data)
.await
.unwrap();
let full_order = single_full_order(&mut db, &order.uid)
let full_order = single_full_order_without_quote(&mut db, &order.uid)
.await
.unwrap()
.unwrap();
Expand Down
2 changes: 1 addition & 1 deletion crates/model/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,12 +22,12 @@ num = { workspace = true }
primitive-types = { workspace = true }
secp256k1 = { workspace = true }
serde = { workspace = true }
serde_json = { workspace = true }
serde_with = { workspace = true }
strum = { workspace = true }
web3 = { workspace = true, features = ["signing"] }

[dev-dependencies]
serde_json = { workspace = true }
maplit = { workspace = true }
testlib = { path = "../testlib" }

Expand Down
25 changes: 25 additions & 0 deletions crates/model/src/order.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ use {
},
anyhow::{anyhow, Result},
app_data::{hash_full_app_data, AppDataHash},
bigdecimal::BigDecimal,
chrono::{offset::Utc, DateTime},
derive_more::Debug as DeriveDebug,
hex_literal::hex,
Expand Down Expand Up @@ -691,6 +692,10 @@ pub struct OrderMetadata {
/// Full app data that `OrderData::app_data` is a hash of. Can be None if
/// the backend doesn't know about the full app data.
pub full_app_data: Option<String>,
/// If the order was created with a quote, then this field contains that
/// quote informations for reference.
#[serde(default, skip_serializing_if = "Option::is_none")]
pub quote: Option<OrderQuote>,
}

// uid as 56 bytes: 32 for orderDigest, 20 for ownerAddress and 4 for validTo
Expand Down Expand Up @@ -964,6 +969,26 @@ impl BuyTokenDestination {
}
}

/// A quote from which particular order was created.
#[serde_as]
#[derive(Eq, PartialEq, Clone, Default, Deserialize, Serialize, DeriveDebug)]
#[serde(rename_all = "camelCase")]
pub struct OrderQuote {
#[serde_as(as = "DisplayFromStr")]
pub gas_amount: BigDecimal,
#[serde_as(as = "DisplayFromStr")]
pub gas_price: BigDecimal,
#[serde_as(as = "DisplayFromStr")]
pub sell_token_price: BigDecimal,
#[serde_as(as = "HexOrDecimalU256")]
pub sell_amount: U256,
#[serde_as(as = "HexOrDecimalU256")]
pub buy_amount: U256,
pub solver: H160,
pub verified: bool,
pub metadata: serde_json::Value,
}

#[cfg(test)]
mod tests {
use {
Expand Down
118 changes: 46 additions & 72 deletions crates/orderbook/src/database/orders.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use {
super::Postgres,
crate::{dto::TokenMetadata, orderbook::AddOrderError},
crate::dto::TokenMetadata,
anyhow::{Context as _, Result},
app_data::AppDataHash,
async_trait::async_trait,
Expand Down Expand Up @@ -40,6 +40,7 @@ use {
order_class_into,
order_kind_from,
order_kind_into,
order_quote_into_model,
sell_token_source_from,
sell_token_source_into,
signing_scheme_from,
Expand Down Expand Up @@ -67,7 +68,6 @@ pub trait OrderStoring: Send + Sync {
new_quote: Option<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>>;
/// All orders of a single user ordered by creation date descending (newest
/// orders first).
async fn user_orders(
Expand All @@ -77,39 +77,7 @@ pub trait OrderStoring: Send + Sync {
limit: Option<u64>,
) -> Result<Vec<Order>>;
async fn latest_order_event(&self, order_uid: &OrderUid) -> Result<Option<OrderEvent>>;
async fn single_order_with_quote(&self, uid: &OrderUid) -> Result<Option<OrderWithQuote>>;
}

pub struct OrderWithQuote {
pub order: Order,
pub quote: Option<orders::Quote>,
}

impl OrderWithQuote {
pub fn try_new(order: Order, quote: Option<Quote>) -> Result<Self, AddOrderError> {
Ok(Self {
quote: quote
.map(|quote| {
Ok::<database::orders::Quote, AddOrderError>(orders::Quote {
order_uid: ByteArray(order.metadata.uid.0),
gas_amount: quote.data.fee_parameters.gas_amount,
gas_price: quote.data.fee_parameters.gas_price,
sell_token_price: quote.data.fee_parameters.sell_token_price,
sell_amount: u256_to_big_decimal(&quote.sell_amount),
buy_amount: u256_to_big_decimal(&quote.buy_amount),
solver: ByteArray(quote.data.solver.0),
verified: quote.data.verified,
metadata: quote
.data
.metadata
.try_into()
.map_err(AddOrderError::MetadataSerializationFailed)?,
})
})
.transpose()?,
order,
})
}
async fn single_order(&self, uid: &OrderUid) -> Result<Option<Order>>;
}

#[derive(Debug)]
Expand Down Expand Up @@ -348,26 +316,9 @@ impl OrderStoring for Postgres {
.start_timer();

let mut ex = self.pool.acquire().await?;
let order = match database::orders::single_full_order(&mut ex, &ByteArray(uid.0)).await? {
Some(order) => Some(order),
None => {
// try to find the order in the JIT orders table
database::jit_orders::get_by_id(&mut ex, &ByteArray(uid.0)).await?
}
};
order.map(full_order_into_model_order).transpose()
}

async fn single_order_with_quote(&self, uid: &OrderUid) -> Result<Option<OrderWithQuote>> {
let _timer = super::Metrics::get()
.database_queries
.with_label_values(&["single_order_with_quote"])
.start_timer();

let mut ex = self.pool.acquire().await?;
let order = orders::single_full_order_with_quote(&mut ex, &ByteArray(uid.0)).await?;
order
.map(|order_with_quote| {
let order = match orders::single_full_order_with_quote(&mut ex, &ByteArray(uid.0)).await? {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could have an early return to None in order to avoid one indexation level.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure where we can do the early return, because for the None match we are checking jit orders.

Copy link
Contributor

@squadgazzz squadgazzz Feb 18, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

orders::single_full_order_with_quote(&mut ex, &ByteArray(uid.0))
    .await?
    .or_else(|| async {
        database::jit_orders::get_by_id(&mut ex, &ByteArray(uid.0))
            .await?
            .map(full_order_into_model_order)
    })
    .transpose()

Or

if let Some(order) = orders::single_full_order_with_quote(&mut ex, &ByteArray(uid.0)).await? {
  return Ok(Some(order))
}

database::jit_orders::get_by_id(&mut ex, &ByteArray(uid.0))
  .await?
  .map(full_order_into_model_order)
  .transpose()
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @squadgazzz ! exactly I meant that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I get what you meant. Refactored.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not much changed tbh :)
The main benefit of an early return is reducing indentation, making the code easier to read.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So improved the code readability by moving the Quote match to separate function, but in my opinion I don't see here an issue with the indentation.

Some(order_with_quote) => {
let quote = match (
order_with_quote.quote_buy_amount,
order_with_quote.quote_sell_amount,
Expand Down Expand Up @@ -401,12 +352,22 @@ impl OrderStoring for Postgres {
_ => None,
};

Ok(OrderWithQuote {
order: full_order_into_model_order(order_with_quote.full_order)?,
quote,
})
})
.transpose()
let order = full_order_with_quote_into_model_order(
order_with_quote.full_order,
quote.as_ref(),
)?;
Some(order)
}
None => {
// try to find the order in the JIT orders table
database::jit_orders::get_by_id(&mut ex, &ByteArray(uid.0))
.await?
.map(full_order_into_model_order)
.transpose()?
}
};

Ok(order)
}

async fn orders_for_tx(&self, tx_hash: &H256) -> Result<Vec<Order>> {
Expand Down Expand Up @@ -608,6 +569,14 @@ fn calculate_status(order: &FullOrder) -> OrderStatus {
}

fn full_order_into_model_order(order: FullOrder) -> Result<Order> {
full_order_with_quote_into_model_order(order, None)
}

/// If quote is provided, then it is used to extract quote metadata field value.
fn full_order_with_quote_into_model_order(
order: FullOrder,
quote: Option<&orders::Quote>,
) -> Result<Order> {
let status = calculate_status(&order);
let pre_interactions = extract_interactions(&order, database::orders::ExecutionTime::Pre)?;
let post_interactions = extract_interactions(&order, database::orders::ExecutionTime::Post)?;
Expand Down Expand Up @@ -660,6 +629,7 @@ fn full_order_into_model_order(order: FullOrder) -> Result<Order> {
.map(String::from_utf8)
.transpose()
.context("full app data isn't utf-8")?,
quote: quote.map(order_quote_into_model).transpose()?,
};
let data = OrderData {
sell_token: H160(order.sell_token.0),
Expand Down Expand Up @@ -1218,16 +1188,14 @@ mod tests {
assert_eq!(interactions, order.interactions);

// Test `single_order_with_quote`
let single_order_with_quote = db.single_order_with_quote(&uid).await.unwrap().unwrap();
assert_eq!(single_order_with_quote.order, order);
assert_eq!(
single_order_with_quote.quote.clone().unwrap().sell_amount,
u256_to_big_decimal(&quote.sell_amount)
);
let mut single_order_with_quote = db.single_order(&uid).await.unwrap().unwrap();

assert_eq!(
single_order_with_quote.quote.unwrap().buy_amount,
u256_to_big_decimal(&quote.buy_amount)
single_order_with_quote.metadata.quote,
Some(quote.try_to_model_order_quote().unwrap())
);
single_order_with_quote.metadata.quote = None; // already compared, now compary orders
assert_eq!(single_order_with_quote, order);
}

#[tokio::test]
Expand Down Expand Up @@ -1280,10 +1248,16 @@ mod tests {
},
..Default::default()
};
db.insert_order(&order, Some(quote)).await.unwrap();
db.insert_order(&order, Some(quote.clone())).await.unwrap();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the db.insert_order() signature is now quite weird to me. The order struct now contains quote metadata but we still pass an optional quote into it. This immediately begs the question what is supposed to happen when order.metadata.quote is different from the passed in quote?
Can this API be updated to not have this conflict anymore?
I suspect the underlying problem is probably that we are using the Order and OrderMetadata struct in too many places so it currently wears a lot of different hats which don't quite work in all places. 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The main issue is that shared::order_quoting::Quote returned by validate_and_construct_order() can be converted to model::order::OrderQuote (order.metadata.quote field) but cannot be converted back (for storing in db) because of lack of some fields and I'm not sure if it is sensible to add all of the missing fields (mainly quote id which will be irrelevant after clearing quotes table, but also most of the QuoteData fields).

Also order.metadata.quote is created from the quote so it always has the same values.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also order.metadata.quote is created from the quote so it always has the same values.

That doesn't really help to reduce currently created confusion where we operate with 2 quotes related to the same order but in different representations.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still thinking about how this part can be improved:

services/crates/orderbook/src/database/orders.rs

Lines 240 to 242 in 81c3029

if let Some(quote) = quote {
insert_quote(&order.metadata.uid, &quote, &mut ex).await?;
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The main issue is that shared::order_quoting::Quote returned by validate_and_construct_order() can be converted to model::order::OrderQuote (order.metadata.quote field) but cannot be converted back

I'd say the main issue is that we pass a ton of data into the DB call that is actually not needed. I believe if we only pass the necessary data (signed order data + a few additional data) we'd not have the issue of passing 2 possibly conflicting Quote structs into the same call.


let mut single_order_with_quote = db.single_order(&uid).await.unwrap().unwrap();

let single_order_with_quote = db.single_order_with_quote(&uid).await.unwrap().unwrap();
assert_eq!(single_order_with_quote.order, order);
assert!(single_order_with_quote.quote.unwrap().verified);
assert_eq!(
single_order_with_quote.metadata.quote,
Some(quote.try_to_model_order_quote().unwrap())
);
assert!(single_order_with_quote.metadata.quote.unwrap().verified);
single_order_with_quote.metadata.quote = None; // already compared, now compary orders
assert_eq!(single_order_with_quote, order);
}
}
Loading
Loading