Skip to content

feat(api-server, matching-engine): implement markets endpoints#1407

Merged
akirillo merged 1 commit intomainfrom
andrew/markets-endpoints
Feb 26, 2026
Merged

feat(api-server, matching-engine): implement markets endpoints#1407
akirillo merged 1 commit intomainfrom
andrew/markets-endpoints

Conversation

@akirillo
Copy link
Copy Markdown
Contributor

@akirillo akirillo commented Feb 25, 2026

Summary

  • Implement get_liquidity_for_pair in the matching engine to aggregate buy/sell depth across all pools
  • Add Book::total_matchable_amount() helper to sum matchable amounts across all orders in a book
  • Wire up the v2 market depth endpoints (GET /v2/markets/depth, GET /v2/markets/:mint/depth) so they return real data instead of panicking

Test plan

  • cargo build -p matching-engine-core
  • cargo test -p matching-engine-core (47 tests pass, including new liquidity query tests)
  • cargo build -p api-server

TODO:

  • Auth server changes for updating relayer fee rates in response if necessary

@claude
Copy link
Copy Markdown

claude bot commented Feb 25, 2026

Claude finished @akirillo's task in 3m 54s —— View job


Code Review

  • Gather context
  • Read changed files
  • Analyze code
  • Post review

Overall this is clean and well-structured. The approach of using the existing all_pools_book for aggregation is sensible. A few issues to flag:


Issue 1 — total_matchable_amount() includes orders that cannot be filled (medium)

crates/workers/matching-engine/matching-engine-core/src/book.rs:111-113

pub fn total_matchable_amount(&self) -> Amount {
    self.order_map.values().map(|order| order.matchable_amount).sum()
}

Orders where matchable_amount < min_fill_size are unmatchable — find_match correctly rejects them because the range min_fill_size..=matchable_amount is empty when start > end. But total_matchable_amount() counts them anyway, overstating reported market depth. Consider filtering:

pub fn total_matchable_amount(&self) -> Amount {
    self.order_map
        .values()
        .filter(|order| order.matchable_amount >= order.min_fill_size)
        .map(|order| order.matchable_amount)
        .sum()
}

Fix this →


Issue 2 — internal_match_fee_rates and external_match_fee_rates are always identical (medium)

crates/workers/api-server/src/http/market.rs:61-68

let fees = self.get_fee_rates(token)?;
Ok(MarketInfo {
    base,
    quote,
    price,
    internal_match_fee_rates: fees.clone(),
    external_match_fee_rates: fees,   // same value
})

The two fields exist in MarketInfo presumably because they can differ. In the settlement path, external_relayer_fee is set to Default::default() (zero) for Ring1/Ring2 settlements (ring1.rs:223, ring2.rs:352), while internal_relayer_fee uses the actual relayer fee. This suggests the external_match_fee_rates.relayer_fee_rate should be zero (or a different value), not a copy of the internal fee. Worth confirming with the team — if they're intentionally the same today this deserves a comment explaining why.


Issue 3 — test_total_matchable_amount placed under the wrong section header (minor)

crates/workers/matching-engine/matching-engine-core/src/book.rs:443-448

// -------------------------------
// | Self-Match Prevention Tests |  ← wrong
// -------------------------------

#[test]
fn test_total_matchable_amount() { ... }

test_total_matchable_amount is not a self-match prevention test. It should be moved above that section or placed under its own header.


Issue 4 — No test for multi-pool aggregation in get_liquidity_for_pair (minor)

crates/workers/matching-engine/matching-engine-core/src/engine.rs tests

test_get_liquidity_for_pair only uses a single pool. The main value of all_pools_book is aggregating across pools, so there should be a test that inserts sell-side orders into two different pools and verifies both are reflected in get_liquidity_for_pair. As written, the test doesn't distinguish from querying a single-pool book.

Fix this →


Nit — GetMarketsHandler wraps sync work in async closures (minor)

crates/workers/api-server/src/http/market.rs:127-129

let futs = tokens.iter()
    .map(|t| async { self.calculator.get_market_info(t) })
    .collect::<Vec<_>>();
let results = join_all(futs).await;

get_market_info is fully synchronous. join_all over sync-wrapped futures is harmless but runs them sequentially (no real concurrency). It's either a copy-paste from GetMarketDepthsHandler (where get_market_depth is genuinely async) or unnecessary indirection. Could just be a map(...).filter_map(|r| r.ok()).collect() without the async machinery.


