Update vault data source trait with additional methods #2352
Conversation
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
Contributor
WalkthroughIntroduces a new fetch_all_tokens query module for retrieving ERC20 tokens from the local database with optional filtering by chain IDs and orderbook addresses. Extends VaultsDataSource trait with balance_changes_list and tokens_list methods. Integrates local DB token fetching into vault operations with subgraph fallback support and result merging. Changes
Sequence DiagramsequenceDiagram
actor Client
participant RaindexVault
participant LocalDbVaults
participant SubgraphVaults
participant LocalDbExecutor
participant Subgraph
Client->>RaindexVault: get_all_vault_tokens(chain_ids)
RaindexVault->>RaindexVault: partition chain_ids<br/>(local_db vs subgraph)
par Local DB Path
RaindexVault->>LocalDbVaults: tokens_list(local_chain_ids)
LocalDbVaults->>LocalDbVaults: build FetchAllTokensArgs
LocalDbVaults->>LocalDbExecutor: fetch_all_tokens
LocalDbExecutor-->>LocalDbVaults: Vec<LocalDbToken>
LocalDbVaults->>LocalDbVaults: convert to RaindexVaultToken
LocalDbVaults-->>RaindexVault: local_tokens
and Subgraph Path
RaindexVault->>SubgraphVaults: tokens_list(subgraph_chain_ids)
SubgraphVaults->>Subgraph: query tokens
Subgraph-->>SubgraphVaults: token data
SubgraphVaults->>SubgraphVaults: convert to RaindexVaultToken
SubgraphVaults-->>RaindexVault: subgraph_tokens
end
RaindexVault->>RaindexVault: merge local_tokens<br/>+ subgraph_tokens
RaindexVault-->>Client: all_tokens
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45–60 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
Collaborator
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
![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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/common/src/raindex_client/local_db/vaults.rs (1)
172-177: Add tests forbalance_changes_listandtokens_listmethods.The LocalDbVaults implementation defines
balance_changes_list(line 112) andtokens_list(line 133), but the test module only coverslist()andget_by_id(). These new trait methods lack test coverage.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (7)
crates/common/src/local_db/query/fetch_all_tokens/mod.rs(1 hunks)crates/common/src/local_db/query/fetch_all_tokens/query.sql(1 hunks)crates/common/src/local_db/query/mod.rs(1 hunks)crates/common/src/raindex_client/local_db/query/fetch_all_tokens.rs(1 hunks)crates/common/src/raindex_client/local_db/query/mod.rs(1 hunks)crates/common/src/raindex_client/local_db/vaults.rs(2 hunks)crates/common/src/raindex_client/vaults.rs(5 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
crates/**/*.rs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
crates/**/*.rs: For Rust crates incrates/*, run lints usingnix develop -c cargo clippy --workspace --all-targets --all-features -D warnings
For Rust crates incrates/*, run tests usingnix develop -c cargo test --workspaceor--package <crate>
Files:
crates/common/src/raindex_client/local_db/query/mod.rscrates/common/src/local_db/query/mod.rscrates/common/src/raindex_client/local_db/query/fetch_all_tokens.rscrates/common/src/raindex_client/local_db/vaults.rscrates/common/src/local_db/query/fetch_all_tokens/mod.rscrates/common/src/raindex_client/vaults.rs
**/crates/**
📄 CodeRabbit inference engine (AGENTS.md)
Rust workspace organized as
crates/*with subdirectories: cli, common, bindings, js_api, quote, subgraph, settings, math, integration_tests
Files:
crates/common/src/raindex_client/local_db/query/mod.rscrates/common/src/local_db/query/mod.rscrates/common/src/raindex_client/local_db/query/fetch_all_tokens.rscrates/common/src/raindex_client/local_db/vaults.rscrates/common/src/local_db/query/fetch_all_tokens/query.sqlcrates/common/src/local_db/query/fetch_all_tokens/mod.rscrates/common/src/raindex_client/vaults.rs
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Rust: format code withnix develop -c cargo fmt --all
Rust: lint withnix develop -c rainix-rs-static(preconfigured flags included)
Rust: crates and modules usesnake_case; types usePascalCase
Files:
crates/common/src/raindex_client/local_db/query/mod.rscrates/common/src/local_db/query/mod.rscrates/common/src/raindex_client/local_db/query/fetch_all_tokens.rscrates/common/src/raindex_client/local_db/vaults.rscrates/common/src/local_db/query/fetch_all_tokens/mod.rscrates/common/src/raindex_client/vaults.rs
🧠 Learnings (22)
📓 Common learnings
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: 1994
File: crates/common/src/raindex_client/vaults.rs:59-59
Timestamp: 2025-07-16T05:52:05.576Z
Learning: User findolor prefers to handle documentation updates for getter methods in batch via dedicated PRs rather than addressing them individually during feature development, as mentioned for the formatted amount string fields in crates/common/src/raindex_client/vaults.rs.
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: 1872
File: packages/webapp/src/__tests__/handleVaultDeposit.test.ts:20-53
Timestamp: 2025-06-07T05:19:46.330Z
Learning: In the rain.orderbook codebase, simple wrapper/adapter functions that just delegate to other functions (like handleVaultDeposit) don't need extensive edge case testing for missing parameters or error handling - the current test coverage focusing on core functionality is sufficient.
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 2237
File: crates/common/src/raindex_client/local_db/sync.rs:79-89
Timestamp: 2025-10-18T10:38:41.273Z
Learning: In `crates/common/src/raindex_client/local_db/sync.rs`, the sync_database method currently only supports indexing a single orderbook per chain ID, which is why `.first()` is used to select the orderbook configuration. Multi-orderbook support per chain ID is planned for future PRs.
📚 Learning: 2025-10-06T11:44:07.888Z
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`.
Applied to files:
crates/common/src/raindex_client/local_db/query/mod.rscrates/common/src/raindex_client/local_db/vaults.rscrates/common/src/raindex_client/vaults.rs
📚 Learning: 2025-10-21T05:15:50.518Z
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 2256
File: crates/common/src/raindex_client/local_db/query/fetch_tables.rs:7-7
Timestamp: 2025-10-21T05:15:50.518Z
Learning: In Rust 2021+ editions, passing a reference to a temporary into an async function within the same expression (e.g., `exec.query_json(&fetch_tables_stmt()).await`) is safe and idiomatic. The temporary lifetime is extended across the await point, so this pattern should not be flagged as an issue.
Applied to files:
crates/common/src/raindex_client/local_db/query/fetch_all_tokens.rs
📚 Learning: 2025-05-20T10:20:08.206Z
Learnt from: 0xgleb
Repo: rainlanguage/rain.orderbook PR: 1859
File: crates/quote/src/quote_debug.rs:472-492
Timestamp: 2025-05-20T10:20:08.206Z
Learning: In the Rain Orderbook codebase, the `#[tokio::test(flavor = "multi_thread")]` annotation is specifically needed for tests that use `LocalEvm`, not just for consistency across all async tests.
Applied to files:
crates/common/src/raindex_client/local_db/query/fetch_all_tokens.rs
📚 Learning: 2025-07-16T10:40:05.717Z
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.
Applied to files:
crates/common/src/raindex_client/local_db/vaults.rscrates/common/src/raindex_client/vaults.rs
📚 Learning: 2025-10-14T07:51:55.148Z
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 2202
File: crates/common/src/raindex_client/local_db/sync.rs:33-34
Timestamp: 2025-10-14T07:51:55.148Z
Learning: In `crates/common/src/raindex_client/local_db/sync.rs`, the hard-coded `DEFAULT_SYNC_CHAIN_ID` constant (set to `SUPPORTED_LOCAL_DB_CHAINS[0]`) will be replaced with proper chain ID handling in downstream PRs as part of the multi-network/orderbook implementation.
Applied to files:
crates/common/src/raindex_client/local_db/vaults.rscrates/common/src/local_db/query/fetch_all_tokens/mod.rscrates/common/src/raindex_client/vaults.rs
📚 Learning: 2025-08-01T07:44:53.910Z
Learnt from: brusherru
Repo: rainlanguage/rain.orderbook PR: 2036
File: crates/js_api/src/filters/raindex_filter_store.rs:319-336
Timestamp: 2025-08-01T07:44:53.910Z
Learning: In the rainlanguage/rain.orderbook project's RaindexFilterStore (crates/js_api/src/filters/raindex_filter_store.rs), the team chose a simplified monolithic approach with hard-coded keys and default auto-save behavior over configurable stores. The update_vaults method intentionally auto-saves to both localStorage and URL after each update as the default behavior, following a design evolution from a previous configurable approach.
Applied to files:
crates/common/src/raindex_client/local_db/vaults.rscrates/common/src/raindex_client/vaults.rs
📚 Learning: 2025-06-20T07:51:08.790Z
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 1941
File: crates/js_api/src/raindex/vaults.rs:175-181
Timestamp: 2025-06-20T07:51:08.790Z
Learning: In the RaindexClient vault methods, `&self` parameters are intentionally kept for API consistency to make all vault operations instance methods, even when the methods don't use client state. This is a design preference for maintaining a uniform interface.
Applied to files:
crates/common/src/raindex_client/local_db/vaults.rscrates/common/src/raindex_client/vaults.rs
📚 Learning: 2025-08-01T09:07:20.383Z
Learnt from: brusherru
Repo: rainlanguage/rain.orderbook PR: 2044
File: packages/orderbook/README.md:186-189
Timestamp: 2025-08-01T09:07:20.383Z
Learning: In the rainlanguage/rain.orderbook project, Rust methods on structs like RaindexVaultsList are exported as JavaScript getters in WASM bindings using #[wasm_bindgen(getter)]. This means while the Rust code uses method calls like items(), the JavaScript/WASM API exposes them as property access like .items. The README.md correctly documents the JavaScript API surface, not the Rust implementation details.
Applied to files:
crates/common/src/raindex_client/local_db/vaults.rscrates/common/src/raindex_client/vaults.rs
📚 Learning: 2025-10-18T10:38:41.273Z
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 2237
File: crates/common/src/raindex_client/local_db/sync.rs:79-89
Timestamp: 2025-10-18T10:38:41.273Z
Learning: In `crates/common/src/raindex_client/local_db/sync.rs`, the sync_database method currently only supports indexing a single orderbook per chain ID, which is why `.first()` is used to select the orderbook configuration. Multi-orderbook support per chain ID is planned for future PRs.
Applied to files:
crates/common/src/raindex_client/local_db/vaults.rscrates/common/src/raindex_client/vaults.rs
📚 Learning: 2025-07-16T05:52:05.576Z
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 1994
File: crates/common/src/raindex_client/vaults.rs:59-59
Timestamp: 2025-07-16T05:52:05.576Z
Learning: User findolor prefers to handle documentation updates for getter methods in batch via dedicated PRs rather than addressing them individually during feature development, as mentioned for the formatted amount string fields in crates/common/src/raindex_client/vaults.rs.
Applied to files:
crates/common/src/raindex_client/local_db/vaults.rscrates/common/src/raindex_client/vaults.rs
📚 Learning: 2025-10-06T11:13:29.956Z
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 2123
File: crates/common/src/raindex_client/local_db/mod.rs:23-29
Timestamp: 2025-10-06T11:13:29.956Z
Learning: In `crates/common/src/raindex_client/local_db/mod.rs`, the `Default` implementation for `LocalDb` that creates an RPC client pointing to `http://localhost:4444` is acceptable because the RPC client must be explicitly configured before actual usage in production scenarios.
Applied to files:
crates/common/src/raindex_client/local_db/vaults.rscrates/common/src/raindex_client/vaults.rs
📚 Learning: 2025-10-06T14:13:18.531Z
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.
Applied to files:
crates/common/src/raindex_client/local_db/vaults.rscrates/common/src/raindex_client/vaults.rs
📚 Learning: 2025-06-24T08:46:03.368Z
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.
Applied to files:
crates/common/src/raindex_client/local_db/vaults.rs
📚 Learning: 2025-05-19T12:09:10.694Z
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.
Applied to files:
crates/common/src/raindex_client/local_db/vaults.rscrates/common/src/raindex_client/vaults.rs
📚 Learning: 2025-07-31T19:34:11.716Z
Learnt from: brusherru
Repo: rainlanguage/rain.orderbook PR: 2044
File: crates/common/src/raindex_client/vaults_list.rs:363-423
Timestamp: 2025-07-31T19:34:11.716Z
Learning: In the rainlanguage/rain.orderbook project, for WASM-exposed functionality like VaultsList, the team prefers to keep comprehensive tests in the non-WASM environment due to the complexity of recreating objects like RaindexVaults in WASM. WASM tests focus on basic functionality and error cases since the WASM code reuses the already-tested non-WASM implementation.
Applied to files:
crates/common/src/raindex_client/local_db/vaults.rs
📚 Learning: 2025-06-07T05:19:46.330Z
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 1872
File: packages/webapp/src/__tests__/handleVaultDeposit.test.ts:20-53
Timestamp: 2025-06-07T05:19:46.330Z
Learning: In the rain.orderbook codebase, simple wrapper/adapter functions that just delegate to other functions (like handleVaultDeposit) don't need extensive edge case testing for missing parameters or error handling - the current test coverage focusing on core functionality is sufficient.
Applied to files:
crates/common/src/raindex_client/local_db/vaults.rs
📚 Learning: 2025-10-06T11:28:30.692Z
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 2145
File: crates/common/src/raindex_client/local_db/query/fetch_orders/query.sql:6-7
Timestamp: 2025-10-06T11:28:30.692Z
Learning: In `crates/common/src/raindex_client/local_db/query/fetch_orders/query.sql`, the orderbook_address is currently hardcoded to '0x2f209e5b67A33B8fE96E28f24628dF6Da301c8eB' because the system only supports a single orderbook at the moment. Multiorderbook logic is not yet implemented and will be added in the future.
Applied to files:
crates/common/src/raindex_client/vaults.rs
📚 Learning: 2025-07-15T08:01:38.534Z
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.
Applied to files:
crates/common/src/raindex_client/vaults.rs
📚 Learning: 2025-10-02T19:17:20.332Z
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 2163
File: crates/common/src/raindex_client/orders.rs:738-741
Timestamp: 2025-10-02T19:17:20.332Z
Learning: In crates/common/src/raindex_client/orders.rs, fetch_dotrain_source() is intentionally called in try_from_sg_order for every order conversion because the dotrain source information is needed immediately. A future optimization with local DB logic is planned to eliminate the network round-trip concern.
Applied to files:
crates/common/src/raindex_client/vaults.rs
📚 Learning: 2025-07-04T10:26:24.289Z
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 1956
File: crates/common/src/raindex_client/mod.rs:116-135
Timestamp: 2025-07-04T10:26:24.289Z
Learning: In crates/common/src/raindex_client/mod.rs, the get_multi_subgraph_args method intentionally treats Some(empty vector) the same as None for chain_ids parameter. Both cases should return all networks to support UI behavior where no selection or empty selection means "show all networks". Only when specific chain IDs are provided should the results be filtered.
Applied to files:
crates/common/src/raindex_client/vaults.rs
📚 Learning: 2025-06-18T18:18:44.330Z
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 1938
File: crates/js_api/src/raindex/mod.rs:92-99
Timestamp: 2025-06-18T18:18:44.330Z
Learning: In crates/js_api/src/raindex/mod.rs, the get_subgraph_url_for_chain method and get_multi_subgraph_args method intentionally duplicate lookup logic because they serve different purposes: get_subgraph_url_for_chain returns only the URL string, while get_multi_subgraph_args returns MultiSubgraphArgs structures containing both URL and network information (name/label). This duplication is acceptable and by design.
Applied to files:
crates/common/src/raindex_client/vaults.rs
🧬 Code graph analysis (5)
crates/common/src/raindex_client/local_db/query/mod.rs (1)
crates/common/src/raindex_client/local_db/query/fetch_all_tokens.rs (1)
fetch_all_tokens(6-12)
crates/common/src/local_db/query/mod.rs (1)
crates/common/src/raindex_client/local_db/query/fetch_all_tokens.rs (1)
fetch_all_tokens(6-12)
crates/common/src/raindex_client/local_db/query/fetch_all_tokens.rs (8)
crates/common/src/raindex_client/mod.rs (1)
local_db(228-230)crates/common/src/local_db/query/fetch_all_tokens/mod.rs (1)
build_fetch_all_tokens_stmt(31-59)crates/common/src/raindex_client/local_db/mod.rs (1)
exec(72-72)crates/common/src/local_db/pipeline/adapters/window.rs (1)
cfg(153-163)crates/common/src/local_db/export.rs (1)
executor(97-98)crates/common/src/raindex_client/local_db/executor.rs (2)
create_sql_capturing_callback(170-193)from_ref(25-27)crates/common/src/local_db/query/mod.rs (1)
from(69-71)crates/common/src/local_db/query/sql_statement.rs (1)
sql(99-101)
crates/common/src/raindex_client/local_db/vaults.rs (4)
crates/common/src/raindex_client/local_db/query/fetch_all_tokens.rs (1)
fetch_all_tokens(6-12)crates/common/src/raindex_client/local_db/query/fetch_vault_balance_changes.rs (1)
fetch_vault_balance_changes(8-17)crates/common/src/raindex_client/local_db/query/fetch_vaults.rs (1)
fetch_vaults(24-30)crates/common/src/raindex_client/vaults.rs (14)
vaults(1348-1369)vault(1502-1506)vault(1507-1511)vault(1567-1567)new(50-52)new(216-221)try_from_local_db(809-844)try_from_local_db(1556-1596)chain_id(126-128)chain_id(173-175)chain_id(267-269)chain_id(294-296)chain_id(1942-1943)from_local_db_token(1617-1628)
crates/common/src/local_db/query/fetch_all_tokens/mod.rs (2)
crates/common/src/local_db/query/mod.rs (1)
from(69-71)crates/common/src/local_db/query/sql_statement.rs (2)
sql(99-101)params(103-105)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (18)
- GitHub Check: test
- GitHub Check: git-clean
- GitHub Check: test
- GitHub Check: test
- GitHub Check: standard-tests (ubuntu-latest, rainix-wasm-browser-test)
- GitHub Check: standard-tests (ubuntu-latest, rainix-wasm-artifacts)
- GitHub Check: standard-tests (ubuntu-latest, rainix-rs-static)
- GitHub Check: standard-tests (ubuntu-latest, test-js-bindings)
- GitHub Check: standard-tests (ubuntu-latest, rainix-rs-artifacts, true)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-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: standard-tests (ubuntu-latest, rainix-sol-static)
- GitHub Check: standard-tests (ubuntu-latest, rainix-wasm-test)
- GitHub Check: build-tauri (ubuntu-22.04, true)
- GitHub Check: Deploy-Preview-Push
- GitHub Check: Deploy-Docs-Preview
🔇 Additional comments (14)
crates/common/src/local_db/query/mod.rs (1)
6-6: LGTM!The new module export follows the existing naming conventions and is correctly placed in alphabetical order among the other
fetch_*modules.crates/common/src/raindex_client/local_db/query/mod.rs (1)
3-3: LGTM!The module export is correctly placed and follows the established pattern for exposing local DB query functionality through the raindex_client layer.
crates/common/src/local_db/query/fetch_all_tokens/query.sql (1)
1-7: LGTM!The SQL query is well-structured:
SELECT DISTINCTcorrectly deduplicates tokens that may appear multiple times.WHERE 1=1pattern enables clean dynamic clause injection.- Placeholder comments align with the
SqlStatementbuilder pattern.- Deterministic ordering by the composite key.
crates/common/src/raindex_client/local_db/query/fetch_all_tokens.rs (2)
1-12: LGTM!The function follows the established pattern for local DB query wrappers:
- Generic over
LocalDbQueryExecutorwith?Sizedbound for trait object support.- Proper error propagation from the statement builder.
- Consistent with sibling functions like
fetch_vaultsandfetch_vault_balance_changes.
44-46: No changes needed. Line 45's direct access toexpected_stmt.sqlis valid because the field haspub(crate)visibility and the test is within the same crate. The getter methodsql()exists for external API usage.crates/common/src/raindex_client/local_db/vaults.rs (2)
1-18: LGTM!Imports are well-organized and all additions are required by the new
balance_changes_listandtokens_listmethod implementations.
111-131: LGTM!The implementation correctly:
- Derives
OrderbookIdentifierfrom the vault's metadata.- Fetches balance changes using the vault's key fields.
- Converts each result through
try_from_local_dbwith proper error propagation.The unused
_pageparameter aligns with the documented pattern where LocalDb returns all results without pagination. Based on learnings, this is intentional behavior.crates/common/src/raindex_client/vaults.rs (5)
55-80: Trait surface for vault data sources looks coherentThe added
balance_changes_listandtokens_listmethods round out the data‑source abstraction and match how you’re now using local vs subgraph sources elsewhere; no issues from an API or object‑safety perspective.
343-359: Verify local‑DB → subgraph fallback semantics for paged balance historyThe new
get_balance_changesfirst consults local DB and, if the page is empty, falls back to subgraph for that same page. This is probably what you want for partially indexed chains, but forpage > 1it can mix sources across pages (local for earlier pages, subgraph for later ones) or re‑serve already indexed events if local DB has gaps.If the UI never passes
page > 1yet, this is fine; otherwise it might be safer to only fall back to subgraph whenpageisNone/Some(1)or when the entire vault has no local data at all.
1237-1286: Local vs subgraph token fetching andchainIdshandlingThe split of
chain_idsintolocal_idsandsg_ids, plus the “if no local DB, push local‑eligible IDs back intosg_ids” path, is clean and matches the existingget_vaultspattern.One behavioral detail to confirm: when
chain_idsisNoneor an emptyChainIds,get_all_vault_tokensnow goes through subgraph only and never consults local DB. If the intent is “local DB is only used when the caller explicitly targets local‑supported networks”, this is consistent; if the expectation is “local DB should always be preferred when available, even for the global view”, you may want to mirror the partitioning logic for theNone/empty cases as well. Based on learnings aboutget_multi_subgraph_argstreatingNoneand empty lists as “all networks”.
1391-1442: Subgraph implementations for balance changes and tokens look sound; check pagination assumptionsBoth
balance_changes_listandtokens_listforSubgraphVaultscorrectly:
- use the orderbook‑scoped subgraph client for the vault path,
- map subgraph results back into
RaindexVaultBalanceChange/RaindexVaultToken,- and recover the
chain_idfrommulti_subgraph_argsin the same way aslist.Only thing to double‑check is the
SgPaginationArgsyou pass intovault_balance_changes_list(page defaulting to 1,page_size: 1000): make sure this lines up with the special pagination behavior ofvault_balance_changes_listin the subgraph client and the existing tests that assert onfirst/skipvalues in the GraphQL body. Based on learnings about that method’s hard‑coded pagination behavior.
1617-1628: Local DB token mapping is straightforward and preserves required metadata
RaindexVaultToken::from_local_db_tokencorrectly carries overchain_id, address, name, symbol, and decimals fromLocalDbToken, and uses the address string as the tokenid, which avoids depending on any subgraph‑specific ID scheme for local records. This looks good and should compose cleanly with the mixed local/subgraph token list.crates/common/src/local_db/query/fetch_all_tokens/mod.rs (2)
7-22: Token DTO and argument struct are minimal and appropriate
LocalDbTokenandFetchAllTokensArgshave the right fields and derivations for both query execution and testability; exposingchain_idsandorderbook_addressesas plainVecs keeps the builder simple and flexible.
30-59: SQL builder correctly handles optional filters and parameter binding
build_fetch_all_tokens_stmtnicely mirrors the pattern used by other local‑DB queries: it conditionally bindsCHAIN_IDS_CLAUSE/ORDERBOOKS_CLAUSEusingbind_list_clausewhen filters are non‑empty and callsclear_clauseotherwise. The use ofiter().cloned().map(SqlValue::from)for bothu32andAddressarguments looks correct for building the parameter list.
Comment on lines
+61
to
+95
| #[cfg(test)] | ||
| mod tests { | ||
| use super::*; | ||
|
|
||
| #[test] | ||
| fn builds_stmt_with_no_filters() { | ||
| let args = FetchAllTokensArgs::default(); | ||
| let stmt = build_fetch_all_tokens_stmt(&args).expect("should build"); | ||
| assert!(!stmt.sql.contains(CHAIN_IDS_CLAUSE)); | ||
| assert!(!stmt.sql.contains(ORDERBOOKS_CLAUSE)); | ||
| assert!(stmt.params.is_empty()); | ||
| } | ||
|
|
||
| #[test] | ||
| fn builds_stmt_with_chain_ids() { | ||
| let args = FetchAllTokensArgs { | ||
| chain_ids: vec![1, 137], | ||
| orderbook_addresses: vec![], | ||
| }; | ||
| let stmt = build_fetch_all_tokens_stmt(&args).expect("should build"); | ||
| assert!(stmt.sql.contains("chain_id IN")); | ||
| assert_eq!(stmt.params.len(), 2); | ||
| } | ||
|
|
||
| #[test] | ||
| fn builds_stmt_with_orderbook_addresses() { | ||
| let args = FetchAllTokensArgs { | ||
| chain_ids: vec![], | ||
| orderbook_addresses: vec![Address::from([0xab; 20])], | ||
| }; | ||
| let stmt = build_fetch_all_tokens_stmt(&args).expect("should build"); | ||
| assert!(stmt.sql.contains("orderbook_address IN")); | ||
| assert_eq!(stmt.params.len(), 1); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Prefer using sql() / params() accessors in tests instead of fields
The tests currently access stmt.sql and stmt.params directly. If SqlStatement is intended to encapsulate its internals (and given there are sql() / params() helpers already), it would be more robust to use those methods here:
- assert!(!stmt.sql.contains(CHAIN_IDS_CLAUSE));
- assert!(!stmt.sql.contains(ORDERBOOKS_CLAUSE));
- assert!(stmt.params.is_empty());
+ assert!(!stmt.sql().contains(CHAIN_IDS_CLAUSE));
+ assert!(!stmt.sql().contains(ORDERBOOKS_CLAUSE));
+ assert!(stmt.params().is_empty());
@@
- assert!(stmt.sql.contains("chain_id IN"));
- assert_eq!(stmt.params.len(), 2);
+ assert!(stmt.sql().contains("chain_id IN"));
+ assert_eq!(stmt.params().len(), 2);
@@
- assert!(stmt.sql.contains("orderbook_address IN"));
- assert_eq!(stmt.params.len(), 1);
+ assert!(stmt.sql().contains("orderbook_address IN"));
+ assert_eq!(stmt.params().len(), 1);This keeps the tests aligned with the public API even if the internal layout of SqlStatement changes.
🤖 Prompt for AI Agents
In crates/common/src/local_db/query/fetch_all_tokens/mod.rs around lines 61 to
95, the unit tests access SqlStatement internals via stmt.sql and stmt.params;
switch those to the public accessors stmt.sql() and stmt.params() (e.g.,
assert!(!stmt.sql().contains(...)), assert!(stmt.params().is_empty()),
assert_eq!(stmt.params().len(), N)) so the tests use the public API and remain
stable if the struct fields change.
Comment on lines
+133
to
+169
| async fn tokens_list( | ||
| &self, | ||
| chain_ids: Option<Vec<u32>>, | ||
| ) -> Result<Vec<RaindexVaultToken>, RaindexError> { | ||
| let Some(chain_ids) = chain_ids else { | ||
| return Ok(Vec::new()); | ||
| }; | ||
|
|
||
| if chain_ids.is_empty() { | ||
| return Ok(Vec::new()); | ||
| } | ||
|
|
||
| let orderbook_addresses = chain_ids | ||
| .iter() | ||
| .map(|&chain_id| self.client.get_orderbooks_by_chain_id(chain_id)) | ||
| .collect::<Result<Vec<_>, _>>()? | ||
| .into_iter() | ||
| .flatten() | ||
| .map(|cfg| cfg.address) | ||
| .collect::<Vec<_>>(); | ||
|
|
||
| if orderbook_addresses.is_empty() { | ||
| return Ok(Vec::new()); | ||
| } | ||
|
|
||
| let fetch_args = FetchAllTokensArgs { | ||
| chain_ids, | ||
| orderbook_addresses, | ||
| }; | ||
|
|
||
| let local_tokens = fetch_all_tokens(self.db, fetch_args).await?; | ||
|
|
||
| Ok(local_tokens | ||
| .into_iter() | ||
| .map(RaindexVaultToken::from_local_db_token) | ||
| .collect()) | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider extracting shared orderbook address collection logic.
The implementation is correct, but lines 145-156 duplicate the orderbook address collection pattern from lines 68-79 in the list() method. Consider extracting this to a helper method to reduce duplication.
impl<'a> LocalDbVaults<'a> {
pub(crate) fn new(db: &'a LocalDb, client: Rc<RaindexClient>) -> Self {
Self { db, client }
}
+ fn collect_orderbook_addresses(&self, chain_ids: &[u32]) -> Result<Vec<alloy::primitives::Address>, RaindexError> {
+ let addresses = chain_ids
+ .iter()
+ .map(|&chain_id| self.client.get_orderbooks_by_chain_id(chain_id))
+ .collect::<Result<Vec<_>, _>>()?
+ .into_iter()
+ .flatten()
+ .map(|cfg| cfg.address)
+ .collect();
+ Ok(addresses)
+ }
+
fn convert_local_db_vaults(Then both list() and tokens_list() can call self.collect_orderbook_addresses(&chain_ids)?.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In crates/common/src/raindex_client/local_db/vaults.rs around lines 133 to 169,
the logic that collects orderbook addresses from chain_ids is duplicated (lines
~145-156 and earlier in list()); extract that block into a private helper on the
impl, e.g. fn collect_orderbook_addresses(&self, chain_ids: &Vec<u32>) ->
Result<Vec<AddressType>, RaindexError> that calls
self.client.get_orderbooks_by_chain_id for each chain_id, flattens the results
and maps to cfg.address, returning Ok(vec) or Err(...). Replace the duplicated
code in both list() and tokens_list() with let orderbook_addresses =
self.collect_orderbook_addresses(&chain_ids)?; and keep the existing
early-return when the returned Vec is empty.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Chained PR
Motivation
After implementing the proper data source logic for vaults, it's now time to implement it for vaults.
Solution
Checks
By submitting this for review, I'm confirming I've done the following:
Summary by CodeRabbit
New Features
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.