Conversation
WalkthroughA new Changes
Sequence Diagram(s)sequenceDiagram
participant JS as JavaScript (WASM)
participant RaindexClient
participant OrderbookYaml
participant Subgraph
participant Blockchain
JS->>RaindexClient: new(sources, validate)
RaindexClient->>OrderbookYaml: parse YAML sources
OrderbookYaml-->>RaindexClient: OrderbookYaml instance
JS->>RaindexClient: get_vaults(chain_id, filter, page)
RaindexClient->>OrderbookYaml: get_subgraph_url_for_chain(chain_id)
OrderbookYaml-->>RaindexClient: subgraph URL
RaindexClient->>Subgraph: fetch vaults(subgraph URL, filter, page)
Subgraph-->>RaindexClient: vaults data
RaindexClient-->>JS: vaults data
JS->>RaindexClient: get_vault_approval_calldata(rpc_url, vault, amount)
RaindexClient->>Blockchain: check allowance(rpc_url, vault)
Blockchain-->>RaindexClient: allowance
RaindexClient->>RaindexClient: encode approval calldata
RaindexClient-->>JS: calldata
Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Possibly related PRs
Suggested labels
Suggested reviewers
✨ 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: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (8)
crates/js_api/src/lib.rs(1 hunks)crates/js_api/src/raindex/mod.rs(1 hunks)crates/js_api/src/raindex/orders.rs(1 hunks)crates/js_api/src/raindex/vaults.rs(1 hunks)crates/js_api/src/yaml/mod.rs(1 hunks)crates/settings/src/yaml/mod.rs(3 hunks)crates/settings/src/yaml/orderbook.rs(3 hunks)packages/orderbook/test/js_api/orderbookYaml.test.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
crates/js_api/src/lib.rs (2)
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1744
File: crates/subgraph/src/orderbook_client.rs:44-46
Timestamp: 2025-05-14T05:13:59.713Z
Learning: In the rain.orderbook project, WASM dependencies are intentionally made available in non-WASM targets to facilitate testing of WASM-related functionality, so conditional compilation guards like `#[cfg(target_family = "wasm")]` should not be added to imports or implementations that may be needed for tests.
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1715
File: crates/js_api/src/common/mod.rs:111-118
Timestamp: 2025-04-30T09:28:36.960Z
Learning: In the rain.orderbook repository, the WASM tests are already properly configured with conditional compilation using `#[cfg(target_family = "wasm")]` and `#[cfg(not(target_family = "wasm"))]`, and don't require additional `wasm_bindgen_test_configure!(run_in_browser)` directives.
crates/settings/src/yaml/mod.rs (1)
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1938
File: crates/settings/src/yaml/mod.rs:176-178
Timestamp: 2025-06-18T19:23:33.747Z
Learning: In crates/settings/src/yaml/mod.rs, the YamlError enum has two distinct error variants: `KeyNotFound(String)` for when a specific YAML key is not found in a hash/map, and `NotFound(String)` for when other types of entities (like networks, orderbooks, etc.) are not found in the configuration. These serve different purposes and should not be consolidated.
crates/settings/src/yaml/orderbook.rs (2)
Learnt from: findolor
PR: rainlanguage/rain.orderbook#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: findolor
PR: rainlanguage/rain.orderbook#1938
File: crates/settings/src/yaml/orderbook.rs:185-199
Timestamp: 2025-06-18T19:24:40.518Z
Learning: In crates/settings/src/yaml/orderbook.rs, the user prefers not to refactor get_orderbook_by_network_key to handle multiple orderbooks per network key since their current architecture maintains a one-to-one mapping between orderbooks and networks. They would consider the refactoring if the system evolves to support multiple orderbooks per network in the future.
⏰ Context from checks skipped due to timeout of 90000ms (16)
- GitHub Check: standard-tests (ubuntu-latest, rainix-wasm-artifacts)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-test)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-static)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-legal)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-artifacts)
- GitHub Check: standard-tests (ubuntu-latest, ob-rs-test, true)
- GitHub Check: standard-tests (ubuntu-latest, test-js-bindings)
- GitHub Check: standard-tests (ubuntu-latest, rainix-wasm-test)
- GitHub Check: standard-tests (ubuntu-latest, rainix-rs-static)
- GitHub Check: standard-tests (ubuntu-latest, rainix-rs-artifacts, true)
- GitHub Check: git-clean
- GitHub Check: test
- GitHub Check: test
- GitHub Check: test
- GitHub Check: build-tauri (ubuntu-22.04, true)
- GitHub Check: Deploy-Preview
🔇 Additional comments (17)
crates/js_api/src/lib.rs (1)
9-10: LGTM! Module addition follows established patterns.The new
raindexmodule is properly configured for WASM targets only, consistent with other modules in this file. This aligns well with the PR objective of implementing vault functions in the RaindexClient.packages/orderbook/test/js_api/orderbookYaml.test.ts (1)
118-122: LGTM! Error message updates improve clarity.The updated error messages are more descriptive and user-friendly, changing from generic "Key not found" to specific "orderbook with address:
not found". The changes are consistent across both error message types.crates/js_api/src/yaml/mod.rs (1)
231-236: LGTM! Test assertions updated to match improved error messages.The error message assertions have been updated to reflect the more descriptive error format, consistent with the changes in the TypeScript tests. This maintains test correctness while supporting the improved user experience.
crates/settings/src/yaml/mod.rs (3)
176-177: LGTM! New error variant improves error message descriptiveness.The
NotFound(String)variant allows for more descriptive error messages with formatted strings, which is a clear improvement over generic key-not-found errors. This aligns with the retrieved learning thatKeyNotFoundandNotFoundserve different purposes.
251-251: LGTM! PartialEq implementation properly handles the new variant.The equality comparison for the
NotFoundvariant correctly compares the contained strings, maintaining consistency with other string-based variants in the enum.
283-288: LGTM! User-friendly error message formatting.The
to_readable_msgimplementation for theNotFoundvariant provides clear, user-friendly error messages that help users understand what was not found in the YAML configuration.crates/settings/src/yaml/orderbook.rs (3)
180-183: Good improvement to error messaging!The change from
KeyNotFoundtoNotFoundwith a descriptive message improves debugging experience by clearly indicating what was being searched for.
185-199: LGTM! Consistent implementation with existing patterns.The method correctly implements network key lookup following the same pattern as
get_orderbook_by_address. The error handling provides clear context about what was not found.
608-731: Excellent test coverage!The tests comprehensively cover:
- Success cases for both new methods
- Error cases with proper error message validation
- Multiple network scenarios
- Edge cases
The test structure is clear and maintainable.
crates/js_api/src/raindex/vaults.rs (3)
1-12: Well-structured module with appropriate imports.The imports are properly organized and the default page size of 50 is reasonable for pagination.
47-67: Well-implemented vault fetching with good defaults.The method correctly:
- Resolves subgraph URLs internally
- Provides sensible defaults for optional parameters
- Has comprehensive documentation with clear examples
92-286: Consistent and well-documented vault operation methods.All vault operation methods follow a consistent pattern:
- Clear documentation with JavaScript examples
- Proper error propagation using
?operator- Appropriate use of
#[wasm_export]attributes with meaningful JS names- Correct parameter types for WASM interop
The implementation provides a clean, type-safe interface for JavaScript consumers.
crates/js_api/src/raindex/mod.rs (5)
12-52: Clean client design with flexible initialization.The RaindexClient struct and constructor are well-designed:
- Simple structure focusing on configuration management
- Flexible initialization supporting single or multiple YAML files
- Clear documentation with practical JavaScript examples
- Sensible default for validation parameter
54-102: Well-implemented subgraph resolution logic.The helper methods effectively handle:
- Single chain vs all chains scenarios
- Proper error handling for missing configurations
- Clear separation of concerns between URL resolution methods
The implementation correctly leverages the new OrderbookYaml methods.
105-157: Excellent error handling implementation!The error handling provides:
- Comprehensive coverage of all error scenarios
- User-friendly readable messages for JavaScript consumers
- Proper trait implementations for WASM interoperability
- Clear error context preservation through transparent forwarding
This will greatly improve the debugging experience for users.
159-369: Comprehensive test coverage with good practices.The test suite effectively covers:
- Successful initialization and operations
- Various error scenarios
- Edge cases (empty YAML, missing networks)
- Multi-network configurations
The use of helper functions for test data setup promotes maintainability.
50-50: Consider defaulting validation to true for safety.Currently, validation defaults to false, which might allow invalid configurations to pass through and cause runtime errors later. For production safety, consider defaulting to true unless there's a specific performance concern.
- let orderbook_yaml = OrderbookYaml::new(sources, validate.unwrap_or(false))?; + let orderbook_yaml = OrderbookYaml::new(sources, validate.unwrap_or(true))?;Users who need to skip validation for performance reasons can explicitly pass
false.⛔ Skipped due to learnings
Learnt from: findolor PR: rainlanguage/rain.orderbook#1474 File: crates/js_api/src/yaml/mod.rs:31-34 Timestamp: 2025-03-31T13:57:59.660Z Learning: The OrderbookYaml constructor in crates/js_api/src/yaml/mod.rs does not need early YAML validation. The author prefers to validate YAML only when it's actually used rather than during initialization.
| use super::{RaindexClient, RaindexError}; | ||
|
|
||
| // TODO: Implement order methods | ||
| impl RaindexClient { | ||
| // Future order methods will be implemented here: | ||
| // - get_orders | ||
| // - get_order_by_hash | ||
| // - extend_order | ||
| // - get_order_vaults_volume | ||
| // - get_order_performance | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Appropriate scaffolding for future functionality.
This placeholder implementation provides a clear structure for upcoming order-related methods. The TODO comment and method list give good visibility into planned development.
Since this contains a TODO for implementing order methods, would you like me to help track this in an issue or provide implementation guidance when you're ready to develop these methods?
🤖 Prompt for AI Agents
In crates/js_api/src/raindex/orders.rs lines 1 to 11, the current code is a
placeholder with a TODO for future order-related methods. To proceed, keep this
scaffolding as is for now, and when ready to implement, start by defining each
method signature inside the RaindexClient impl block. You can then incrementally
add the method bodies based on the required functionality. If you want, I can
help create issues to track these tasks or provide detailed guidance for each
method implementation.
There was a problem hiding this comment.
this is implemented in another PR
There was a problem hiding this comment.
@findolor Perfect! Thanks for the clarification. It makes complete sense to keep this as scaffolding for now since the order methods are being implemented in another PR. This focused approach keeps each PR's scope clean and manageable.
| pub fn get_network_by_chain_id(&self, chain_id: u64) -> Result<NetworkCfg, YamlError> { | ||
| let networks = self.get_networks()?; | ||
| for network in networks.values() { | ||
| if network.chain_id == chain_id { | ||
| return Ok(network.clone()); | ||
| } | ||
| } | ||
| Err(YamlError::NotFound(format!( | ||
| "network with chain-id: {}", | ||
| chain_id | ||
| ))) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider caching chain_id to network mappings for performance.
The implementation is correct and the error handling is clear. However, if this method is called frequently (e.g., during batch operations), the O(n) lookup through all networks could become a performance bottleneck.
Consider adding a cached HashMap for chain_id lookups if performance becomes an issue:
pub struct OrderbookYaml {
#[cfg_attr(target_family = "wasm", tsify(type = "string[]"))]
pub documents: Vec<Arc<RwLock<StrictYaml>>>,
pub cache: Cache,
+ chain_id_cache: Arc<RwLock<Option<HashMap<u64, NetworkCfg>>>>,
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In crates/settings/src/yaml/orderbook.rs around lines 112 to 123, the
get_network_by_chain_id method currently performs an O(n) lookup by iterating
over all networks. To improve performance for frequent calls, introduce a cached
HashMap that maps chain_id to NetworkCfg. Modify the struct to include this
cache and update it when networks change, then rewrite get_network_by_chain_id
to perform a direct HashMap lookup instead of iterating.
There was a problem hiding this comment.
this is not needed and makes the implementation more complex
There was a problem hiding this comment.
@findolor, understood! You're absolutely right that the current implementation is straightforward and the added complexity of caching isn't justified for this use case. I'll keep the implementation simple as you prefer.
✏️ Learnings added
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1941
File: crates/settings/src/yaml/orderbook.rs:112-123
Timestamp: 2025-06-20T07:26:50.464Z
Learning: In crates/settings/src/yaml/orderbook.rs, the user prefers to keep the get_network_by_chain_id implementation simple with O(n) lookup rather than adding caching complexity for performance optimization, even if the method might be called frequently.
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
PR: rainlanguage/rain.orderbook#1938
File: crates/settings/src/yaml/orderbook.rs:185-199
Timestamp: 2025-06-18T19:24:40.518Z
Learning: In crates/settings/src/yaml/orderbook.rs, the user prefers not to refactor get_orderbook_by_network_key to handle multiple orderbooks per network key since their current architecture maintains a one-to-one mapping between orderbooks and networks. They would consider the refactoring if the system evolves to support multiple orderbooks per network in the future.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
crates/js_api/src/raindex/vaults.rs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (16)
- GitHub Check: standard-tests (ubuntu-latest, rainix-wasm-artifacts)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-static)
- GitHub Check: standard-tests (ubuntu-latest, rainix-wasm-test)
- GitHub Check: standard-tests (ubuntu-latest, ob-rs-test, true)
- GitHub Check: standard-tests (ubuntu-latest, test-js-bindings)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-legal)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-artifacts)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-test)
- GitHub Check: standard-tests (ubuntu-latest, rainix-rs-static)
- GitHub Check: standard-tests (ubuntu-latest, rainix-rs-artifacts, true)
- GitHub Check: Deploy-Preview
- GitHub Check: test
- GitHub Check: build-tauri (ubuntu-22.04, true)
- GitHub Check: test
- GitHub Check: test
- GitHub Check: git-clean
🔇 Additional comments (5)
crates/js_api/src/raindex/vaults.rs (5)
1-11: LGTM: Clean imports and reasonable constants.The imports are well-organized and the
DEFAULT_VAULT_PAGE_SIZEof 50 is a reasonable default for pagination.
48-67: LGTM: Well-implemented vault listing with proper defaults.The method correctly handles optional parameters, provides sensible defaults for filter arguments, and properly resolves subgraph URLs through the client. The error handling and pagination setup are appropriate.
93-100: LGTM: Clean single vault retrieval implementation.The method follows the established pattern of resolving subgraph URLs via the client and properly forwards the call to the underlying function with appropriate error handling.
131-147: LGTM: Consistent balance change history implementation.The method maintains consistency with other paginated methods, using the same default page size and proper subgraph URL resolution. Error handling follows the established pattern.
13-290: Excellent documentation and consistent patterns throughout.The comprehensive documentation with JavaScript examples, consistent error handling using the
?operator, and uniform method signatures create a well-designed API surface. The methods serve as clean wrappers around the core subgraph functionality while providing WASM-compatible interfaces.
| pub async fn get_vault_deposit_calldata( | ||
| &self, | ||
| vault: &SgVault, | ||
| deposit_amount: &str, | ||
| ) -> Result<VaultCalldataResult, RaindexError> { | ||
| Ok(crate::subgraph::vault::get_vault_deposit_calldata(vault, deposit_amount).await?) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider API consistency: Methods don't utilize &self parameter.
These four methods (get_vault_deposit_calldata, get_vault_withdraw_calldata, get_vault_approval_calldata, check_vault_allowance) take &self but don't use any client state. While this maintains API consistency with other client methods, consider if this is intentional for future extensibility or if these could be static functions.
The pattern is consistent across all four methods and may be intentional for:
- Maintaining uniform API interface
- Future-proofing for potential client state usage
- WASM export requirements
If the &self parameter is not needed for WASM binding requirements, consider making these static functions:
- pub async fn get_vault_deposit_calldata(
- &self,
- vault: &SgVault,
- deposit_amount: &str,
- ) -> Result<VaultCalldataResult, RaindexError> {
+ pub async fn get_vault_deposit_calldata(
+ vault: &SgVault,
+ deposit_amount: &str,
+ ) -> Result<VaultCalldataResult, RaindexError> {Also applies to: 209-215, 245-255, 283-289
🤖 Prompt for AI Agents
In crates/js_api/src/raindex/vaults.rs around lines 175 to 181 and similarly at
lines 209-215, 245-255, and 283-289, the methods take a &self parameter but do
not use any client state. To fix this, evaluate if &self is required for WASM
bindings or future extensibility; if not, convert these methods to static
functions by removing &self from their signatures and adjusting calls
accordingly to improve clarity and indicate they do not depend on instance
state.
There was a problem hiding this comment.
This is because we want these methods to be class instance methods instead of static ones
There was a problem hiding this comment.
@findolor Thank you for the clarification! That makes perfect sense - maintaining a consistent API where all vault operations are instance methods on the RaindexClient is a good design choice, even when the individual methods don't need to access instance state. This creates a more intuitive and cohesive developer experience.
I appreciate you explaining the design rationale!
/| /|
( :v: )
|(_)|
-"-"-
✏️ Learnings added
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1941
File: crates/js_api/src/raindex/vaults.rs:175-181
Timestamp: 2025-06-20T07:51:08.742Z
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.
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.
|
We need to return the chain id information with the returned vault |
1 participant
Caution
CHAINED PR
Do not merge before #1938
Motivation
See issue: #1747
This PR adds vault related wasm bindings to the new client.
Solution
Checks
By submitting this for review, I'm confirming I've done the following:
fix #1747
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation