Conversation
Introduce an async trait to abstract the three raindex calls in get_order (get_orders, get_quotes, get_trades_list), enabling unit tests that exercise determine_order_type and build_order_detail with real deserialized RaindexOrder/RaindexTrade instances via a mock data source. Adds seven tests covering the happy path, not-found, empty trades, query failure, default order type, 401 without auth, and 500 on client init failure.
📝 WalkthroughWalkthroughMonolithic Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant GET_Handler as "GET Handler\nget_order"
participant OrderDataSource as "OrderDataSource\n(trait impl)"
participant RaindexClient as "RaindexClient"
participant Logic as "Helpers\n(determine/build/map)"
Client->>GET_Handler: GET /v1/order/{hash}
GET_Handler->>OrderDataSource: get_orders_by_hash(hash)
OrderDataSource->>RaindexClient: get_orders(filters)
RaindexClient-->>OrderDataSource: Vec<RaindexOrder>
OrderDataSource-->>GET_Handler: RaindexOrder(s)
GET_Handler->>OrderDataSource: get_order_quotes(&order)
OrderDataSource->>RaindexClient: order.get_quotes()
RaindexClient-->>OrderDataSource: Vec<RaindexOrderQuote>
OrderDataSource-->>GET_Handler: quotes
GET_Handler->>OrderDataSource: get_order_trades(&order)
OrderDataSource->>RaindexClient: order.get_trades_list()
RaindexClient-->>OrderDataSource: Vec<RaindexTrade>
OrderDataSource-->>GET_Handler: trades
GET_Handler->>Logic: determine_order_type(order)
Logic-->>GET_Handler: OrderType
GET_Handler->>Logic: build_order_detail(order, type, io_ratio, trades)
Logic-->>GET_Handler: OrderDetail
GET_Handler-->>Client: Json<OrderDetail>
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/routes/order/get_order.rs`:
- Around line 61-67: The quotes and trades futures are swallowing errors via
unwrap_or_default on the calls to get_order_quotes and get_order_trades; change
these to propagate errors instead (use ? on the Result returned by
ds.get_order_quotes(&order).await and ds.get_order_trades(&order).await) so the
handler returns an ApiError when lookups fail, and update the enclosing function
signature/return type to support returning ApiError; also update
MockOrderDataSource (fields and its get_order_quotes/get_order_trades
implementations) to return Result<Vec<_>, ApiError> so tests can cover Err
paths, and add tests exercising the error case (or, if you intentionally keep
graceful degradation, replace unwrap_or_default with code that logs a warning on
Err and extend MockOrderDataSource to return Err for tests).
🧹 Nitpick comments (4)
src/routes/order/get_order.rs (3)
72-81: Order type detection looks correct.Case-insensitive check for "dca" in the deployment name with a
Solverdefault is straightforward and reasonable. The test on Line 460-463 covers the default path; consider adding a test with a mock order whoseparsedMetacontains a DCA deployment to cover theOrderType::Dcabranch.
89-99: Only the first input/output vault is used.If an order can legitimately have multiple input or output vaults, this silently discards the rest. Fine for an MVP that only targets single-pair orders, but worth a brief comment in the code to document this assumption so future maintainers don't miss it.
108-134:io_ratioappears twice in the response.The field is set both inside
order_details(Line 113) and at the top level (Line 129). If this duplication is intentional (e.g. for backward compatibility or different consumers), a brief comment explaining why would help. Otherwise, consider removing one.src/routes/order/mod.rs (1)
29-61: Implementations are consistent and properly instrumented with tracing.All three methods follow the same error-mapping pattern with
tracing::error!before converting toApiError::Internal. Note thatget_order_quotesandget_order_tradespassNonefor all pagination parameters, fetching the entire dataset. For orders with a large number of trades, this could become a performance concern. Consider adding pagination support or a limit in a follow-up.
findolor
changed the base branch from
2026-02-16-raindex-client-implementation
to
findolor/admin-registry-management
Replace .unwrap_or_default() with ? on get_order_quotes and get_order_trades so failures surface as ApiError instead of being silently swallowed. Update MockOrderDataSource to wrap quotes and trades in Result and add tests for both error paths.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/routes/order/get_order.rs (1)
56-71: Error propagation from quotes and trades now correctly uses?.This addresses the previously raised concern about
unwrap_or_default()silently swallowing errors. The fallback to"-"on lines 63–67 only applies when a quote exists but lacks data (e.g., a failed on-chain quote), which is the right semantic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/order/get_order.rs` around lines 56 - 71, The process_get_order function currently should propagate errors from data source calls instead of swallowing them; update ds.get_order_quotes and ds.get_order_trades calls in process_get_order to use the ? operator so errors bubble up (retain the io_ratio computation that falls back to "-" only when a quote exists but its data is None), ensure build_order_detail is called with the propagated Result flow, and reference the symbols process_get_order, ds.get_order_quotes, ds.get_order_trades, io_ratio, and build_order_detail when making these changes.
🧹 Nitpick comments (4)
src/routes/order/get_order.rs (4)
107-107: Silent fallback to0on timestamp conversion overflow.
try_into().unwrap_or(0)will silently yieldcreated_at: 0iftimestamp_added()exceedsu64::MAX. This is unlikely but could confuse consumers who see epoch-zero. Atracing::warnbefore falling back would make debugging easier. Same pattern appears inmap_tradeat line 138.Suggested improvement
- let created_at: u64 = order.timestamp_added().try_into().unwrap_or(0); + let created_at: u64 = order.timestamp_added().try_into().unwrap_or_else(|_| { + tracing::warn!(order_hash = ?order.order_hash(), "timestamp overflow, defaulting to 0"); + 0 + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/order/get_order.rs` at line 107, The conversion of order.timestamp_added() to u64 uses try_into().unwrap_or(0) which silently maps overflow to epoch-zero; change created_at handling in get_order.rs to match on order.timestamp_added().try_into() (or use .map_err) and on Err emit a tracing::warn including the original timestamp value and context (e.g., "timestamp_added overflow, falling back to 0") before using 0, and apply the same pattern in map_trade where the same try_into().unwrap_or(0) occurs so consumers and logs show a warning when the fallback is used.
350-382: MockOrderDataSource discards the original error variant — adequate for current tests but limits future assertions.The
matcharms onErr(_)ignore the stored error and construct a new hardcodedApiError::Internal(...). Current tests only checkmatches!(result, Err(ApiError::Internal(_))), so this works, but if you later want to assert on the exact error message the mock was seeded with, you'd need to clone and return it instead.Optionally forward the stored error
async fn get_orders_by_hash(&self, _hash: B256) -> Result<Vec<RaindexOrder>, ApiError> { match &self.orders { Ok(orders) => Ok(orders.clone()), - Err(_) => Err(ApiError::Internal("failed to query orders".into())), + Err(e) => Err(e.clone()), } }This requires
ApiErrorto implementClone. If that's not desirable, the current approach is fine.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/order/get_order.rs` around lines 350 - 382, The mock currently discards the original ApiError in MockOrderDataSource (fields orders/trades/quotes) and returns a new ApiError::Internal in each Err(_) arm; change each match arm to return the stored error (e.g., Err(e.clone()) or Err(e.to_owned())) instead of constructing a new one so tests can assert the original message/variant; to do this add Clone for ApiError or otherwise make ApiError clonable and ensure the match arms in get_orders_by_hash, get_order_quotes, and get_order_trades return the cloned stored error (use the existing variable names orders/trades/quotes in those match expressions).
73-82:determine_order_typeuses ASCIIto_lowercase— fine for "dca" but fragile for future deployment names with non-ASCII characters.Minor:
to_lowercase()is Unicode-aware in Rust'sString, so this is correct. However, the function only has a test for the defaultSolverpath (line 489–492). There is no test exercising theDcabranch whenparsedMetacontains aDotrainGuiStateV1with a deployment name including "dca".Consider adding a unit test for the DCA classification path. If constructing a
RaindexOrderwith populatedparsedMetais difficult, a comment explaining why it's omitted would suffice.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/order/get_order.rs` around lines 73 - 82, Add coverage for the DCA branch in determine_order_type by adding a unit test that constructs a RaindexOrder whose parsed_meta includes ParsedMeta::DotrainGuiStateV1 with gui_state.selected_deployment containing "dca" (test case-insensitive variants like "DCA" and "dCa") and assert determine_order_type(&order) == OrderType::Dca; if creating a RaindexOrder with populated parsed_meta is impractical, add a short comment next to determine_order_type (or in the test module) explaining why the Dca branch isn't tested and noting the reliance on gui_state.selected_deployment for classification.
109-134:io_ratiois duplicated in the response: both at top level and insideorder_details.
io_ratiois set identically at line 114 (order_details.io_ratio) and line 130 (top-levelio_ratio). If theOrderDetailstruct genuinely requires both, consider deriving one from the other or documenting why the duplication exists. If one is vestigial, removing it would tighten the API surface.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/order/get_order.rs` around lines 109 - 134, The response currently duplicates io_ratio by setting it inside OrderDetailsInfo and again as a top-level field when constructing OrderDetail; update the OrderDetail construction to eliminate the duplication—either remove the top-level io_ratio assignment if the OrderDetail struct no longer needs it, or consolidate by assigning the top-level io_ratio from OrderDetailsInfo (e.g., use the same io_ratio variable or call order_details.io_ratio) to avoid two independent values; adjust the OrderDetail struct definition accordingly (remove the top-level io_ratio field if vestigial, or document/derive it consistently) and ensure OrderDetail/OrderDetailsInfo and any callers compile after the change.
🤖 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/order/get_order.rs`:
- Around line 90-100: Add a brief explanatory comment above the extraction of
inputs/outputs clarifying the single-vault assumption: state that OrderDetail
only models a single input/output pair (reference the use of
order.inputs_list().items(), order.outputs_list().items(), inputs.first(),
outputs.first() and the OrderDetail type), and note whether this is a business
invariant or a known limitation to be revisited if multi-vault RaindexOrder
support is added in the future; keep the comment concise and located immediately
above the input/output .first() calls.
---
Duplicate comments:
In `@src/routes/order/get_order.rs`:
- Around line 56-71: The process_get_order function currently should propagate
errors from data source calls instead of swallowing them; update
ds.get_order_quotes and ds.get_order_trades calls in process_get_order to use
the ? operator so errors bubble up (retain the io_ratio computation that falls
back to "-" only when a quote exists but its data is None), ensure
build_order_detail is called with the propagated Result flow, and reference the
symbols process_get_order, ds.get_order_quotes, ds.get_order_trades, io_ratio,
and build_order_detail when making these changes.
---
Nitpick comments:
In `@src/routes/order/get_order.rs`:
- Line 107: The conversion of order.timestamp_added() to u64 uses
try_into().unwrap_or(0) which silently maps overflow to epoch-zero; change
created_at handling in get_order.rs to match on
order.timestamp_added().try_into() (or use .map_err) and on Err emit a
tracing::warn including the original timestamp value and context (e.g.,
"timestamp_added overflow, falling back to 0") before using 0, and apply the
same pattern in map_trade where the same try_into().unwrap_or(0) occurs so
consumers and logs show a warning when the fallback is used.
- Around line 350-382: The mock currently discards the original ApiError in
MockOrderDataSource (fields orders/trades/quotes) and returns a new
ApiError::Internal in each Err(_) arm; change each match arm to return the
stored error (e.g., Err(e.clone()) or Err(e.to_owned())) instead of constructing
a new one so tests can assert the original message/variant; to do this add Clone
for ApiError or otherwise make ApiError clonable and ensure the match arms in
get_orders_by_hash, get_order_quotes, and get_order_trades return the cloned
stored error (use the existing variable names orders/trades/quotes in those
match expressions).
- Around line 73-82: Add coverage for the DCA branch in determine_order_type by
adding a unit test that constructs a RaindexOrder whose parsed_meta includes
ParsedMeta::DotrainGuiStateV1 with gui_state.selected_deployment containing
"dca" (test case-insensitive variants like "DCA" and "dCa") and assert
determine_order_type(&order) == OrderType::Dca; if creating a RaindexOrder with
populated parsed_meta is impractical, add a short comment next to
determine_order_type (or in the test module) explaining why the Dca branch isn't
tested and noting the reliance on gui_state.selected_deployment for
classification.
- Around line 109-134: The response currently duplicates io_ratio by setting it
inside OrderDetailsInfo and again as a top-level field when constructing
OrderDetail; update the OrderDetail construction to eliminate the
duplication—either remove the top-level io_ratio assignment if the OrderDetail
struct no longer needs it, or consolidate by assigning the top-level io_ratio
from OrderDetailsInfo (e.g., use the same io_ratio variable or call
order_details.io_ratio) to avoid two independent values; adjust the OrderDetail
struct definition accordingly (remove the top-level io_ratio field if vestigial,
or document/derive it consistently) and ensure OrderDetail/OrderDetailsInfo and
any callers compile after the change.
ℹ️ 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 (6)
Cargo.tomlsrc/routes/order/cancel.rssrc/routes/order/deploy_dca.rssrc/routes/order/deploy_solver.rssrc/routes/order/get_order.rssrc/test_helpers.rs
💤 Files with no reviewable changes (1)
- src/test_helpers.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- Cargo.toml
- src/routes/order/deploy_dca.rs
- src/routes/order/cancel.rs
- src/routes/order/deploy_solver.rs
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/routes/order/get_order.rs (1)
91-101: Single-vault comment: partially addressed.The comment at Line 90 says "The current application only supports single-pair orders" but stops short of clarifying whether this is a hard domain invariant or a known gap to revisit. The prior review requested that the comment state which it is.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/order/get_order.rs` around lines 91 - 101, The existing inline comment about single-pair orders is ambiguous; update the comment near the order handling code (around order.inputs_list(), order.outputs_list(), and the input/output extraction) to explicitly state whether "only supports single-pair orders" is a hard domain invariant (and must be enforced/validated) or a known limitation to be revisited (i.e., a TODO), and if it is an invariant, add a brief note describing where that invariant comes from and consider adding a validation check (e.g., assert or explicit error) so the intent is clear to future readers; reference the symbols order, inputs_list, outputs_list, input, and output when editing the comment.
🧹 Nitpick comments (4)
Cargo.toml (1)
16-16: Consider nativeasync fnin traits instead ofasync-traitThe codebase uses
async-traitin trait implementations (order/mod.rs, swap/mod.rs, get_order.rs). Since Rust 1.75 (Dec 2023) stabilized async fn in traits natively, this dependency could be removed if the project's MSRV is ≥ 1.75. This would eliminate a proc-macro compile dependency and avoid the runtime boxing overhead thatasync-traitintroduces.Currently, no explicit MSRV is set in Cargo.toml. Consider either (a) declaring
rust-version = "1.75"or later if applicable, or (b) refactoring trait implementations to use native async fn syntax if the project targets that version or newer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Cargo.toml` at line 16, The project currently depends on the async-trait crate; decide whether to remove it by either (A) declaring a minimum Rust version >= 1.75 in Cargo.toml (add rust-version = "1.75" or later) to allow native async fn in traits, or (B) refactor trait definitions and implementations in files using async-trait (e.g., order::mod.rs, swap::mod.rs, get_order.rs) to use native async fn trait syntax and remove #[async_trait] usages and the async-trait dependency; update Cargo.toml to remove the async-trait line if you fully refactor or add the rust-version field if you choose to require 1.75+.src/routes/order/get_order.rs (3)
489-493: Missing test for the DCA order type path indetermine_order_type.
test_determine_order_type_solver_defaultcovers only the defaultSolverbranch. There is no test exercising theOrderType::Dcareturn whenparsedMetacontains aDotrainGuiStateV1with aselected_deploymentthat includes"dca".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/order/get_order.rs` around lines 489 - 493, Add a unit test that exercises the DCA branch of determine_order_type by constructing a mock Order whose parsedMeta contains a DotrainGuiStateV1 with selected_deployment including "dca" and asserting determine_order_type(&order) == OrderType::Dca; update or add a new async test (similar to test_determine_order_type_solver_default) that uses mock_order(), mutates its parsedMeta or provides a mocked parsed_meta field containing the DotrainGuiStateV1 JSON/struct with "dca" in selected_deployment, and calls determine_order_type to verify the Dca result.
73-82: Document the"dca"naming convention relied on bydetermine_order_type.The
contains("dca")check (Line 76) silently depends on every DCA deployment having"dca"in itsselected_deploymentname. Add a brief comment stating this naming contract; without it, future deployments with names like"dollar-cost"would silently fall through toSolver.📝 Suggested comment
fn determine_order_type(order: &RaindexOrder) -> OrderType { for meta in order.parsed_meta() { if let ParsedMeta::DotrainGuiStateV1(gui_state) = meta { + // DCA orders are identified by the convention that `selected_deployment` + // contains the substring "dca" (case-insensitive). if gui_state.selected_deployment.to_lowercase().contains("dca") { return OrderType::Dca; } } } OrderType::Solver }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/order/get_order.rs` around lines 73 - 82, Add a short clarifying comment above the contains("dca") check in determine_order_type to document the naming contract: explain that detection relies on selected_deployment containing the substring "dca" (case-insensitive) for all DCA deployments, and note the consequence that deployments named without that substring (e.g., "dollar-cost") will be treated as Solver; reference ParsedMeta::DotrainGuiStateV1 and gui_state.selected_deployment so future maintainers know why the substring check exists and can update it if the naming convention changes.
166-311:stub_raindex_client,order_json, andmock_orderare duplicated fromsrc/test_helpers.rs.All three helpers in Lines 166-311 are identical to the
pub(crate)versions already insrc/test_helpers.rs. Import them instead to avoid divergence.♻️ Proposed refactor
use crate::test_helpers::{ - basic_auth_header, mock_invalid_raindex_config, seed_api_key, TestClientBuilder, + basic_auth_header, mock_invalid_raindex_config, mock_order, seed_api_key, TestClientBuilder, };Then remove the local
stub_raindex_client,order_json, andmock_orderdefinitions (Lines 166-311). Themock_ordercall sites do not need to change.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/order/get_order.rs` around lines 166 - 311, Remove the duplicated local helper definitions stub_raindex_client, order_json, and mock_order and instead import the existing pub(crate) helpers from the shared test_helpers module (use the same names: stub_raindex_client, order_json, mock_order); delete the local functions (the bodies currently in Lines 166-311) and add a use/import for those symbols so existing mock_order call sites remain unchanged.
🤖 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/order/get_order.rs`:
- Line 108: Replace the silent unwrap_or(0) on timestamp conversions so
overflow/convert errors are logged and not silently mapped to epoch: change the
conversion of order.timestamp_added() (where created_at is set) to attempt the
try_into() and on Err emit a warn/error with context (include order id or
relevant key) and then use a safe fallback value, and apply the identical change
in map_trade for the trade.timestamp() conversion at the other occurrence;
reference the order.timestamp_added(), created_at variable, and
map_trade/trade.timestamp() to find spots to modify.
---
Duplicate comments:
In `@src/routes/order/get_order.rs`:
- Around line 91-101: The existing inline comment about single-pair orders is
ambiguous; update the comment near the order handling code (around
order.inputs_list(), order.outputs_list(), and the input/output extraction) to
explicitly state whether "only supports single-pair orders" is a hard domain
invariant (and must be enforced/validated) or a known limitation to be revisited
(i.e., a TODO), and if it is an invariant, add a brief note describing where
that invariant comes from and consider adding a validation check (e.g., assert
or explicit error) so the intent is clear to future readers; reference the
symbols order, inputs_list, outputs_list, input, and output when editing the
comment.
---
Nitpick comments:
In `@Cargo.toml`:
- Line 16: The project currently depends on the async-trait crate; decide
whether to remove it by either (A) declaring a minimum Rust version >= 1.75 in
Cargo.toml (add rust-version = "1.75" or later) to allow native async fn in
traits, or (B) refactor trait definitions and implementations in files using
async-trait (e.g., order::mod.rs, swap::mod.rs, get_order.rs) to use native
async fn trait syntax and remove #[async_trait] usages and the async-trait
dependency; update Cargo.toml to remove the async-trait line if you fully
refactor or add the rust-version field if you choose to require 1.75+.
In `@src/routes/order/get_order.rs`:
- Around line 489-493: Add a unit test that exercises the DCA branch of
determine_order_type by constructing a mock Order whose parsedMeta contains a
DotrainGuiStateV1 with selected_deployment including "dca" and asserting
determine_order_type(&order) == OrderType::Dca; update or add a new async test
(similar to test_determine_order_type_solver_default) that uses mock_order(),
mutates its parsedMeta or provides a mocked parsed_meta field containing the
DotrainGuiStateV1 JSON/struct with "dca" in selected_deployment, and calls
determine_order_type to verify the Dca result.
- Around line 73-82: Add a short clarifying comment above the contains("dca")
check in determine_order_type to document the naming contract: explain that
detection relies on selected_deployment containing the substring "dca"
(case-insensitive) for all DCA deployments, and note the consequence that
deployments named without that substring (e.g., "dollar-cost") will be treated
as Solver; reference ParsedMeta::DotrainGuiStateV1 and
gui_state.selected_deployment so future maintainers know why the substring check
exists and can update it if the naming convention changes.
- Around line 166-311: Remove the duplicated local helper definitions
stub_raindex_client, order_json, and mock_order and instead import the existing
pub(crate) helpers from the shared test_helpers module (use the same names:
stub_raindex_client, order_json, mock_order); delete the local functions (the
bodies currently in Lines 166-311) and add a use/import for those symbols so
existing mock_order call sites remain unchanged.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Cargo.tomlsrc/routes/order/get_order.rssrc/test_helpers.rs
💤 Files with no reviewable changes (1)
- src/test_helpers.rs
|
|
||
| let trade_entries: Vec<OrderTradeEntry> = trades.iter().map(map_trade).collect(); | ||
|
|
||
| let created_at: u64 = order.timestamp_added().try_into().unwrap_or(0); |
There was a problem hiding this comment.
Silent unwrap_or(0) on timestamp conversion should emit a warning.
If order.timestamp_added() holds a value that overflows u64, the response will show created_at = 0 (Unix epoch) with no diagnostic trace. Same pattern is repeated at Line 139 for trade timestamps.
As per coding guidelines, route handlers must log errors and key decisions; a silent fallback here violates that intent.
🛡️ Proposed fix
- let created_at: u64 = order.timestamp_added().try_into().unwrap_or(0);
+ let created_at: u64 = order.timestamp_added().try_into().unwrap_or_else(|_| {
+ tracing::warn!(order_hash = ?order.order_hash(), "timestamp_added overflows u64, defaulting to 0");
+ 0
+ });Apply the same pattern to Line 139 in map_trade:
- let timestamp: u64 = trade.timestamp().try_into().unwrap_or(0);
+ let timestamp: u64 = trade.timestamp().try_into().unwrap_or_else(|_| {
+ tracing::warn!(trade_id = ?trade.id(), "trade timestamp overflows u64, defaulting to 0");
+ 0
+ });📝 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.
| let created_at: u64 = order.timestamp_added().try_into().unwrap_or(0); | |
| let created_at: u64 = order.timestamp_added().try_into().unwrap_or_else(|_| { | |
| tracing::warn!(order_hash = ?order.order_hash(), "timestamp_added overflows u64, defaulting to 0"); | |
| 0 | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/routes/order/get_order.rs` at line 108, Replace the silent unwrap_or(0)
on timestamp conversions so overflow/convert errors are logged and not silently
mapped to epoch: change the conversion of order.timestamp_added() (where
created_at is set) to attempt the try_into() and on Err emit a warn/error with
context (include order id or relevant key) and then use a safe fallback value,
and apply the identical change in map_trade for the trade.timestamp() conversion
at the other occurrence; reference the order.timestamp_added(), created_at
variable, and map_trade/trade.timestamp() to find spots to modify.
There was a problem hiding this comment.
These are Unix timestamps from on-chain data — overflow into u64 isn't realistic. Leaving as-is.
There was a problem hiding this comment.
@findolor, understood — you're right that Unix timestamps in practice won't exceed u64 bounds. Thanks for the clarification.
🧠 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.
2 participants
Dependent PRs
Motivation
See issues:
Implements the
GET /v1/order/{order_hash}endpoint that looks up a single order by its hash and returns full order details including quotes, trades, and order type classification.Solution
src/routes/order.rsinto a directory module (src/routes/order/mod.rs) with one file per route:get_order.rs,cancel.rs,deploy_dca.rs,deploy_solver.rsget_orderhandler that resolves an order hash via Raindex, fetches associated quotes and trades, determines order type (DCA vs Solver) from parsed GUI metadata, and returns a fullOrderDetailresponseOrderDataSourcetrait (backed byasync-trait) to decouple the handler from Raindex internals, enabling unit testabilityApiErrorinstead of silently returning empty resultsasync-traitdependencyChecks
By submitting this for review, I'm confirming I've done the following:
fix #24
Summary by CodeRabbit
Refactor
New Features
Chores