Add rust tests for gui state management#1682
Conversation
WalkthroughThis set of changes primarily updates internal testing and struct visibility in the GUI components of the JavaScript API. It adds new test helpers and comprehensive wasm-bindgen tests for select token and state management logic in Rust, modifies the visibility of a struct field, and expands test data in a YAML fixture. Several previously exported methods for state clearing and preset checks are removed from the Rust GUI implementation, and corresponding tests in the TypeScript suite are deleted. Minor test mocks related to a removed method are also cleaned up. No production logic is altered outside of test-only code and struct field visibility. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test Module
participant DotrainOrderGui as DotrainOrderGui
participant TokenCfg as TokenCfg
participant YAML as YAML Config
Test->>DotrainOrderGui: add_record_to_yaml(...)
DotrainOrderGui->>TokenCfg: add_record(...)
TokenCfg->>YAML: Update token records
YAML-->>TokenCfg: Confirmation
TokenCfg-->>DotrainOrderGui: Result
DotrainOrderGui-->>Test: Done
Test->>DotrainOrderGui: remove_record_from_yaml(...)
DotrainOrderGui->>TokenCfg: remove_record(...)
TokenCfg->>YAML: Remove token record
YAML-->>TokenCfg: Confirmation
TokenCfg-->>DotrainOrderGui: Result
DotrainOrderGui-->>Test: Done
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
💤 Files with no reviewable changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (16)
🪧 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: 1
🔭 Outside diff range comments (4)
crates/js_api/src/gui/select_tokens.rs (2)
76-81:⚠️ Potential issuePotential data‑loss if the on‑chain lookup fails
save_select_tokenremoves an existing token record before the async call to the RPC/ERC20::token_info.
If that call errors (network hic‑cup, invalid address, CORS, …), the original record is already deleted, leaving the YAML in an inconsistent state.Move the removal after the on‑chain fetch succeeds, or wrap both operations in a rollback‑capable routine.
- if TokenCfg::parse_from_yaml(self.dotrain_order.dotrain_yaml().documents, &key, None) - .is_ok() - { - TokenCfg::remove_record_from_yaml(self.dotrain_order.orderbook_yaml().documents, &key)?; - } - - let address = Address::from_str(&address)?; + let address = Address::from_str(&address)?; … + // Safe to replace now that we have verified on‑chain info + if TokenCfg::parse_from_yaml(self.dotrain_order.dotrain_yaml().documents, &key, None) + .is_ok() + { + TokenCfg::remove_record_from_yaml( + self.dotrain_order.orderbook_yaml().documents, + &key, + )?; + }
61-108: 🧹 Nitpick (assertive)Long‑running network call on the critical path
save_select_tokenperforms an unconditionalerc20.token_info(None).await?, which can add noticeable latency to the UI.
Consider:
- Caching previously fetched token‑info by
(chainId,address).- Allowing the caller to supply known decimals/name/symbol and skipping the RPC in that case.
crates/js_api/src/gui/state_management.rs (2)
244-249: 🛠️ Refactor suggestionDeserialization does not notify the front‑end
After loading a saved state,
deserialize_stateupdatesselfbut never triggersexecute_state_update_callback.
Any connected UI will remain stale until the next manual change.self.state_update_callback = dotrain_order_gui.state_update_callback; +// Notify listeners that the state has been refreshed +self.execute_state_update_callback()?; Ok(())
32-36: 🧹 Nitpick (assertive)Hash stability concern
get_dotrain_hashhashes the bincode representation of the dotrain string.
Bincode’s output can change with crate version or compile‑time features, producing different hashes for identical strings across builds.
Hashing the raw UTF‑8 bytes (or a canonicalised YAML) is safer and reproducible.-let dotrain_bytes = bincode::serialize(dotrain)?; -let hash = Sha256::digest(&dotrain_bytes); +let hash = Sha256::digest(dotrain.as_bytes());
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
crates/js_api/src/gui/mod.rs(1 hunks)crates/js_api/src/gui/order_operations.rs(1 hunks)crates/js_api/src/gui/select_tokens.rs(1 hunks)crates/js_api/src/gui/state_management.rs(2 hunks)packages/orderbook/test/js_api/gui.test.ts(0 hunks)
💤 Files with no reviewable changes (1)
- packages/orderbook/test/js_api/gui.test.ts
⏰ Context from checks skipped due to timeout of 90000ms (16)
- GitHub Check: standard-tests (ubuntu-latest, rainix-wasm-artifacts)
- GitHub Check: test
- GitHub Check: standard-tests (ubuntu-latest, test-js-bindings)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-static)
- GitHub Check: git-clean
- GitHub Check: standard-tests (ubuntu-latest, rainix-rs-static)
- GitHub Check: standard-tests (ubuntu-latest, rainix-wasm-test)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-test)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-legal)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-artifacts)
- GitHub Check: Deploy-Preview
- 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: test
🔇 Additional comments (1)
crates/js_api/src/gui/mod.rs (1)
543-546: YAML addition looks good
token4is introduced in theselect‑tokenslist with correct indentation and the same schema as the other entries.
No further action required.
2 participants
Caution
Do not merge before #1681
Motivation
See issue: #1634
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
New Features
Bug Fixes
Refactor
Tests