Add rust tests for quote wasm bindings #1710
Conversation
…uote-bindings-test
WalkthroughThis set of changes refactors several Rust and TypeScript modules to improve type safety, error handling, and test coverage for quoting operations, particularly in the context of WebAssembly (WASM) bindings. The Changes
Sequence Diagram(s)sequenceDiagram
participant JS_Client
participant WASM_Bindings
participant Rust_QuoteLogic
JS_Client->>WASM_Bindings: Call quoting function (e.g., do_quote_targets)
WASM_Bindings->>Rust_QuoteLogic: Parse input, call Rust logic
Rust_QuoteLogic-->>WASM_Bindings: Return Result<T, QuoteBindingsError>
WASM_Bindings-->>JS_Client: Return structured { value, error } object
Note over JS_Client: JS checks for .error or .value and handles accordingly
Assessment against linked issues
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:
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: 6
📜 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 (11)
Cargo.toml(1 hunks)crates/js_api/src/gui/state_management.rs(1 hunks)crates/quote/Cargo.toml(1 hunks)crates/quote/src/js_api/mod.rs(1 hunks)crates/quote/src/lib.rs(1 hunks)crates/quote/src/quote.rs(2 hunks)crates/subgraph/Cargo.toml(0 hunks)crates/subgraph/src/types/common.rs(2 hunks)packages/orderbook/test/quote/test.test.ts(8 hunks)packages/ui-components/src/__tests__/TanstackOrderQuote.test.ts(5 hunks)packages/ui-components/src/lib/components/detail/TanstackOrderQuote.svelte(1 hunks)
💤 Files with no reviewable changes (1)
- crates/subgraph/Cargo.toml
🧰 Additional context used
🪛 Biome (1.9.4)
packages/orderbook/test/quote/test.test.ts
[error] 192-192: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
[error] 334-334: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
[error] 345-345: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: build-tauri (ubuntu-22.04, true)
- GitHub Check: test
- GitHub Check: standard-tests (ubuntu-latest, rainix-rs-artifacts, true)
- GitHub Check: test
🔇 Additional comments (14)
crates/js_api/src/gui/state_management.rs (1)
291-291: Improved code organization by moving imports to the tests module.Moving
js_sys::{eval, Reflect}from the main module scope to the tests module is a good practice. This confines potentially unsafe functionality likeevalto test code only and keeps imports localized to where they're actually used.crates/quote/Cargo.toml (2)
31-31: Making wasm-bindgen-utils available unconditionally.Moving
wasm-bindgen-utilsfrom target-specific dependencies to general dependencies ensures it's available for all target environments. This is necessary to support the expanded compilation conditions inlib.rs.
45-46: Added test dependencies to support wasm testing.Adding
rain_orderbook_commonandwasm-bindgen-testto dev-dependencies provides the necessary tools to test WebAssembly bindings, directly supporting the PR's objective to enhance test coverage.crates/quote/src/lib.rs (1)
9-9: Expanded compilation condition to include tests.Changing the
js_apimodule compilation condition from#[cfg(target_family = "wasm")]to#[cfg(any(target_family = "wasm", test))]is key to achieving the PR objective. This change enables thejs_apimodule to be compiled and included during test builds regardless of the target family, allowing proper testing of the WebAssembly bindings even in non-wasm environments.packages/ui-components/src/lib/components/detail/TanstackOrderQuote.svelte (1)
46-51: Improved error handling with explicit error propagationThe updated query function correctly handles the new result structure from the
getOrderQuotefunction. This change properly propagates errors by throwing an exception when an error is detected, making debugging and error handling more straightforward.packages/ui-components/src/__tests__/TanstackOrderQuote.test.ts (5)
15-25: Test updated to match new API result structureThe mock correctly reflects the updated return type of
getOrderQuote, which now returns an object with avalueproperty containing the array of results.
50-67: Test updated to match new API result structureThe mock correctly reflects the updated return type structure with wrapped values.
92-109: Test updated to match new API result structureThe mock updates here match the new return type pattern consistently with the other test cases.
130-140: Test updated to match new API result structureError handling case properly updated to match the new return type structure.
162-172: Test updated to match new API result structureZero ratio test case correctly updated with the new structure.
crates/subgraph/src/types/common.rs (1)
90-109: Unconditional WASM trait implementation for SgOrderThe changes here remove the conditional compilation for the
Tsifyderive macro and theimpl_wasm_traits!macro invocation, making them always available regardless of the compilation target. This is a good simplification that aligns with the PR's goal of improving test coverage for quote WASM bindings.By making these traits unconditionally available, we can ensure consistent behavior across all environments and make testing easier.
crates/quote/src/quote.rs (2)
106-110: Unconditional WASM trait implementation for BatchQuoteTargetRemoving the conditional compilation for the
Tsifyderive macro and making the WASM trait implementation unconditional is a good approach. This simplifies the code and ensures consistent behavior across all compilation targets.
202-206: Unconditional WASM trait implementation for BatchQuoteSpecSimilar to the change for
BatchQuoteTarget, removing the conditional compilation here simplifies the code and ensures consistent behavior across all targets. This change supports the goal of improved test coverage for WASM bindings.crates/quote/src/js_api/mod.rs (1)
57-60: Hex parsing oforder_hashrelies on implementation details
U256::from_strcurrently accepts both decimal and0x-prefixed hex, but this is implementation-defined and may change.
Parsing explicitly as hex makes intent clear and avoids silent breakage:-let order_hash = U256::from_str(order_hash)?; +let order_hash = U256::from_hex(order_hash)?;(Requires
use alloy::hex::FromHex).Please verify tests still pass.
| @@ -53,7 +53,7 @@ test-context = "0.3.0" | |||
| portpicker = "0.1.1" | |||
| rain-erc = { git = "https://github.com/rainlanguage/rain.erc", rev = "0106e645ebd49334addc698c5aad9a85370eb54d" } | |||
| rain-error-decoding = { git = "https://github.com/rainlanguage/rain.error", rev = "72d9577fdaf7135113847027ba951f9a43b41827" } | |||
| wasm-bindgen-utils = "0.0.7" | |||
| wasm-bindgen-utils = { git = "https://github.com/rainlanguage/rain.wasm", rev = "4379d24a671742a58ec5cdab2c39fecbda59619d" } | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Using git dependency for more precise version control.
Switching from a fixed version to a specific git commit for wasm-bindgen-utils provides better control over the exact implementation being used. This helps ensure consistency across the workspace, particularly important for WebAssembly bindings.
🏁 Script executed:
#!/bin/bash
# Check if this commit is used in other workspace crates
grep -r "wasm-bindgen-utils.*4379d24a671742a58ec5cdab2c39fecbda59619d" --include="Cargo.toml" ./crates/Length of output: 103
🏁 Script executed:
#!/bin/bash
# Search all Cargo.toml files in the repository for any reference to wasm-bindgen-utils
rg "wasm-bindgen-utils" --glob Cargo.toml -n .Length of output: 687
🏁 Script executed:
#!/bin/bash
# Show the section around the git override in the root manifest
sed -n '45,70p' Cargo.toml
# Show the dependency section in the Tauri app manifest
sed -n '30,60p' tauri-app/src-tauri/Cargo.tomlLength of output: 2092
Ensure Tauri app picks up the git‐pinned wasm-bindgen-utils version
You’ve added wasm-bindgen-utils = { git = …, rev = … } to the workspace dependencies, but the Tauri app’s WASM target is still locked to version "0.0". To have it use the exact commit you specified above, change its dependency to reference the workspace entry:
--- tauri-app/src-tauri/Cargo.toml
@@ [target.'cfg(target_family = "wasm")'.dependencies]
-wasm-bindgen-utils = "0.0"
+wasm-bindgen-utils = { workspace = true }This ensures the Tauri‐side WASM build also uses the Git commit override you added.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| wasm-bindgen-utils = { git = "https://github.com/rainlanguage/rain.wasm", rev = "4379d24a671742a58ec5cdab2c39fecbda59619d" } | |
| # tauri-app/src-tauri/Cargo.toml | |
| [target.'cfg(target_family = "wasm")'.dependencies] | |
| wasm-bindgen-utils = { workspace = true } |
There was a problem hiding this comment.
We are going to update this dependency later on
There was a problem hiding this comment.
@findolor I understand, you're planning to update the wasm-bindgen-utils dependency in the Tauri app in a future PR. That makes sense to keep the current PR focused on adding Rust tests for the quote wasm bindings. Let me know if you need any assistance with the current implementation.
| @@ -1,6 +1,6 @@ | |||
| import assert from 'assert'; | |||
| import { getLocal } from 'mockttp'; | |||
| import { describe, it, beforeEach, afterEach } from 'vitest'; | |||
| import { describe, it, beforeEach, afterEach, expect } from 'vitest'; | |||
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Prefer a single assertion library
vitest’s expect API is imported, yet the remainder of the file predominantly relies on Node’s built-in assert.
Using two different assertion styles in the same suite is confusing for readers and can lead to inconsistent failure messages and diff output.
Consider migrating all remaining assert.* calls to expect (or vice-versa) for consistency.
There was a problem hiding this comment.
We can keep this for now
There was a problem hiding this comment.
@findolor Understood! Not changing the assertion styles for now makes sense, as keeping PRs focused is often more practical than expanding scope to address every potential improvement.
| const result = await doQuoteTargets([target], mockServer.url + '/rpc-url'); | ||
| if (!result.value) expect.fail('Expected value'); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Use template literals instead of string concatenation
Biome flagged this line. A template literal keeps the string readable and avoids accidental spacing issues.
-const result = await doQuoteTargets([target], mockServer.url + '/rpc-url');
+const result = await doQuoteTargets([target], `${mockServer.url}/rpc-url`);Repeat for similar concatenations (e.g. lines 116, 334, 345).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const result = await doQuoteTargets([target], mockServer.url + '/rpc-url'); | |
| if (!result.value) expect.fail('Expected value'); | |
| const result = await doQuoteTargets([target], `${mockServer.url}/rpc-url`); | |
| if (!result.value) expect.fail('Expected value'); |
🧰 Tools
🪛 Biome (1.9.4)
[error] 192-192: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
There was a problem hiding this comment.
No need to do this for now
There was a problem hiding this comment.
@findolor Understood! I'll respect your decision not to change the string concatenation to template literals at this time.
| const result = getId(orderbook, orderHash); | ||
| const expected = '0xca228cb816102ef9f8e0f9a87bb34e06c49c4d4ddf5a2a0ec229ab671475c235'; | ||
| assert.equal(result, expected); | ||
| if (result.error) expect.fail('Expected value'); | ||
| assert.equal(result.value, expected); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Guard against both failure channels
You correctly fail the test when result.error exists, but, if the WASM layer ever returns an object with neither value nor error (e.g. an unexpected shape), assert.equal(result.value, expected) will throw a TypeError before the intent is clear.
- if (result.error) expect.fail('Expected value');
- assert.equal(result.value, expected);
+if (result.error || result.value === undefined) {
+ expect.fail(`Unexpected error or empty value: ${JSON.stringify(result.error)}`);
+}
+expect(result.value).toEqual(expected);This makes the intent explicit and prevents dereferencing undefined.
There was a problem hiding this comment.
This not a possibility
There was a problem hiding this comment.
@findolor I understand. If you're certain that the API contract guarantees either a value or an error will always be present (but never neither), then the current implementation is sufficient. Thanks for the clarification.
| let res = quotes | ||
| .into_iter() | ||
| .map(|q| match q { | ||
| Ok(v) => QuoteResultEnum::Success { | ||
| value: v, | ||
| error: None, | ||
| }, | ||
| Err(e) => QuoteResultEnum::Err { | ||
| value: None, | ||
| error: e.to_string(), | ||
| }, | ||
| }) | ||
| .collect(); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Return user-friendly error messages to JS caller
JS consumers expect readableMsg (see tests) yet the mapping here propagates e.to_string(), losing the nicely formatted message provided by QuoteBindingsError::to_readable_msg().
-Err(e) => QuoteResultEnum::Err {
- value: None,
- error: e.to_string(),
-},
+Err(e) => QuoteResultEnum::Err {
+ value: None,
+ error: e.to_readable_msg(),
+},Apply the same change in do_quote_specs for parity.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let res = quotes | |
| .into_iter() | |
| .map(|q| match q { | |
| Ok(v) => QuoteResultEnum::Success { | |
| value: v, | |
| error: None, | |
| }, | |
| Err(e) => QuoteResultEnum::Err { | |
| value: None, | |
| error: e.to_string(), | |
| }, | |
| }) | |
| .collect(); | |
| let res = quotes | |
| .into_iter() | |
| .map(|q| match q { | |
| Ok(v) => QuoteResultEnum::Success { | |
| value: v, | |
| error: None, | |
| }, | |
| Err(e) => QuoteResultEnum::Err { | |
| value: None, | |
| error: e.to_readable_msg(), | |
| }, | |
| }) | |
| .collect(); |
There was a problem hiding this comment.
Normal message is fine for our case at the moment
There was a problem hiding this comment.
@findolor Understood! I'll respect your preference to keep the normal error message format for now. Let me know if you need any other assistance with the code.
| #[derive(Serialize, Deserialize, Debug, Clone, Tsify)] | ||
| #[serde(untagged)] | ||
| pub enum QuoteResultEnum { | ||
| Success { | ||
| value: OrderQuoteValue, | ||
| #[tsify(type = "undefined")] | ||
| error: Option<String>, | ||
| }, | ||
| Err { | ||
| #[tsify(type = "undefined")] | ||
| value: Option<OrderQuoteValue>, | ||
| error: String, | ||
| }, | ||
| } | ||
| impl_wasm_traits!(QuoteResultEnum); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
serde(untagged) may cause ambiguous (de)serialisation
Both Success and Err variants expose the same field names (value, error) with optional types. With #[serde(untagged)], Serde chooses the first variant that can be deserialised, which becomes ambiguous when a JSON object accidentally contains both keys or neither.
Adding an explicit tag avoids this trap and produces clearer JSON:
-#[serde(untagged)]
-pub enum QuoteResultEnum {
+#[serde(tag = "status", rename_all = "lowercase")]
+pub enum QuoteResultEnum {
Success {
value: OrderQuoteValue,
#[tsify(type = "undefined")]
error: Option<String>,
},
Err {Downstream TS remains unchanged because the discriminant is ignored when using the generated typings.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #[derive(Serialize, Deserialize, Debug, Clone, Tsify)] | |
| #[serde(untagged)] | |
| pub enum QuoteResultEnum { | |
| Success { | |
| value: OrderQuoteValue, | |
| #[tsify(type = "undefined")] | |
| error: Option<String>, | |
| }, | |
| Err { | |
| #[tsify(type = "undefined")] | |
| value: Option<OrderQuoteValue>, | |
| error: String, | |
| }, | |
| } | |
| impl_wasm_traits!(QuoteResultEnum); | |
| #[derive(Serialize, Deserialize, Debug, Clone, Tsify)] | |
| #[serde(tag = "status", rename_all = "lowercase")] | |
| pub enum QuoteResultEnum { | |
| Success { | |
| value: OrderQuoteValue, | |
| #[tsify(type = "undefined")] | |
| error: Option<String>, | |
| }, | |
| Err { | |
| #[tsify(type = "undefined")] | |
| value: Option<OrderQuoteValue>, | |
| error: String, | |
| }, | |
| } | |
| impl_wasm_traits!(QuoteResultEnum); |
There was a problem hiding this comment.
This is the expected response, no need to change it
There was a problem hiding this comment.
@findolor Understood! I'll respect your design decision to keep the #[serde(untagged)] attribute as is. Thanks for clarifying.
Motivation
See issue: #1601 and #1635
Solution
Checks
By submitting this for review, I'm confirming I've done the following:
[ ] included screenshots (if this involves a front-end change)fix #1635
Summary by CodeRabbit