Implement new wasm macro to base DotrainOrderGui methods#1452
Implement new wasm macro to base DotrainOrderGui methods#1452
Conversation
WalkthroughThis update revises dependency management and error handling across multiple modules. In the root configuration, a dependency is switched from a version specification to a specific version. In the core libraries, a new method is added and import statements are consolidated. The DotrainOrderGui API has several method signatures updated to return simpler types while replacing wrappers with direct values. Test files and UI components have been adjusted to use actual instances of DotrainOrderGui and improved error propagation logic, including the introduction of a utility function for extracting WASM-encoded results. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as UI Component
participant GUI as DotrainOrderGui
participant API as WASM/API
UI->>GUI: Call API methods (e.g., getDeploymentDetails, getTokenInfo)
GUI-->>UI: Return result { value / error }
alt Error exists
UI->>UI: Throw new Error(result.error.msg)
else No error
UI->>UI: Process result.value
end
sequenceDiagram
participant WA as Webapp Route
participant GUI as DotrainOrderGui
WA->>GUI: Instantiate DotrainOrderGui
WA->>GUI: Call chooseDeployment with parameters
GUI-->>WA: Return WasmEncodedResult { value / error }
alt Error in result
WA->>WA: Throw Error(result.error.msg)
else No error
WA->>WA: Proceed with initialization
end
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms (16)
🔇 Additional comments (2)
🪧 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 (
|
| } | ||
|
|
||
| #[wasm_bindgen(js_name = "getGuiConfig")] | ||
| #[wasm_export(js_name = "getGuiConfig", unchecked_return_type = "GuiCfg")] |
There was a problem hiding this comment.
apparently unchecked_return_type isn't necessary when the Result::Ok is the same type. according to @rouzwelt
![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 (1)
crates/js_api/src/gui/mod.rs (1)
166-193: 🧹 Nitpick (assertive)Sequential fetch might warrant concurrency.
Currently, token information is fetched in a for-loop, which may slow performance when handling many tokens. Consider using a concurrency approach (e.g.,
futures::future::join_all) to fetch all tokens simultaneously.- for key in token_keys.iter() { - result.push(self.get_token_info(key.clone()).await?); - } + use futures::future::join_all; + let infos = join_all( + token_keys.into_iter().map(|k| self.get_token_info(k)) + ).await; + for info in infos { + result.push(info?); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (10)
crates/js_api/src/gui/mod.rs(9 hunks)packages/orderbook/test/js_api/gui.test.ts(34 hunks)packages/ui-components/src/__tests__/DeploymentsSection.test.ts(2 hunks)packages/ui-components/src/__tests__/StrategyPage.test.ts(7 hunks)packages/ui-components/src/lib/components/deployment/DeploymentSteps.svelte(6 hunks)packages/ui-components/src/lib/components/deployment/DeploymentsSection.svelte(1 hunks)packages/ui-components/src/lib/components/deployment/SelectToken.svelte(2 hunks)packages/ui-components/src/lib/components/deployment/StrategyPage.svelte(1 hunks)packages/webapp/src/routes/deploy/+layout.ts(1 hunks)packages/webapp/src/routes/deploy/[strategyName]/[deploymentKey]/+layout.ts(1 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
packages/orderbook/test/js_api/gui.test.ts (2)
crates/settings/src/gui.rs (1)
deployments(89-163)crates/js_api/src/gui/order_operations.rs (1)
deployment(149-167)
🪛 Biome (1.9.4)
packages/orderbook/test/js_api/gui.test.ts
[error] 445-445: This let declares a variable that is only assigned once.
'deploymentDetail' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
[error] 536-536: This let declares a variable that is only assigned once.
'gui' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
[error] 620-620: This let declares a variable that is only assigned once.
'gui' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
[error] 792-792: This let declares a variable that is only assigned once.
'gui' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
[error] 914-914: This let declares a variable that is only assigned once.
'result' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
[error] 988-988: This let declares a variable that is only assigned once.
'gui' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
[error] 1141-1141: This let declares a variable that is only assigned once.
'result' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
[error] 1176-1176: This let declares a variable that is only assigned once.
'result' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
[error] 1220-1220: This let declares a variable that is only assigned once.
'result' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
[error] 1262-1262: This let declares a variable that is only assigned once.
'result' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
[error] 1557-1557: This let declares a variable that is only assigned once.
'gui' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
⏰ Context from checks skipped due to timeout of 90000ms (16)
- GitHub Check: test
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-legal)
- GitHub Check: build-tauri (ubuntu-22.04, true)
- 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-wasm-test)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-artifacts)
- GitHub Check: standard-tests (ubuntu-latest, test-js-bindings)
- GitHub Check: test
- GitHub Check: Deploy-Preview
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-static)
- GitHub Check: git-clean
- GitHub Check: standard-tests (ubuntu-latest, rainix-rs-artifacts, true)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-test)
- GitHub Check: standard-tests (ubuntu-latest, ob-rs-test, true)
- GitHub Check: test
🔇 Additional comments (47)
crates/js_api/src/gui/mod.rs (14)
21-24: Imports look appropriate.These additions for
std::collections::{BTreeMap, HashMap}andwasm_bindgen_utilsare valid and necessary for the ensuing code references.
51-65: Constructor clarity.This constructor provides a straightforward way to initialize a
DotrainOrderGuiinstance with default or dummy values. Please verify that returningDotrainOrder::dummy()is intended for production usage or strictly for tests, as it may lead to unexpected behaviors if used in live code.
66-74: Clear and concise deployment keys retrieval.This async method properly constructs and parses deployment keys. The usage of
Resultfor error handling is consistent with Rust best practices.
76-95: Stateful deployment selection.Persisting the new
dotrain_orderandselected_deploymentfields inselfis a neat way to keep the state locally. This approach seems to align with the typical user workflow for selecting deployments.
97-105: Graceful fallback for missing GUI config.The error handling ensures that a missing GUI config is properly surfaced. The method remains concise and clear.
107-121: Deployment retrieval logic.Fetching the current deployment by
self.selected_deploymentis simple and robust. ReturningDeploymentNotFoundfor invalid keys is helpful for debugging.
126-164: Comprehensive token information with fallback.The function comprehensively fetches token details from local config, then falls back to on-chain queries if some fields are missing. This approach is well-structured:
- The default fallback logic is clear.
- Network calls are awaited properly.
No immediate concerns.
195-204: Strategy detail retrieval is straightforward.This method just parses strategy details and returns them. The error handling is consistent with the rest of the file.
206-217: Deployment details in a HashMap.Directly returning a
HashMapfor deployment details is efficient, removing the need for intermediate structs. Implementation is clean.
219-232: Single deployment detail lookup.Extracting the requested deployment detail from the HashMap is a logical reuse of
get_deployment_details. Error messaging is user-friendly.
234-247: Fetching the current deployment detail.This reuses the same parsing method for deployment details, then filters by
self.selected_deployment. Code is succinct and coherent.
249-259: Dotrain text generation.Concatenating the frontmatter and the Rain document body is a neat approach for rebuilding the text. Implementation is concise.
261-270: Rainlang composition with scenario binding.Stitching together the final Rainlang expression with
update_scenario_bindings()is a good pattern. The method’s flow is clear.
357-365: WasmEncodedError conversion.This
impl From<GuiError>properly encapsulates the error details in a WASM-friendly structure.packages/webapp/src/routes/deploy/[strategyName]/[deploymentKey]/+layout.ts (1)
12-16: Robust error handling on detail retrieval.Performing a result check before destructuring safeguards against undefined data. Ensure all client-facing errors are clearly communicated in the UI or logs if further troubleshooting is needed.
packages/ui-components/src/lib/components/deployment/StrategyPage.svelte (1)
29-33: Improved resilience when fetching strategy details.By verifying
result.errorand throwing an exception, you prevent further processing on invalid data. This helps maintain UI integrity.packages/ui-components/src/lib/components/deployment/SelectToken.svelte (2)
22-26: Improved error handling for getTokenInfo call in onMount lifecycle.The implementation now properly checks for errors in the result from gui.getTokenInfo before assigning values, ensuring that errors are explicitly handled rather than potentially causing silent failures.
38-42: Consistent error handling pattern implemented for token info retrieval.This follows the same improved pattern as in onMount, checking for errors explicitly before using the result value, creating a consistent approach to error handling throughout the component.
packages/ui-components/src/lib/components/deployment/DeploymentsSection.svelte (1)
8-24: Enhanced error handling for deployment details.The component now properly handles potential errors from DotrainOrderGui.getDeploymentDetails by checking for result.error and displaying user-friendly error messages. It also ensures deployment tiles are only rendered when result.value exists, improving reliability.
packages/webapp/src/routes/deploy/+layout.ts (1)
18-22:Details
❓ Verification inconclusive
Improved error handling pattern, but with a potential issue in data handling.
The code correctly implements error checking for the result from getStrategyDetails, but there appears to be an issue with the resolved data handling. The function returns the strategy details object directly, but the validStrategies array (defined on line 12) is never populated. Successful strategies should likely be added to this array.
Please verify if validStrategies should be populated with the returned strategy data from lines 18-22. Consider modifying to:
-return { ...registryDotrain, details: result.value }; +validStrategies.push({ ...registryDotrain, details: result.value }); +return;
Potential Data Handling Oversight in Error Handling Flow
The revised error handling with
DotrainOrderGui.getStrategyDetailsis now more robust. However, it's worth noting that although the code correctly throws an error when one is present, the returned strategy details are not added to thevalidStrategiesarray (declared on line 12). Please verify whether the intent is to populatevalidStrategieswith these details. If so, consider updating the code as follows:- return { ...registryDotrain, details: result.value }; + validStrategies.push({ ...registryDotrain, details: result.value }); + return;packages/orderbook/test/js_api/gui.test.ts (6)
354-364: Well-designed utility function for centralizing error handling.The new
extractWasmEncodedDatafunction effectively centralizes error handling logic for all WASM-encoded results, improving code maintainability and consistency throughout the test suite. It properly fails tests when errors occur and correctly extracts values when operations succeed.
366-372: Consistent error handling applied to getDeploymentKeys.The extractWasmEncodedData function is correctly used to process results from getDeploymentKeys, ensuring that any errors are properly caught and providing type safety for the returned data.
383-389: Improved instantiation and result handling for DotrainOrderGui.The code correctly uses the actual DotrainOrderGui class and properly handles errors when calling chooseDeployment and getGuiConfig through the extractWasmEncodedData function.
1252-1254: New method for removing field values.The addition of the removeFieldValue method expands the API capabilities and is correctly verified in the test by checking that the getAllFieldValues returns an empty array after removal.
1622-1629: Improved error handling for token info retrieval.The code now properly checks for and handles errors when retrieving token information, ensuring tests can verify both the success and error cases appropriately.
1728-1731: Consistent error handling for removed tokens.The test properly verifies that removing a select token results in the expected error when attempting to get token information, ensuring that the error handling behavior works as expected.
packages/ui-components/src/__tests__/DeploymentsSection.test.ts (4)
2-2: LGTM: Improved type importsThe import statement now correctly includes
type Mockfromvitest, which follows best practices for TypeScript by being explicit about imported types.
13-19: LGTM: Well-structured mock deployment dataGood implementation of the mock deployment data structure with clear key-value pairs in the Map.
20-20: LGTM: Updated mock to align with new API return structureThe mock now correctly returns an object with a
valueproperty containing the deployments Map, which aligns with the updated DotrainOrderGui API that returns a Result wrapper instead of direct values.
37-39: LGTM: Updated error handling approachThe error handling mock now correctly follows the new pattern of returning an object with an
errorproperty instead of rejecting the promise, which aligns with the new WASM-based error handling approach.packages/ui-components/src/__tests__/StrategyPage.test.ts (9)
4-4: LGTM: Improved type importsThe import statement now correctly includes
type Mockfromvitest, which follows best practices for TypeScript by being explicit about imported types.
24-29: LGTM: Updated mock structure to match new API return typeThe mock strategy details now correctly wraps the data in a
valueproperty, aligning with the updated DotrainOrderGui API that returns a Result wrapper.
30-30: LGTM: Consistent mock implementationThe mock for
getStrategyDetailscorrectly resolves with the updated structure that includes avalueproperty.
47-52: LGTM: Consistent mock data structureThe mock strategy details structure is consistent with the previous test case, maintaining the same pattern throughout the file.
58-58: LGTM: Updated mock implementationThe mock for
getStrategyDetailscontinues to use the consistent pattern, resolving with an object containing avalueproperty.
76-86: LGTM: Clear separation of mocks and improved error handlingThe code now clearly separates the fetch mock and DotrainOrderGui mock implementations. The error structure using an object with an
error.msgproperty aligns with the WASM error handling approach.
102-108: LGTM: Consistent mock data structureThe mock structure remains consistent with the rest of the file, wrapping the strategy details in a
valueproperty.
127-132: LGTM: Maintained consistency across all test casesThe mock data structure remains consistent across all test cases in the file, ensuring that all tests align with the updated API return types.
Also applies to: 158-163
139-139: LGTM: Consistent mocking approachThe mocking approach for
getStrategyDetailsis consistent throughout the file, always returning a structure with avalueproperty.Also applies to: 170-170
packages/ui-components/src/lib/components/deployment/DeploymentSteps.svelte (8)
7-7: LGTM: Updated import to include TokenInfo typeThe import statement now correctly includes the
TokenInfotype, which is necessary for the updated variable type declaration.
13-14: LGTM: Updated type importsThe code now correctly imports
OrderIOCfgas a type, following TypeScript best practices.
51-51: LGTM: Updated variable type to match new API return typeThe type of
allTokenInfoshas been updated fromAllTokenInfostoTokenInfo[], aligning with the changes in the API return types. This promotes type safety and clarity.
80-85: LGTM: Improved error handling for getCurrentDeploymentThe code now correctly checks for error results before accessing the deployment data, throwing an appropriate error with the message from the result. This enhances robustness and error propagation.
95-99: LGTM: Consistent error handling for token inputsThe error handling approach for
getAllTokenInputsfollows the same pattern as other methods, checking for errors before accessing the data.
107-111: LGTM: Consistent error handling for token outputsThe error handling approach for
getAllTokenOutputscontinues the consistent pattern, ensuring robustness throughout the component.
142-146: LGTM: Robust error handling for token info retrievalThe code now properly checks for errors in the result from
getAllTokenInfosbefore proceeding, maintaining the consistent error handling pattern.
213-217: LGTM: Consistent error handling in areAllTokensSelectedThe error handling in the
areAllTokensSelectedmethod follows the same robust pattern, ensuring that errors are properly caught and propagated.
| let deploymentDetail = extractWasmEncodedData<NameAndDescriptionCfg>( | ||
| gui.getCurrentDeploymentDetails() | ||
| ); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider using const instead of let for variables only assigned once.
Multiple instances of variables declared with let that are never reassigned could use const instead, including deploymentDetail here and many others throughout the file as indicated by static analysis.
-let deploymentDetail = extractWasmEncodedData<NameAndDescriptionCfg>(
+const deploymentDetail = extractWasmEncodedData<NameAndDescriptionCfg>(This applies to similar instances at lines 536, 620, 792, 914, 988, 1141, 1176, 1220, 1262, and 1557.
📝 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 deploymentDetail = extractWasmEncodedData<NameAndDescriptionCfg>( | |
| gui.getCurrentDeploymentDetails() | |
| ); | |
| const deploymentDetail = extractWasmEncodedData<NameAndDescriptionCfg>( | |
| gui.getCurrentDeploymentDetails() | |
| ); |
🧰 Tools
🪛 Biome (1.9.4)
[error] 445-445: This let declares a variable that is only assigned once.
'deploymentDetail' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
There was a problem hiding this comment.
is this change supposed to be in this PR?
2 participants
Motivation
This PR implements our new wasm macro to the base DotrainOrderGui methods.
Solution
Checks
By submitting this for review, I'm confirming I've done the following:
Summary by CodeRabbit
Chores
Refactor
Tests