Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
93 changes: 91 additions & 2 deletions crates/database/src/trades.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@ pub fn trades<'a>(
ex: &'a mut PgConnection,
owner_filter: Option<&'a Address>,
order_uid_filter: Option<&'a OrderUid>,
offset: i64,
limit: i64,
) -> instrument::Instrumented<BoxStream<'a, Result<TradesQueryRow, sqlx::Error>>> {
const COMMON_QUERY: &str = r#"
SELECT
Expand All @@ -49,27 +51,44 @@ LEFT OUTER JOIN LATERAL (
) AS settlement ON true"#;

const QUERY: &str = const_format::concatcp!(
"(",
COMMON_QUERY,
" JOIN orders o ON o.uid = t.order_uid",
" WHERE ($1 IS NULL OR o.owner = $1)",
" AND ($2 IS NULL OR o.uid = $2)",
" ORDER BY t.block_number DESC, t.log_index DESC",
" LIMIT $3 + $4",
Copy link
Contributor

@fafk fafk Dec 4, 2025

Choose a reason for hiding this comment

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

This query is super confusing.

Can we do

SELECT
    t.block_number,
    t.log_index,
    t.order_uid,
    t.buy_amount,
    t.sell_amount,
    t.sell_amount - t.fee_amount AS sell_amount_before_fees,
    COALESCE(o.owner, jit_o.owner) AS owner,
    COALESCE(o.buy_token, jit_o.buy_token) AS buy_token,
    COALESCE(o.sell_token, jit_o.sell_token) AS sell_token,
    settlement.tx_hash,
    settlement.auction_id
FROM trades t
LEFT JOIN LATERAL (
    SELECT tx_hash, auction_id
    FROM settlements s
    WHERE s.block_number = t.block_number
      AND s.log_index > t.log_index
    ORDER BY s.log_index ASC
    LIMIT 1
) AS settlement ON true
LEFT JOIN orders o
    ON o.uid = t.order_uid
LEFT JOIN onchain_placed_orders onchain_o
    ON onchain_o.uid = t.order_uid
LEFT JOIN jit_orders jit_o
    ON jit_o.uid = t.order_uid
WHERE
    (
        $1 IS NULL
        OR o.owner = $1
        OR onchain_o.sender = $1
        OR jit_o.owner = $1
    )
  AND ($2 IS NULL OR t.order_uid = $2)
ORDER BY t.block_number DESC, t.log_index DESC
LIMIT $3 OFFSET $4;

instead all the unions and the overfetching of $3 + $4?

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'd experiment with this in a separate PR, since your version confuses me much more 😅

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that it would be best to be able to see the query as a whole, in order to reason about its correctness easier.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd experiment with this in a separate PR, since your version confuses me much more 😅

I found 2 things confusing:

  1. I had to write a test to output the final string to make things easier on me to review
  2. The $3 + $4 threw me off (as it did copilot hehe), but an unusual pattern that you up to the offset and then the limit and then discard everything up to the offset in the outer layer 😅

The query I propose gets rid off the concatenations and just fetches everything at once so there is no need to "overfetch and discard". Anyway I think your SQL works, so I am approving and leaving it to your discretion if you wanna follow up with some refactoring. 🙌

Copy link
Contributor Author

@squadgazzz squadgazzz Dec 4, 2025

Choose a reason for hiding this comment

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

The $3 + $4 threw me off

We have the same approach in the get_user_orders query.

const QUERY: &str = const_format::concatcp!(
"(SELECT ", orders::SELECT,
" FROM ", orders::FROM,
" LEFT OUTER JOIN onchain_placed_orders onchain_o on onchain_o.uid = o.uid",
" WHERE o.owner = $1",
" ORDER BY creation_timestamp DESC LIMIT $2 + $3 ) ",
" UNION ",
" (SELECT ", orders::SELECT,
" FROM ", orders::FROM,
" LEFT OUTER JOIN onchain_placed_orders onchain_o on onchain_o.uid = o.uid",
" WHERE onchain_o.sender = $1 ",
" ORDER BY creation_timestamp DESC LIMIT $2 + $3 ) ",
" UNION ",
" (SELECT ", jit_orders::SELECT,
" FROM ", jit_orders::FROM,
" WHERE o.owner = $1 AND NOT EXISTS (SELECT 1 FROM orders ord WHERE o.uid = ord.uid)",
" ORDER BY creation_timestamp DESC LIMIT $2 + $3 ) ",
" ORDER BY creation_timestamp DESC ",
" LIMIT $2 ",
" OFFSET $3 ",
);

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think it legitimizes the pattern. 😅

")",
" UNION ",
"(",
COMMON_QUERY,
" JOIN orders o ON o.uid = t.order_uid",
" LEFT OUTER JOIN onchain_placed_orders onchain_o",
" ON onchain_o.uid = t.order_uid",
" WHERE onchain_o.sender = $1",
" AND ($2 IS NULL OR o.uid = $2)",
" ORDER BY t.block_number DESC, t.log_index DESC",
" LIMIT $3 + $4",
")",
" UNION ",
"(",
COMMON_QUERY,
" JOIN jit_orders o ON o.uid = t.order_uid",
" WHERE ($1 IS NULL OR o.owner = $1)",
" AND ($2 IS NULL OR o.uid = $2)",
" ORDER BY t.block_number DESC, t.log_index DESC",
" LIMIT $3 + $4",
")",
" ORDER BY block_number DESC, log_index DESC",
" LIMIT $3",
" OFFSET $4",
);

