Merged
Conversation
Contributor
## Walkthrough
This set of changes introduces and reorganizes testing for the `rain_orderbook_js_api` crate, particularly focusing on the GUI's token selection functionality. The workspace is updated to include the new crate, and its dependencies are adjusted for conditional compilation. Test assertions are refactored for idiomatic Rust style. The `DotrainOrderGui` struct receives a `Default` implementation, and several internal methods are optimized to reduce unnecessary cloning and improve code clarity. Test modules are reorganized to separate WASM and non-WASM tests, with new integration tests added that utilize HTTP mocking to simulate Ethereum RPC endpoints. The `get_dotrain_hash` method and related calls are updated to use owned `String` types instead of string slices.
## Changes
| File(s) | Change Summary |
|-------------------------------------------------------------------------|----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
| Cargo.toml | Added new workspace dependency `rain_orderbook_js_api` with path `crates/js_api`. |
| crates/js_api/Cargo.toml | Added conditional dev-dependency on `httpmock` for non-WASM targets; retains unconditional dev-dependency on `wasm-bindgen-test`. |
| crates/js_api/src/gui/deposits.rs<br>crates/js_api/src/gui/field_values.rs | Refactored test assertions from `assert_eq!(..., true/false)` to idiomatic `assert!(...)` and `assert!(!...)`. No logic changes. |
| crates/js_api/src/gui/mod.rs | Added `Default` implementation for `DotrainOrderGui`. Modified `generate_dotrain_text` to use owned `String`. Removed commented TODOs. Introduced non-WASM test module with async integration test using HTTP mock server to test token info retrieval and selection logic. |
| crates/js_api/src/gui/order_operations.rs | Simplified code by removing unnecessary cloning, improving iteration, and streamlining error handling logic. No changes to public APIs or overall logic. |
| crates/js_api/src/gui/select_tokens.rs | Reorganized test modules into WASM and non-WASM submodules. Expanded tests for token selection, including a new async integration test using HTTP mock server to simulate Ethereum RPC and validate token saving logic. No changes to core logic or public API. |
| crates/js_api/src/gui/state_management.rs | Changed `get_dotrain_hash` and related calls to use owned `String` instead of `&str`. Improved string emptiness check with idiomatic Rust. |
| crates/js_api/src/lib.rs | Changed conditional compilation for `gui` module to include both WASM and test builds (`#[cfg(any(target_family = "wasm", test))]`). No changes to public API. |
## Sequence Diagram(s)
```mermaid
sequenceDiagram
participant Test as Integration Test (non-WASM)
participant MockServer as HTTP Mock Server
participant Gui as DotrainOrderGui
participant RPC as Simulated Ethereum RPC
Test->>MockServer: Start mock RPC endpoint
Test->>Gui: Load YAML configuration
Test->>Gui: save_select_token(key, address, rpc_url)
Gui->>RPC: Fetch token metadata (symbol, decimals)
RPC-->>Gui: Return token metadata
Gui-->>Test: Return result (token info set or error)
Test->>Gui: Attempt save_select_token with missing key
Gui-->>Test: Return TokenNotFound error
Test->>Gui: Attempt save_select_token with deployment lacking select tokens
Gui-->>Test: Return SelectTokensNotSet errorPossibly related PRs
Suggested reviewers
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 14
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
Cargo.toml(1 hunks)crates/common/src/erc20.rs(2 hunks)crates/js_api/Cargo.toml(1 hunks)crates/js_api/src/gui/deposits.rs(1 hunks)crates/js_api/src/gui/field_values.rs(2 hunks)crates/js_api/src/gui/mod.rs(3 hunks)crates/js_api/src/gui/order_operations.rs(6 hunks)crates/js_api/src/gui/select_tokens.rs(1 hunks)crates/js_api/src/gui/state_management.rs(4 hunks)crates/js_api/src/lib.rs(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
crates/js_api/src/gui/field_values.rs (1)
crates/js_api/src/gui/state_management.rs (1)
is_preset(260-266)
crates/js_api/src/gui/mod.rs (8)
crates/settings/src/yaml/orderbook.rs (1)
new(31-59)crates/settings/src/yaml/dotrain.rs (1)
new(26-50)crates/js_api/src/yaml/mod.rs (2)
new(32-34)to_readable_msg(63-70)crates/settings/src/yaml/context.rs (2)
new(192-199)to_readable_msg(38-49)crates/settings/src/yaml/mod.rs (2)
new(26-26)to_readable_msg(254-333)crates/settings/src/yaml/cache.rs (1)
new(10-15)crates/common/src/dotrain_order/mod.rs (1)
new(150-187)crates/common/src/erc20.rs (4)
token_info(105-162)decimals(51-67)name(69-85)symbol(87-103)
⏰ Context from checks skipped due to timeout of 90000ms (16)
- 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-legal)
- 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-wasm-artifacts)
- GitHub Check: standard-tests (ubuntu-latest, ob-rs-test, true)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-test)
- GitHub Check: standard-tests (ubuntu-latest, rainix-rs-artifacts, true)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-static)
- GitHub Check: test
- GitHub Check: test
- GitHub Check: test
- GitHub Check: git-clean
- GitHub Check: build-tauri (ubuntu-22.04, true)
🔇 Additional comments (14)
crates/js_api/Cargo.toml (1)
36-38: Good addition of the conditional dev-dependency for HTTP mocking.Adding the
httpmockcrate only for non-WASM targets is appropriate since HTTP mocking is typically not compatible with WebAssembly environments. This allows for proper testing of network interactions in native environments while avoiding compilation issues in WASM builds.Cargo.toml (1)
81-83: Appropriate workspace dependency addition.Adding the
rain_orderbook_js_apias a workspace dependency allows other crates within the workspace to depend on it, which is good for code reuse and integration, especially when implementing token network call tests as mentioned in the PR objectives.crates/js_api/src/lib.rs (1)
1-1: Broadened compilation scope to include tests.The conditional compilation has been expanded to include both WebAssembly targets and test builds. This will allow the
guimodule to be compiled and used during testing, even in non-WASM environments.This is a good change that facilitates better test coverage, especially for the new integration tests that use HTTP mocking for token selection functionality.
crates/common/src/erc20.rs (2)
16-16: Import reordering.The
FromStrimport was moved in the import list.
201-392: Comprehensive test coverage for ERC20 interface.Great addition of unit tests for the ERC20 contract interface methods, using HTTP mocking to simulate RPC responses.
The tests thoroughly cover:
- Success and error cases for
decimals(),name(),symbol(), andtoken_info()- Proper ABI encoding/decoding
- Error handling for malformed responses
- Multicall functionality
This significantly improves reliability and maintainability of the token interface code.
crates/js_api/src/gui/order_operations.rs (5)
199-199: Simplified error handling.Removed redundant
Ok()wrapping sinceget_current_deployment()already returns aResult.This improves code readability by avoiding unnecessary nesting of Result types.
262-262: Optimized memory usage by transferring ownership.The token address is now moved instead of cloned, reducing unnecessary memory allocations.
This is a good optimization as it avoids redundant cloning of immutable data.
296-297: Simplified map iteration.The code now uses the more idiomatic
.keys().map(...)instead of extracting keys from key-value pairs.This change makes the code more readable and directly expresses the intent to iterate over keys.
428-429: Improved iteration efficiency inget_vault_ids.The iterators now yield references directly instead of cloning the vault IDs.
This avoids unnecessary cloning of
Option<U256>values, which can be expensive.Also applies to: 438-439
470-490: Refactored pattern matching for clarity and efficiency.Changed from match statement to
if letpattern and improved variable ownership handling to reduce cloning.The refactoring makes the code more maintainable and slightly more efficient by:
- Using a clearer
if letpattern for the specific case- Moving token addresses instead of cloning them
- Following a more consistent structure for the error handling path
crates/js_api/src/gui/state_management.rs (4)
30-32: Changed parameter type from borrowed to owned String.The
get_dotrain_hashfunction now takes ownership of the input string rather than borrowing it.This change is consistent with the actual usage pattern in the codebase, where the function is typically called with owned strings that don't need to be used after the hash calculation.
53-53: More idiomatic string emptiness check.Changed from string comparison with empty string to the more idiomatic
is_empty()method.This is a minor improvement that makes the code more readable and follows Rust best practices.
149-151: Updated function call to match new signature.Changed to pass an owned String to
get_dotrain_hashinstead of a string reference.This change properly aligns with the updated function signature.
176-176: Updated function call to match new signature.Changed to pass an owned String to
get_dotrain_hashinstead of a string reference.This ensures consistency with the updated function signature.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
crates/js_api/src/gui/mod.rs (3)
65-69: DeriveDefaultinstead of hand‑rolling it
You can save a few lines by adding#[derive(Default)]to the struct and removing this manualimpl; it already fulfils the exact same semantics (new()just delegates to the same field defaults).
258-260: Avoid the extra allocation unless Clippy still objects
Calling.to_string()forces an unnecessary heap allocation becauseRainDocument::createalready acceptsimpl Into<String>. Passing the&strdirectly (orinto()) keeps this zero‑copy.
1367-1370: 🧹 Nitpick (assertive)Tighten the HTTP‑mock assertions
Consider making the mock fail loudly when the RPC call count changes:-then.body(...); +then.body(...) + .expect(1); // the code should hit this endpoint exactly onceThis guards against silent regressions if the production code stops calling the RPC (or calls it more than once). While it’s “just a test”, catching such drift early is valuable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
crates/js_api/src/gui/mod.rs(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (16)
- GitHub Check: Deploy-Preview
- 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-rs-artifacts, true)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-artifacts)
- GitHub Check: standard-tests (ubuntu-latest, rainix-wasm-artifacts)
- 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-static)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-test)
- GitHub Check: standard-tests (ubuntu-latest, ob-rs-test, true)
- GitHub Check: test
- GitHub Check: build-tauri (ubuntu-22.04, true)
- GitHub Check: test
- GitHub Check: test
- GitHub Check: git-clean
hardyjosh
reviewed
Apr 28, 2025
| let token = output.token.as_ref().unwrap(); | ||
| let token_info = self.get_token_info(token.key.clone()).await?; | ||
| output_token_infos.insert(token.address.clone(), token_info); | ||
| if let ApprovalCalldataResult::Calldatas(calldatas) = approval_calldata { |
Contributor
There was a problem hiding this comment.
were these modifications required for this PR? i can't see any tests for this file, it would be better if we are making updates that we have accompanying tests, otherwise i get nervous we're introducing a bug
hardyjosh
reviewed
Apr 28, 2025
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (9)
crates/js_api/src/gui/deposits.rs (2)
289-289: Great improvement to the assertion styleUsing
assert!(!has_any_deposit)instead ofassert_eq!(has_any_deposit, false)is more idiomatic in Rust and makes the test more readable.
294-294: Great improvement to the assertion styleUsing
assert!(has_any_deposit)instead ofassert_eq!(has_any_deposit, true)is more idiomatic in Rust and makes the test more readable.crates/js_api/src/gui/field_values.rs (4)
198-198: Excellent improvement to boolean assertion styleUsing
assert!(!field_value.is_preset)instead ofassert_eq!(field_value.is_preset, false)is more idiomatic in Rust and improves test readability.
203-203: Excellent improvement to boolean assertion styleUsing
assert!(field_value.is_preset)instead ofassert_eq!(field_value.is_preset, true)is more idiomatic in Rust and improves test readability.
226-226: Excellent improvement to boolean assertion styleUsing
assert!(!field_values[0].is_preset)instead ofassert_eq!(field_values[0].is_preset, false)follows Rust best practices for boolean assertions.
229-229: Excellent improvement to boolean assertion styleUsing
assert!(field_values[1].is_preset)instead ofassert_eq!(field_values[1].is_preset, true)follows Rust best practices for boolean assertions.crates/js_api/src/gui/mod.rs (1)
65-69:#[derive(Default)]would remove boiler-plate (same remark as before)The new manual
impl Defaultsimply callsnew().
A#[derive(Default)]on the struct would achieve the same thing and keeps the code a tad slimmer.Previous iterations of this PR already discussed this, so feel free to ignore if you still prefer the explicit impl.
crates/js_api/src/gui/select_tokens.rs (2)
170-347: Same earlier feedback: thoughts on adding negative/aggregate assertionsThe WASM test suite still omits a quick check for
are_all_tokens_selected()after the first successful save. This would harden coverage without much code.We already discussed this in a previous review, so treat this as a gentle reminder only.
357-524: Mirror the mock-call expectation & extract the YAML fixtureThis non-WASM test repeats patterns noted earlier:
- Add
.expect(1)to the RPC mock for stronger guarantees.- The standalone YAML fixture duplicates the one in
mod.rs. Extracting a single source (e.g.tests/fixtures/select_token.yaml) avoids future drift.No functional issues – purely maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
crates/js_api/src/gui/deposits.rs(1 hunks)crates/js_api/src/gui/field_values.rs(2 hunks)crates/js_api/src/gui/mod.rs(3 hunks)crates/js_api/src/gui/order_operations.rs(6 hunks)crates/js_api/src/gui/select_tokens.rs(1 hunks)crates/js_api/src/gui/state_management.rs(4 hunks)
🧰 Additional context used
🧠 Learnings (2)
crates/js_api/src/gui/state_management.rs (1)
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1687
File: crates/js_api/src/gui/mod.rs:258-259
Timestamp: 2025-04-22T12:50:39.581Z
Learning: In `DotrainOrderGui::generate_dotrain_text()`, the call to `.to_string()` on `self.dotrain_order.dotrain()` is necessary because there are two different implementations of the `dotrain()` method - one for WASM targets returning a String and one for non-WASM targets returning a &str. The `.to_string()` ensures type compatibility across different compilation targets.
crates/js_api/src/gui/mod.rs (1)
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1687
File: crates/js_api/src/gui/mod.rs:258-259
Timestamp: 2025-04-22T12:50:39.581Z
Learning: In `DotrainOrderGui::generate_dotrain_text()`, the call to `.to_string()` on `self.dotrain_order.dotrain()` is necessary because there are two different implementations of the `dotrain()` method - one for WASM targets returning a String and one for non-WASM targets returning a &str. The `.to_string()` ensures type compatibility across different compilation targets.
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: test
- GitHub Check: build-tauri (ubuntu-22.04, true)
- GitHub Check: standard-tests (ubuntu-latest, rainix-rs-artifacts, true)
- GitHub Check: test
- GitHub Check: git-clean
- GitHub Check: test
- GitHub Check: Deploy-Preview
🔇 Additional comments (1)
crates/js_api/src/gui/mod.rs (1)
258-259:to_string()call is the correct cross-target compromiseBecause
dotrain()returnsStringfor WASM and&strfor non-WASM, cloning into an ownedStringhere is unavoidable.
Good catch in maintaining compile-time portability. Nothing to change.
hardyjosh
approved these changes
Apr 29, 2025
3 tasks
This was referenced Jul 7, 2025
Merged
This was referenced Jul 21, 2025
4 tasks
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Caution
Do not merge before #1685
Motivation
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
Chores
Refactor
New Features
Tests
Style