Add vault related data structures and functions #1950
Conversation
WalkthroughThis change introduces a comprehensive vault management system to the RaindexClient module, adding new data structures, methods, and error handling for vaults, tokens, balance changes, and allowances. It migrates and extends vault-related functionality, updates tests for dynamic configuration, and provides new TypeScript bindings and integration tests for the JavaScript API. Changes
Sequence Diagram(s)sequenceDiagram
participant JS as JS API
participant WASM as WASM Bindings
participant Client as RaindexClient
participant Subgraph as Subgraph
participant RPC as RPC Endpoint
JS->>WASM: Call get_vaults(chain_id, filters, page)
WASM->>Client: get_vaults(chain_id, filters, page)
Client->>Subgraph: Query vaults with filters
Subgraph-->>Client: Return vault data
Client->>WASM: Return vault list
WASM->>JS: Return vault list
JS->>WASM: Call get_deposit_calldata(vault, amount)
WASM->>Client: get_deposit_calldata(vault, amount)
Client->>Client: Validate amount
Client->>Client: Generate calldata
Client->>WASM: Return calldata
WASM->>JS: Return calldata
JS->>WASM: Call get_allowance(vault)
WASM->>Client: get_allowance(vault)
Client->>RPC: Query ERC20 allowance
RPC-->>Client: Return allowance value
Client->>WASM: Return allowance
WASM->>JS: Return allowance
Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Suggested labels
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
npm error Exit handler never called! ✨ 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: 7
♻️ Duplicate comments (1)
crates/common/src/raindex_client/vaults.rs (1)
547-547: Arc instance creation pattern.Same issue as in orders.rs - creating new Arc<RwLock instances.
Also applies to: 622-622
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.locktauri-app/src-tauri/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (26)
crates/common/Cargo.toml(1 hunks)crates/common/src/add_order.rs(1 hunks)crates/common/src/deposit.rs(1 hunks)crates/common/src/dotrain_add_order_lsp.rs(1 hunks)crates/common/src/dotrain_order/mod.rs(1 hunks)crates/common/src/erc20.rs(1 hunks)crates/common/src/lib.rs(1 hunks)crates/common/src/meta.rs(5 hunks)crates/common/src/raindex_client/mod.rs(1 hunks)crates/common/src/raindex_client/orders.rs(1 hunks)crates/common/src/raindex_client/transactions.rs(1 hunks)crates/common/src/raindex_client/vaults.rs(1 hunks)crates/common/src/remove_order.rs(1 hunks)crates/common/src/transaction.rs(1 hunks)crates/common/src/types/order_detail_extended.rs(1 hunks)crates/common/src/withdraw.rs(1 hunks)crates/js_api/src/lib.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)crates/subgraph/src/performance/vol.rs(1 hunks)flake.nix(1 hunks)packages/orderbook/test/js_api/order.test.ts(1 hunks)packages/orderbook/test/js_api/orderbookYaml.test.ts(1 hunks)packages/orderbook/test/js_api/raindexClient.test.ts(1 hunks)packages/orderbook/test/js_api/vault.test.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (14)
crates/common/src/erc20.rs (1)
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.
crates/common/src/dotrain_add_order_lsp.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/common/src/withdraw.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/common/src/remove_order.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/common/src/transaction.rs (1)
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.
crates/common/src/dotrain_order/mod.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/common/src/deposit.rs (1)
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.
flake.nix (1)
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.
packages/orderbook/test/js_api/order.test.ts (1)
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1907
File: packages/orderbook/test/common/test.test.ts:75-77
Timestamp: 2025-06-04T10:21:01.388Z
Learning: The DotrainOrder.create API in packages/orderbook/test/common/test.test.ts is internal and not used directly in consumer applications, so API changes here don't require external breaking change documentation.
crates/common/src/add_order.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/js_api/src/lib.rs (3)
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1710
File: crates/quote/Cargo.toml:30-30
Timestamp: 2025-05-14T04:49:14.621Z
Learning: In the rain.orderbook repository, wasm-bindgen-utils is intentionally used as an unconditional dependency because non-wasm targets (like tests) sometimes need to use it.
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.
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.
crates/common/Cargo.toml (3)
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.
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1710
File: crates/quote/Cargo.toml:30-30
Timestamp: 2025-05-14T04:49:14.621Z
Learning: In the rain.orderbook repository, wasm-bindgen-utils is intentionally used as an unconditional dependency because non-wasm targets (like tests) sometimes need to use it.
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.
🪛 Biome (1.9.4)
packages/orderbook/test/js_api/raindexClient.test.ts
[error] 1-1: A Node.js builtin module should be imported with the node: protocol.
Using the node: protocol is more explicit and signals that the imported module belongs to Node.js.
Unsafe fix: Add the node: protocol.
(lint/style/useNodejsImportProtocol)
⏰ Context from checks skipped due to timeout of 90000ms (14)
- GitHub Check: standard-tests (ubuntu-latest, test-js-bindings)
- GitHub Check: standard-tests (ubuntu-latest, ob-rs-test, true)
- GitHub Check: Deploy-Preview
- GitHub Check: standard-tests (ubuntu-latest, rainix-wasm-test)
- 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-rs-static)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-legal)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-test)
- GitHub Check: standard-tests (ubuntu-latest, rainix-wasm-artifacts)
- GitHub Check: standard-tests (ubuntu-latest, rainix-rs-artifacts, true)
- GitHub Check: test
- GitHub Check: git-clean
- GitHub Check: test
🔇 Additional comments (35)
crates/settings/src/yaml/mod.rs (3)
176-178: LGTM! Well-implemented error variant addition.The new
NotFoundvariant follows the established pattern and maintains the important distinction fromKeyNotFoundas noted in the learning context.
251-251: Proper PartialEq implementation for the new variant.The equality comparison for
NotFoundis implemented consistently with other string-based variants.
283-288: Clear and user-friendly error message formatting.The error message format is helpful and follows the established pattern of providing context-rich feedback to users.
packages/orderbook/test/js_api/vault.test.ts (1)
12-12: Good cleanup of unused imports.Removing
SgVaultWithSubgraphNameandSgDepositreduces unnecessary dependencies while maintaining test functionality.crates/common/src/withdraw.rs (1)
58-58: Proper test environment separation for WASM builds.This change correctly restricts test compilation to non-WASM targets, aligning with the project's strategy for separating native and WASM test environments.
crates/common/src/lib.rs (1)
10-10: Proper addition of the new raindex_client module.The module is correctly exposed in the public API to provide the vault and orderbook querying functionality.
packages/orderbook/test/js_api/order.test.ts (1)
13-13: Good cleanup of unused imports.Removing
SgOrderWithSubgraphNameandOrderWithSortedVaultsstreamlines the imports while maintaining test functionality.crates/common/src/remove_order.rs (1)
78-78: LGTM! Test compilation properly restricted to non-WASM targets.This change aligns with the coordinated effort to separate test environments by target platform, ensuring WASM builds exclude tests that depend on non-WASM specific functionality.
crates/common/src/dotrain_add_order_lsp.rs (1)
123-123: LGTM! Consistent test compilation restriction applied.This change matches the pattern across the
crates/commondirectory, properly excluding WASM targets from test compilation.crates/common/src/dotrain_order/mod.rs (1)
628-628: LGTM! Test environment segregation properly implemented.The conditional compilation change correctly restricts test compilation to non-WASM targets, maintaining consistency with the broader refactoring effort.
crates/common/src/deposit.rs (1)
148-148: LGTM! Test compilation correctly configured for target separation.This change maintains the established pattern of excluding WASM targets from test compilation, ensuring compatibility with the project's test infrastructure.
crates/common/src/transaction.rs (1)
103-103: LGTM! Test compilation configuration completed consistently.This final change completes the coordinated effort to properly segregate test environments across the
crates/commondirectory, ensuring WASM builds exclude incompatible test dependencies.crates/common/src/erc20.rs (1)
201-201: LGTM: Appropriate platform-specific test scoping.The conditional compilation change correctly restricts these tests to non-WASM targets, which aligns with the broader strategy of separating test environments for different platforms while maintaining WASM-compatible functionality in the main codebase.
crates/common/src/add_order.rs (1)
363-363: LGTM: Consistent platform-specific test configuration.This change maintains consistency with other files in the
crates/common/srcdirectory by properly scoping tests to non-WASM targets, supporting the overall strategy for WASM-compatible development.packages/orderbook/test/js_api/orderbookYaml.test.ts (1)
118-122: LGTM: Improved error messaging for better UX.The updated test assertions correctly reflect the improved error message format that provides more descriptive feedback ("orderbook with address: ... not found" vs generic key errors). This enhances the user experience by making error messages more actionable and context-specific.
crates/js_api/src/yaml/mod.rs (1)
231-236: LGTM: Consistent error message improvements across the stack.The test assertions correctly reflect the enhanced error messaging that provides descriptive context ("orderbook with address: ... not found") instead of generic key errors. This maintains consistency between the Rust backend and JavaScript frontend error handling.
flake.nix (1)
223-223: LGTM: Appropriate inclusion of enhanced crate in WASM test suite.Adding
rain_orderbook_commonto the WASM test pipeline is correct given the new WASM-compatible functionality (likeraindex_client) introduced in this crate. This ensures the WASM bindings and client features are properly tested in the WebAssembly environment.crates/common/src/types/order_detail_extended.rs (1)
25-25: LGTM! Metadata decoding updated correctly.The change correctly adapts to the trait implementation shift from
SgRainMetaV1toStringby accessing the inner valuemeta.0before callingtry_decode_rainlangsource().crates/js_api/src/lib.rs (1)
24-30: TypeScript types look good for blockchain context.The
AddressandHextemplate literal types with "0x" prefix are appropriate for blockchain applications. Both types are currently identical - consider if more specific typing (e.g., length constraints for addresses) would be beneficial in the future.crates/common/Cargo.toml (1)
51-55: Excellent dependency scoping for target platforms.The separation of WASM-specific (
wasm-bindgen-test) and non-WASM-specific (httpmock,rain_orderbook_test_fixtures) dev dependencies ensures clean builds and appropriate test tooling for each target platform.crates/common/src/meta.rs (2)
24-27: Clean trait refactoring to target String directly.Moving the
TryDecodeRainlangSourceimplementation fromSgRainMetaV1toStringsimplifies the interface and makes the trait more reusable. The updated decode call correctly operates on the string directly.
73-73: Test updates correctly handle wrapper type.All tests properly extract the inner string (
meta.0) before callingtry_decode_rainlangsource(), maintaining compatibility with the trait implementation change.Also applies to: 80-80, 90-90, 100-100
crates/common/src/raindex_client/transactions.rs (2)
10-17: Well-structured transaction representation.The struct design with proper types (
Address,U256) and WASM bindings is clean and appropriate for blockchain transaction data.
52-62: Robust conversion implementation with proper error handling.The
TryFrom<SgTransaction>implementation correctly handles string parsing with appropriate error propagation for address and numeric conversions.crates/subgraph/src/performance/vol.rs (1)
25-32: LGTM! Simplified WASM trait derivation aligns with project-wide WASM support.The unconditional
Tsifyderivation andimpl_wasm_traits!macro usage is consistent with the crate's design where WASM dependencies are always available (as evidenced by the unconditional import ofwasm_bindgen_utilsat line 9).crates/settings/src/yaml/orderbook.rs (2)
112-123: LGTM! Network lookup by chain ID is implemented correctly.The method efficiently searches through all networks and returns a descriptive error message when not found.
185-199: LGTM! Orderbook lookup by network key follows the established pattern.The implementation is consistent with existing lookup methods. As per your previous feedback, I understand you prefer to avoid refactoring the duplicate search logic while there are only a few lookup methods.
packages/orderbook/test/js_api/raindexClient.test.ts (2)
85-94: LGTM! Well-designed error handling helper.The
extractWasmEncodedDatafunction provides clean error handling and logging for WASM results throughout the tests.
1-1: Use the node: protocol for Node.js builtin imports.This makes it explicit that the imported module is a Node.js builtin.
-import assert from 'assert'; +import assert from 'node:assert';⛔ Skipped due to learnings
Learnt from: findolor PR: rainlanguage/rain.orderbook#1474 File: packages/orderbook/test/js_api/orderbookYaml.test.ts:1-1 Timestamp: 2025-03-31T13:51:32.841Z Learning: The project maintainers prefer not to use the node: protocol (e.g., 'node:assert') for Node.js built-in modules in import statements, opting instead for the direct import format (e.g., 'assert').crates/common/src/raindex_client/mod.rs (2)
51-92: LGTM! Well-documented public API with clear examples.The RaindexClient struct and constructor are properly documented with helpful JavaScript examples for WASM users.
159-197: LGTM! Comprehensive error enum with good coverage.The RaindexError enum covers all potential failure modes with appropriate error variants.
crates/common/src/raindex_client/orders.rs (2)
476-1615: Excellent test coverage!The tests are comprehensive, well-structured, and cover various scenarios including edge cases. Good use of mocking for external dependencies.
99-125: Optimize the vaults() method to reduce iterations and allocations.The current implementation iterates through the vaults multiple times and creates several intermediate collections. This can be optimized to a single pass.
Apply this diff to optimize the method:
pub fn vaults(&self) -> Vec<RaindexVault> { - let mut vaults: Vec<RaindexVault> = Vec::new(); - - let input_ids: HashSet<String> = self.inputs.iter().map(|v| v.id()).collect(); - let output_ids: HashSet<String> = self.outputs.iter().map(|v| v.id()).collect(); - - // First add inputs (excluding input_outputs) - for vault in &self.inputs { - if !output_ids.contains(&vault.id()) { - vaults.push(vault.clone()); - } - } - // Then add outputs (excluding input_outputs) - for vault in &self.outputs { - if !input_ids.contains(&vault.id()) { - vaults.push(vault.clone()); - } - } - // Finally add input_outputs (only once for vaults that are both input and output) - for vault in &self.inputs { - if output_ids.contains(&vault.id()) { - let input_output_vault = vault.with_vault_type(RaindexVaultType::InputOutput); - vaults.push(input_output_vault); - } - } - vaults + let mut vaults: Vec<RaindexVault> = Vec::new(); + let mut seen_ids: HashSet<String> = HashSet::new(); + + // Build a set of output IDs for quick lookup + let output_ids: HashSet<String> = self.outputs.iter().map(|v| v.id()).collect(); + + // Process inputs + for vault in &self.inputs { + let vault_id = vault.id(); + if output_ids.contains(&vault_id) { + // This vault is both input and output + vaults.push(vault.with_vault_type(RaindexVaultType::InputOutput)); + } else { + // This vault is input only + vaults.push(vault.clone()); + } + seen_ids.insert(vault_id); + } + + // Process outputs that haven't been seen as inputs + for vault in &self.outputs { + if !seen_ids.contains(&vault.id()) { + vaults.push(vault.clone()); + } + } + + vaults }⛔ Skipped due to learnings
Learnt from: findolor PR: rainlanguage/rain.orderbook#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.crates/common/src/raindex_client/vaults.rs (2)
358-378: Well-implemented approval logic with proper validation.The approval calldata generation correctly checks existing allowance and prevents unnecessary approvals. This is a good security practice.
709-1192: Comprehensive test coverage with good error case handling.The tests thoroughly cover all functionality including error cases, edge conditions, and integration scenarios.
| #[cfg(not(target_family = "wasm"))] | ||
| #[wasm_bindgen(getter = blockNumber)] | ||
| pub fn block_number(&self) -> String { | ||
| self.block_number.to_string() | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove wasm_bindgen attributes from non-WASM methods.
The non-WASM versions of block_number() and timestamp() methods shouldn't have #[wasm_bindgen] attributes since they're only compiled for non-WASM targets. This creates confusion and potential compilation issues.
Apply this diff to remove unnecessary attributes:
#[cfg(not(target_family = "wasm"))]
-#[wasm_bindgen(getter = blockNumber)]
pub fn block_number(&self) -> String { #[cfg(not(target_family = "wasm"))]
-#[wasm_bindgen(getter)]
pub fn timestamp(&self) -> String {Also applies to: 45-49
🤖 Prompt for AI Agents
In crates/common/src/raindex_client/transactions.rs around lines 34 to 38 and 45
to 49, remove the #[wasm_bindgen] attributes from the block_number() and
timestamp() methods that are conditionally compiled for non-WASM targets. These
attributes should only be present on WASM-targeted methods to avoid confusion
and compilation issues. Simply delete the #[wasm_bindgen(getter = ...)] lines
from the non-WASM versions of these methods.
There was a problem hiding this comment.
this gives us compiler errors if we remove the attributes
There was a problem hiding this comment.
@findolor Thank you for clarifying! You're absolutely right. When #[wasm_bindgen] is applied to an impl block, all methods within that block need the wasm_bindgen attributes, even if they're conditionally compiled for different targets. This is due to how the wasm_bindgen macro expansion works - it processes the entire impl block and expects consistent attribute usage.
I appreciate you testing this and providing the feedback. This is valuable information for understanding wasm_bindgen's requirements.
🐰✨ ears perk up with curiosity Thanks for sharing this insight!
✏️ Learnings added
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1950
File: crates/common/src/raindex_client/transactions.rs:34-38
Timestamp: 2025-06-24T13:30:02.947Z
Learning: When using #[wasm_bindgen] on an impl block, all methods within that block must have wasm_bindgen attributes, even if they are conditionally compiled for non-WASM targets using #[cfg(not(target_family = "wasm"))]. Removing these attributes causes compiler errors because the wasm_bindgen macro expansion processes the entire impl block and expects consistent attribute usage across all methods.
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 should we create an issue for this in rain.wasm?
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!
| BigInt::from_str(&self.vault_id.to_string()) | ||
| .map_err(|e| RaindexError::JsError(e.to_string().into())) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Extract BigInt conversion logic into helper methods.
The BigInt conversion with error handling is repeated throughout the file. Consider creating helper methods to reduce duplication.
Add helper methods:
#[cfg(target_family = "wasm")]
impl RaindexVault {
fn u256_to_bigint(value: &U256) -> Result<BigInt, RaindexError> {
BigInt::from_str(&value.to_string())
.map_err(|e| RaindexError::JsError(e.to_string().into()))
}
}Then use it:
#[cfg(target_family = "wasm")]
#[wasm_bindgen(getter = vaultId)]
pub fn vault_id(&self) -> Result<BigInt, RaindexError> {
- BigInt::from_str(&self.vault_id.to_string())
- .map_err(|e| RaindexError::JsError(e.to_string().into()))
+ Self::u256_to_bigint(&self.vault_id)
}Also applies to: 84-86, 139-145, 430-433, 441-444
🤖 Prompt for AI Agents
In crates/common/src/raindex_client/vaults.rs around lines 74 to 76 and also at
lines 84-86, 139-145, 430-433, and 441-444, the BigInt conversion from U256 with
error handling is duplicated. To fix this, create a helper method inside the
RaindexVault impl block (conditioned on wasm target) that takes a U256 reference
and returns a Result<BigInt, RaindexError> by converting the U256 to string and
parsing it to BigInt with proper error mapping. Then replace all repeated
conversion code with calls to this helper method to reduce duplication and
improve maintainability.
| #[cfg(not(target_family = "wasm"))] | ||
| #[wasm_bindgen(getter = blockNumber)] | ||
| pub fn block_number(&self) -> String { | ||
| self.block_number.to_string() | ||
| } |
There was a problem hiding this comment.
@findolor should we create an issue for this in rain.wasm?
| #[wasm_export(js_name = "getDepositCalldata", unchecked_return_type = "Hex")] | ||
| pub async fn get_deposit_calldata( | ||
| &self, | ||
| deposit_amount: String, |
| #[wasm_export(js_name = "getWithdrawCalldata", unchecked_return_type = "Hex")] | ||
| pub async fn get_withdraw_calldata( | ||
| &self, | ||
| withdraw_amount: String, |
| #[wasm_export(js_name = "getApprovalCalldata", unchecked_return_type = "Hex")] | ||
| pub async fn get_approval_calldata( | ||
| &self, | ||
| deposit_amount: String, |
| #[serde(rename_all = "camelCase")] | ||
| #[wasm_bindgen] | ||
| pub struct RaindexVaultBalanceChange { | ||
| __typename: String, |
There was a problem hiding this comment.
shouldn't this be an enum? __typename as a string is an artifact of graphql
| #[wasm_bindgen] | ||
| pub struct RaindexVaultBalanceChange { | ||
| __typename: String, | ||
| amount: U256, |
There was a problem hiding this comment.
we should also have a formatted version ready for display, instead of having to do parseUnits on the front end
| #[cfg(target_family = "wasm")] | ||
| #[wasm_bindgen(getter = newVaultBalance)] | ||
| pub fn new_vault_balance(&self) -> Result<BigInt, RaindexError> { | ||
| BigInt::from_str(&self.new_vault_balance.to_string()) |
There was a problem hiding this comment.
this is repeated a lot - can we move elsewhere?
There was a problem hiding this comment.
this is improved in the follow-up PR and no longer repeated
| self.amount.to_string() | ||
| } | ||
| #[cfg(target_family = "wasm")] | ||
| #[wasm_bindgen(getter = newVaultBalance)] |
There was a problem hiding this comment.
is a getter the right approach if it's a Result?
would it be better to do all these fallible operations when originally creating the type so we don't have to do it later?
There was a problem hiding this comment.
i'll check if we can do that
There was a problem hiding this comment.
this is also improved in the followup PR but we are not doing the operation in the constructor. I think I can create an issue for this and implement it on top of the latest changes
3 participants
Caution
CHAINED PR
Do not merge before #1947
Motivation
See issue: #1747
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