sqlx::query_as(QUERY)
.bind(owner_filter)
.bind(order_uid_filter)
.bind(limit)
.bind(offset)
.fetch(ex)
.instrument(info_span!("trades"))
}
Expand Down Expand Up @@ -213,7 +232,8 @@ mod tests {
order_uid_filter: Option<&OrderUid>,
expected: &[TradesQueryRow],
) {
let mut filtered = trades(db, owner_filter, order_uid_filter)
// Use large limit to get all trades
let mut filtered = trades(db, owner_filter, order_uid_filter, 0, 1000)
.into_inner()
.try_collect::<Vec<_>>()
.await
Expand Down Expand Up @@ -287,7 +307,7 @@ mod tests {
}

let now = std::time::Instant::now();
trades(&mut db, Some(&ByteArray([2u8; 20])), None)
trades(&mut db, Some(&ByteArray([2u8; 20])), None, 0, 100)
.into_inner()
.try_collect::<Vec<_>>()
.await
Expand Down Expand Up @@ -655,4 +675,73 @@ mod tests {
Some(123)
);
}

#[tokio::test]
#[ignore]
async fn postgres_trades_pagination() {
let mut db = PgConnection::connect("postgresql://").await.unwrap();
let mut db = db.begin().await.unwrap();
crate::clear_DANGER_(&mut db).await.unwrap();

// Create 5 trades with the same owner
let (owners, order_ids) = generate_owners_and_order_ids(1, 5).await;
let owner = owners[0];

let mut expected_trades = Vec::new();
for (i, order_id) in order_ids.iter().enumerate() {
let trade = add_order_and_trade(
&mut db,
owner,
*order_id,
EventIndex {
block_number: i.try_into().unwrap(),
log_index: 0,
},
None,
None,
)
.await;
expected_trades.push(trade);
}

// Sort expected trades by block_number DESC (matching query ORDER BY)
expected_trades.sort_by(|a, b| b.block_number.cmp(&a.block_number));

// Test limit: get first 2 trades (blocks 4 and 3 in DESC order)
let result = trades(&mut db, Some(&owner), None, 0, 2)
.into_inner()
.try_collect::<Vec<_>>()
.await
.unwrap();
assert_eq!(result.len(), 2);
assert_eq!(result[0], expected_trades[0]); // block 4
assert_eq!(result[1], expected_trades[1]); // block 3

// Test offset: skip first 2, get next 2 (blocks 2 and 1 in DESC order)
let result = trades(&mut db, Some(&owner), None, 2, 2)
.into_inner()
.try_collect::<Vec<_>>()
.await
.unwrap();
assert_eq!(result.len(), 2);
assert_eq!(result[0], expected_trades[2]); // block 2
assert_eq!(result[1], expected_trades[3]); // block 1

// Test offset beyond available trades
let result = trades(&mut db, Some(&owner), None, 10, 2)
.into_inner()
.try_collect::<Vec<_>>()
.await
.unwrap();
assert_eq!(result.len(), 0);

// Test large limit returns all available trades in DESC order
let result = trades(&mut db, Some(&owner), None, 0, 100)
.into_inner()
.try_collect::<Vec<_>>()
.await
.unwrap();
assert_eq!(result.len(), 5);
assert_eq!(result, expected_trades);
}
}
66 changes: 65 additions & 1 deletion crates/orderbook/openapi.yml
Original file line number Diff line number Diff line change
Expand Up @@ -210,9 +210,16 @@ paths:
/api/v1/trades:
get:
operationId: getTrades
summary: Get existing trades.
summary: Get existing trades (unpaginated).
deprecated: true
description: |
**Deprecated:** This endpoint is deprecated and will be removed in the future. Please use `/api/v2/trades` instead, which provides pagination support.

