Open
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
📝 WalkthroughWalkthroughThis PR refactors the trades routing module, decomposing a monolithic routes file into separate submodules for distinct endpoints. It implements the GET /v1/trades/tx/{tx_hash} endpoint with business logic for aggregating trade data, introduces a TradesDataSource abstraction layer, and adds a NotYetIndexed error variant for HTTP 202 responses to handle indexing delays. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Handler as API Handler<br/>(get_trades_by_tx)
participant DS as TradesDataSource<br/>(RaindexTradesDataSource)
participant RC as RaindexClient
participant ODS as OrderDataSource
Client->>Handler: GET /v1/trades/tx/{tx_hash}
Note over Handler: Validate auth, rate limit, span
Handler->>DS: get_trades_by_tx(tx_hash)
DS->>RC: get_all_orderbooks()
RC-->>DS: [orderbooks]
loop For each orderbook
DS->>RC: get_trades_for_transaction(...)
alt Success
RC-->>DS: trades
else IndexingTimeout
DS-->>Handler: ApiError::NotYetIndexed
Handler-->>Client: 202 Accepted
else Other Error
DS-->>Handler: ApiError::Internal
Handler-->>Client: 500 Internal Error
end
end
DS->>ODS: fetch order details per trade
ODS-->>DS: order data
DS->>DS: aggregate totals, compute ratios
DS-->>Handler: Json<TradesByTxResponse>
Handler-->>Client: 200 OK with trades data
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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
Comment on lines
+152
to
+169
| let zero = Float::parse("0".to_string()).map_err(|e| { | ||
| tracing::error!(error = %e, "float parse error"); | ||
| ApiError::Internal("float parse error".into()) | ||
| })?; | ||
| let abs_output = (zero - output_vc.amount()).map_err(|e| { | ||
| tracing::error!(error = %e, "float sub error"); | ||
| ApiError::Internal("float arithmetic error".into()) | ||
| })?; | ||
| let actual_io_ratio = (input_vc.amount() / abs_output) | ||
| .map_err(|e| { | ||
| tracing::error!(error = %e, "float div error"); | ||
| ApiError::Internal("float arithmetic error".into()) | ||
| })? | ||
| .format() | ||
| .map_err(|e| { | ||
| tracing::error!(error = %e, "float format error"); | ||
| ApiError::Internal("float format error".into()) | ||
| })?; |
There was a problem hiding this comment.
Division-by-zero risk if output amount is zero.
abs_output (line 156) and abs_total_output (line 197) are computed as zero - output_amount, assuming output amounts are negative. If any trade has a zero output amount, the division on lines 160 and 201 will attempt to divide by zero. While the Float library likely returns an Err that is handled, the resulting ApiError::Internal("float arithmetic error") is opaque to the caller. Consider adding an explicit check and returning a more descriptive error or handling the zero-output edge case gracefully (e.g., reporting the ratio as "N/A" or "0").
Also applies to: 193-210
🤖 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 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.
Collaborator
Author
There was a problem hiding this comment.
Not applicable — the smart contract guarantees non-zero output amounts for indexed trades.
OrderBookV6.takeOrders4() explicitly skips orders with zero output before emitting the TakeOrderV3 event the subgraph indexes:
} else if (orderIOCalculation.outputMax.isZero()) {
emit OrderZeroAmount(msg.sender, order.owner, orderHash);
} else {
// only non-zero outputMax reaches here
emit TakeOrderV3(msg.sender, takeOrderConfig, takerInput, takerOutput);
}Both computation branches derive amounts from this non-zero outputMax, so the vault balance decrease is always non-zero.
There was a problem hiding this comment.
@findolor, thank you for the clarification! That's an important architectural guarantee I wasn't aware of. If the contract ensures all indexed trades have non-zero outputs by filtering at the event emission level, then the division-by-zero check is indeed unnecessary here. I appreciate you taking the time to explain the upstream contract logic.
✏️ Learnings added
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 the Rain OrderBookV6 contract, the takeOrders4() function guarantees that all TakeOrderV3 events (which the subgraph indexes) contain non-zero output amounts. The contract explicitly skips orders where orderIOCalculation.outputMax.isZero() and emits OrderZeroAmount instead. Therefore, when processing trades from the indexed data in the API, output amounts are guaranteed to be non-zero and division-by-zero checks are not required for IO ratio calculations.
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: 36
File: src/routes/order/deploy_dca.rs:14-16
Timestamp: 2026-02-24T10:02:37.086Z
Learning: In the Rain orderbook architecture, vault naming uses the order's perspective, not the user's perspective. From the order's viewpoint: "input" vaults hold tokens the order receives (asks for), and "output" vaults hold tokens the order sends out (offers). For example, in a DCA order where a user sells USDC to buy ETH, the order's output vault holds USDC (what it sends out). The SDK's `get_vaults_and_deposits` maps deposits to `deployment.order.outputs`, so budget deposits must go to the output vault even though from a user perspective it might seem like an "input."
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.
This was referenced Feb 24, 2026
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.
Replaces #39.
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
Chores