-
Notifications
You must be signed in to change notification settings - Fork 7
Update order quote wasm bindings to use wasm export macro #1706
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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 WalkthroughThe changes update the Rust and JavaScript codebases to improve error handling and result typing for quote-related operations. The Rust API for quoting is refactored to use strongly typed result wrapper structs and a unified error enum, replacing generic JavaScript values and manual error handling. This leads to updated function signatures and the introduction of new result structs and error types. Corresponding test files and Svelte components are modified to explicitly check for errors and handle result objects more robustly. Additionally, the Changes
Sequence Diagram(s)sequenceDiagram
participant JS_Client as JavaScript Client
participant Wasm_API as Rust Wasm API
participant QuoteEngine as Quote Engine
JS_Client->>Wasm_API: Call do_quote_targets(params)
Wasm_API->>QuoteEngine: Perform quote operation
QuoteEngine-->>Wasm_API: Return quote result or error
Wasm_API-->>JS_Client: Return DoQuoteTargetsResult (Success/Error)
JS_Client->>Wasm_API: Call get_order_quote(order, rpcUrl)
Wasm_API->>QuoteEngine: Perform order quote
QuoteEngine-->>Wasm_API: Return quote or error
Wasm_API-->>JS_Client: Return DoOrderQuoteResult (Success/Error)
Suggested labels
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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 6
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (2)
Cargo.lock
is excluded by!**/*.lock
tauri-app/src-tauri/Cargo.lock
is excluded by!**/*.lock
📒 Files selected for processing (5)
Cargo.toml
(1 hunks)crates/js_api/src/gui/state_management.rs
(1 hunks)crates/quote/src/js_api/mod.rs
(1 hunks)packages/orderbook/test/quote/test.test.ts
(8 hunks)packages/ui-components/src/lib/components/detail/TanstackOrderQuote.svelte
(1 hunks)
🧰 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] 347-347: 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 (7)
- GitHub Check: standard-tests (ubuntu-latest, rainix-rs-static)
- GitHub Check: standard-tests (ubuntu-latest, test-js-bindings)
- GitHub Check: test
- GitHub Check: test
- GitHub Check: test
- GitHub Check: build-tauri (ubuntu-22.04, true)
- GitHub Check: git-clean
🔇 Additional comments (9)
crates/js_api/src/gui/state_management.rs (1)
291-291
: Good security improvement:eval
andReflect
are now limited to the test scope.Moving these imports from the module level to the test module scope is a positive change that confines potentially unsafe JavaScript evaluation functions to testing code only. This aligns with the overall PR objective of improving the safety and robustness of the WebAssembly bindings.
Cargo.toml (1)
56-56
:✅ Verification successful
Dependency updated to specific Git commit hash.
The dependency for
wasm-bindgen-utils
is now pinned to a specific commit in the rain.wasm repository. This provides better versioning control but introduces a potential maintenance concern.
🏁 Script executed:
#!/bin/bash # Check if this dependency is being actively maintained curl -s "https://api.github.com/repos/rainlanguage/rain.wasm/commits?sha=4379d24a671742a58ec5cdab2c39fecbda59619d" | jq '.[] | {date: .commit.author.date, message: .commit.message}' | head -n 10Length of output: 379
Pinning to a Git commit is intentional—but keep it up-to-date
The dependency for
wasm-bindgen-utils
is pinned to commit 4379d24a671742a58ec5cdab2c39fecbda59619d to ensure reproducible builds. Our check shows therain.wasm
repo is actively maintained (latest commits on 2025-04-28), so you’ll be able to track updates easily.• No immediate issue—just remember that this won’t auto-update.
• Consider periodically bumping the commit or switching to a published crate version (if available) to surface new fixes and improvements.
• If you rely on semver guarantees, evaluate whether a crates.io release could better suit your workflow.packages/orderbook/test/quote/test.test.ts (6)
3-3
: Added proper test assertion tools from Vitest.Importing
expect
from Vitest enhances the test suite with more explicit assertion capabilities.
26-27
: Enhanced result validation with explicit error checking.Now properly validating the structured result object by checking for error presence before accessing the value.
117-118
: Improved error handling and result type validation.The tests now explicitly check for the presence of the
value
property before proceeding with assertions, making the tests more robust against unexpected result structures.Also applies to: 192-194
276-277
: Added explicit error checking before assertions.Properly verifying the success case before accessing the result value, improving test reliability.
Also applies to: 290-290
334-343
: Explicit error case testing with proper error message validation.Now testing both the presence of an error and the specific error message, providing better test coverage for failure scenarios.
🧰 Tools
🪛 Biome (1.9.4)
[error] 334-334: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
345-356
: Added proper gas parameter testing with result validation.The test now correctly verifies successful execution with the gas parameter and validates the returned result structure.
🧰 Tools
🪛 Biome (1.9.4)
[error] 347-347: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
crates/quote/src/js_api/mod.rs (1)
57-61
:U256::from_str
expects decimal – hex order hashes will fail
U256::from_str
in Alloy parses base-10. Most on-chain order hashes are supplied as0x
-prefixed hex strings, which will triggerU256ParseError
.-let order_hash = U256::from_str(order_hash)?; +let order_hash = U256::from_hex(order_hash)?;Please either switch to
from_hex
or document the expected format and adjust callers.
packages/ui-components/src/lib/components/detail/TanstackOrderQuote.svelte
Outdated
Show resolved
Hide resolved
console.log(error); | ||
assert.fail('expected to resolve, but failed'); | ||
} | ||
const result = await doQuoteTargets([target], mockServer.url + '/rpc-url'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧹 Nitpick (assertive)
Use template literals instead of string concatenation.
Static analysis identified style issues with string concatenation.
Apply these style improvements:
- const result = await doQuoteTargets([target], mockServer.url + '/rpc-url');
+ const result = await doQuoteTargets([target], `${mockServer.url}/rpc-url`);
- const doQuoteTargetsResult = await doQuoteTargets([target], mockServer.url + '/rpc-url');
+ const doQuoteTargetsResult = await doQuoteTargets([target], `${mockServer.url}/rpc-url`);
- mockServer.url + '/rpc-url',
+ `${mockServer.url}/rpc-url`,
Also applies to: 334-334, 347-347
🧰 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to do this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@findolor Understood! I'll respect your preference to keep the string concatenation as is.
✏️ Learnings added
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1706
File: packages/orderbook/test/quote/test.test.ts:192-192
Timestamp: 2025-04-28T14:02:08.650Z
Learning: In the rain.orderbook repository, string concatenation (using +) is acceptable and preferred over template literals in some cases, despite linter suggestions.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Template literals are usually a bit more readable IMO but in this case it doesn't make a difference
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!
crates/quote/src/js_api/mod.rs
Outdated
pub fn to_readable_msg(&self) -> String { | ||
match self { | ||
Self::QuoteError(e) => format!("Quote error: {}", e), | ||
Self::FromHexError(e) => format!("Failed to parse orderbook address: {}", e), | ||
Self::U256ParseError(e) => format!("Failed to parse u256 value: {}", e), | ||
Self::SerdeWasmBindgenError(err) => format!("Data serialization error: {}", err), | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧹 Nitpick (assertive)
to_readable_msg
is easy to forget when new variants are added
Manually matching every variant is brittle. Implementing Display
(or relying on thiserror
’s autogenerated messages) lets you delegate:
impl std::fmt::Display for QuoteBindingsError {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
std::fmt::Debug::fmt(self, f)
}
}
impl QuoteBindingsError {
pub fn to_readable_msg(&self) -> String { self.to_string() }
}
Reduces maintenance overhead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to do this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@findolor Understood! I'll respect your preference to keep the explicit error message formatting in the to_readable_msg
method. Thanks for the feedback.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The AI comment doesn't really make sense. Since there is no catch all case, adding new variants will result in a compilation error, which makes it hard to forget
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
♻️ Duplicate comments (2)
crates/quote/src/js_api/mod.rs (2)
229-233
: Structured error lost by converting throughJsError
Previous feedback noted that wrapping in
JsError
discards thereadable_msg
produced byto_readable_msg()
. The current code still does so:JsError::new(&value.to_string()).into()If retaining detailed error metadata for JS callers is desirable, convert via
WasmEncodedError
instead:- JsError::new(&value.to_string()).into() + WasmEncodedError::from(value).into_js_value()Re-surfacing this in case it was left in unintentionally.
18-31
:serde(untagged)
enum remains potentially ambiguousBoth variants expose the same optional fields (
value
,error
), so payloads missingvalue
could deserialize toSuccess
rather thanErr
, depending on variant order. Earlier review highlighted this edge-case; re-flagging for awareness should requirements change.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
crates/quote/src/js_api/mod.rs
(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Deploy-Preview
- GitHub Check: test
🔇 Additional comments (1)
crates/quote/src/js_api/mod.rs (1)
134-136
: 🧹 Nitpick (assertive)Redundant
mut res
declaration duplicated hereThe same pattern appears in
do_quote_specs
. Deleting it keeps the codebase consistent and warning-free.- let mut res: Vec<QuoteResultEnum> = vec![];
Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
packages/ui-components/src/__tests__/TanstackOrderQuote.test.ts (1)
2-13
: 🛠️ Refactor suggestionAdd
afterEach
hook to avoid cross-test mock leakage
getOrderQuote
is re-mocked in every test, but Vitest will preserve previousmockResolvedValueOnce
calls if the module isn’t reset.
Adding anafterEach
that clears all mocks prevents brittle order-dependent failures when the file grows or tests are reordered.-import { test, vi, type Mock } from 'vitest'; +import { afterEach, test, vi, type Mock } from 'vitest'; … +afterEach(() => { + vi.resetAllMocks(); +});
♻️ Duplicate comments (1)
crates/quote/src/js_api/mod.rs (1)
225-229
: Consider returning structuredWasmEncodedError
instead of opaqueJsError
(Same concern raised previously – re-flagging for visibility)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
crates/quote/src/js_api/mod.rs
(1 hunks)packages/ui-components/src/__tests__/TanstackOrderQuote.test.ts
(5 hunks)
🔇 Additional comments (2)
packages/ui-components/src/__tests__/TanstackOrderQuote.test.ts (2)
117-121
: Verify expected pair text (ETH/USD
vsETH/USDT
)The refreshed mock still returns
pairName: 'ETH/USDT'
, yet the assertion looks for'ETH/USD'
(missing T).
If the UI is truly supposed to show the trimmed symbol it should do so consistently, otherwise this expectation will fail or hide a bug.Please double-check the component output and adjust either the mock or the assertion accordingly.
14-25
: Mock shape update matches new API – looks goodWrapping the quote array in a
{ value: [...] }
object aligns the tests with the refactoredgetOrderQuote
return type. No further changes required here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some minor comments. Looks good overall
crates/quote/src/js_api/mod.rs
Outdated
pub fn to_readable_msg(&self) -> String { | ||
match self { | ||
Self::QuoteError(e) => format!("Quote error: {}", e), | ||
Self::FromHexError(e) => format!("Failed to parse orderbook address: {}", e), | ||
Self::U256ParseError(e) => format!("Failed to parse u256 value: {}", e), | ||
Self::SerdeWasmBindgenError(err) => format!("Data serialization error: {}", err), | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The AI comment doesn't really make sense. Since there is no catch all case, adding new variants will result in a compilation error, which makes it hard to forget
console.log(error); | ||
assert.fail('expected to resolve, but failed'); | ||
} | ||
const result = await doQuoteTargets([target], mockServer.url + '/rpc-url'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Template literals are usually a bit more readable IMO but in this case it doesn't make a difference
Closing this one as the changes in this PR somehow ended up in main and this only changes a lockfile |
Motivation
See issue: rainlanguage/rain.wasm#17
Solution
Checks
By submitting this for review, I'm confirming I've done the following:
[ ] included screenshots (if this involves a front-end change)Summary by CodeRabbit
Refactor
Bug Fixes
Chores