Exactly one of `owner` or `orderUid` must be set.

Results are sorted by block number and log index descending (newest trades first).

**Note:** This endpoint returns all matching trades without pagination. For paginated results, use `/api/v2/trades`.
parameters:
- name: owner
in: query
Expand All @@ -233,6 +240,63 @@ paths:

### If `orderUid` is specified:

Return all trades related to that `orderUid`. Given that an order
may be partially fillable, it is possible that an individual order
may have *multiple* trades.
content:
application/json:
schema:
type: array
items:
$ref: "#/components/schemas/Trade"
/api/v2/trades:
get:
operationId: getTradesV2
summary: Get existing trades (paginated).
description: |
Exactly one of `owner` or `orderUid` must be set.

Results are paginated and sorted by block number and log index descending (newest trades first).

To enumerate all trades start with `offset` 0 and keep increasing the
`offset` by the total number of returned results. When a response
contains less than `limit` the last page has been reached.
parameters:
- name: owner
in: query
schema:
$ref: "#/components/schemas/Address"
required: false
- name: orderUid
in: query
schema:
$ref: "#/components/schemas/UID"
required: false
- name: offset
in: query
description: |
The pagination offset. Defaults to 0.
schema:
type: integer
required: false
- name: limit
in: query
description: |
The maximum number of trades to return. Defaults to 10. Must be between 1 and 1000.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is quite the breaking change, no?
Anyone using the existing endpoint will suddenly get a lot fewer orders. It should be pretty straight forward to introduce a v2 endpoint for the pagination while keeping the old one the same.
We could also immediately adjust the v1 description to mark it as deprecated.

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, makes sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the PR.

schema:
type: integer
minimum: 1
maximum: 1000
required: false
responses:
"200":
description: |-
### If `owner` is specified:

Return all trades related to that `owner`.

### If `orderUid` is specified:

Return all trades related to that `orderUid`. Given that an order
may be partially fillable, it is possible that an individual order
may have *multiple* trades.
Expand Down
5 changes: 5 additions & 0 deletions crates/orderbook/src/api.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ mod get_solver_competition_v2;
mod get_token_metadata;
mod get_total_surplus;
mod get_trades;
mod get_trades_v2;
mod get_user_orders;
mod post_order;
mod post_quote;
Expand Down Expand Up @@ -69,6 +70,10 @@ pub fn handle_all_routes(
"v1/get_trades",
box_filter(get_trades::get_trades(database_read.clone())),
),
(
"v2/get_trades",
box_filter(get_trades_v2::get_trades(database_read.clone())),
),
(
"v1/cancel_order",
box_filter(cancel_order::cancel_order(orderbook.clone())),
Expand Down
Loading
Loading