Conversation
Add trades-by-transaction endpoint that queries all configured orderbook subgraphs, fetches order owners concurrently, and computes per-trade and aggregate IO ratios. Includes NotYetIndexed (202) error for unindexed txs, TradesDataSource trait for testability, and comprehensive unit tests. fix #28
📝 WalkthroughWalkthroughImplements HTTP GET endpoint Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant APIHandler as GET /v1/trades/tx/{tx_hash}
participant RateLimiter
participant Auth
participant RaindexProvider as Raindex Provider
participant Processor as Data Processor
participant Response
Client->>APIHandler: HTTP GET request
APIHandler->>RateLimiter: Check global rate limit
RateLimiter-->>APIHandler: ✓ Allowed
APIHandler->>Auth: Validate authenticated key
Auth-->>APIHandler: ✓ Authenticated
APIHandler->>RaindexProvider: Acquire read lock
RaindexProvider-->>APIHandler: Lock acquired
APIHandler->>RaindexProvider: get_trades_for_transaction(tx_hash)
alt Transaction Found & Indexed
RaindexProvider-->>Processor: RaindexTradesListResult
Processor->>Processor: Compute IO ratios & totals
Processor->>Processor: Build TradeByTxEntry list
Processor-->>APIHandler: TradesByTxResponse
APIHandler->>Response: 200 OK + JSON
else Transaction Not Found
RaindexProvider-->>APIHandler: NotFound error
APIHandler->>Response: 404 Not Found
else Indexing In Progress
RaindexProvider-->>APIHandler: TransactionIndexingTimeout
APIHandler->>Response: 202 Accepted (NotYetIndexed)
else Other Error
RaindexProvider-->>APIHandler: Internal error
APIHandler->>Response: 500 Internal Server Error
end
Response-->>Client: HTTP Response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
src/routes/trades/get_by_tx.rs (2)
83-97: UseB256directly as theorder_cachekey instead ofString.The order hash goes through a round-trip:
order_hash()→.to_string()→B256::from_str(...)in multiple places (lines 88, 171). Theorder_cacheis keyed byString(line 110) and looked up via.to_string()(line 130). UsingB256as the key eliminates all string conversions, removes the parse-failure error paths (lines 88–91, 171–174), and simplifies the code.Sketch of the refactor
- let order_cache: HashMap< - String, - Option<rain_orderbook_common::raindex_client::orders::RaindexOrder>, - > = order_results + let order_cache: HashMap< + B256, + Option<rain_orderbook_common::raindex_client::orders::RaindexOrder>, + > = order_results .into_iter() .collect::<Result<Vec<_>, _>>()? .into_iter() - .map(|(hash, order)| (hash.to_string(), order)) + .map(|(hash, order)| (hash, order)) .collect();Then look up with the
B256directly instead of converting to string first, and skip the re-parse at line 171.Also applies to: 109-117, 130-138, 171-174
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/trades/get_by_tx.rs` around lines 83 - 97, The code currently round-trips order_hash() to String and back to B256 and uses order_cache keyed by String; change order_cache to use B256 keys and stop converting order hashes to strings: build unique_hashes as Vec<B256> by mapping order_hash() (or its bytes) directly without parse-from-string (update the unique_hashes block and remove the B256::from_str error handling), change all lookups/insertions to use the B256 key (replace .to_string() lookups with direct B256 lookups), and remove the redundant re-parse at the later lookup (the parse in the block around the earlier re-parse, e.g., where order_cache is accessed at lines referenced in the review) so there is no string conversion or parse failure path.
119-127: RepetitiveFloat::parse("0")with identical error handling.
Float::parse("0".to_string())is called four times (lines 120, 124, 152, 193) with the same error-mapping boilerplate. A small helper would reduce noise:Example helper
fn float_zero() -> Result<Float, ApiError> { Float::parse("0".to_string()).map_err(|e| { tracing::error!(error = %e, "float parse error"); ApiError::Internal("float parse error".into()) }) }Also applies to: 152-155, 193-196
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/trades/get_by_tx.rs` around lines 119 - 127, Create a small helper function (e.g., float_zero -> Result<Float, ApiError>) that encapsulates Float::parse("0".to_string()) with the current map_err closure (tracing::error! and ApiError::Internal) and replace the repeated calls initializing total_input, total_output and any other zero-initialized Float occurrences in get_by_tx (the variables named total_input, total_output and other similar parse sites) to call float_zero() instead; ensure you update use sites to handle the Result the same way as before so behavior/error logging remains identical.src/routes/trades/mod.rs (1)
28-53: Sequential orderbook iteration — early-return onTradesIndexingTimeoutdiscards partial results.If orderbook A returns trades and orderbook B hits
TradesIndexingTimeout, the already-collected trades from A are discarded. This all-or-nothing behavior seems intentional for consistency, but it's worth documenting this design decision since it might surprise callers. If the number of configured orderbooks grows, the sequential iteration could also become a latency bottleneck — parallel queries withjoin_allwould help there.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/trades/mod.rs` around lines 28 - 53, The current loop over orderbooks collects trades into all_trades but returns early on RaindexError::TradesIndexingTimeout, discarding partial results; change the logic in the loop that calls self.client.get_trades_for_transaction to (a) record a timeout occurrence instead of immediate return (e.g., set a flag like saw_timeout) and continue collecting from other orderbooks so all_trades preserves partial results, and (b) after the loop, if saw_timeout is true return ApiError::NotYetIndexed (or include both partial results and a NotYetIndexed indicator per design); additionally consider switching the sequential loop to parallel queries using futures::future::join_all on calls to get_trades_for_transaction to reduce latency and document the chosen behavior around partial results and timeouts.src/error.rs (1)
61-67: Info-level logging is appropriate here.Good call using
tracing::info!rather thanwarn!since this is an expected transient state, not an error. However, consider adding a unit test for theNotYetIndexedvariant similar to the existing error tests (lines 146–174) to ensure the 202 status andNOT_YET_INDEXEDcode are correctly returned.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/error.rs` around lines 61 - 67, Add a unit test that mirrors the existing error tests (lines 146–174) to verify ApiError::NotYetIndexed behavior: construct or trigger ApiError::NotYetIndexed, call the same conversion/response code path used by the other error tests, and assert the HTTP status is 202 and the error code string equals "NOT_YET_INDEXED"; reference the ApiError::NotYetIndexed variant and the response conversion function used in the other tests to place the assertion so it fails if the status or code 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/trades/get_by_address.rs`:
- Line 40: The handler currently calls todo!() inside the async closure passed
to run_with_client, which will panic at runtime; replace the todo!() with a
proper error return (e.g., return an Err(ApiError::Internal("not
implemented".into())) or construct and return an appropriate ApiError result
type from the async block) or gate the entire route behind a feature flag so it
is not registered; locate the closure passed to run_with_client in
get_by_address.rs and update it to return a Result type consistent with the
route handler instead of calling todo!(), referencing ApiError::Internal (or the
project's error constructor) to produce a graceful error response.
In `@src/routes/trades/get_by_tx.rs`:
- Around line 152-169: The code computes abs_output from zero -
output_vc.amount() and then divides input_vc.amount() by abs_output to get
actual_io_ratio (and similarly for abs_total_output/total_io_ratio); add an
explicit zero check after computing abs_output and abs_total_output: if either
is zero (or very close to zero), avoid the divide and return a clear,
descriptive result (e.g., set actual_io_ratio/total_io_ratio to "N/A" or "0" or
return ApiError::BadRequest with a descriptive message) instead of relying on
the Float error; update the branches around the Float division/format steps
(references: zero, abs_output, actual_io_ratio, abs_total_output,
total_io_ratio) to short-circuit on zero and log a specific message before
returning the chosen response.
---
Nitpick comments:
In `@src/error.rs`:
- Around line 61-67: Add a unit test that mirrors the existing error tests
(lines 146–174) to verify ApiError::NotYetIndexed behavior: construct or trigger
ApiError::NotYetIndexed, call the same conversion/response code path used by the
other error tests, and assert the HTTP status is 202 and the error code string
equals "NOT_YET_INDEXED"; reference the ApiError::NotYetIndexed variant and the
response conversion function used in the other tests to place the assertion so
it fails if the status or code change.
In `@src/routes/trades/get_by_tx.rs`:
- Around line 83-97: The code currently round-trips order_hash() to String and
back to B256 and uses order_cache keyed by String; change order_cache to use
B256 keys and stop converting order hashes to strings: build unique_hashes as
Vec<B256> by mapping order_hash() (or its bytes) directly without
parse-from-string (update the unique_hashes block and remove the B256::from_str
error handling), change all lookups/insertions to use the B256 key (replace
.to_string() lookups with direct B256 lookups), and remove the redundant
re-parse at the later lookup (the parse in the block around the earlier
re-parse, e.g., where order_cache is accessed at lines referenced in the review)
so there is no string conversion or parse failure path.
- Around line 119-127: Create a small helper function (e.g., float_zero ->
Result<Float, ApiError>) that encapsulates Float::parse("0".to_string()) with
the current map_err closure (tracing::error! and ApiError::Internal) and replace
the repeated calls initializing total_input, total_output and any other
zero-initialized Float occurrences in get_by_tx (the variables named
total_input, total_output and other similar parse sites) to call float_zero()
instead; ensure you update use sites to handle the Result the same way as before
so behavior/error logging remains identical.
In `@src/routes/trades/mod.rs`:
- Around line 28-53: The current loop over orderbooks collects trades into
all_trades but returns early on RaindexError::TradesIndexingTimeout, discarding
partial results; change the logic in the loop that calls
self.client.get_trades_for_transaction to (a) record a timeout occurrence
instead of immediate return (e.g., set a flag like saw_timeout) and continue
collecting from other orderbooks so all_trades preserves partial results, and
(b) after the loop, if saw_timeout is true return ApiError::NotYetIndexed (or
include both partial results and a NotYetIndexed indicator per design);
additionally consider switching the sequential loop to parallel queries using
futures::future::join_all on calls to get_trades_for_transaction to reduce
latency and document the chosen behavior around partial results and timeouts.
ℹ️ 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 (9)
AGENTS.mdCargo.tomllib/rain.orderbooksrc/error.rssrc/main.rssrc/routes/trades.rssrc/routes/trades/get_by_address.rssrc/routes/trades/get_by_tx.rssrc/routes/trades/mod.rs
💤 Files with no reviewable changes (1)
- src/routes/trades.rs
The new submodule version adds owner and io_ratio directly to RaindexTrade and returns RaindexTradesListResult with pair summaries, eliminating the need for separate order lookups and manual Float arithmetic. Also adapts to get_orders pagination and async get_raindex_client changes.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/routes/trades/mod.rs (1)
11-14: Consider extending the trait for address-based queries to maintain consistency.The
get_trades_by_addresshandler currently uses directshared_raindexState access viarun_with_client(), whileget_trades_by_txuses theTradesDataSourcetrait abstraction. Since the address-based handler is still incomplete, adding a corresponding trait method would maintain consistency with the existing data source abstraction pattern once the implementation is finalized.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/trades/mod.rs` around lines 11 - 14, Add a new trait method to TradesDataSource (e.g., async fn get_trades_by_address(&self, address: H160) -> Result<RaindexTradesListResult, ApiError>) so address-based queries follow the same abstraction as get_trades_by_tx, then update all implementations of TradesDataSource to implement get_trades_by_address and refactor the get_trades_by_address handler to call state.run_with_client().get_trades_by_address(...) instead of accessing shared_raindex directly; ensure the signature and return type match RaindexTradesListResult/ApiError used by get_trades_by_tx.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/routes/trades/mod.rs`:
- Around line 11-14: Add a new trait method to TradesDataSource (e.g., async fn
get_trades_by_address(&self, address: H160) -> Result<RaindexTradesListResult,
ApiError>) so address-based queries follow the same abstraction as
get_trades_by_tx, then update all implementations of TradesDataSource to
implement get_trades_by_address and refactor the get_trades_by_address handler
to call state.run_with_client().get_trades_by_address(...) instead of accessing
shared_raindex directly; ensure the signature and return type match
RaindexTradesListResult/ApiError used by get_trades_by_tx.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6f67f612-ca02-455d-a43a-e11730689ce0
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
lib/rain.orderbooksrc/raindex/config.rssrc/routes/order/mod.rssrc/routes/swap/mod.rssrc/routes/trades/get_by_tx.rssrc/routes/trades/mod.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/rain.orderbook
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/routes/trades/get_by_address.rs (1)
39-39:⚠️ Potential issue | 🔴 CriticalRemove the
todo!()panic from this handler.Any request that reaches this route will abort the task instead of returning an
ApiError, so the documented API contract is never honored.Minimal fix
- todo!() + Err(ApiError::Internal( + "get_trades_by_address is not implemented".into(), + ))As per coding guidelines, "Never use
expectorunwrapin 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/trades/get_by_address.rs` at line 39, The handler in src/routes/trades/get_by_address.rs currently calls todo!(), which panics; replace that panic with proper error handling that returns an ApiError through the handler's Result type (i.e., remove todo!() and return Err(ApiError::... ) or implement the real logic and propagate errors using ?). Locate the get_by_address route handler (the function containing todo!()) and ensure you never use unwrap/expect; instead construct or map errors into ApiError (e.g., ApiError::internal or an appropriate variant) so the API contract is honored.
🤖 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/trades/get_by_tx.rs`:
- Around line 101-115: The code currently takes only the first pair summary
(result.summary().and_then(|s| s.first())) to populate TradesTotals, which drops
all other pairs; instead iterate over all summaries from result.summary() and
aggregate totals: sum each summary's numeric input and output amounts to produce
total_input_amount and total_output_amount, and compute average_io_ratio as an
appropriate aggregate (e.g., a weighted average using per-pair IO ratios
weighted by input/output or total volumes). Replace uses of
summary.formatted_total_input()/formatted_total_output()/formatted_average_io_ratio()
for the single summary with aggregation over all summaries and then format the
final aggregated values into strings for TradesTotals when building
TradesByTxResponse.
---
Duplicate comments:
In `@src/routes/trades/get_by_address.rs`:
- Line 39: The handler in src/routes/trades/get_by_address.rs currently calls
todo!(), which panics; replace that panic with proper error handling that
returns an ApiError through the handler's Result type (i.e., remove todo!() and
return Err(ApiError::... ) or implement the real logic and propagate errors
using ?). Locate the get_by_address route handler (the function containing
todo!()) and ensure you never use unwrap/expect; instead construct or map errors
into ApiError (e.g., ApiError::internal or an appropriate variant) so the API
contract is honored.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4710cca8-411c-4ffa-9833-46d28a5b6d4a
📒 Files selected for processing (6)
src/error.rssrc/main.rssrc/routes/order/mod.rssrc/routes/trades/get_by_address.rssrc/routes/trades/get_by_tx.rssrc/routes/trades/mod.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- src/main.rs
- src/routes/trades/mod.rs
- src/error.rs
| let summary = result.summary().and_then(|s| s.first()).ok_or_else(|| { | ||
| tracing::error!("no pair summary in trades result"); | ||
| ApiError::Internal("missing pair summary".into()) | ||
| })?; | ||
|
|
||
| Ok(Json(TradesByTxResponse { | ||
| tx_hash, | ||
| block_number, | ||
| timestamp, | ||
| sender, | ||
| trades: trade_entries, | ||
| totals: TradesTotals { | ||
| total_input_amount: summary.formatted_total_input().to_string(), | ||
| total_output_amount: summary.formatted_total_output().to_string(), | ||
| average_io_ratio: summary.formatted_average_io_ratio().to_string(), |
There was a problem hiding this comment.
Don't derive transaction totals from only the first pair summary.
result.summary() is pair-scoped, so first() makes totals ignore every pair after the first while trades still includes them all. For mixed-pair transactions, total_input_amount, total_output_amount, and average_io_ratio become wrong.
Minimal guard
- let summary = result.summary().and_then(|s| s.first()).ok_or_else(|| {
- tracing::error!("no pair summary in trades result");
- ApiError::Internal("missing pair summary".into())
- })?;
+ let summaries = result.summary().ok_or_else(|| {
+ tracing::error!("no pair summary in trades result");
+ ApiError::Internal("missing pair summary".into())
+ })?;
+ if summaries.len() != 1 {
+ tracing::error!(
+ summary_count = summaries.len(),
+ "expected exactly one pair summary for tx"
+ );
+ return Err(ApiError::Internal(
+ "mixed-pair transaction summaries are not supported".into(),
+ ));
+ }
+ let summary = &summaries[0];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/routes/trades/get_by_tx.rs` around lines 101 - 115, The code currently
takes only the first pair summary (result.summary().and_then(|s| s.first())) to
populate TradesTotals, which drops all other pairs; instead iterate over all
summaries from result.summary() and aggregate totals: sum each summary's numeric
input and output amounts to produce total_input_amount and total_output_amount,
and compute average_io_ratio as an appropriate aggregate (e.g., a weighted
average using per-pair IO ratios weighted by input/output or total volumes).
Replace uses of
summary.formatted_total_input()/formatted_total_output()/formatted_average_io_ratio()
for the single summary with aggregation over all summaries and then format the
final aggregated values into strings for TradesTotals when building
TradesByTxResponse.
There was a problem hiding this comment.
Currently we don't have mixed pair transactions in st0x. if we ever have those we would also need to change the response.
There was a problem hiding this comment.
@findolor, understood — that's a useful constraint to know. Since mixed-pair transactions are intentionally out of scope for now, first() is a reasonable simplification.
That said, it might be worth adding a lightweight defensive guard to catch unexpected multi-pair data in production before it silently produces wrong totals:
let summaries = result.summary().ok_or_else(|| {
tracing::error!("no pair summary in trades result");
ApiError::Internal("missing pair summary".into())
})?;
if summaries.len() > 1 {
tracing::warn!(
summary_count = summaries.len(),
"unexpected multi-pair summary; only the first pair will be used for totals"
);
}
let summary = &summaries[0];This keeps the current single-pair behaviour intact while surfacing the anomaly in logs if it ever occurs. Up to you whether to add it — happy to close this comment if you'd prefer to leave the code as-is for now.
✏️ Learnings added
Learnt from: findolor
Repo: ST0x-Technology/st0x.rest.api PR: 50
File: src/routes/trades/get_by_tx.rs:101-115
Timestamp: 2026-03-10T07:35:34.283Z
Learning: In src/routes/trades/get_by_tx.rs, using result.summary().and_then(|s| s.first()) to derive TradesTotals is intentional: st0x does not currently have mixed-pair transactions, so only one pair summary is expected per transaction. If mixed-pair transactions are ever supported, the TradesByTxResponse structure and this aggregation logic would both need to change. Don't flag single-pair summary assumptions in this file as bugs unless the product scope has changed to include multi-pair transactions.
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: 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.
Learnt from: findolor
Repo: ST0x-Technology/st0x.rest.api PR: 50
File: src/routes/trades/get_by_tx.rs:152-169
Timestamp: 2026-02-25T07:09:51.723Z
Learning: In trade-processing code (e.g., src/routes/trades/get_by_tx.rs and related files), rely on the guarantee from Rain OrderBookV6 takeOrders4 that all TakeOrderV3 events have non-zero output amounts. Since orders with outputMax.isZero() are skipped and OrderZeroAmount is emitted, division-by-zero checks for IO ratio calculations when processing indexed trades from the API are unnecessary. Apply this guidance to similar files handling indexed trade data, and ensure tests reflect the non-zero assumption.
Replaces #39.
Dependent PR
Motivation
See issues:
Solution
Adds the
GET /v1/trades/tx/{tx_hash}endpoint that retrieves all trades associated with a given transaction hash. Updatesrain.orderbooksubmodule, addsNotYetIndexed(202) error variant, restructures trades module into a directory withTradesDataSourcetrait, and includes 6 unit tests.Checks
By submitting this for review, I'm confirming I've done the following:
fix #28
Summary by CodeRabbit
Release Notes
New Features
/v1/trades/tx/{tx_hash}to fetch trades by transaction and/v1/trades/{address}to fetch trades by address with pagination support.Bug Fixes
Documentation