@akirillo akirillo force-pushed the andrew/markets-endpoints branch from acc2d3e to b22e10e Compare February 25, 2026 22:40
@claude
Copy link
Copy Markdown

claude bot commented Feb 25, 2026

Claude finished @akirillo's task in 2m 27s —— View job


Code Review

  • Gather context
  • Read changed files
  • Analyze code
  • Post review

All five issues from the previous review have been addressed. Here's a full re-review of the current state.


Previously Flagged Issues — All Resolved ✅

# Issue Status
1 total_matchable_amount() counted unmatchable orders Fixed — filter added, dedicated test added
2 internal_match_fee_rates / external_match_fee_rates always identical Acknowledged — explanatory comment added at market.rs:68-70
3 test_total_matchable_amount under wrong section header Fixed — moved to new // Total Matchable Amount // section
4 No multi-pool test for get_liquidity_for_pair Fixed — test_get_liquidity_for_pair_multi_pool added
Nit GetMarketsHandler unnecessarily wrapping sync work in async Fixed — now a plain filter_map

New Observations

Minor — test_total_matchable_amount_excludes_unfillable comment is inaccurate

crates/workers/matching-engine/matching-engine-core/src/book.rs:480-481

// Order with matchable_amount (100) >= min_fill_size (1) — should be counted
let order1 = create_test_order(100);

The comment claims min_fill_size = 1, but create_test_order in book.rs doesn't set min_fill_size — it inherits whatever mock_order() defaults to. The test is correct as written (it passes), but the comment is misleading. Either set min_fill_size explicitly, or drop the (1) annotation from the comment.


Nit — Float roundtrip for buy-side total_quantity introduces approximation

crates/workers/api-server/src/http/market.rs:87-93

let buy_usd = Token::usdc().convert_to_decimal(buy_amount_quote);
let buy_quantity_base = if market.price.price > 0.0 {
    let base_decimal = buy_usd / market.price.price;
    token.convert_from_decimal(base_decimal)
} else {
    0u128
};

The sell-side total_quantity is the raw u128 directly from the book; the buy-side goes u128 → f64 → f64 / f64 → u128, introducing floating-point rounding. For a display-only endpoint this is acceptable, but it's worth being aware that the two sides have different precision characteristics.


Overall this is clean — the implementation is well-structured, the previous feedback was addressed thoroughly, and the new tests (especially test_get_liquidity_for_pair_multi_pool and test_total_matchable_amount_excludes_unfillable) meaningfully improve coverage.

@akirillo
Copy link
Copy Markdown
Contributor Author

Claude finished @akirillo's task in 3m 54s —— View job

Code Review

  • Gather context
  • Read changed files
  • Analyze code
  • Post review

Overall this is clean and well-structured. The approach of using the existing all_pools_book for aggregation is sensible. A few issues to flag:

Issue 1 — total_matchable_amount() includes orders that cannot be filled (medium)

crates/workers/matching-engine/matching-engine-core/src/book.rs:111-113

pub fn total_matchable_amount(&self) -> Amount {
    self.order_map.values().map(|order| order.matchable_amount).sum()
}

Orders where matchable_amount < min_fill_size are unmatchable — find_match correctly rejects them because the range min_fill_size..=matchable_amount is empty when start > end. But total_matchable_amount() counts them anyway, overstating reported market depth. Consider filtering:

pub fn total_matchable_amount(&self) -> Amount {
    self.order_map
        .values()
        .filter(|order| order.matchable_amount >= order.min_fill_size)
        .map(|order| order.matchable_amount)
        .sum()
}

Fix this →

Issue 2 — internal_match_fee_rates and external_match_fee_rates are always identical (medium)

crates/workers/api-server/src/http/market.rs:61-68

let fees = self.get_fee_rates(token)?;
Ok(MarketInfo {
    base,
    quote,
    price,
    internal_match_fee_rates: fees.clone(),
    external_match_fee_rates: fees,   // same value
})

The two fields exist in MarketInfo presumably because they can differ. In the settlement path, external_relayer_fee is set to Default::default() (zero) for Ring1/Ring2 settlements (ring1.rs:223, ring2.rs:352), while internal_relayer_fee uses the actual relayer fee. This suggests the external_match_fee_rates.relayer_fee_rate should be zero (or a different value), not a copy of the internal fee. Worth confirming with the team — if they're intentionally the same today this deserves a comment explaining why.

Issue 3 — test_total_matchable_amount placed under the wrong section header (minor)

crates/workers/matching-engine/matching-engine-core/src/book.rs:443-448

