Implement get_orders_by_token endpoint and refactor orders module#52
Open
Implement get_orders_by_token endpoint and refactor orders module#52
Conversation
Split orders.rs into orders/ module matching the order/ pattern: - mod.rs: trait, impl, shared helpers, test fixtures - get_by_token.rs: handler, process fn, tests - get_by_owner.rs: stub handler - get_by_tx.rs: stub handler
📝 WalkthroughWalkthroughRefactors and expands the orders subsystem: removes the old monolithic orders module, adds modular route handlers (by token, owner, tx), introduces an OrdersListDataSource trait and Raindex-backed implementation, adds pagination/summary helpers and tests, updates OpenAPI and adds a futures dependency. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (4)
src/routes/orders/mod.rs (2)
63-101:build_order_summaryonly uses the first input/output vault.This is fine for a summary view, but consider logging a warning (or documenting the choice) when an order has multiple input/output vaults, so it's clear to future maintainers that data is intentionally omitted.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/orders/mod.rs` around lines 63 - 101, In build_order_summary, detect when order.inputs_list().items() or order.outputs_list().items() contain more than one vault and emit a warning (e.g., using tracing::warn!) that includes the order identifier (order.order_hash()) and the number of input/output vaults so maintainers know additional vaults are being ignored; keep the existing behavior of selecting inputs.first() and outputs.first(), but add the warning checks before those .first() calls referencing the inputs/outputs variables and include clear context like "multiple input vaults, using first" and "multiple output vaults, using first".
103-117:build_paginationdoes not guard againstpage = 0.If
pageis 1-based (as suggested by the#[param(example = 1)]annotation andunwrap_or(1)default in the handler), thenpage = 0would producehas_more = trueeven when there are no more results. Consider clampingpageto a minimum of 1.Proposed fix
pub(crate) fn build_pagination(total_count: u32, page: u32, page_size: u32) -> OrdersPagination { + let page = page.max(1); let total_orders = total_count as u64;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/orders/mod.rs` around lines 103 - 117, build_pagination currently treats page as 1-based but doesn't guard against page == 0, which can yield incorrect has_more; clamp page to at least 1 inside build_pagination (e.g., compute let page = page.max(1)) and use that clamped value for computing total_pages and has_more (ensure has_more compares the clamped page against total_pages after computing total_pages with page_size handling preserved).src/types/orders.rs (1)
26-38: Consider composingOrdersByTokenParamswithOrdersPaginationParamsto avoid field duplication.
pageandpage_sizeare duplicated betweenOrdersPaginationParams(lines 10-17) andOrdersByTokenParams. You could flattenOrdersPaginationParamsinto this struct to keep a single source of truth for pagination fields.Also note:
pageisOption<u32>here, butOrdersListDataSource::get_orders_listinsrc/routes/orders/mod.rs(line 21) acceptspage: Option<u16>. This creates a silent truncation risk if a caller passes a value >u16::MAX. Ensure a safe conversion or align the types.#!/bin/bash # Check how page value flows from handler to data source rg -n 'get_orders_list\|process_get_orders_by_token' --type=rust -C3🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/types/orders.rs` around lines 26 - 38, OrdersByTokenParams duplicates pagination fields and mismatches page types; embed OrdersPaginationParams into OrdersByTokenParams (e.g., add a single field like pagination: OrdersPaginationParams) and use serde flattening/appropriate form/query flatten attributes so page and page_size are not duplicated, then update usages (process_get_orders_by_token) to access pagination.page/page_size; lastly align types between OrdersPaginationParams and OrdersListDataSource::get_orders_list by either changing the pagination.page type to u16 or, if keeping u32, convert safely when calling OrdersListDataSource::get_orders_list (use try_into() / checked conversion and return a proper error if value > u16::MAX) to avoid silent truncation.src/routes/orders/get_by_token.rs (1)
250-260: Rename the test to match its asserted status.The name says
returns_404, but Line 259 assertsUnprocessableEntity(422). Renaming avoids confusion during failures triage.Suggested patch
- async fn test_get_orders_by_token_invalid_address_returns_404() { + async fn test_get_orders_by_token_invalid_address_returns_422() {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/orders/get_by_token.rs` around lines 250 - 260, Rename the test function test_get_orders_by_token_invalid_address_returns_404 to reflect the asserted status (e.g., test_get_orders_by_token_invalid_address_returns_unprocessable_entity or test_get_orders_by_token_invalid_address_returns_422) so the name matches the assertion of Status::UnprocessableEntity; update only the function identifier in the test definition to the new descriptive name (preserving its body and any references).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/routes/orders/get_by_owner.rs`:
- Around line 36-45: Replace the panic-inducing todo!() in the closure passed to
raindex.run_with_client in get_by_owner.rs with a graceful error result: have
the closure return a Result that uses the ApiError type (not panics) so the
outer .map_err(ApiError::from)? can propagate a client-friendly error;
specifically modify the closure passed to run_with_client to return
Err(ApiError::appropriate_variant("descriptive message")) (choose the correct
ApiError constructor used elsewhere in the codebase) instead of calling todo!(),
ensuring the async block signature matches run_with_client's expected Result
type and the span.instrument usage remains unchanged.
In `@src/routes/orders/get_by_token.rs`:
- Around line 92-103: The endpoint currently reads OrdersByTokenParams but only
forwards params.side and params.page to process_get_orders_by_token, so
page_size is ignored; either propagate params.page_size into the pagination flow
or remove it from the endpoint contract. Update the call site inside the
run_with_client closure to extract let page_size = params.page_size (or rename
if different) and pass it into process_get_orders_by_token (or into
RaindexOrdersListDataSource/pagination helper) so the pagination logic uses it,
and adjust process_get_orders_by_token signature and OrdersListResponse
construction accordingly (or remove page_size from OrdersByTokenParams/OpenAPI
if it should not be supported).
- Around line 68-83: The OpenAPI attribute on the get_by_token route is missing
the 422 response used by runtime validation; update the #[utoipa::path(...)]
responses list in src/routes/orders/get_by_token.rs (the attribute that
documents the GET /v1/orders/token/{address}) to include (status = 422,
description = "Unprocessable Entity", body = ApiErrorResponse) so that the
documented responses (which reference OrdersByTokenParams, OrdersListResponse,
ApiErrorResponse) match the actual behavior when an invalid address triggers a
422 Unprocessable Entity.
- Around line 53-57: The code silently swallows quote resolution failures when
computing io_ratio from quotes_result; update the block that computes io_ratio
(the quotes_result -> io_ratio logic) to detect and log failures using
tracing::warn! (or tracing::error! as appropriate) before falling back to "-" —
specifically log when quotes_result is Err or when quotes.first().and_then(|q|
q.data.as_ref()) is None, and include relevant order context (e.g., token/order
id available in this handler) and the underlying error or missing-data reason,
then proceed to use the "-" fallback as before.
In `@src/routes/orders/get_by_tx.rs`:
- Around line 35-44: Replace the panic-causing todo!() inside the async closure
passed to raindex.run_with_client in get_by_tx.rs with a graceful "not
implemented" error return; locate the closure (the move |_client| async move {
todo!() } passed into raindex.run_with_client) and return an ApiError indicating
"not implemented" (e.g., use an existing ApiError::not_implemented variant or
construct an ApiError via ApiError::from with a clear message) so the handler
responds with an explicit error instead of panicking.
In `@src/routes/orders/mod.rs`:
- Around line 123-129: The route registration includes handlers that currently
call todo!() and will panic; update the routes() in mod.rs to avoid exposing
these stubs by either removing get_by_tx::get_orders_by_tx and
get_by_owner::get_orders_by_address from the rocket::routes! list or by
implementing/returning a graceful "not implemented" error in those handlers
(get_by_tx::get_orders_by_tx and get_by_owner::get_orders_by_address) instead of
panic; choose the approach that fits current release requirements (remove the
handlers if endpoints are not ready, or replace todo!() with a proper HTTP
501/handler error response if you want the endpoint to remain declared).
- Around line 16-28: The OrdersListDataSource trait currently defines
get_orders_list(&self, filters: GetOrdersFilters, page: Option<u16>) but callers
(OrdersByTokenParams and OrdersPaginationParams) use Option<u32>, causing a
lossy cast in get_by_token.rs; update the trait signature to accept page:
Option<u32> (change get_orders_list in trait and all implementations) so page
widths are consistent, then handle any necessary validation/truncation inside
the implementing functions (or at call sites) rather than silently casting;
ensure references to get_orders_list, OrdersListDataSource, OrdersByTokenParams,
OrdersPaginationParams and the get_by_token.rs usage are updated accordingly.
---
Nitpick comments:
In `@src/routes/orders/get_by_token.rs`:
- Around line 250-260: Rename the test function
test_get_orders_by_token_invalid_address_returns_404 to reflect the asserted
status (e.g.,
test_get_orders_by_token_invalid_address_returns_unprocessable_entity or
test_get_orders_by_token_invalid_address_returns_422) so the name matches the
assertion of Status::UnprocessableEntity; update only the function identifier in
the test definition to the new descriptive name (preserving its body and any
references).
In `@src/routes/orders/mod.rs`:
- Around line 63-101: In build_order_summary, detect when
order.inputs_list().items() or order.outputs_list().items() contain more than
one vault and emit a warning (e.g., using tracing::warn!) that includes the
order identifier (order.order_hash()) and the number of input/output vaults so
maintainers know additional vaults are being ignored; keep the existing behavior
of selecting inputs.first() and outputs.first(), but add the warning checks
before those .first() calls referencing the inputs/outputs variables and include
clear context like "multiple input vaults, using first" and "multiple output
vaults, using first".
- Around line 103-117: build_pagination currently treats page as 1-based but
doesn't guard against page == 0, which can yield incorrect has_more; clamp page
to at least 1 inside build_pagination (e.g., compute let page = page.max(1)) and
use that clamped value for computing total_pages and has_more (ensure has_more
compares the clamped page against total_pages after computing total_pages with
page_size handling preserved).
In `@src/types/orders.rs`:
- Around line 26-38: OrdersByTokenParams duplicates pagination fields and
mismatches page types; embed OrdersPaginationParams into OrdersByTokenParams
(e.g., add a single field like pagination: OrdersPaginationParams) and use serde
flattening/appropriate form/query flatten attributes so page and page_size are
not duplicated, then update usages (process_get_orders_by_token) to access
pagination.page/page_size; lastly align types between OrdersPaginationParams and
OrdersListDataSource::get_orders_list by either changing the pagination.page
type to u16 or, if keeping u32, convert safely when calling
OrdersListDataSource::get_orders_list (use try_into() / checked conversion and
return a proper error if value > u16::MAX) to avoid silent truncation.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
Cargo.tomlsrc/main.rssrc/routes/order/mod.rssrc/routes/orders.rssrc/routes/orders/get_by_owner.rssrc/routes/orders/get_by_token.rssrc/routes/orders/get_by_tx.rssrc/routes/orders/mod.rssrc/routes/swap/mod.rssrc/types/orders.rs
💤 Files with no reviewable changes (1)
- src/routes/orders.rs
Comment on lines
+36
to
+45
| async move { | ||
| tracing::info!(address = ?address, params = ?params, "request received"); | ||
| let raindex = shared_raindex.read().await; | ||
| raindex | ||
| .run_with_client(move |_client| async move { todo!() }) | ||
| .await | ||
| .map_err(ApiError::from)? | ||
| } | ||
| .instrument(span.0) | ||
| .await |
There was a problem hiding this comment.
Same todo!() panic risk as get_by_tx.rs — return a proper error instead.
This is a registered, client-reachable route. Replace todo!() with a graceful error response.
Proposed fix
async move {
tracing::info!(address = ?address, params = ?params, "request received");
- let raindex = shared_raindex.read().await;
- raindex
- .run_with_client(move |_client| async move { todo!() })
- .await
- .map_err(ApiError::from)?
+ Err(ApiError::BadRequest("endpoint not yet implemented".into()))
}
.instrument(span.0)
.awaitAs per coding guidelines: "Never use expect or unwrap in production code; handle errors gracefully or exit with a message".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| async move { | |
| tracing::info!(address = ?address, params = ?params, "request received"); | |
| let raindex = shared_raindex.read().await; | |
| raindex | |
| .run_with_client(move |_client| async move { todo!() }) | |
| .await | |
| .map_err(ApiError::from)? | |
| } | |
| .instrument(span.0) | |
| .await | |
| async move { | |
| tracing::info!(address = ?address, params = ?params, "request received"); | |
| Err(ApiError::BadRequest("endpoint not yet implemented".into())) | |
| } | |
| .instrument(span.0) | |
| .await |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/routes/orders/get_by_owner.rs` around lines 36 - 45, Replace the
panic-inducing todo!() in the closure passed to raindex.run_with_client in
get_by_owner.rs with a graceful error result: have the closure return a Result
that uses the ApiError type (not panics) so the outer .map_err(ApiError::from)?
can propagate a client-friendly error; specifically modify the closure passed to
run_with_client to return Err(ApiError::appropriate_variant("descriptive
message")) (choose the correct ApiError constructor used elsewhere in the
codebase) instead of calling todo!(), ensuring the async block signature matches
run_with_client's expected Result type and the span.instrument usage remains
unchanged.
Collaborator
Author
There was a problem hiding this comment.
This is fine as we are adding these endpoints one by one.
There was a problem hiding this comment.
@findolor, understood! I appreciate the incremental approach for implementing these endpoints. The todo!() placeholders make sense as stubs while you build out each endpoint in separate iterations.
🧠 Learnings used
Learnt from: CR
Repo: ST0x-Technology/st0x.rest.api PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-19T09:28:42.183Z
Learning: Applies to **/*.rs : Keep OpenAPI annotations (`#[utoipa::path(...)]`) in sync when adding or modifying routes
Learnt from: findolor
Repo: ST0x-Technology/st0x.rest.api PR: 18
File: src/fairings/rate_limiter.rs:158-203
Timestamp: 2026-02-13T09:37:37.357Z
Learning: When multiple guards or layers write to the same cached rate-limit information, document and enforce the execution order so that the client-visible headers reflect the intended quota (global vs per-key). In this repo, rate-limiter.rs and auth-related guards intentionally write to the same CachedRateLimitInfo entry; ensure the per-key quota is what’s surfaced to clients and that the order is explicit in route signatures. Include tests that verify the final headers reflect the intended quota and that writes are intentional and auditable.
Learnt from: findolor
Repo: ST0x-Technology/st0x.rest.api PR: 38
File: src/routes/swap/mod.rs:115-129
Timestamp: 2026-02-17T09:35:57.431Z
Learning: In Rust server code for swap endpoints, if the response contains a non-empty approvals array, the behavior uses a two-phase flow: first the client sends an approval transaction, then queries the endpoint again to obtain the actual swap calldata. In the approval response, set to = approval.spender() to signal the eventual target, and set data = Bytes::new() (empty) to indicate 'no transaction yet.' Prefer this approach over using Address::ZERO for clarity. This pattern is specific to the swap flow in src/routes/swap/mod.rs and should be followed in similar swap-related endpoints with analogous approval semantics.
src/routes/orders/get_by_token.rs
Outdated
Comment on lines
+53
to
+57
| let io_ratio = quotes_result | ||
| .ok() | ||
| .and_then(|quotes| quotes.first().and_then(|q| q.data.as_ref()).cloned()) | ||
| .map(|d| d.formatted_ratio) | ||
| .unwrap_or_else(|| "-".into()); |
There was a problem hiding this comment.
Log quote-fetch failures before applying the "-" fallback.
This silently drops errors and makes degraded responses hard to diagnose. Emit a warning with order context when quote resolution fails.
Suggested patch
- let io_ratio = quotes_result
- .ok()
- .and_then(|quotes| quotes.first().and_then(|q| q.data.as_ref()).cloned())
- .map(|d| d.formatted_ratio)
- .unwrap_or_else(|| "-".into());
+ let io_ratio = match quotes_result {
+ Ok(quotes) => quotes
+ .first()
+ .and_then(|q| q.data.as_ref())
+ .map(|d| d.formatted_ratio.clone())
+ .unwrap_or_else(|| "-".into()),
+ Err(err) => {
+ tracing::warn!(
+ order_hash = ?order.order_hash(),
+ error = ?err,
+ "quote fetch failed; using fallback io_ratio"
+ );
+ "-".into()
+ }
+ };As per coding guidelines, "Every route handler must log appropriately using tracing (request received, errors, key decisions)".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/routes/orders/get_by_token.rs` around lines 53 - 57, The code silently
swallows quote resolution failures when computing io_ratio from quotes_result;
update the block that computes io_ratio (the quotes_result -> io_ratio logic) to
detect and log failures using tracing::warn! (or tracing::error! as appropriate)
before falling back to "-" — specifically log when quotes_result is Err or when
quotes.first().and_then(|q| q.data.as_ref()) is None, and include relevant order
context (e.g., token/order id available in this handler) and the underlying
error or missing-data reason, then proceed to use the "-" fallback as before.
Comment on lines
+35
to
+44
| async move { | ||
| tracing::info!(tx_hash = ?tx_hash, "request received"); | ||
| let raindex = shared_raindex.read().await; | ||
| raindex | ||
| .run_with_client(move |_client| async move { todo!() }) | ||
| .await | ||
| .map_err(ApiError::from)? | ||
| } | ||
| .instrument(span.0) | ||
| .await |
There was a problem hiding this comment.
todo!() will panic at runtime on a registered, reachable route.
This handler is mounted in routes() and documented in OpenAPI, so clients can call it. The todo!() macro panics unconditionally, violating the guideline to never panic in production code. Return an explicit "not implemented" error instead.
Proposed fix
async move {
tracing::info!(tx_hash = ?tx_hash, "request received");
- let raindex = shared_raindex.read().await;
- raindex
- .run_with_client(move |_client| async move { todo!() })
- .await
- .map_err(ApiError::from)?
+ Err(ApiError::BadRequest("endpoint not yet implemented".into()))
}
.instrument(span.0)
.awaitAs per coding guidelines: "Never use expect or unwrap in production code; handle errors gracefully or exit with a message".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/routes/orders/get_by_tx.rs` around lines 35 - 44, Replace the
panic-causing todo!() inside the async closure passed to raindex.run_with_client
in get_by_tx.rs with a graceful "not implemented" error return; locate the
closure (the move |_client| async move { todo!() } passed into
raindex.run_with_client) and return an ApiError indicating "not implemented"
(e.g., use an existing ApiError::not_implemented variant or construct an
ApiError via ApiError::from with a clear message) so the handler responds with
an explicit error instead of panicking.
Collaborator
Author
There was a problem hiding this comment.
This is fine as we are adding these endpoints one by one.
There was a problem hiding this comment.
@findolor, understood! I acknowledge your incremental approach to implementing these endpoints. The todo!() placeholders are clear scaffolding for the stub handlers.
🧠 Learnings used
Learnt from: CR
Repo: ST0x-Technology/st0x.rest.api PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-19T09:28:42.183Z
Learning: Applies to **/*.rs : Never use `expect` or `unwrap` in production code; handle errors gracefully or exit with a message
Learnt from: CR
Repo: ST0x-Technology/st0x.rest.api PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-19T09:28:42.183Z
Learning: Applies to **/*.rs : All API errors must go through the `ApiError` enum, never return raw status codes
Learnt from: findolor
Repo: ST0x-Technology/st0x.rest.api PR: 18
File: src/fairings/rate_limiter.rs:158-203
Timestamp: 2026-02-13T09:37:37.357Z
Learning: When multiple guards or layers write to the same cached rate-limit information, document and enforce the execution order so that the client-visible headers reflect the intended quota (global vs per-key). In this repo, rate-limiter.rs and auth-related guards intentionally write to the same CachedRateLimitInfo entry; ensure the per-key quota is what’s surfaced to clients and that the order is explicit in route signatures. Include tests that verify the final headers reflect the intended quota and that writes are intentional and auditable.
Learnt from: findolor
Repo: ST0x-Technology/st0x.rest.api PR: 38
File: src/routes/swap/mod.rs:115-129
Timestamp: 2026-02-17T09:35:57.431Z
Learning: In Rust server code for swap endpoints, if the response contains a non-empty approvals array, the behavior uses a two-phase flow: first the client sends an approval transaction, then queries the endpoint again to obtain the actual swap calldata. In the approval response, set to = approval.spender() to signal the eventual target, and set data = Bytes::new() (empty) to indicate 'no transaction yet.' Prefer this approach over using Address::ZERO for clarity. This pattern is specific to the swap flow in src/routes/swap/mod.rs and should be followed in similar swap-related endpoints with analogous approval semantics.
Comment on lines
+16
to
+28
| #[async_trait(?Send)] | ||
| pub(crate) trait OrdersListDataSource { | ||
| async fn get_orders_list( | ||
| &self, | ||
| filters: GetOrdersFilters, | ||
| page: Option<u16>, | ||
| ) -> Result<(Vec<RaindexOrder>, u32), ApiError>; | ||
|
|
||
| async fn get_order_quotes( | ||
| &self, | ||
| order: &RaindexOrder, | ||
| ) -> Result<Vec<RaindexOrderQuote>, ApiError>; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find and examine how page parameter is handled before calling get_orders_list
fd 'get_by_token.rs' -x cat {}Repository: ST0x-Technology/st0x.rest.api
Length of output: 9346
🏁 Script executed:
# Find all calls to get_orders_list in the codebase
rg 'get_orders_list' -A 2Repository: ST0x-Technology/st0x.rest.api
Length of output: 842
🏁 Script executed:
# Check if OrdersByTokenParams is used elsewhere and its page type definition
rg 'OrdersByTokenParams' -B 2 -A 5Repository: ST0x-Technology/st0x.rest.api
Length of output: 2405
🏁 Script executed:
# Verify the exact trait definition in orders/mod.rs lines 16-28
sed -n '16,28p' src/routes/orders/mod.rsRepository: ST0x-Technology/st0x.rest.api
Length of output: 424
🏁 Script executed:
# Check if there are any other routes or functions that might call the trait method
fd '\.rs$' src/routes/orders -x basename {}Repository: ST0x-Technology/st0x.rest.api
Length of output: 124
🏁 Script executed:
# Check the other route handlers to see if they also use get_orders_list and how they handle page
cat src/routes/orders/get_by_owner.rsRepository: ST0x-Technology/st0x.rest.api
Length of output: 1752
🏁 Script executed:
# Check get_by_tx.rs and OrdersPaginationParams definition
cat src/routes/orders/get_by_tx.rsRepository: ST0x-Technology/st0x.rest.api
Length of output: 1730
🏁 Script executed:
# Check OrdersPaginationParams definition in types/orders.rs
rg 'struct OrdersPaginationParams' -A 10Repository: ST0x-Technology/st0x.rest.api
Length of output: 611
page type is Option<u16> but callers use Option<u32> — confirm intentionality of lossy conversion.
The trait expects page: Option<u16> while both OrdersByTokenParams and OrdersPaginationParams define page: Option<u32>. In get_by_token.rs, this is resolved with a silent cast: Some(page_num as u16). This truncates any page number greater than 65535. While the practical impact is minimal (would require over 1M items to reach such a page), this violates type safety. Either change the trait to accept Option<u32> and handle truncation/validation internally, or standardize query parameters to u16 from the start.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/routes/orders/mod.rs` around lines 16 - 28, The OrdersListDataSource
trait currently defines get_orders_list(&self, filters: GetOrdersFilters, page:
Option<u16>) but callers (OrdersByTokenParams and OrdersPaginationParams) use
Option<u32>, causing a lossy cast in get_by_token.rs; update the trait signature
to accept page: Option<u32> (change get_orders_list in trait and all
implementations) so page widths are consistent, then handle any necessary
validation/truncation inside the implementing functions (or at call sites)
rather than silently casting; ensure references to get_orders_list,
OrdersListDataSource, OrdersByTokenParams, OrdersPaginationParams and the
get_by_token.rs usage are updated accordingly.
Comment on lines
+123
to
+129
| pub fn routes() -> Vec<Route> { | ||
| rocket::routes![ | ||
| get_by_tx::get_orders_by_tx, | ||
| get_by_owner::get_orders_by_address, | ||
| get_by_token::get_orders_by_token | ||
| ] | ||
| } |
There was a problem hiding this comment.
Routes include stub handlers that panic — see comments on get_by_tx.rs and get_by_owner.rs.
Both get_orders_by_tx and get_orders_by_address contain todo!() and will panic if clients reach them. If these endpoints are not ready, consider either removing them from the route registration or replacing the stubs with graceful "not implemented" errors.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/routes/orders/mod.rs` around lines 123 - 129, The route registration
includes handlers that currently call todo!() and will panic; update the
routes() in mod.rs to avoid exposing these stubs by either removing
get_by_tx::get_orders_by_tx and get_by_owner::get_orders_by_address from the
rocket::routes! list or by implementing/returning a graceful "not implemented"
error in those handlers (get_by_tx::get_orders_by_tx and
get_by_owner::get_orders_by_address) instead of panic; choose the approach that
fits current release requirements (remove the handlers if endpoints are not
ready, or replace todo!() with a proper HTTP 501/handler error response if you
want the endpoint to remain declared).
Collaborator
Author
There was a problem hiding this comment.
This is fine as we are adding these endpoints one by one.
There was a problem hiding this comment.
@findolor, understood! I'll keep that in mind for incremental endpoint development in this PR. Thank you for clarifying.
🧠 Learnings used
Learnt from: findolor
Repo: ST0x-Technology/st0x.rest.api PR: 18
File: src/fairings/rate_limiter.rs:158-203
Timestamp: 2026-02-13T09:37:37.357Z
Learning: When multiple guards or layers write to the same cached rate-limit information, document and enforce the execution order so that the client-visible headers reflect the intended quota (global vs per-key). In this repo, rate-limiter.rs and auth-related guards intentionally write to the same CachedRateLimitInfo entry; ensure the per-key quota is what’s surfaced to clients and that the order is explicit in route signatures. Include tests that verify the final headers reflect the intended quota and that writes are intentional and auditable.
Learnt from: findolor
Repo: ST0x-Technology/st0x.rest.api PR: 38
File: src/routes/swap/mod.rs:115-129
Timestamp: 2026-02-17T09:35:57.431Z
Learning: In Rust server code for swap endpoints, if the response contains a non-empty approvals array, the behavior uses a two-phase flow: first the client sends an approval transaction, then queries the endpoint again to obtain the actual swap calldata. In the approval response, set to = approval.spender() to signal the eventual target, and set data = Bytes::new() (empty) to indicate 'no transaction yet.' Prefer this approach over using Address::ZERO for clarity. This pattern is specific to the swap flow in src/routes/swap/mod.rs and should be followed in similar swap-related endpoints with analogous approval semantics.
…pport, u16 types - Log warning on quote fetch failure before falling back to "-" in orders list - Add 422 response to OpenAPI annotation for get_orders_by_token - Wire page_size parameter through trait, implementation, and route handler - Change page and page_size params from u32 to u16 to match SDK types - Update rain.orderbook submodule with page_size support in get_orders
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/routes/orders/get_by_token.rs (2)
46-50: Consider normalizing pagination bounds before querying.Line 46/47 accepts
0and very largepageSizevalues. Adding a min/max guard avoids invalid paging states and overly expensive requests.Suggested patch
- let page_num = page.unwrap_or(1); - let effective_page_size = page_size.unwrap_or(DEFAULT_PAGE_SIZE as u16); + let page_num = page.filter(|p| *p > 0).unwrap_or(1); + let effective_page_size = page_size + .filter(|s| *s > 0) + .map(|s| s.min(200)) + .unwrap_or(DEFAULT_PAGE_SIZE as u16);Also applies to: 75-75
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/orders/get_by_token.rs` around lines 46 - 50, Normalize pagination inputs before calling get_orders_list: ensure page_num = max(1, page.unwrap_or(1)) and clamp effective_page_size to a safe range (e.g. min 1, max MAX_PAGE_SIZE) instead of accepting 0 or arbitrarily large values; introduce or use a MAX_PAGE_SIZE constant (alongside DEFAULT_PAGE_SIZE), coerce types as needed, then pass the bounded Some(page_num) and Some(effective_page_size) into ds.get_orders_list(...) (also apply the same normalization to the other call site around line 75).
266-276: Rename the test to match the asserted status code.The test name says
returns_404but Line 275 assertsStatus::UnprocessableEntity(422). This makes failures harder to read quickly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/orders/get_by_token.rs` around lines 266 - 276, Rename the test function test_get_orders_by_token_invalid_address_returns_404 to reflect the asserted status (UnprocessableEntity/422) — e.g., test_get_orders_by_token_invalid_address_returns_422 — by updating the async fn name in the file so the function name and the assertion Status::UnprocessableEntity are consistent; ensure any references to that test name (if any) are updated accordingly.src/routes/orders/mod.rs (1)
83-84: Avoid silently defaulting invalid timestamps to0.Line 83 masks conversion failures and can emit incorrect
created_at. Prefer logging and returning anApiErrorso bad data is visible.Suggested patch
- let created_at: u64 = order.timestamp_added().try_into().unwrap_or(0); + let created_at: u64 = order.timestamp_added().try_into().map_err(|_| { + tracing::error!(timestamp = order.timestamp_added(), "invalid order timestamp"); + ApiError::Internal("invalid order timestamp".into()) + })?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/orders/mod.rs` around lines 83 - 84, Replace the silent fallback on timestamp conversion: instead of let created_at: u64 = order.timestamp_added().try_into().unwrap_or(0); perform the try_into() and handle Err by logging the conversion error and returning an ApiError (e.g. ApiError::BadData or ApiError::InvalidTimestamp) up the call stack; for example, match order.timestamp_added().try_into() { Ok(ts) => ts, Err(e) => { logger.error!("invalid timestamp for order id {}: {}", order.id(), e); return Err(ApiError::BadData("invalid timestamp")); } } so bad data is visible and the handler returns an error rather than using 0.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/routes/orders/get_by_token.rs`:
- Around line 116-123: The handler currently maps errors from
raindex.run_with_client to ApiError but doesn't log failures; wrap the
raindex.run_with_client call to log both client-acquisition and processing
errors: when calling raindex.run_with_client (and inside the closure using
RaindexOrdersListDataSource and process_get_orders_by_token), add tracing::error
calls on Err paths and include the error details before mapping to ApiError, and
also emit a tracing::info/tracing::debug at the start of the handler to indicate
request received and key params (addr, side, page, page_size); ensure references
are made to the existing symbols raindex.run_with_client,
RaindexOrdersListDataSource, process_get_orders_by_token, and ApiError so the
logs capture both acquisition failures and processing failures.
---
Nitpick comments:
In `@src/routes/orders/get_by_token.rs`:
- Around line 46-50: Normalize pagination inputs before calling get_orders_list:
ensure page_num = max(1, page.unwrap_or(1)) and clamp effective_page_size to a
safe range (e.g. min 1, max MAX_PAGE_SIZE) instead of accepting 0 or arbitrarily
large values; introduce or use a MAX_PAGE_SIZE constant (alongside
DEFAULT_PAGE_SIZE), coerce types as needed, then pass the bounded Some(page_num)
and Some(effective_page_size) into ds.get_orders_list(...) (also apply the same
normalization to the other call site around line 75).
- Around line 266-276: Rename the test function
test_get_orders_by_token_invalid_address_returns_404 to reflect the asserted
status (UnprocessableEntity/422) — e.g.,
test_get_orders_by_token_invalid_address_returns_422 — by updating the async fn
name in the file so the function name and the assertion
Status::UnprocessableEntity are consistent; ensure any references to that test
name (if any) are updated accordingly.
In `@src/routes/orders/mod.rs`:
- Around line 83-84: Replace the silent fallback on timestamp conversion:
instead of let created_at: u64 =
order.timestamp_added().try_into().unwrap_or(0); perform the try_into() and
handle Err by logging the conversion error and returning an ApiError (e.g.
ApiError::BadData or ApiError::InvalidTimestamp) up the call stack; for example,
match order.timestamp_added().try_into() { Ok(ts) => ts, Err(e) => {
logger.error!("invalid timestamp for order id {}: {}", order.id(), e); return
Err(ApiError::BadData("invalid timestamp")); } } so bad data is visible and the
handler returns an error rather than using 0.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
lib/rain.orderbooksrc/routes/order/mod.rssrc/routes/orders/get_by_token.rssrc/routes/orders/mod.rssrc/routes/swap/mod.rssrc/types/orders.rs
✅ Files skipped from review due to trivial changes (1)
- lib/rain.orderbook
🚧 Files skipped from review as they are similar to previous changes (1)
- src/routes/swap/mod.rs
Comment on lines
+116
to
+123
| let response = raindex | ||
| .run_with_client(move |client| async move { | ||
| let ds = RaindexOrdersListDataSource { client: &client }; | ||
| process_get_orders_by_token(&ds, addr, side, page, page_size).await | ||
| }) | ||
| .await | ||
| .map_err(ApiError::from)??; | ||
| Ok(Json(response)) |
There was a problem hiding this comment.
Add explicit handler-level error logging for failure paths.
Line 122 maps errors to ApiError, but handler-boundary failures are not logged here. Please log both client acquisition and processing failures in this route.
Suggested patch
let response = raindex
.run_with_client(move |client| async move {
let ds = RaindexOrdersListDataSource { client: &client };
- process_get_orders_by_token(&ds, addr, side, page, page_size).await
+ process_get_orders_by_token(&ds, addr, side, page, page_size)
+ .await
+ .map_err(|e| {
+ tracing::error!(address = ?addr, error = ?e, "failed to process get_orders_by_token");
+ e
+ })
})
.await
- .map_err(ApiError::from)??;
+ .map_err(|e| {
+ tracing::error!(address = ?addr, error = %e, "failed to acquire raindex client");
+ ApiError::from(e)
+ })??;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let response = raindex | |
| .run_with_client(move |client| async move { | |
| let ds = RaindexOrdersListDataSource { client: &client }; | |
| process_get_orders_by_token(&ds, addr, side, page, page_size).await | |
| }) | |
| .await | |
| .map_err(ApiError::from)??; | |
| Ok(Json(response)) | |
| let response = raindex | |
| .run_with_client(move |client| async move { | |
| let ds = RaindexOrdersListDataSource { client: &client }; | |
| process_get_orders_by_token(&ds, addr, side, page, page_size) | |
| .await | |
| .map_err(|e| { | |
| tracing::error!(address = ?addr, error = ?e, "failed to process get_orders_by_token"); | |
| e | |
| }) | |
| }) | |
| .await | |
| .map_err(|e| { | |
| tracing::error!(address = ?addr, error = %e, "failed to acquire raindex client"); | |
| ApiError::from(e) | |
| })??; | |
| Ok(Json(response)) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/routes/orders/get_by_token.rs` around lines 116 - 123, The handler
currently maps errors from raindex.run_with_client to ApiError but doesn't log
failures; wrap the raindex.run_with_client call to log both client-acquisition
and processing errors: when calling raindex.run_with_client (and inside the
closure using RaindexOrdersListDataSource and process_get_orders_by_token), add
tracing::error calls on Err paths and include the error details before mapping
to ApiError, and also emit a tracing::info/tracing::debug at the start of the
handler to indicate request received and key params (addr, side, page,
page_size); ensure references are made to the existing symbols
raindex.run_with_client, RaindexOrdersListDataSource,
process_get_orders_by_token, and ApiError so the logs capture both acquisition
failures and processing failures.
Collaborator
Author
There was a problem hiding this comment.
Errors are already logged at the origin — get_orders_list and get_order_quotes both emit tracing::error! before returning. No other handler in the codebase logs at the handler boundary either, so adding it here would be inconsistent.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the implementation of get_orders_list and get_order_quotes in src/routes/orders/mod.rs
echo "=== Checking get_orders_list implementation ==="
ast-grep --pattern $'async fn get_orders_list($$$) {
$$$
}'
echo -e "\n=== Checking get_order_quotes implementation ==="
ast-grep --pattern $'async fn get_order_quotes($$$) {
$$$
}'
echo -e "\n=== Checking RaindexOrdersListDataSource implementations ==="
rg -A 20 "impl.*OrdersListDataSource.*for.*RaindexOrdersListDataSource" src/routes/orders/mod.rs
echo -e "\n=== Checking other route handlers for error logging patterns ==="
echo "--- get_orders_by_owner.rs ---"
rg -A 10 "run_with_client|map_err" src/routes/orders/get_by_owner.rs
echo -e "\n--- get_orders_by_tx.rs ---"
rg -A 10 "run_with_client|map_err" src/routes/orders/get_by_tx.rs
echo -e "\n--- order/mod.rs ---"
rg -A 10 "run_with_client|map_err" src/routes/order/mod.rs
echo -e "\n--- swap/mod.rs ---"
rg -A 10 "run_with_client|map_err" src/routes/swap/mod.rs
Length of output: 4513
@findolor, you're absolutely right on both counts. I verified the implementations and found that:
- ✅ Errors are logged at the origin: Both
get_orders_listandget_order_quotesinsrc/routes/orders/mod.rsemittracing::error!before returning errors - ✅ Consistent with other handlers: I checked
get_orders_by_owner.rs,get_orders_by_tx.rs,order/mod.rs, andswap/mod.rs— none log at the handler boundary; they all use.map_err(ApiError::from)?or similar
Your handler already logs "request received" at line 109, and the lower-level functions log errors at their source. This pattern avoids duplicate logging and is actually a good separation of concerns. My suggestion was inconsistent with the codebase patterns. Thanks for the clarification!
✏️ Learnings added
Learnt from: findolor
Repo: ST0x-Technology/st0x.rest.api PR: 52
File: src/routes/orders/get_by_token.rs:116-123
Timestamp: 2026-02-25T12:18:58.001Z
Learning: In the st0x.rest.api Rust codebase, the error-logging pattern is: route handlers log "request received" and key decisions, while lower-level data-access functions (like OrdersListDataSource implementations) log errors at their origin using tracing::error! before returning ApiError. Handlers should NOT duplicate error logging at the boundary; they propagate errors with map_err(ApiError::from)? or similar. This pattern is used consistently across handlers in src/routes/orders/, src/routes/order/, and src/routes/swap/.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: CR
Repo: ST0x-Technology/st0x.rest.api PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-19T09:28:42.183Z
Learning: Applies to **/*.rs : Every route handler must log appropriately using tracing (request received, errors, key decisions)
Learnt from: findolor
Repo: ST0x-Technology/st0x.rest.api PR: 18
File: src/fairings/rate_limiter.rs:158-203
Timestamp: 2026-02-13T09:37:37.357Z
Learning: When multiple guards or layers write to the same cached rate-limit information, document and enforce the execution order so that the client-visible headers reflect the intended quota (global vs per-key). In this repo, rate-limiter.rs and auth-related guards intentionally write to the same CachedRateLimitInfo entry; ensure the per-key quota is what’s surfaced to clients and that the order is explicit in route signatures. Include tests that verify the final headers reflect the intended quota and that writes are intentional and auditable.
Learnt from: findolor
Repo: ST0x-Technology/st0x.rest.api PR: 38
File: src/routes/swap/mod.rs:115-129
Timestamp: 2026-02-17T09:35:57.431Z
Learning: In Rust server code for swap endpoints, if the response contains a non-empty approvals array, the behavior uses a two-phase flow: first the client sends an approval transaction, then queries the endpoint again to obtain the actual swap calldata. In the approval response, set to = approval.spender() to signal the eventual target, and set data = Bytes::new() (empty) to indicate 'no transaction yet.' Prefer this approach over using Address::ZERO for clarity. This pattern is specific to the swap flow in src/routes/swap/mod.rs and should be followed in similar swap-related endpoints with analogous approval semantics.
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Dependent PRs
Motivation
The orders endpoint group needed a working
get_orders_by_tokenhandler to query paginated order lists filtered by token address, with optional side (input/output) filtering and IO ratio quotes. The existingorders.rssingle-file layout also needed refactoring into a module directory to match the establishedorder/pattern and support future endpoint additions.Solution
rain.orderbooksubmodule and addedfuturescrate for concurrent quote fetchingorder/mod.rsandswap/mod.rsto work with the newget_ordersAPI that returns a result object with.orders()and.total_count()accessorsOrderSideenum andOrdersByTokenParamsquery struct intypes/orders.rsget_orders_by_token: full handler with token filter construction, paginated order fetching, concurrent quote resolution, andOrderSummarybuildingorders.rsintoorders/module directory:mod.rs— trait, impl, shared helpers (build_order_summary,build_pagination), test fixturesget_by_token.rs— handler, process function, unit + integration testsget_by_owner.rs— stub handler (todo!())get_by_tx.rs— stub handler (todo!())All 127 tests pass, formatter and clippy clean.
Checks
By submitting this for review, I'm confirming I've done the following:
Summary by CodeRabbit
New Features
Refactor
Chores