Move common js_api file to main js_api directory and add tests #1715
Conversation
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the WalkthroughThis change refactors the WASM JavaScript API for generating order calldata by moving it from the Changes
Sequence Diagram(s)sequenceDiagram
participant JS as JavaScript (WASM consumer)
participant WASM as WASM Bindgen (js_api/common)
participant Parser as Frontmatter Parser
participant RPC as RPC Client
JS->>WASM: getAddOrderCalldata(dotrain, deployment)
WASM->>Parser: parseFrontmatter(dotrain)
Parser-->>WASM: config
WASM->>RPC: fetch deployment info (by key)
RPC-->>WASM: deployment details
WASM->>WASM: prepare add order args
WASM->>WASM: generate calldata bytes
WASM-->>JS: AddOrderCalldata(bytes)
JS->>WASM: getRemoveOrderCalldata(order)
WASM->>WASM: prepare remove order args
WASM->>WASM: generate calldata bytes
WASM-->>JS: RemoveOrderCalldata(bytes)
Assessment against linked issues
Possibly related PRs
Suggested reviewers
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
🔭 Outside diff range comments (2)
packages/orderbook/test/common/test.test.ts (2)
77-84: 🧹 Nitpick (assertive)Trim whitespace before comparison to avoid brittle failures
DotrainOrder.composeDeploymentToRainlangmight emit or omit trailing/leading new-lines depending on microscopic changes in the compiler or running OS.
A strict equality check will break the build for purely cosmetic newline differences.- assert.equal(result, expected); + assert.equal(result.trim(), expected.trim());Do the same for the second expectation below to keep both tests robust.
97-104: 🧹 Nitpick (assertive)See the note above – apply
.trim()here as well to make the test future-proof.
📜 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 (15)
Cargo.toml(1 hunks)crates/common/src/frontmatter.rs(1 hunks)crates/common/src/js_api/mod.rs(0 hunks)crates/common/src/lib.rs(1 hunks)crates/common/src/test_helpers.rs(2 hunks)crates/js_api/Cargo.toml(1 hunks)crates/js_api/src/common/mod.rs(1 hunks)crates/js_api/src/gui/state_management.rs(1 hunks)crates/js_api/src/lib.rs(1 hunks)crates/subgraph/Cargo.toml(0 hunks)crates/subgraph/src/types/common.rs(22 hunks)packages/orderbook/test/common/test.test.ts(1 hunks)packages/orderbook/test/js_api/common.test.ts(1 hunks)packages/webapp/src/__tests__/OrderRemoveModal.test.ts(1 hunks)packages/webapp/src/lib/components/OrderRemoveModal.svelte(1 hunks)
💤 Files with no reviewable changes (2)
- crates/subgraph/Cargo.toml
- crates/common/src/js_api/mod.rs
🧰 Additional context used
🧠 Learnings (1)
packages/webapp/src/lib/components/OrderRemoveModal.svelte (1)
Learnt from: hardingjam
PR: rainlanguage/rain.orderbook#1512
File: packages/ui-components/src/__tests__/OrderDetail.test.ts:0-0
Timestamp: 2025-04-09T09:28:05.097Z
Learning: The OrderDetail component has been refactored to use an `onRemove` callback approach instead of the previous `handleOrderRemoveModal` pattern for order removal functionality, as part of PR #1512.
🪛 Biome (1.9.4)
packages/orderbook/test/js_api/common.test.ts
[error] 145-145: This let declares a variable that is only assigned once.
'order' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
⏰ Context from checks skipped due to timeout of 90000ms (16)
- GitHub Check: build-tauri (ubuntu-22.04, true)
- GitHub Check: git-clean
- GitHub Check: test
- GitHub Check: test
- GitHub Check: test
- GitHub Check: standard-tests (ubuntu-latest, test-js-bindings)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-test)
- GitHub Check: standard-tests (ubuntu-latest, rainix-wasm-artifacts)
- GitHub Check: Deploy-Preview
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-static)
- GitHub Check: standard-tests (ubuntu-latest, rainix-rs-artifacts, true)
- GitHub Check: standard-tests (ubuntu-latest, ob-rs-test, true)
- GitHub Check: standard-tests (ubuntu-latest, rainix-wasm-test)
- 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-rs-static)
🔇 Additional comments (14)
crates/js_api/src/gui/state_management.rs (1)
291-291: Good move restricting JS interop utilities to test code.Importing
js_sys::{eval, Reflect}only within the test module is a better practice than having it in the main module scope, as it limits the exposure of JS interop to testing code.crates/common/src/frontmatter.rs (1)
164-164: Test assertions updated to match numeric values.The test assertions have been correctly updated to reflect the changes in test data, where string values like "value1" and "value2" were replaced with numeric values "10" and "20".
Also applies to: 170-170
Cargo.toml (1)
56-56:✅ Verification successful
Consider trade-offs of Git dependency versus published crate.
Switching from a versioned crate
"0.0.7"to a Git repository with a specific commit hash ensures reproducible builds but may make it harder to track and update dependencies.
🏁 Script executed:
#!/bin/bash # Check if wasm-bindgen-utils is used in other repos that might be affected by this change # Look for the package in the workspace echo "Checking usage of wasm-bindgen-utils in the workspace:" rg "wasm-bindgen-utils" --type rust # Check if any other dependencies are using git references echo -e "\nChecking for other git dependencies in Cargo.toml:" rg "git =" Cargo.tomlLength of output: 801
No action needed: Git dependency is consistent with other internal crates
This workspace already pulls all internal
rainlanguagecrates directly from Git (ensuring reproducible builds), including:
alloy-ethers-typecastrain-ercrain-error-decodingwasm-bindgen-utilsSwitching
wasm-bindgen-utilsto a Git reference aligns with the existing practice. No changes required.packages/webapp/src/__tests__/OrderRemoveModal.test.ts (1)
8-10: Mock updated to match the new return type structure.The mock implementation has been correctly updated to return a structured object with a
valueproperty, which aligns with the new WASM bindings return type ingetRemoveOrderCalldata.crates/js_api/Cargo.toml (1)
36-38: Good addition of conditional dev-dependency for testingAdding
httpmockas a dev-dependency only for non-WASM targets is a good approach. This will allow for proper HTTP mocking in tests while keeping the WASM build clean.crates/js_api/src/lib.rs (1)
1-2: Appropriate conditional compilation for improved testabilityExposing the
commonmodule for both WASM and test environments is a good approach. This ensures the module can be properly tested without needing to compile for WASM specifically.packages/webapp/src/lib/components/OrderRemoveModal.svelte (1)
27-36: Improved error handling for the new API structureThe updated code properly handles the new return structure from
getRemoveOrderCalldata, checking for errors before proceeding with the transaction. This is a good improvement that provides clearer error reporting to users.crates/common/src/test_helpers.rs (1)
60-66: Values changed to numbers – looks goodBindings
key1andkey2were migrated from strings to numbers and the YAML / front-matter stay in sync with the updated Rust tests.packages/orderbook/test/js_api/common.test.ts (1)
8-9: AwaitmockServer.start/stopfor deterministic setup & teardown
mockttpreturns a Promise from bothstartandstop. Although Vitest handles returned Promises, being explicit avoids foot-guns if the function signature ever changes.-beforeEach(() => mockServer.start(8080)); -afterEach(() => mockServer.stop()); +beforeEach(async () => { await mockServer.start(8080); }); +afterEach(async () => { await mockServer.stop(); });crates/subgraph/src/types/common.rs (5)
3-3: LGTM: Unconditional import of wasm_bindgen_utils.The import is now unconditional rather than being wrapped in a
#[cfg(target_family = "wasm")]directive, which aligns with the refactoring effort to centralize WASM-related functionality.
5-5: LGTM: Standardizing Tsify derivation.The
Tsifyderive macro is now applied unconditionally to all relevant structs and enums, which ensures consistent TypeScript type definitions. This change supports the centralized WASM binding approach in the js_api crate.Also applies to: 11-11, 21-21, 29-29, 40-40, 51-51, 60-60, 73-73, 82-82, 103-103, 110-110, 118-118, 127-127, 134-134, 144-144, 155-155, 175-175, 182-182, 191-191, 206-206, 219-219, 235-235, 251-251, 267-267, 284-284, 291-291, 304-304, 310-310, 323-323, 333-333, 340-340, 347-347, 354-354, 361-361, 365-365
101-101: LGTM: Relocated WASM trait implementations.The
impl_wasm_traits!macro invocations forSgOrderandSgVaulthave been moved from the conditionalimplsmodule to appear directly after their respective struct definitions. This improves code readability by keeping related implementations together.Also applies to: 173-173
338-338: LGTM: New WASM trait implementations for order operations.New
impl_wasm_traits!invocations forSgAddOrderandSgRemoveOrdersupport the js_api crate's functions for generating order calldata. This enables proper serialization/deserialization when these types cross the Rust/JavaScript boundary.Also applies to: 345-345
657-679: LGTM: Remaining WASM trait implementations are still conditionally defined.The module containing additional
impl_wasm_traits!invocations is still conditionally compiled only for WASM targets. This is appropriate for types that aren't directly used by the centralized js_api functions but still need WASM interoperability when targeting WASM.
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 (2)
Cargo.toml(1 hunks)crates/js_api/src/gui/state_management.rs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (16)
- GitHub Check: test
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-legal)
- GitHub Check: standard-tests (ubuntu-latest, rainix-wasm-test)
- GitHub Check: standard-tests (ubuntu-latest, test-js-bindings)
- 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-wasm-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-rs-artifacts, true)
- GitHub Check: standard-tests (ubuntu-latest, ob-rs-test, true)
- GitHub Check: build-tauri (ubuntu-22.04, true)
- GitHub Check: test
- GitHub Check: Deploy-Preview
- GitHub Check: git-clean
- GitHub Check: test
🔇 Additional comments (1)
crates/js_api/src/gui/state_management.rs (1)
293-293: Restrict js_sys imports to test scope
Great refactor: movinguse js_sys::{eval, Reflect};into the#[cfg(test)]module confines unsafe/interpreter‐specific APIs to tests only, reducing surface for runtime issues in production.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
crates/js_api/src/common/mod.rs (1)
15-22: (Repeat) Optional ergonomic helper still missingThe
.0field access onAddOrderCalldata/RemoveOrderCalldataremains slightly clunky for downstream users. ImplementingDeref<Target = Bytes>(or anas_ref()method) would remove that friction.I’m not reopening the discussion—just documenting that the earlier suggestion hasn’t been adopted.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
crates/common/src/lib.rs(1 hunks)crates/js_api/src/common/mod.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
crates/js_api/src/common/mod.rs (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.848Z
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.
⏰ Context from checks skipped due to timeout of 90000ms (16)
- GitHub Check: git-clean
- GitHub Check: test
- 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-wasm-artifacts)
- GitHub Check: standard-tests (ubuntu-latest, rainix-wasm-test)
- GitHub Check: test
- GitHub Check: standard-tests (ubuntu-latest, rainix-rs-static)
- 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, rainix-sol-static)
- GitHub Check: standard-tests (ubuntu-latest, rainix-rs-artifacts, true)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-test)
- GitHub Check: build-tauri (ubuntu-22.04, true)
- GitHub Check: Deploy-Preview
- GitHub Check: test
🔇 Additional comments (1)
crates/common/src/lib.rs (1)
23-24: Conditional compilation fortest_helperssuccessfully re-instatedRe-applying the
#[cfg(test)]gate keeps test-only utilities out of release builds and public docs, addressing the concern raised in the earlier review.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
crates/js_api/src/common/mod.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
crates/js_api/src/common/mod.rs (2)
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1715
File: crates/js_api/src/common/mod.rs:111-118
Timestamp: 2025-04-30T09:28:36.848Z
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#1715
File: crates/js_api/src/common/mod.rs:101-105
Timestamp: 2025-04-30T10:32:53.857Z
Learning: In the rain.orderbook project, the `get_remove_order_calldata` function takes `SgOrder` by value rather than by reference, as per maintainer preference.
⏰ Context from checks skipped due to timeout of 90000ms (4)
- 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-sol-test)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-static)
|
@findolor i'm a bit confused by this PR - the issue is just about missing tests, but then there are a bunch of other changes |
|
@hardyjosh while writing the tests, i've also implemented the wasm macro and moved the logic to be inside the js api. that's why there are other changes. otherwise we needed to do a second round of PR to update the tests again with wasm export in mind |
There was a problem hiding this comment.
We should probably have non-happy path tests
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
crates/js_api/src/common/mod.rs (2)
11-12:std::ops::Derefimport and.deref()call remain unnecessary
deployment_refis already a&Deployment; invoking.deref()and keeping theDerefimport add no extra value and make the call-site slightly noisier. This was pointed out in an earlier review and intentionally left unchanged, so marking as duplicate.Also applies to: 84-86
70-94: Minor copy optimisation remains but acknowledged
Bytes::copy_from_slice(&calldata)incurs an extra allocation; returningcalldata.into()would avoid it. You explicitly preferred the current approach earlier, so not re-raising—just noting for completeness.Also applies to: 101-105
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
tauri-app/src-tauri/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (1)
crates/js_api/src/common/mod.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
crates/js_api/src/common/mod.rs (3)
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.637Z
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:101-105
Timestamp: 2025-04-30T10:32:53.866Z
Learning: In the rain.orderbook project, the `get_remove_order_calldata` function takes `SgOrder` by value rather than by reference, as per maintainer preference.
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-legal)
- GitHub Check: test
- GitHub Check: standard-tests (ubuntu-latest, ob-rs-test, true)
- GitHub Check: Deploy-Preview
- GitHub Check: test
- GitHub Check: test
- GitHub Check: git-clean
- GitHub Check: build-tauri (ubuntu-22.04, true)
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
crates/js_api/src/common/mod.rs (3)
11-12: UnnecessaryDerefimport and call
deployment_refis already a&Deployment; cloning it directly is sufficient.
The extraDerefimport and.deref()call add noise without functional gain.Also applies to: 85-86
55-59: Expose the readable error message to JS callersUsing
value.to_string()surfaces the enum variant name, whereasto_readable_msg()was designed for human-friendly text. Forwarding the readable variant improves DX for browser consumers.
75-94: Avoid double cloning of thedotrainstring
dotrain.to_string()is invoked twice (lines 79 and 85), allocating two identicalStrings.
Taking ownership once and re-using it trims a small allocation and simplifies the code.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
crates/js_api/src/common/mod.rs(1 hunks)crates/js_api/src/lib.rs(1 hunks)packages/webapp/src/__tests__/OrderRemoveModal.test.ts(3 hunks)packages/webapp/src/lib/components/OrderRemoveModal.svelte(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
packages/webapp/src/lib/components/OrderRemoveModal.svelte (1)
Learnt from: hardingjam
PR: rainlanguage/rain.orderbook#1512
File: packages/ui-components/src/__tests__/OrderDetail.test.ts:0-0
Timestamp: 2025-04-09T09:28:05.097Z
Learning: The OrderDetail component has been refactored to use an `onRemove` callback approach instead of the previous `handleOrderRemoveModal` pattern for order removal functionality, as part of PR #1512.
crates/js_api/src/common/mod.rs (5)
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.
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1715
File: crates/js_api/src/common/mod.rs:101-105
Timestamp: 2025-04-30T10:32:53.866Z
Learning: In the rain.orderbook project, the `get_remove_order_calldata` function takes `SgOrder` by value rather than by reference, as per maintainer preference.
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1715
File: crates/js_api/src/common/mod.rs:15-22
Timestamp: 2025-05-14T05:52:04.270Z
Learning: The project doesn't require `#[repr(transparent)]` for newtype wrappers in WASM contexts such as `AddOrderCalldata` and `RemoveOrderCalldata` as the current implementation is working as expected without it.
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1715
File: crates/js_api/src/common/mod.rs:55-59
Timestamp: 2025-05-14T05:51:50.277Z
Learning: In the Rain Orderbook project, the error handling for WASM exported functions uses WasmEncodedError which includes both the original error message and a human-readable version, making direct modification of the JsValue conversion unnecessary.
🔇 Additional comments (1)
crates/js_api/src/lib.rs (1)
3-5: LGTM – makescommonavailable for unit testsExposing the new
commonmodule behindcfg(any(target_family = "wasm", test))keeps the WASM bindings accessible in both browser and native test contexts.
No issues spotted.
Motivation
See issue: #1621
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 #1621
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests
Chores