Conversation
|
""" WalkthroughThe changes modularize the Changes
Sequence Diagram(s)sequenceDiagram
participant Client as OrderbookSubgraphClient
participant Subgraph as Subgraph GraphQL API
Client->>Subgraph: Query (e.g., order_detail, order_trades_list, etc.)
Subgraph-->>Client: GraphQL Response (data or error)
alt Paginated Query
loop While more pages
Client->>Subgraph: Query with pagination variables
Subgraph-->>Client: Page of results
end
Client->>Client: Aggregate all pages
end
Client->>Client: Process/compute (e.g., performance, volume)
Client-->>Caller: Return result or error
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 16th. To opt out, configure ✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (7)
crates/subgraph/src/orderbook_client.rs(0 hunks)crates/subgraph/src/orderbook_client/mod.rs(1 hunks)crates/subgraph/src/orderbook_client/order.rs(1 hunks)crates/subgraph/src/orderbook_client/order_trade.rs(1 hunks)crates/subgraph/src/orderbook_client/performance.rs(1 hunks)crates/subgraph/src/orderbook_client/transaction.rs(1 hunks)crates/subgraph/src/orderbook_client/vault.rs(1 hunks)
💤 Files with no reviewable changes (1)
- crates/subgraph/src/orderbook_client.rs
🧰 Additional context used
🧬 Code Graph Analysis (1)
crates/subgraph/src/orderbook_client/vault.rs (1)
crates/subgraph/src/orderbook_client/mod.rs (1)
new(76-78)
🔇 Additional comments (1)
crates/subgraph/src/orderbook_client/performance.rs (1)
205-220: Test relies on vector ordering that may not be guaranteed
vault_volumes[0]is assumed to be the input vault and index 1 the output vault.
Unlessget_vaults_volguarantees a stable ordering, the test could become flaky. Sort byid(or useiter().find(|v| v.id == …)) before asserting.
| let mut filters = SgVaultsListQueryFilters { | ||
| owner_in: filter_args.owners.clone(), | ||
| balance_gt: None, | ||
| }; | ||
|
|
||
| if filter_args.hide_zero_balance { | ||
| filters.balance_gt = Some(SgBigInt("0".to_string())); | ||
| } | ||
|
|
||
| let variables = SgVaultsListQueryVariables { | ||
| first: pagination_variables.first, | ||
| skip: pagination_variables.skip, | ||
| filters: if !filter_args.owners.is_empty() || filter_args.hide_zero_balance { | ||
| Some(filters) | ||
| } else { | ||
| None | ||
| }, | ||
| }; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Filter construction mutates then conditionally discards
filters.balance_gt is mutated before determining whether filters will even be included.
If both owners.is_empty() and hide_zero_balance == false, the mutation is wasted.
Very minor, but you can flip the order and avoid the extra mutation.
🤖 Prompt for AI Agents
In crates/subgraph/src/orderbook_client/vault.rs around lines 22 to 39, the code
sets filters.balance_gt before checking if filters will be used, causing
unnecessary mutation when filters is not included. To fix this, first check if
hide_zero_balance is true and only then create filters with balance_gt set;
otherwise, create filters without setting balance_gt. Then conditionally include
filters based on owners and hide_zero_balance without mutating filters
unnecessarily.
There was a problem hiding this comment.
no need to do this for now
There was a problem hiding this comment.
@findolor I understand, no problem! It was just a minor optimization suggestion. Let's proceed with the code as is.
| let pagination_vars = Self::parse_pagination_args(pagination_args); | ||
| let res = self | ||
| .query_paginated( | ||
| pagination_vars, | ||
| VaultBalanceChangesListPageQueryClient::new(self.url.clone()), | ||
| SgPaginationWithIdQueryVariables { | ||
| id: SgBytes(id.inner().to_string()), | ||
| skip: Some(0), | ||
| first: Some(200), | ||
| }, | ||
| 200, | ||
| ) |
There was a problem hiding this comment.
Pagination parameters are ignored – always queries first=200, skip=0
vault_balance_changes_list receives a pagination_args, parses it into pagination_vars, but the GraphQL variables passed to the query are hard-coded to first: 200, skip: 0, and the page_size argument to query_paginated is also a constant 200.
This means callers cannot request an arbitrary page/size even though the function signature advertises that capability.
- let pagination_vars = Self::parse_pagination_args(pagination_args);
- let res = self
- .query_paginated(
- pagination_vars,
- VaultBalanceChangesListPageQueryClient::new(self.url.clone()),
- SgPaginationWithIdQueryVariables {
- id: SgBytes(id.inner().to_string()),
- skip: Some(0),
- first: Some(200),
- },
- 200,
+ let pagination_vars = Self::parse_pagination_args(pagination_args);
+ let res = self
+ .query_paginated(
+ pagination_vars,
+ VaultBalanceChangesListPageQueryClient::new(self.url.clone()),
+ SgPaginationWithIdQueryVariables {
+ id: SgBytes(id.inner().to_string()),
+ skip: Some(pagination_vars.skip),
+ first: Some(pagination_vars.first),
+ },
+ pagination_vars.first,
)Without this fix every call will always return the first 200 balance changes only.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let pagination_vars = Self::parse_pagination_args(pagination_args); | |
| let res = self | |
| .query_paginated( | |
| pagination_vars, | |
| VaultBalanceChangesListPageQueryClient::new(self.url.clone()), | |
| SgPaginationWithIdQueryVariables { | |
| id: SgBytes(id.inner().to_string()), | |
| skip: Some(0), | |
| first: Some(200), | |
| }, | |
| 200, | |
| ) | |
| let pagination_vars = Self::parse_pagination_args(pagination_args); | |
| let res = self | |
| .query_paginated( | |
| pagination_vars, | |
| VaultBalanceChangesListPageQueryClient::new(self.url.clone()), | |
| SgPaginationWithIdQueryVariables { | |
| id: SgBytes(id.inner().to_string()), | |
| skip: Some(pagination_vars.skip), | |
| first: Some(pagination_vars.first), | |
| }, | |
| pagination_vars.first, | |
| ) |
🤖 Prompt for AI Agents
In crates/subgraph/src/orderbook_client/vault.rs around lines 82 to 93, the
pagination arguments passed to the GraphQL query are hard-coded to first=200 and
skip=0, ignoring the parsed pagination_vars. To fix this, replace the hard-coded
SgPaginationWithIdQueryVariables with one constructed from the parsed
pagination_vars so that the skip and first values reflect the caller's requested
page and size. Also, update the page_size argument in query_paginated to match
the requested page size instead of the constant 200.
There was a problem hiding this comment.
we have pagination in this function. in here the parameters are not used directly but some other logic is used to paginate the request
There was a problem hiding this comment.
@findolor Thanks for clarifying. It appears that the pagination works differently in vault_balance_changes_list compared to other methods like vaults_list. While you're passing the pagination parameters to query_paginated as the first argument, the GraphQL query variables use hard-coded values (skip: Some(0), first: Some(200)).
If the pagination is handled by special logic inside the query_paginated method that uses the pagination_vars parameter rather than the query variables, that would explain why it works as expected despite the hard-coded values.
Is this an intentional design difference from other pagination methods in the codebase? If you're confident the pagination works correctly, we can move forward with the current implementation.
There was a problem hiding this comment.
yes the pagination works fine in our application
There was a problem hiding this comment.
@findolor Great! Thanks for confirming that pagination is working properly in your application. Since this appears to be an intentional design choice in your implementation and functions correctly, we can consider this discussion resolved.
🐰
✏️ Learnings added
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1858
File: crates/subgraph/src/orderbook_client/vault.rs:81-92
Timestamp: 2025-05-19T12:09:10.673Z
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.
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.
There was a problem hiding this comment.
@findolor is there a good reason why query_paginated take both pagination_variables: SgQueryPaginationVariables and page_query_variables / why the duplication happens?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (2)
crates/subgraph/src/orderbook_client/mod.rs (1)
57-61: Flattening errors to plain strings loses variant information (duplicate)You previously clarified that this simplification is acceptable, so no action required here.
crates/subgraph/src/orderbook_client/order_trade.rs (1)
57-74: Duplicate of a nitpick already raised on a previous commit – see earlier thread about replacing theif … elsewith an early return.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (10)
crates/cli/src/commands/order/detail.rs(1 hunks)crates/cli/src/commands/order/remove.rs(1 hunks)crates/quote/src/quote.rs(1 hunks)crates/subgraph/src/cynic_client.rs(2 hunks)crates/subgraph/src/orderbook_client/mod.rs(1 hunks)crates/subgraph/src/orderbook_client/order.rs(1 hunks)crates/subgraph/src/orderbook_client/order_trade.rs(1 hunks)crates/subgraph/src/orderbook_client/performance.rs(1 hunks)crates/subgraph/src/orderbook_client/vault.rs(1 hunks)crates/subgraph/src/vault_balance_changes_query.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
crates/subgraph/src/orderbook_client/order.rs (1)
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1858
File: crates/subgraph/src/orderbook_client/order.rs:94-108
Timestamp: 2025-05-19T11:40:45.851Z
Learning: Order hashes are unique in the subgraph data model, so a query by hash (`order_detail_by_hash`) will never return multiple orders with the same hash.
crates/subgraph/src/orderbook_client/vault.rs (1)
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1858
File: crates/subgraph/src/orderbook_client/vault.rs:81-92
Timestamp: 2025-05-19T12:09:10.673Z
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.
🧬 Code Graph Analysis (3)
crates/subgraph/src/vault_balance_changes_query.rs (2)
crates/subgraph/src/cynic_client.rs (1)
get_base_url(19-19)crates/subgraph/src/orderbook_client/mod.rs (1)
get_base_url(69-71)
crates/subgraph/src/cynic_client.rs (2)
crates/subgraph/src/orderbook_client/mod.rs (1)
get_base_url(69-71)crates/subgraph/src/vault_balance_changes_query.rs (1)
get_base_url(20-22)
crates/subgraph/src/orderbook_client/order_trade.rs (4)
crates/subgraph/src/orderbook_client/performance.rs (3)
setup_client(53-56)default_sg_erc20(58-66)default_sg_trade(112-179)crates/subgraph/src/orderbook_client/order.rs (1)
setup_client(122-125)crates/subgraph/src/orderbook_client/transaction.rs (3)
setup_client(70-73)default_sg_transaction(75-82)default_sg_erc20(84-92)crates/subgraph/src/orderbook_client/vault.rs (4)
setup_client(138-141)default_sg_transaction(240-247)default_sg_orderbook(153-157)default_sg_erc20(143-151)
⏰ Context from checks skipped due to timeout of 90000ms (16)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-test)
- GitHub Check: standard-tests (ubuntu-latest, rainix-wasm-artifacts)
- GitHub Check: standard-tests (ubuntu-latest, test-js-bindings)
- GitHub Check: standard-tests (ubuntu-latest, rainix-rs-static)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-static)
- GitHub Check: standard-tests (ubuntu-latest, rainix-wasm-test)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-legal)
- GitHub Check: standard-tests (ubuntu-latest, ob-rs-test, true)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-artifacts)
- GitHub Check: git-clean
- GitHub Check: standard-tests (ubuntu-latest, rainix-rs-artifacts, true)
- GitHub Check: Deploy-Preview
- GitHub Check: test
- GitHub Check: test
- GitHub Check: test
- GitHub Check: build-tauri (ubuntu-22.04, true)
🔇 Additional comments (8)
crates/cli/src/commands/order/detail.rs (1)
21-21: Updated parameter passing to use reference semantics.The code now passes a reference to the order ID instead of an owned value, which aligns with the updated signature of the
order_detailmethod in theOrderbookSubgraphClientimplementation. This change is part of a broader refactoring to improve consistency and potentially performance across the codebase.crates/cli/src/commands/order/remove.rs (1)
29-29: Updated parameter passing to use reference semantics.The code now passes a reference to the order ID instead of an owned value, which aligns with the updated signature of the
order_detailmethod in theOrderbookSubgraphClientimplementation. This is consistent with similar changes in other CLI commands, ensuring uniform API usage patterns throughout the codebase.crates/quote/src/quote.rs (1)
160-160: Updated parameter passing to use reference semantics.The code now passes a reference to the constructed
Idvalue rather than an owned value. This change properly aligns with the updated signature of theorder_detailmethod in theOrderbookSubgraphClientimplementation, improving consistency with other areas of the codebase that have undergone similar refactoring.crates/subgraph/src/vault_balance_changes_query.rs (1)
20-21: Updated method return type to use reference semantics.The
get_base_urlmethod now returns a reference to the internal URL instead of potentially cloning it. This change aligns with theCynicClienttrait definition and is part of a broader pattern of optimizing ownership semantics across the codebase.crates/subgraph/src/orderbook_client/mod.rs (1)
30-31: 👍 Constant is now public – duplicates removedMaking
ALL_PAGES_QUERY_PAGE_SIZEpubsolves the “magic number” duplication flagged in an earlier review. Nice catch!crates/subgraph/src/orderbook_client/order.rs (1)
72-91: 🧹 Nitpick (assertive)Prefer early-exit to reduce nesting
A single
breakwhenpage_datais empty removes theelsearm and one level of indentation, making the “happy path” clearer.- if page_data.is_empty() { - break; - } - all_pages_merged.extend(page_data); - page += 1 + if page_data.is_empty() { + break; + } + all_pages_merged.extend(page_data); + page += 1;Likely an incorrect or invalid review comment.
crates/subgraph/src/orderbook_client/vault.rs (2)
106-121: 🧹 Nitpick (assertive)Apply early-exit pattern here as well
For consistency with other paging loops and simpler control flow.
- if page_data.is_empty() { - break; - } - all_pages_merged.extend(page_data); - page += 1 + if page_data.is_empty() { + break; + } + all_pages_merged.extend(page_data); + page += 1;Likely an incorrect or invalid review comment.
53-71: 🧹 Nitpick (assertive)Same early-exit simplification as in
orders_list_all- if page_data.is_empty() { - break; - } - all_pages_merged.extend(page_data); - page += 1 + if page_data.is_empty() { + break; + } + all_pages_merged.extend(page_data); + page += 1;Likely an incorrect or invalid review comment.
| pub async fn order_vaults_volume( | ||
| &self, | ||
| order_id: cynic::Id, | ||
| start_timestamp: Option<u64>, | ||
| end_timestamp: Option<u64>, | ||
| ) -> Result<Vec<VaultVolume>, OrderbookSubgraphClientError> { | ||
| let trades = self | ||
| .order_trades_list_all(order_id, start_timestamp, end_timestamp) | ||
| .await?; | ||
| Ok(get_vaults_vol(&trades)?) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Keep API consistent – accept &Id to avoid needless moves
order_vaults_volume takes ownership of order_id while immediately passing it to another call.
Passing a reference (as you already do in order_detail) avoids an extra move/clone, keeps the public API uniform, and allows callers to keep using the same Id value afterward.
-pub async fn order_vaults_volume(
- &self,
- order_id: cynic::Id,
+pub async fn order_vaults_volume(
+ &self,
+ order_id: &cynic::Id,
...
- let trades = self
- .order_trades_list_all(order_id, start_timestamp, end_timestamp)
+ let trades = self
+ .order_trades_list_all(order_id.clone(), start_timestamp, end_timestamp)You will need to tweak order_trades_list_all to accept &Id as well (mirroring order_detail).
📝 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.
| pub async fn order_vaults_volume( | |
| &self, | |
| order_id: cynic::Id, | |
| start_timestamp: Option<u64>, | |
| end_timestamp: Option<u64>, | |
| ) -> Result<Vec<VaultVolume>, OrderbookSubgraphClientError> { | |
| let trades = self | |
| .order_trades_list_all(order_id, start_timestamp, end_timestamp) | |
| .await?; | |
| Ok(get_vaults_vol(&trades)?) | |
| } | |
| pub async fn order_vaults_volume( | |
| &self, | |
| order_id: &cynic::Id, | |
| start_timestamp: Option<u64>, | |
| end_timestamp: Option<u64>, | |
| ) -> Result<Vec<VaultVolume>, OrderbookSubgraphClientError> { | |
| let trades = self | |
| .order_trades_list_all(order_id.clone(), start_timestamp, end_timestamp) | |
| .await?; | |
| Ok(get_vaults_vol(&trades)?) | |
| } |
🤖 Prompt for AI Agents
In crates/subgraph/src/orderbook_client/performance.rs around lines 5 to 15,
change the order_vaults_volume function to accept a reference to Id (&Id)
instead of taking ownership. Update the function signature and all uses of
order_id inside the function to use a reference. Also, modify the
order_trades_list_all method to accept &Id instead of Id to maintain consistency
and avoid unnecessary moves or clones. This keeps the API uniform and allows
callers to reuse the same Id value.
There was a problem hiding this comment.
this is not needed as we need to change every implementation if this is used. our current logic works as expected at the moment
There was a problem hiding this comment.
@findolor I understand your point - changing the parameter type would require updating multiple implementations, and your current logic works as expected. You're right to avoid unnecessary changes that could introduce inconsistencies. Thanks for clarifying the design decision!
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
crates/subgraph/src/orderbook_client/vault.rs (2)
22-30: (Duplicate) Early mutation offiltersbefore need checkThe filter’s
balance_gtfield is mutated before verifying whether the filter object will be used, causing an unnecessary mutation whenowners.is_empty()andhide_zero_balanceis false.
This was discussed earlier and consciously kept; mentioning for completeness.
76-92: 💡 Verification agent❓ Verification inconclusive
Pagination contract is unclear – caller’s
page_sizeis silently ignored
vault_balance_changes_listacceptspagination_args, but the GraphQL variables are hard-coded tofirst = 200and the internal page size passed toquery_paginatedis also200, regardless of the caller’s requestedpage_size.
If the larger page size is intentional (to minimise network round-trips) please document this deviation in a doc-comment; otherwise, wire the caller’s values through.- SgPaginationWithIdQueryVariables { - id: SgBytes(id.inner().to_string()), - skip: Some(0), - first: Some(200), - }, - 200, + SgPaginationWithIdQueryVariables { + id: SgBytes(id.inner().to_string()), + skip: Some(pagination_vars.skip), + first: Some(pagination_vars.first), + }, + pagination_vars.first,
Respect the caller’s pagination_args in
vault_balance_changes_listThe implementation currently ignores the provided
pagination_argsand hard-codes both GraphQL variables and the internal page size to200. To allow callers to control page size and offset, wire throughpagination_vars.skipandpagination_vars.firstin both the query variables and thequery_paginatedcall:- SgPaginationWithIdQueryVariables { - id: SgBytes(id.inner().to_string()), - skip: Some(0), - first: Some(200), - }, - 200, + SgPaginationWithIdQueryVariables { + id: SgBytes(id.inner().to_string()), + skip: Some(pagination_vars.skip), + first: Some(pagination_vars.first), + }, + pagination_vars.first,• File:
crates/subgraph/src/orderbook_client/vault.rs
• Lines: 76–92This change ensures the caller’s requested
page_sizeand offset are respected.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
crates/subgraph/src/orderbook_client/mod.rs(1 hunks)crates/subgraph/src/orderbook_client/vault.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
crates/subgraph/src/orderbook_client/mod.rs (2)
Learnt from: findolor
PR: rainlanguage/rain.orderbook#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: findolor
PR: rainlanguage/rain.orderbook#1858
File: crates/subgraph/src/cynic_client.rs:27-29
Timestamp: 2025-05-19T12:25:02.620Z
Learning: In the Rain Orderbook codebase, when passing a URL from `self.get_base_url()` to `reqwest::Client::post()`, the `.clone()` is necessary because `get_base_url()` returns a reference (`&Url`), but an owned `Url` is needed for the reqwest client in the async context to avoid lifetime issues.
crates/subgraph/src/orderbook_client/vault.rs (1)
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1858
File: crates/subgraph/src/orderbook_client/vault.rs:81-92
Timestamp: 2025-05-19T12:09:10.673Z
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.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Deploy-Preview
🔇 Additional comments (1)
crates/subgraph/src/orderbook_client/mod.rs (1)
29-29: Good call exposingALL_PAGES_QUERY_PAGE_SIZEaspubMaking the constant public prevents the “magic-number drift” that was highlighted in earlier reviews.
No further action required.
| let vaults_page1: Vec<SgVault> = (0..ALL_PAGES_QUERY_PAGE_SIZE) | ||
| .map(|_| default_sg_vault()) | ||
| .collect(); | ||
| let vaults_page2: Vec<SgVault> = (0..50).map(|_| default_sg_vault()).collect(); | ||
|
|
||
| sg_server.mock(|when, then| { |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Potentially heavy test fixture creation
(0..ALL_PAGES_QUERY_PAGE_SIZE).map(|_| default_sg_vault()) builds 200 full SgVault structs twice, which slows the test suite.
Consider using repeat_with(default_sg_vault).take(n) or creating a lighter stub when only the len() is asserted.
Not critical, but speeds CI runs on large suites.
🤖 Prompt for AI Agents
In crates/subgraph/src/orderbook_client/vault.rs around lines 508 to 513, the
test creates two large vectors of full SgVault structs using map and collect,
which is inefficient and slows down tests. Replace the map calls with
repeat_with(default_sg_vault).take(n) to generate the vectors more efficiently,
or if only the length is checked, create lighter stub structs instead of full
SgVault instances to speed up the test suite and CI runs.
There was a problem hiding this comment.
the tests run good as far as i can see, i don't see a need to update this
There was a problem hiding this comment.
@findolor Understood! If the tests are running well and performance isn't an issue, there's no need to change the existing implementation. The suggestion was just a potential optimization for test execution speed, but if it's working well for you now, that's what matters most.
🧠 Learnings used
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1858
File: crates/subgraph/src/orderbook_client/vault.rs:81-92
Timestamp: 2025-05-19T12:09:10.673Z
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.
There was a problem hiding this comment.
repeat_with(default_sg_vault).take(n) is a bit more readable but it's fine as is
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
tauri-app/src-tauri/src/commands/order.rs(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (16)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-legal)
- GitHub Check: standard-tests (ubuntu-latest, rainix-wasm-test)
- GitHub Check: standard-tests (ubuntu-latest, rainix-wasm-artifacts)
- GitHub Check: standard-tests (ubuntu-latest, test-js-bindings)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-artifacts)
- GitHub Check: standard-tests (ubuntu-latest, ob-rs-test, true)
- GitHub Check: test
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-static)
- GitHub Check: standard-tests (ubuntu-latest, rainix-rs-static)
- GitHub Check: test
- GitHub Check: test
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-test)
- GitHub Check: standard-tests (ubuntu-latest, rainix-rs-artifacts, true)
- GitHub Check: build-tauri (ubuntu-22.04, true)
- GitHub Check: Deploy-Preview
- GitHub Check: git-clean
| e | ||
| })? | ||
| .order_detail(id.into()) | ||
| .order_detail(&id.into()) |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Match updated order_detail signature, consider extracting Id conversion
Passing &id.into() correctly aligns with the new signature (&Id). To avoid borrowing a temporary and improve readability, extract the conversion into a local variable:
- .order_detail(&id.into())
+ let order_id = id.into();
+ .order_detail(&order_id)Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In tauri-app/src-tauri/src/commands/order.rs at line 68, the call to
order_detail currently passes &id.into() directly, which borrows a temporary. To
fix this, create a local variable by converting id into Id first, then pass a
reference to this variable to order_detail. This avoids borrowing a temporary
and improves code clarity.
| e | ||
| })? | ||
| .order_detail(id.into()) | ||
| .order_detail(&id.into()) |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consistent order_remove_calldata update, apply the same extraction
The change to &id.into() here also matches the refactored signature. For consistency and clearer lifetimes, you can extract the Id conversion before passing it:
- .order_detail(&id.into())
+ let order_id = id.into();
+ .order_detail(&order_id)Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In tauri-app/src-tauri/src/commands/order.rs at line 120, the call to
order_detail uses &id.into() inline, which can cause unclear lifetimes. To fix
this, extract the conversion of id into a separate variable of type Id before
passing it to order_detail, ensuring consistent and clearer lifetime handling as
done in order_remove_calldata.
3 participants
Motivation
See issue: #1822
Solution
Checks
By submitting this for review, I'm confirming I've done the following:
fix #1822
Summary by CodeRabbit
New Features
Bug Fixes
Tests