// -------------------------------
// | Self-Match Prevention Tests |  ← wrong
// -------------------------------

#[test]
fn test_total_matchable_amount() { ... }

test_total_matchable_amount is not a self-match prevention test. It should be moved above that section or placed under its own header.

Issue 4 — No test for multi-pool aggregation in get_liquidity_for_pair (minor)

crates/workers/matching-engine/matching-engine-core/src/engine.rs tests

test_get_liquidity_for_pair only uses a single pool. The main value of all_pools_book is aggregating across pools, so there should be a test that inserts sell-side orders into two different pools and verifies both are reflected in get_liquidity_for_pair. As written, the test doesn't distinguish from querying a single-pool book.

Fix this →

Nit — GetMarketsHandler wraps sync work in async closures (minor)

crates/workers/api-server/src/http/market.rs:127-129

let futs = tokens.iter()
    .map(|t| async { self.calculator.get_market_info(t) })
    .collect::<Vec<_>>();
let results = join_all(futs).await;

get_market_info is fully synchronous. join_all over sync-wrapped futures is harmless but runs them sequentially (no real concurrency). It's either a copy-paste from GetMarketDepthsHandler (where get_market_depth is genuinely async) or unnecessary indirection. Could just be a map(...).filter_map(|r| r.ok()).collect() without the async machinery.

Addressed all issues mentioned here

@akirillo akirillo requested a review from sehyunc February 25, 2026 23:44
Copy link
Copy Markdown
Contributor

@sehyunc sehyunc left a comment

Choose a reason for hiding this comment

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

Cross-repo compatibility risk:

This PR changes GET /v2/markets to add_admin_authenticated_route in crates/workers/api-server/src/http.rs (around lines 277-282).

typescript-sdk still routes getMarkets() through relayerHttpClient without API-key headers in packages/external-match/src/client.ts (551-553), while rust-sdk has already switched to the auth client path.

Please coordinate/land the matching typescript-sdk update (or keep this endpoint unauthenticated until that client is updated), otherwise TS clients can start returning 401 on getMarkets().

Copy link
Copy Markdown
Contributor

@sehyunc sehyunc left a comment

Choose a reason for hiding this comment

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

Behavior note on market handlers:

In crates/workers/api-server/src/http/market.rs, both get_markets and get_market_depths currently drop per-token errors via .ok() and return partial results (128-131, 160-167).

That can silently mask state/price failures and make token availability appear inconsistent to clients. Prefer surfacing an error for the request (or returning explicit per-token error metadata) instead of silently filtering failed entries.

Copy link
Copy Markdown
Contributor

@sehyunc sehyunc left a comment

Choose a reason for hiding this comment

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

Approved.

Non-blocking follow-ups:

  1. Cross-repo compatibility risk:
    GET /v2/markets is now admin-authenticated in crates/workers/api-server/src/http.rs (277-282). typescript-sdk still calls getMarkets() via relayerHttpClient in packages/external-match/src/client.ts (551-553) without the API-key header path used by auth-proxied endpoints. Please coordinate the matching typescript-sdk update to avoid 401 for TS clients.

  2. Silent partial responses:
    In crates/workers/api-server/src/http/market.rs, handlers currently drop per-token errors via .ok() and return partial results (128-131, 160-167). Prefer surfacing request-level failure (or explicit per-token error metadata) rather than silently filtering failed entries.

@akirillo
Copy link
Copy Markdown
Contributor Author

Approved.

Non-blocking follow-ups:

  1. Cross-repo compatibility risk:
    GET /v2/markets is now admin-authenticated in crates/workers/api-server/src/http.rs (277-282). typescript-sdk still calls getMarkets() via relayerHttpClient in packages/external-match/src/client.ts (551-553) without the API-key header path used by auth-proxied endpoints. Please coordinate the matching typescript-sdk update to avoid 401 for TS clients.
  2. Silent partial responses:
    In crates/workers/api-server/src/http/market.rs, handlers currently drop per-token errors via .ok() and return partial results (128-131, 160-167). Prefer surfacing request-level failure (or explicit per-token error metadata) rather than silently filtering failed entries.
  1. Addressed in fix(external-match): route getMarkets through auth server typescript-sdk#269
  2. This mirrors v1 functionality. Generally I think it's ok to give an incomplete response rather than fail the whole endpoint here, if it becomes confusing to customers we can change

@akirillo akirillo merged commit 137a805 into main Feb 26, 2026
7 of 8 checks passed
@akirillo akirillo deleted the andrew/markets-endpoints branch February 26, 2026 00:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants