Add orders total count for pagination support#2460
Open
Conversation
New SQL query that counts orders matching the same filters as fetch_orders but without the expensive joins for order details. Includes builder, row extraction helper, and unit tests.
Add page/page_size fields to FetchOrdersArgs and generate LIMIT/OFFSET clauses in the SQL query when both are provided. Includes tests for offset calculation, edge cases, and clause omission.
OrderbookSubgraphClient::orders_count paginates through orders_list to count total orders. MultiOrderbookSubgraphClient::orders_count sums counts across all subgraphs, gracefully handling errors.
Replace Vec<RaindexOrder> return type with RaindexOrdersListResult containing both orders and total_count for pagination. Both local DB and subgraph data sources now fetch the count when a page is requested.
Update OrdersListTable to use dataSelector to extract orders from
the new result shape. Update JS API and component tests to use
{ orders, totalCount } instead of plain arrays.
Contributor
WalkthroughAdds optional pagination to order fetching (page and page_size), injects LIMIT/OFFSET into the local DB query when supplied, and adds a new count flow (fetch_orders_count) to return total matching orders; updates data source signatures, subgraph clients, wasm bindings, tests, and UI to consume a paginated result wrapper. Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant OrdersDataSource
participant FetchOrders
participant FetchOrdersCount
participant Local_DB
Client->>OrdersDataSource: list(page?, page_size?)
OrdersDataSource->>FetchOrders: build & execute fetch stmt (PAGINATION_CLAUSE injected if paged)
FetchOrders->>Local_DB: Execute SELECT (with LIMIT/OFFSET if paged)
Local_DB-->>FetchOrders: rows (page of orders)
FetchOrders-->>OrdersDataSource: orders
alt page provided
OrdersDataSource->>FetchOrdersCount: build & execute COUNT stmt
FetchOrdersCount->>Local_DB: Execute COUNT query
Local_DB-->>FetchOrdersCount: count
FetchOrdersCount-->>OrdersDataSource: total_count
else no page
OrdersDataSource-->>OrdersDataSource: total_count = 0
end
OrdersDataSource-->>Client: RaindexOrdersListResult { orders, total_count }
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 3✅ 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/common/src/local_db/query/fetch_orders_count/mod.rs`:
- Around line 108-115: The variable owners_lower is misleading because it is
only sorted and deduplicated, not lowercased; rename it (e.g., owners_sorted or
owners_deduped) and update its usages (the let binding and the subsequent
stmt.bind_list_clause call that maps
owners_lower.into_iter().map(SqlValue::from)) so the name accurately reflects
sorting/deduping; ensure args.owners.clone(), sort(), dedup() remain the same
and only the identifier is changed.
- Around line 131-171: In the branch where input_tokens == output_tokens, avoid
pushing the same tokens twice by creating only one set of positional
placeholders and params: build input_placeholders and push each token once into
stmt.params (using the existing input_placeholders and stmt.push loop), then
reuse the same placeholder list string for both "{input_list}" and
"{output_list}" when constructing COMBINED_TOKENS_CLAUSE_BODY; update stmt.sql
replacements for INPUT_TOKENS_CLAUSE and OUTPUT_TOKENS_CLAUSE accordingly
(referencing COMBINED_TOKENS_CLAUSE_BODY, INPUT_TOKENS_CLAUSE,
OUTPUT_TOKENS_CLAUSE, input_placeholders/output_list_str, and the stmt.push
calls).
In `@crates/common/src/raindex_client/orders.rs`:
- Around line 969-973: The MultiOrderbookSubgraphClient::orders_count currently
swallows errors with .unwrap_or(0), causing callers (like the orders_list caller
that sets total_count) to receive misleading zero totals; change orders_count to
return a Result<u32, E> (or Option<u32>) instead of a bare u32, propagate the
error to callers (or return None) and update the caller logic that computes
total_count to handle the Result/Option (e.g., surface the error to the API
response or log and omit pagination) so failures from the subgraph are not
silently converted into a 0 total; adjust function signature and all call sites
that use orders_count (including where total_count is assigned) accordingly.
In `@crates/subgraph/src/multi_orderbook_client.rs`:
- Around line 482-503: Add a unit test that exercises the pagination boundary
for orders_count by making the mock subgraph return exactly
ALL_PAGES_QUERY_PAGE_SIZE orders on the first response so the loop in
OrderbookSubgraphClient::orders_count continues, then return a second-page
response (either empty or with extra orders) and assert
MultiOrderbookSubgraphClient::orders_count returns the combined total; mirror
the existing test_orders_count_one_subgraph setup but build the first mocked
JSON body with a vector of length ALL_PAGES_QUERY_PAGE_SIZE and a second mock
for the next POST that returns the subsequent page so the pagination path is
exercised.
In `@crates/subgraph/src/orderbook_client/order.rs`:
- Around line 143-168: The loop in orders_count uses a u16 `page` which can
overflow and wrap to 0 after 65,535 iterations; update the pagination counter to
a wider integer (change `page: u16` to `page: u32`) or use `checked_add` and
break on overflow, and ensure the `SgPaginationArgs { page, page_size:
ALL_PAGES_QUERY_PAGE_SIZE }` construction accepts the wider type (or cast
safely) so the loop terminates correctly when exhausting pages; modify the
`orders_count` function accordingly (keeping the call to `orders_list` and the
page_size logic intact).
In `@packages/orderbook/test/js_api/raindexClient.test.ts`:
- Around line 629-637: The test currently only checks the type of
result.totalCount from extractWasmEncodedData(await raindexClient.getOrders())
but not its value; update the assertions after calling raindexClient.getOrders()
to assert the concrete totals (expect totalCount === 2 for the first call that
returns order1 and order2, and totalCount === 1 for the second call when passing
[1]) using the same test flow that references extractWasmEncodedData,
raindexClient.getOrders, result.totalCount, order1, and order2 so regressions in
count computation are caught.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (14)
crates/common/src/local_db/query/fetch_orders/mod.rscrates/common/src/local_db/query/fetch_orders/query.sqlcrates/common/src/local_db/query/fetch_orders_count/mod.rscrates/common/src/local_db/query/fetch_orders_count/query.sqlcrates/common/src/local_db/query/mod.rscrates/common/src/raindex_client/local_db/orders.rscrates/common/src/raindex_client/local_db/query/fetch_orders_count.rscrates/common/src/raindex_client/local_db/query/mod.rscrates/common/src/raindex_client/orders.rscrates/subgraph/src/multi_orderbook_client.rscrates/subgraph/src/orderbook_client/order.rspackages/orderbook/test/js_api/raindexClient.test.tspackages/ui-components/src/__tests__/OrdersListTable.test.tspackages/ui-components/src/lib/components/tables/OrdersListTable.svelte
Comment on lines
+131
to
+171
| if has_inputs && has_outputs && input_tokens == output_tokens { | ||
| let input_placeholders: Vec<String> = input_tokens | ||
| .iter() | ||
| .enumerate() | ||
| .map(|(i, _)| format!("?{}", stmt.params.len() + i + 1)) | ||
| .collect(); | ||
| let input_list_str = input_placeholders.join(", "); | ||
|
|
||
| for token in &input_tokens { | ||
| stmt.push(SqlValue::from(*token)); | ||
| } | ||
|
|
||
| let output_placeholders: Vec<String> = output_tokens | ||
| .iter() | ||
| .enumerate() | ||
| .map(|(i, _)| format!("?{}", stmt.params.len() + i + 1)) | ||
| .collect(); | ||
| let output_list_str = output_placeholders.join(", "); | ||
|
|
||
| for token in &output_tokens { | ||
| stmt.push(SqlValue::from(*token)); | ||
| } | ||
|
|
||
| let combined_clause = COMBINED_TOKENS_CLAUSE_BODY | ||
| .replace("{input_list}", &input_list_str) | ||
| .replace("{output_list}", &output_list_str); | ||
|
|
||
| stmt.sql = stmt.sql.replace(INPUT_TOKENS_CLAUSE, &combined_clause); | ||
| stmt.sql = stmt.sql.replace(OUTPUT_TOKENS_CLAUSE, ""); | ||
| } else { | ||
| stmt.bind_list_clause( | ||
| INPUT_TOKENS_CLAUSE, | ||
| INPUT_TOKENS_CLAUSE_BODY, | ||
| input_tokens.into_iter().map(SqlValue::from), | ||
| )?; | ||
| stmt.bind_list_clause( | ||
| OUTPUT_TOKENS_CLAUSE, | ||
| OUTPUT_TOKENS_CLAUSE_BODY, | ||
| output_tokens.into_iter().map(SqlValue::from), | ||
| )?; | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Duplicate param binding in the combined-tokens branch.
When input_tokens == output_tokens, both {input_list} and {output_list} in COMBINED_TOKENS_CLAUSE_BODY are populated with separate but identical positional params (the same token addresses are pushed to stmt.params twice). This is functionally correct but doubles the number of bound parameters unnecessarily. Consider using a single list and the same placeholders for both sides of the OR clause.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/common/src/local_db/query/fetch_orders_count/mod.rs` around lines 131
- 171, In the branch where input_tokens == output_tokens, avoid pushing the same
tokens twice by creating only one set of positional placeholders and params:
build input_placeholders and push each token once into stmt.params (using the
existing input_placeholders and stmt.push loop), then reuse the same placeholder
list string for both "{input_list}" and "{output_list}" when constructing
COMBINED_TOKENS_CLAUSE_BODY; update stmt.sql replacements for
INPUT_TOKENS_CLAUSE and OUTPUT_TOKENS_CLAUSE accordingly (referencing
COMBINED_TOKENS_CLAUSE_BODY, INPUT_TOKENS_CLAUSE, OUTPUT_TOKENS_CLAUSE,
input_placeholders/output_list_str, and the stmt.push calls).
Collaborator
Author
There was a problem hiding this comment.
Acknowledged. This is functionally correct and the duplication is minimal — just a few extra bound parameters when input and output tokens are identical. Not worth the added complexity of reusing placeholders for a negligible optimization.
Contributor
There was a problem hiding this comment.
@findolor, understood — that's a reasonable tradeoff. Thank you for the clarification!
🧠 Learnings used
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 2442
File: crates/common/src/local_db/query/fetch_vaults/mod.rs:228-244
Timestamp: 2026-02-09T12:31:02.217Z
Learning: In crates/common/src/local_db/query/fetch_vaults/mod.rs tests, whitespace-sensitive SQL string assertions (e.g., checking for "GROUP BY chain_id, orderbook_address, owner, token, vault_id\n") are acceptable for verifying query structure until a mock database testing approach is implemented. These assertions help ensure the SQL template is correctly formed even though they may be fragile to formatting changes.
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 2358
File: crates/common/src/local_db/query/fetch_orders/mod.rs:17-21
Timestamp: 2026-01-16T06:20:47.764Z
Learning: In crates/common/src/local_db/query/fetch_orders/mod.rs, for internal structs like FetchOrdersTokensFilter that are never compared directly (tests compare individual fields instead), PartialEq and Eq derives are not needed. Documentation can be skipped when field names are self-explanatory, especially for internal types.
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 2370
File: crates/common/src/raindex_client/vaults.rs:896-973
Timestamp: 2026-01-07T15:38:42.640Z
Learning: In the rainlanguage/rain.orderbook codebase, findolor considers duplication acceptable when dealing with separate subgraph types (like SgDeposit, SgWithdrawal, SgClearBounty) in match arms, as extracting a helper function would require passing 10+ individual fields and would be less clean than self-contained match arms. This structural duplication is preferred over complex helper functions.
Learnt from: thedavidmeister
Repo: rainlanguage/rain.orderbook PR: 2149
File: src/concrete/arb/RouteProcessorOrderBookV5ArbOrderTaker.sol:37-39
Timestamp: 2025-09-16T07:52:46.075Z
Learning: In the rainlanguage/rain.orderbook codebase, Slither static analysis tool prefers explicit handling of unused return values by capturing them in variables and referencing them in no-op statements (e.g., `(losslessInputAmount);`) rather than discarding them via destructuring with blank elements (e.g., `(uint256 inputTokenAmount, ) = ...`). The explicit pattern satisfies Slither's unused return value checks while the destructuring approach causes Slither to complain.
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 2344
File: crates/common/src/local_db/pipeline/runner/mod.rs:18-31
Timestamp: 2025-12-03T10:40:25.429Z
Learning: In `crates/common/src/local_db/pipeline/runner/mod.rs`, the `TargetSuccess` struct does not need separate `ob_id` or `orderbook_key` fields because the contained `SyncOutcome` already includes orderbook identification information such as chain_id and orderbook_address. This avoids redundant data duplication.
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 2358
File: packages/ui-components/src/lib/components/tables/OrdersListTable.svelte:82-85
Timestamp: 2025-12-11T07:46:32.817Z
Learning: In packages/ui-components/src/lib/components/tables/OrdersListTable.svelte, the token filter intentionally sets both inputs and outputs to the same selectedTokens array (lines 82-85) to achieve OR filtering behavior. This is by design: the single UI token filter should match orders containing the selected token(s) in either inputs OR outputs, not pair-specific filtering. Even though the API supports more granular directional filtering, the UI uses the same tokens in both fields for broader matching.
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 1938
File: crates/settings/src/yaml/orderbook.rs:180-199
Timestamp: 2025-06-18T18:24:32.049Z
Learning: In crates/settings/src/yaml/orderbook.rs, the user prefers to avoid refactoring duplicate search logic between get_orderbook_by_address and get_orderbook_by_network_key when there are only 2 functions, indicating they would consider it if more similar functions are added in the future.
Learnt from: rouzwelt
Repo: rainlanguage/rain.orderbook PR: 0
File: :0-0
Timestamp: 2025-05-21T21:24:53.708Z
Learning: For PR `#1884` in rainlanguage/rain.orderbook, provide a separate comment after each review with PR size statistics in the format: `TOTAL=number ADDITIONS=number DELETIONS=number`, excluding lock files like package-lock.json and cargo.lock.
Learnt from: rouzwelt
Repo: rainlanguage/rain.orderbook PR: 0
File: :0-0
Timestamp: 2025-05-21T21:24:53.708Z
Learning: For PR `#1884` in rainlanguage/rain.orderbook, provide a separate comment after each review with PR size statistics in the format: `TOTAL=number ADDITIONS=number DELETIONS=number`, excluding lock files like package-lock.json and cargo.lock.
Learnt from: rouzwelt
Repo: rainlanguage/rain.orderbook PR: 0
File: :0-0
Timestamp: 2025-05-21T21:24:53.708Z
Learning: For PR `#1884` in rainlanguage/rain.orderbook, provide a separate comment after each review with PR size statistics in the format: `TOTAL=number ADDITIONS=number DELETIONS=number`, excluding lock files like package-lock.json and cargo.lock.
Learnt from: rouzwelt
Repo: rainlanguage/rain.orderbook PR: 0
File: :0-0
Timestamp: 2025-05-21T21:24:53.708Z
Learning: For PR `#1884` in rainlanguage/rain.orderbook, provide a separate comment after each review with PR size statistics in the format: `TOTAL=number ADDITIONS=number DELETIONS=number`, excluding lock files like package-lock.json and cargo.lock.
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 2376
File: crates/common/src/raindex_client/orders.rs:2-2
Timestamp: 2026-01-07T14:21:44.132Z
Learning: When local DB is enabled in the rain.orderbook codebase, the intended design is to use it exclusively without falling back to the subgraph, even when results are empty. Methods like get_trades_list() in trades.rs and get_vaults_volume() in orders.rs follow this pattern—they return local DB results directly when is_chain_supported_local_db() and local_db is available. Mixing data sources could cause inconsistencies. The get_balance_changes() method in vaults.rs that checks !local_changes.is_empty() before returning is an outlier that needs to be fixed to align with the exclusive local DB pattern.
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 2145
File: crates/common/src/raindex_client/local_db/query/create_tables/query.sql:71-72
Timestamp: 2025-10-06T11:44:07.888Z
Learning: The local DB feature in the rain.orderbook codebase is not live yet (as of PR `#2145`), so schema migrations for existing databases are not required when modifying table structures in `crates/common/src/raindex_client/local_db/query/create_tables/query.sql`.
Learnt from: rouzwelt
Repo: rainlanguage/rain.orderbook PR: 0
File: :0-0
Timestamp: 2025-05-21T21:25:47.873Z
Learning: After each new commit review for rainlanguage/rain.orderbook repository, generate a PR size report in a separate comment with the format: "TOTAL=number ADDITIONS=number DELETIONS=number". This report should exclude irrelevant files such as lock files (e.g., package-lock.json, cargo.lock).
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 2465
File: crates/common/src/local_db/query/fetch_trades_by_tx/query.sql:236-255
Timestamp: 2026-02-18T08:41:08.073Z
Learning: In SQL queries within the rain.orderbook codebase (e.g., crates/common/src/local_db/query/fetch_trades_by_tx/query.sql), passthrough CTEs that explicitly list all columns from their source (like `trade_rows` projecting `unioned_trades`) are intentional. They pin the column set explicitly rather than relying on `SELECT *` ordering, ensuring deterministic schema and maintainability.
Learnt from: rouzwelt
Repo: rainlanguage/rain.orderbook PR: 0
File: :0-0
Timestamp: 2025-05-21T21:26:31.566Z
Learning: For PR `#1884` and future PRs in rainlanguage/rain.orderbook repository, provide PR size reports in a separate comment with ONLY the exact format "TOTAL=number\nADDITIONS=number\nDELETIONS=number", without any additional text or explanation. Reports should exclude irrelevant files such as lock files (package-lock.json, yarn.lock, pnpm-lock.yaml, cargo.lock).
Learnt from: rouzwelt
Repo: rainlanguage/rain.orderbook PR: 0
File: :0-0
Timestamp: 2025-05-21T21:25:47.873Z
Learning: After each new commit review for rainlanguage/rain.orderbook repository, generate a PR size report in a separate comment with the exact format: "TOTAL=number\nADDITIONS=number\nDELETIONS=number". This report should exclude irrelevant files such as lock files (package-lock.json, yarn.lock, pnpm-lock.yaml, cargo.lock).
Learnt from: rouzwelt
Repo: rainlanguage/rain.orderbook PR: 0
File: :0-0
Timestamp: 2025-05-21T22:14:22.646Z
Learning: For PR `#1884` in rainlanguage/rain.orderbook, I must consistently report the PR size after EVERY new commit in the exact format "TOTAL=number\nADDITIONS=number\nDELETIONS=number" without any formatting blocks. The report must exclude lock files (package-lock.json, yarn.lock, pnpm-lock.yaml, cargo.lock) and can be included either as part of the review or as a separate comment.
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 2155
File: crates/common/src/raindex_client/trades.rs:133-152
Timestamp: 2025-10-06T14:13:18.531Z
Learning: In the rain.orderbook codebase, the `page` parameter in `RaindexOrder::get_trades_list` method (in crates/common/src/raindex_client/trades.rs) is kept for backwards compatibility with subgraph logic, but the LocalDb fast-path intentionally returns all trades without implementing pagination.
Learnt from: rouzwelt
Repo: rainlanguage/rain.orderbook PR: 0
File: :0-0
Timestamp: 2025-05-21T22:14:22.646Z
Learning: For PR `#1884` in rainlanguage/rain.orderbook, I must consistently report the PR size after EVERY new commit in the exact format "TOTAL=number\nADDITIONS=number\nDELETIONS=number" without any additional text or formatting blocks. The report must exclude lock files (package-lock.json, yarn.lock, pnpm-lock.yaml, cargo.lock) and can be included either as part of the review or as a separate comment.
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 2399
File: crates/rest_api/src/routes/take_orders.rs:74-95
Timestamp: 2026-01-16T12:07:15.521Z
Learning: In Rust projects using utoipa, when you use #[schema(...)] attributes (for example #[schema(example = json!(...))]), the json! macro is handled internally by utoipa's proc-macro at compile time. You do not need to import serde_json::json! in the module for these attributes. Do not add a serde_json::json! import solely for these schema attributes; ensure serde_json is present only if used elsewhere."
Comment on lines
+143
to
+168
| pub async fn orders_count( | ||
| &self, | ||
| filter_args: SgOrdersListFilterArgs, | ||
| ) -> Result<u32, OrderbookSubgraphClientError> { | ||
| let mut count: u32 = 0; | ||
| let mut page: u16 = 1; | ||
|
|
||
| loop { | ||
| let page_data = self | ||
| .orders_list( | ||
| filter_args.clone(), | ||
| SgPaginationArgs { | ||
| page, | ||
| page_size: ALL_PAGES_QUERY_PAGE_SIZE, | ||
| }, | ||
| ) | ||
| .await?; | ||
| let batch_len = page_data.len() as u32; | ||
| count += batch_len; | ||
| if batch_len < ALL_PAGES_QUERY_PAGE_SIZE as u32 { | ||
| break; | ||
| } | ||
| page += 1; | ||
| } | ||
| Ok(count) | ||
| } |
Contributor
There was a problem hiding this comment.
page overflow on u16 with extremely large datasets.
page is u16, so after 65,535 iterations page += 1 wraps to 0, causing an infinite loop re-fetching the first page forever. While unlikely in practice (~13M orders at page_size=200), orders_list_all at Line 116 uses a wider type. Consider using u32 or adding checked_add with a break on overflow.
Proposed fix
pub async fn orders_count(
&self,
filter_args: SgOrdersListFilterArgs,
) -> Result<u32, OrderbookSubgraphClientError> {
let mut count: u32 = 0;
- let mut page: u16 = 1;
+ let mut page: u32 = 1;
loop {
let page_data = self
.orders_list(
filter_args.clone(),
SgPaginationArgs {
- page,
+ page: page as u16,
page_size: ALL_PAGES_QUERY_PAGE_SIZE,
},
)
.await?;
let batch_len = page_data.len() as u32;
count += batch_len;
if batch_len < ALL_PAGES_QUERY_PAGE_SIZE as u32 {
break;
}
- page += 1;
+ page = page.checked_add(1).ok_or_else(|| {
+ OrderbookSubgraphClientError::Empty // or a more descriptive error
+ })?;
}
Ok(count)
}📝 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
| pub async fn orders_count( | |
| &self, | |
| filter_args: SgOrdersListFilterArgs, | |
| ) -> Result<u32, OrderbookSubgraphClientError> { | |
| let mut count: u32 = 0; | |
| let mut page: u16 = 1; | |
| loop { | |
| let page_data = self | |
| .orders_list( | |
| filter_args.clone(), | |
| SgPaginationArgs { | |
| page, | |
| page_size: ALL_PAGES_QUERY_PAGE_SIZE, | |
| }, | |
| ) | |
| .await?; | |
| let batch_len = page_data.len() as u32; | |
| count += batch_len; | |
| if batch_len < ALL_PAGES_QUERY_PAGE_SIZE as u32 { | |
| break; | |
| } | |
| page += 1; | |
| } | |
| Ok(count) | |
| } | |
| pub async fn orders_count( | |
| &self, | |
| filter_args: SgOrdersListFilterArgs, | |
| ) -> Result<u32, OrderbookSubgraphClientError> { | |
| let mut count: u32 = 0; | |
| let mut page: u32 = 1; | |
| loop { | |
| let page_data = self | |
| .orders_list( | |
| filter_args.clone(), | |
| SgPaginationArgs { | |
| page: page as u16, | |
| page_size: ALL_PAGES_QUERY_PAGE_SIZE, | |
| }, | |
| ) | |
| .await?; | |
| let batch_len = page_data.len() as u32; | |
| count += batch_len; | |
| if batch_len < ALL_PAGES_QUERY_PAGE_SIZE as u32 { | |
| break; | |
| } | |
| page = page.checked_add(1).ok_or_else(|| { | |
| OrderbookSubgraphClientError::Empty | |
| })?; | |
| } | |
| Ok(count) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/subgraph/src/orderbook_client/order.rs` around lines 143 - 168, The
loop in orders_count uses a u16 `page` which can overflow and wrap to 0 after
65,535 iterations; update the pagination counter to a wider integer (change
`page: u16` to `page: u32`) or use `checked_add` and break on overflow, and
ensure the `SgPaginationArgs { page, page_size: ALL_PAGES_QUERY_PAGE_SIZE }`
construction accepts the wider type (or cast safely) so the loop terminates
correctly when exhausting pages; modify the `orders_count` function accordingly
(keeping the call to `orders_list` and the page_size logic intact).
Collaborator
Author
There was a problem hiding this comment.
This is not a practical concern — it would require ~13M orders at page_size=200 to overflow, which is far beyond any realistic dataset. Not worth adding complexity for this edge case.
Contributor
There was a problem hiding this comment.
@findolor, understood! I'll defer to your judgment on this. Given the practical constraints of your dataset, the added complexity isn't justified for such an unlikely scenario.
✏️ Learnings added
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 2460
File: crates/subgraph/src/orderbook_client/order.rs:143-168
Timestamp: 2026-02-25T12:20:48.935Z
Learning: In the rainlanguage/rain.orderbook codebase at crates/subgraph/src/orderbook_client/order.rs, user findolor prefers not to implement overflow protection for the u16 `page` counter in `orders_count`, considering it unnecessary since reaching 65,535 pages (requiring ~13M orders at page_size=200) is far beyond any realistic dataset and not worth the added complexity.
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: rainlanguage/rain.orderbook PR: 1956
File: crates/common/src/raindex_client/trades.rs:215-223
Timestamp: 2025-07-04T10:24:56.163Z
Learning: In the rain.orderbook codebase, the get_trade_count method implementation that fetches all trades to count them is intentionally consistent with previous implementations and not considered a performance issue, as indicated by findolor for the trades counting functionality in crates/common/src/raindex_client/trades.rs.
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 2155
File: crates/common/src/raindex_client/trades.rs:133-152
Timestamp: 2025-10-06T14:13:18.531Z
Learning: In the rain.orderbook codebase, the `page` parameter in `RaindexOrder::get_trades_list` method (in crates/common/src/raindex_client/trades.rs) is kept for backwards compatibility with subgraph logic, but the LocalDb fast-path intentionally returns all trades without implementing pagination.
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 1956
File: crates/common/src/raindex_client/orders.rs:609-609
Timestamp: 2025-07-04T10:27:22.544Z
Learning: In the rainlanguage/rain.orderbook codebase, user findolor prefers not to implement overflow protection for trades count casting (usize to u16) at this time, considering it unnecessary for the current scope since the practical risk of orders having 65,535+ trades is extremely low.
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 1947
File: crates/common/src/raindex_client/orders.rs:98-125
Timestamp: 2025-06-24T08:46:03.368Z
Learning: In the vault merging logic in crates/common/src/raindex_client/orders.rs, optimization isn't necessary because the maximum list items are usually around 5 items. For such small datasets, the simple three-loop approach is preferred over HashMap-based optimization due to clarity and minimal performance impact.
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 1956
File: crates/common/src/fuzz/mod.rs:64-64
Timestamp: 2025-07-04T09:02:57.301Z
Learning: In rainlanguage/rain.orderbook, user findolor prefers to limit type consistency changes to only the parts directly related to the current work scope. For example, when updating chain_id fields from u64 to u32 in fuzz-related code, unrelated files like tauri-app wallet commands can remain as u64 if they serve different purposes and aren't part of the current changes.
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 1903
File: crates/settings/src/yaml/orderbook.rs:371-377
Timestamp: 2025-06-17T16:21:24.384Z
Learning: In crates/settings/src/yaml/orderbook.rs tests, the user findolor considers RPC ordering in Vec<Url> assertions to be intentional and not a test brittleness issue. The ordering of RPCs in tests should be preserved as specified.
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 2000
File: crates/common/src/raindex_client/vaults.rs:183-183
Timestamp: 2025-07-16T10:40:05.717Z
Learning: In the rainlanguage/rain.orderbook codebase, user findolor considers breaking changes from Option<U256> to U256 for required fields like decimals in RaindexVaultToken to be acceptable and safe, even when they affect multiple usage sites across the codebase.
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 2012
File: crates/js_api/src/registry.rs:485-512
Timestamp: 2025-07-30T07:41:39.271Z
Learning: In crates/js_api/src/registry.rs, findolor considers the current concurrent fetching of order files using futures::future::join_all without concurrency limits to be acceptable, preferring the simple approach over adding concurrency limiting mechanisms for the DotrainRegistry implementation.
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 1994
File: crates/common/src/raindex_client/vaults.rs:282-292
Timestamp: 2025-07-15T08:01:38.534Z
Learning: In the rainlanguage/rain.orderbook codebase, findolor prefers to avoid concurrency optimizations like using `futures::future::try_join_all` for parallel processing of balance changes, considering such optimizations "not that critical at the moment" when the performance impact is minimal.
Learnt from: 0xgleb
Repo: rainlanguage/rain.orderbook PR: 1911
File: crates/subgraph/src/types/impls.rs:7-15
Timestamp: 2025-07-21T16:34:31.193Z
Learning: In the rainlanguage/rain.orderbook codebase, user 0xgleb considers breaking changes that remove unsafe default behaviors to be intentional and acceptable. Specifically, the get_decimals() method in crates/subgraph/src/types/impls.rs was intentionally changed to return MissingDecimals error instead of defaulting to 18 decimals, as defaulting to 18 is considered unsafe and should never have been done.
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 1858
File: crates/subgraph/src/orderbook_client/transaction.rs:17-35
Timestamp: 2025-05-19T11:44:11.498Z
Learning: Pagination for large add/remove-order sets in transaction_add_orders and transaction_remove_orders methods is a potential future enhancement, but not prioritized in the current implementation.
Learnt from: rouzwelt
Repo: rainlanguage/rain.orderbook PR: 0
File: :0-0
Timestamp: 2025-05-21T21:24:53.708Z
Learning: For PR `#1884` in rainlanguage/rain.orderbook, provide a separate comment after each review with PR size statistics in the format: `TOTAL=number ADDITIONS=number DELETIONS=number`, excluding lock files like package-lock.json and cargo.lock.
Learnt from: rouzwelt
Repo: rainlanguage/rain.orderbook PR: 0
File: :0-0
Timestamp: 2025-05-21T21:24:53.708Z
Learning: For PR `#1884` in rainlanguage/rain.orderbook, provide a separate comment after each review with PR size statistics in the format: `TOTAL=number ADDITIONS=number DELETIONS=number`, excluding lock files like package-lock.json and cargo.lock.
Learnt from: rouzwelt
Repo: rainlanguage/rain.orderbook PR: 0
File: :0-0
Timestamp: 2025-05-21T21:24:53.708Z
Learning: For PR `#1884` in rainlanguage/rain.orderbook, provide a separate comment after each review with PR size statistics in the format: `TOTAL=number ADDITIONS=number DELETIONS=number`, excluding lock files like package-lock.json and cargo.lock.
Learnt from: rouzwelt
Repo: rainlanguage/rain.orderbook PR: 0
File: :0-0
Timestamp: 2025-05-21T21:24:53.708Z
Learning: For PR `#1884` in rainlanguage/rain.orderbook, provide a separate comment after each review with PR size statistics in the format: `TOTAL=number ADDITIONS=number DELETIONS=number`, excluding lock files like package-lock.json and cargo.lock.
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 1858
File: crates/subgraph/src/orderbook_client/vault.rs:81-92
Timestamp: 2025-05-19T12:09:10.694Z
Learning: The `vault_balance_changes_list` function in OrderbookSubgraphClient uses a different pagination approach compared to other list methods. It uses hard-coded GraphQL query parameters (first=200, skip=0) while still accepting pagination arguments, and the pagination is handled by special logic inside the `query_paginated` method that properly processes these values.
Learnt from: rouzwelt
Repo: rainlanguage/rain.orderbook PR: 0
File: :0-0
Timestamp: 2025-05-21T21:25:47.873Z
Learning: After each new commit review for rainlanguage/rain.orderbook repository, generate a PR size report in a separate comment with the format: "TOTAL=number ADDITIONS=number DELETIONS=number". This report should exclude irrelevant files such as lock files (e.g., package-lock.json, cargo.lock).
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 1744
File: crates/js_api/src/subgraph/order.rs:109-114
Timestamp: 2025-05-09T05:28:22.089Z
Learning: In the rain.orderbook project, SubgraphError contains or converts from OrderbookSubgraphClientError, so using OrderbookSubgraphClientError in error creation followed by a conversion to SubgraphError (via ? operator) is acceptable.
Learnt from: rouzwelt
Repo: rainlanguage/rain.orderbook PR: 0
File: :0-0
Timestamp: 2025-05-21T21:26:31.566Z
Learning: For PR `#1884` and future PRs in rainlanguage/rain.orderbook repository, provide PR size reports in a separate comment with ONLY the exact format "TOTAL=number\nADDITIONS=number\nDELETIONS=number", without any additional text or explanation. Reports should exclude irrelevant files such as lock files (package-lock.json, yarn.lock, pnpm-lock.yaml, cargo.lock).
Learnt from: rouzwelt
Repo: rainlanguage/rain.orderbook PR: 0
File: :0-0
Timestamp: 2025-05-21T21:25:47.873Z
Learning: After each new commit review for rainlanguage/rain.orderbook repository, generate a PR size report in a separate comment with the exact format: "TOTAL=number\nADDITIONS=number\nDELETIONS=number". This report should exclude irrelevant files such as lock files (package-lock.json, yarn.lock, pnpm-lock.yaml, cargo.lock).
Learnt from: rouzwelt
Repo: rainlanguage/rain.orderbook PR: 0
File: :0-0
Timestamp: 2025-05-21T22:14:22.646Z
Learning: For PR `#1884` in rainlanguage/rain.orderbook, I must consistently report the PR size after EVERY new commit in the exact format "TOTAL=number\nADDITIONS=number\nDELETIONS=number" without any formatting blocks. The report must exclude lock files (package-lock.json, yarn.lock, pnpm-lock.yaml, cargo.lock) and can be included either as part of the review or as a separate comment.
Learnt from: rouzwelt
Repo: rainlanguage/rain.orderbook PR: 0
File: :0-0
Timestamp: 2025-05-21T22:14:22.646Z
Learning: For PR `#1884` in rainlanguage/rain.orderbook, I must consistently report the PR size after EVERY new commit in the exact format "TOTAL=number\nADDITIONS=number\nDELETIONS=number" without any additional text or formatting blocks. The report must exclude lock files (package-lock.json, yarn.lock, pnpm-lock.yaml, cargo.lock) and can be included either as part of the review or as a separate comment.
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 2399
File: crates/rest_api/src/routes/take_orders.rs:74-95
Timestamp: 2026-01-16T12:07:15.521Z
Learning: In Rust projects using utoipa, when you use #[schema(...)] attributes (for example #[schema(example = json!(...))]), the json! macro is handled internally by utoipa's proc-macro at compile time. You do not need to import serde_json::json! in the module for these attributes. Do not add a serde_json::json! import solely for these schema attributes; ensure serde_json is present only if used elsewhere."
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 2465
File: crates/subgraph/src/orderbook_client/transaction.rs:37-54
Timestamp: 2026-02-18T08:41:10.689Z
Learning: Enforce a Rust public API documentation convention in this directory. Require rustdoc comments on all public items (pub fn, pub struct, pub enum, etc.) describing behavior and usage. Add or update documentation standards in contributing guidelines and consider enabling/using lints (e.g., rustdoc-unsafe or clippy doc-related checks) to ensure public interfaces are documented going forward.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
crates/common/src/raindex_client/orders.rs (1)
966-1008: Error propagation fororders_countnow correctly uses?— past review concern addressed.The previous review flagged that
orders_counterrors were silently swallowed via.unwrap_or(0), which could cause misleadingtotal_count = 0with a non-empty orders list. This is now correctly propagated with?on line 970.The conditional count computation (only when
page.is_some()) is a good optimization for internal callers likefetch_orders_for_pairthat don't need pagination metadata.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/common/src/raindex_client/orders.rs` around lines 966 - 1008, Ensure the orders_count call propagates errors instead of swallowing them and that the pagination-only optimization remains: replace any remaining .unwrap_or(0) use with a proper await + ? on client.orders_count(sg_filter_args.clone()) when computing total_count (used alongside SgOrdersListFilterArgs and page.is_some()), and keep the conditional so count is only fetched when page.is_some(); verify RaindexOrdersListResult is returned with the computed total_count and that no silent defaults remain.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/common/src/local_db/query/fetch_orders_count/mod.rs`:
- Around line 185-265: Add a unit test that exercises the combined token branch
in build_fetch_orders_count_stmt by constructing a FetchOrdersArgs where
input_tokens and output_tokens are identical (e.g., same Address repeated) and
calling build_fetch_orders_count_stmt(&args). Assert the produced stmt.sql
contains the combined-tokens clause (i.e., the manual placeholder path used
instead of INPUT_TOKENS_CLAUSE/OUTPUT_TOKENS_CLAUSE) and does not contain the
separate INPUT_TOKENS_CLAUSE or OUTPUT_TOKENS_CLAUSE; reference the
build_fetch_orders_count_stmt function and the FetchOrdersArgs fields
input_tokens/output_tokens to locate where to add the test. Ensure the test
mirrors existing style (uses address! or Address::ZERO and unwraps the
statement) and adds assertion(s) for the specific combined clause string used in
the implementation.
---
Duplicate comments:
In `@crates/common/src/raindex_client/orders.rs`:
- Around line 966-1008: Ensure the orders_count call propagates errors instead
of swallowing them and that the pagination-only optimization remains: replace
any remaining .unwrap_or(0) use with a proper await + ? on
client.orders_count(sg_filter_args.clone()) when computing total_count (used
alongside SgOrdersListFilterArgs and page.is_some()), and keep the conditional
so count is only fetched when page.is_some(); verify RaindexOrdersListResult is
returned with the computed total_count and that no silent defaults remain.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
crates/common/src/local_db/query/fetch_orders_count/mod.rscrates/common/src/raindex_client/orders.rscrates/subgraph/src/multi_orderbook_client.rspackages/orderbook/test/js_api/raindexClient.test.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/common/src/local_db/query/fetch_orders_count/mod.rs`:
- Around line 185-337: Add a test that mirrors the existing Active test but sets
FetchOrdersArgs.filter to FetchOrdersActiveFilter::Inactive and calls
build_fetch_orders_count_stmt(&args).unwrap(); then assert the generated SQL
contains the inactive marker (e.g. assert!(stmt.sql.contains("?1 = 'inactive'"))
to ensure the Inactive variant is covered; locate the test alongside
builds_count_with_active_filter near the other FetchOrdersActiveFilter tests and
use the same pattern with FetchOrdersArgs and build_fetch_orders_count_stmt.
Comment on lines
+185
to
+337
| #[test] | ||
| fn builds_count_with_no_filters() { | ||
| let args = FetchOrdersArgs { | ||
| chain_ids: vec![1], | ||
| filter: FetchOrdersActiveFilter::All, | ||
| ..FetchOrdersArgs::default() | ||
| }; | ||
| let stmt = build_fetch_orders_count_stmt(&args).unwrap(); | ||
| assert!(stmt.sql.contains("COUNT(*)")); | ||
| assert!(stmt.sql.contains("orders_count")); | ||
| assert!(stmt.sql.contains("?1 = 'all'")); | ||
| assert!(!stmt.sql.contains(OWNERS_CLAUSE)); | ||
| assert!(!stmt.sql.contains(INPUT_TOKENS_CLAUSE)); | ||
| assert!(!stmt.sql.contains(OUTPUT_TOKENS_CLAUSE)); | ||
| assert!(!stmt.sql.contains(ORDER_HASH_CLAUSE)); | ||
| } | ||
|
|
||
| #[test] | ||
| fn builds_count_with_active_filter() { | ||
| let args = FetchOrdersArgs { | ||
| chain_ids: vec![137], | ||
| filter: FetchOrdersActiveFilter::Active, | ||
| ..FetchOrdersArgs::default() | ||
| }; | ||
| let stmt = build_fetch_orders_count_stmt(&args).unwrap(); | ||
| assert!(stmt.sql.contains("?1 = 'active'")); | ||
| } | ||
|
|
||
| #[test] | ||
| fn builds_count_with_owners_and_order_hash() { | ||
| let args = FetchOrdersArgs { | ||
| chain_ids: vec![1], | ||
| owners: vec![address!("0xF3dEe5b36E3402893e6953A8670E37D329683ABB")], | ||
| order_hash: Some(b256!( | ||
| "0x00000000000000000000000000000000000000000000000000000000deadbeef" | ||
| )), | ||
| ..FetchOrdersArgs::default() | ||
| }; | ||
| let stmt = build_fetch_orders_count_stmt(&args).unwrap(); | ||
| assert!(stmt.sql.contains("l.order_owner IN (")); | ||
| assert!(stmt | ||
| .sql | ||
| .contains("COALESCE(la.order_hash, l.order_hash) = ")); | ||
| } | ||
|
|
||
| #[test] | ||
| fn builds_count_with_chain_ids() { | ||
| let args = FetchOrdersArgs { | ||
| chain_ids: vec![137, 1, 137], | ||
| filter: FetchOrdersActiveFilter::All, | ||
| ..FetchOrdersArgs::default() | ||
| }; | ||
| let stmt = build_fetch_orders_count_stmt(&args).unwrap(); | ||
| assert!(!stmt.sql.contains(MAIN_CHAIN_IDS_CLAUSE)); | ||
| assert!(!stmt.sql.contains(LATEST_ADD_CHAIN_IDS_CLAUSE)); | ||
| assert!(stmt.sql.contains("AND oe.chain_id IN (?")); | ||
| } | ||
|
|
||
| #[test] | ||
| fn builds_count_with_orderbooks() { | ||
| let args = FetchOrdersArgs { | ||
| chain_ids: vec![1], | ||
| orderbook_addresses: vec![Address::ZERO], | ||
| ..FetchOrdersArgs::default() | ||
| }; | ||
| let stmt = build_fetch_orders_count_stmt(&args).unwrap(); | ||
| assert!(stmt.sql.contains("AND oe.orderbook_address IN (?")); | ||
| } | ||
|
|
||
| #[test] | ||
| fn builds_count_with_combined_tokens_when_inputs_equal_outputs() { | ||
| use super::super::fetch_orders::FetchOrdersTokensFilter; | ||
|
|
||
| let token_a = address!("0xF3dEe5b36E3402893e6953A8670E37D329683ABB"); | ||
| let token_b = address!("0x1111111111111111111111111111111111111111"); | ||
| let tokens = vec![token_a, token_b]; | ||
|
|
||
| let args = FetchOrdersArgs { | ||
| chain_ids: vec![1], | ||
| tokens: FetchOrdersTokensFilter { | ||
| inputs: tokens.clone(), | ||
| outputs: tokens, | ||
| }, | ||
| ..FetchOrdersArgs::default() | ||
| }; | ||
| let stmt = build_fetch_orders_count_stmt(&args).unwrap(); | ||
|
|
||
| assert!( | ||
| stmt.sql.contains("lower(io2.io_type) = 'input'"), | ||
| "should contain input check in combined clause" | ||
| ); | ||
| assert!( | ||
| stmt.sql.contains("lower(io2.io_type) = 'output'"), | ||
| "should contain output check in combined clause" | ||
| ); | ||
| assert!( | ||
| stmt.sql.contains(" OR "), | ||
| "should use OR to combine input and output checks" | ||
| ); | ||
| assert!( | ||
| !stmt.sql.contains(INPUT_TOKENS_CLAUSE), | ||
| "input tokens placeholder should be replaced" | ||
| ); | ||
| assert!( | ||
| !stmt.sql.contains(OUTPUT_TOKENS_CLAUSE), | ||
| "output tokens placeholder should be replaced" | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn builds_count_with_separate_tokens_when_inputs_differ_from_outputs() { | ||
| use super::super::fetch_orders::FetchOrdersTokensFilter; | ||
|
|
||
| let token_a = address!("0xF3dEe5b36E3402893e6953A8670E37D329683ABB"); | ||
| let token_b = address!("0x1111111111111111111111111111111111111111"); | ||
|
|
||
| let args = FetchOrdersArgs { | ||
| chain_ids: vec![1], | ||
| tokens: FetchOrdersTokensFilter { | ||
| inputs: vec![token_a], | ||
| outputs: vec![token_b], | ||
| }, | ||
| ..FetchOrdersArgs::default() | ||
| }; | ||
| let stmt = build_fetch_orders_count_stmt(&args).unwrap(); | ||
|
|
||
| assert!( | ||
| !stmt.sql.contains(INPUT_TOKENS_CLAUSE), | ||
| "input tokens placeholder should be replaced" | ||
| ); | ||
| assert!( | ||
| !stmt.sql.contains(OUTPUT_TOKENS_CLAUSE), | ||
| "output tokens placeholder should be replaced" | ||
| ); | ||
| let exists_count = stmt.sql.matches("AND EXISTS").count(); | ||
| assert_eq!( | ||
| exists_count, 2, | ||
| "should have two separate EXISTS subqueries when tokens differ" | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn extract_count_returns_value() { | ||
| let rows = vec![LocalDbOrdersCountRow { orders_count: 42 }]; | ||
| assert_eq!(extract_orders_count(&rows), 42); | ||
| } | ||
|
|
||
| #[test] | ||
| fn extract_count_returns_zero_for_empty() { | ||
| let rows: Vec<LocalDbOrdersCountRow> = vec![]; | ||
| assert_eq!(extract_orders_count(&rows), 0); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Optional: Add a test for FetchOrdersActiveFilter::Inactive.
The test suite covers All (line 189) and Active (line 206) but has no corresponding case for Inactive. Since the mapping is a single match arm ("inactive"), the risk is low, but the variant is currently untested.
✅ Suggested addition
+ #[test]
+ fn builds_count_with_inactive_filter() {
+ let args = FetchOrdersArgs {
+ chain_ids: vec![1],
+ filter: FetchOrdersActiveFilter::Inactive,
+ ..FetchOrdersArgs::default()
+ };
+ let stmt = build_fetch_orders_count_stmt(&args).unwrap();
+ assert!(stmt.sql.contains("?1 = 'inactive'"));
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/common/src/local_db/query/fetch_orders_count/mod.rs` around lines 185
- 337, Add a test that mirrors the existing Active test but sets
FetchOrdersArgs.filter to FetchOrdersActiveFilter::Inactive and calls
build_fetch_orders_count_stmt(&args).unwrap(); then assert the generated SQL
contains the inactive marker (e.g. assert!(stmt.sql.contains("?1 = 'inactive'"))
to ensure the Inactive variant is covered; locate the test alongside
builds_count_with_active_filter near the other FetchOrdersActiveFilter tests and
use the same pattern with FetchOrdersArgs and build_fetch_orders_count_stmt.
Open
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.
Motivation
Related issue:
The orders list API returns a plain array of orders with no indication of total count, making it impossible to implement proper pagination on consuming services. The REST API needs to know the total number of matching orders to return pagination metadata (page count, total items) alongside results.
Solution
get_ordersnow returnsRaindexOrdersListResult { orders, totalCount }instead ofVec<RaindexOrder>.Local DB path:
fetch_orders_countSQL query that counts orders using the same filters asfetch_ordersbut without expensive detail joinspage/page_sizefields toFetchOrdersArgsto generateLIMIT/OFFSETclausesLocalDbOrders::listfetches the total count before applying paginationSubgraph path:
OrderbookSubgraphClient::orders_countpaginates throughorders_listto count all matching ordersMultiOrderbookSubgraphClient::orders_countsums counts across subgraphs, gracefully handling errorsFrontend:
OrdersListTable.svelteupdated to usedataSelectorto extract orders from the new result shapegetNextPageParamnow checkslastPage.orders.lengthinstead oflastPage.lengthChecks
By submitting this for review, I'm confirming I've done the following:
Summary by CodeRabbit
New Features
Tests
UI