Implement wasm macro on DotrainOrderGui order operations methods#1458
Implement wasm macro on DotrainOrderGui order operations methods#1458
Conversation
WalkthroughThe changes modify the API and tests across multiple modules. In the GUI code, several wrapper structs have been removed, and function signatures updated to return results directly. The WASM export attribute was changed from Changes
Sequence Diagram(s)sequenceDiagram
participant UI as TokenIOInput.svelte
participant GUI as DotrainOrderGui
UI->>GUI: getVaultIds()
GUI-->>UI: Return Result (vault IDs or error)
alt Error exists
UI->>UI: Set error variable and exit early
else No error
UI->>UI: Assign vaultIds and proceed
end
sequenceDiagram
participant UI as getDeploymentTransactionArgs.ts
participant GUI as DotrainOrderGui
UI->>GUI: getDeploymentTransactionArgs(address)
GUI-->>UI: Return Result (value or error)
alt Error present
UI->>UI: Throw error with returned message
else
UI->>UI: Destructure approvals, calldata, etc.
end
Assessment against linked issues
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms (16)
🔇 Additional comments (1)
✨ Finishing Touches
🪧 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: 0
🧹 Nitpick comments (4)
packages/ui-components/src/lib/components/deployment/DepositInput.svelte (3)
27-31: Good error handling pattern but consider consistency with error presentation.While the error checking is well-implemented, there's an inconsistency in how errors are handled throughout the component. In this method, errors are thrown directly, but in
getTokenSymbol, errors are caught and displayed to the user. Consider adopting a consistent approach for all GUI operations to improve user experience.You could catch errors and display them to the user consistently, similar to how it's done in
getTokenSymbol:const setCurrentDeposit = async () => { try { const deposits = gui.getDeposits(); if (deposits.error) { - throw new Error(deposits.error.msg); + error = deposits.error.msg; + currentDeposit = undefined; + return; } currentDeposit = deposits.value.find((d) => d.token === deposit.token?.key); inputValue = currentDeposit?.amount || ''; } catch { currentDeposit = undefined; } };
60-64: Consider consistent error handling with user feedback.Similar to earlier comment, consider showing errors to the user instead of throwing exceptions for a more consistent experience.
75-79: Maintain consistent error handling approach.Same recommendation as the previous error handling sections - consider using the error display pattern from
getTokenSymbolhere as well.crates/js_api/src/gui/mod.rs (1)
54-64: Question the placeholder constructor.
CallingDotrainOrder::dummy()may mask configuration issues if the user forgets to load a real Dotrain order. If this is strictly for test or default instantiation, this is fine. Otherwise, ensure that callers are aware it returns an uninitialized order.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
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 (31)
Cargo.toml(2 hunks)crates/common/src/dotrain_order/mod.rs(2 hunks)crates/js_api/src/gui/deposits.rs(5 hunks)crates/js_api/src/gui/field_values.rs(6 hunks)crates/js_api/src/gui/mod.rs(8 hunks)crates/js_api/src/gui/order_operations.rs(10 hunks)packages/orderbook/test/js_api/gui.test.ts(48 hunks)packages/ui-components/src/__tests__/DeploymentSteps.test.ts(8 hunks)packages/ui-components/src/__tests__/DeploymentsSection.test.ts(3 hunks)packages/ui-components/src/__tests__/DepositInput.test.ts(2 hunks)packages/ui-components/src/__tests__/FieldDefinitionInput.test.ts(11 hunks)packages/ui-components/src/__tests__/StrategyPage.test.ts(10 hunks)packages/ui-components/src/__tests__/TokenIOInput.test.ts(4 hunks)packages/ui-components/src/__tests__/getDeploymentTransactionArgs.test.ts(2 hunks)packages/ui-components/src/lib/components/deployment/ComposedRainlangModal.svelte(1 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/DepositInput.svelte(4 hunks)packages/ui-components/src/lib/components/deployment/FieldDefinitionInput.svelte(3 hunks)packages/ui-components/src/lib/components/deployment/SelectToken.svelte(2 hunks)packages/ui-components/src/lib/components/deployment/StrategyPage.svelte(1 hunks)packages/ui-components/src/lib/components/deployment/TokenIOInput.svelte(2 hunks)packages/ui-components/src/lib/components/deployment/getDeploymentTransactionArgs.ts(3 hunks)packages/ui-components/src/lib/stores/transactionStore.ts(1 hunks)packages/ui-components/src/lib/types/transaction.ts(1 hunks)packages/ui-components/test-setup.ts(2 hunks)packages/webapp/src/__tests__/handleGuiInitialization.test.ts(1 hunks)packages/webapp/src/lib/services/handleGuiInitialization.ts(3 hunks)packages/webapp/src/routes/deploy/+layout.ts(1 hunks)packages/webapp/src/routes/deploy/[strategyName]/[deploymentKey]/+layout.ts(1 hunks)packages/webapp/test-setup.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (16)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-legal)
- GitHub Check: standard-tests (ubuntu-latest, test-js-bindings)
- GitHub Check: standard-tests (ubuntu-latest, rainix-wasm-test)
- 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-artifacts)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-static)
- 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, ob-rs-test, true)
- GitHub Check: test
- GitHub Check: build-tauri (ubuntu-22.04, true)
- GitHub Check: test
- GitHub Check: git-clean
- GitHub Check: test
- GitHub Check: Deploy-Preview
🔇 Additional comments (124)
packages/ui-components/test-setup.ts (2)
15-19: Improved formatting enhances readabilityThe restructuring of the
pageproperty with each field on its own line improves code readability while maintaining functionality.
56-87: Comprehensive mock implementation for DotrainOrderGuiThis thorough mock setup for the
DotrainOrderGuiclass includes all necessary static and prototype methods that will be used in tests. The approach ensures consistent test behavior and allows for proper testing of components that depend on these methods.The setup aligns well with the PR objective of implementing wasm macro on DotrainOrderGui order operations methods.
packages/webapp/test-setup.ts (1)
11-20: Appropriate mock implementation for DotrainOrderGuiThe implementation correctly mocks the DotrainOrderGui class with its essential methods for testing purposes. This is a simpler mock compared to the one in ui-components, focusing only on the methods needed for webapp tests.
Two methods are specifically mocked:
deserializeStateas a static methodchooseDeploymentas a prototype methodThis approach maintains test isolation while providing the necessary functionality for tests.
packages/ui-components/src/lib/components/deployment/DeploymentsSection.svelte (1)
13-17: Improved error handling with explicit checksThe changes enhance the error handling by explicitly checking for errors in the API response before processing the deployment details. This pattern is more robust as it properly handles the new error-returning structure from the WASM implementation.
Instead of potentially consuming invalid data, the code now properly throws an error with the specific message when the API call fails, allowing for better error reporting and debugging.
packages/webapp/src/routes/deploy/[strategyName]/[deploymentKey]/+layout.ts (1)
12-16: Enhanced error handling for deployment detailsThe implementation now properly checks for errors in the API response before attempting to destructure the result. This improvement ensures that errors from the WASM layer are properly propagated to the UI layer with appropriate error messages.
This pattern makes the code more robust and consistent with the error handling approach used throughout the application.
packages/ui-components/src/lib/types/transaction.ts (1)
7-7:Details
✅ Verification successful
Type simplification in DeploymentArgs.
The type of
deploymentCalldatahas been simplified fromDepositAndAddOrderCalldataResulttostring. This aligns with the overall PR goal of streamlining transaction handling.
🏁 Script executed:
#!/bin/bash # Verify that all usage sites of DeploymentArgs have been updated to handle deploymentCalldata as a string # instead of DepositAndAddOrderCalldataResult # Check for usages of DeploymentArgs and deploymentCalldata rg -A 2 -B 2 "DeploymentArgs" --type ts rg -A 2 -B 2 "deploymentCalldata" --type tsLength of output: 10417
Verified Type Simplification: DeploymentArgs Now Uses String for deploymentCalldata
The change in
packages/ui-components/src/lib/types/transaction.tshas been confirmed. The grep output verifies that all usage sites, including tests and store modules, now treatdeploymentCalldataas a string instead of the previousDepositAndAddOrderCalldataResult. No further modifications are required.packages/ui-components/src/lib/components/deployment/ComposedRainlangModal.svelte (1)
14-19: Improved error handling in generateRainlang.The function now properly checks for errors in the result object and handles them explicitly. This is a more robust approach that provides clearer error messages.
The changes follow a consistent pattern of error-checking that appears to be implemented throughout the codebase, improving overall reliability.
packages/ui-components/src/lib/components/deployment/TokenIOInput.svelte (2)
21-26: Improved error handling for getVaultIds.Error handling has been enhanced by checking for errors in the result object and displaying them to the user, preventing silent failures.
37-42: Enhanced error handling for getTokenInfo.Similar to other changes in this PR, error handling has been improved by explicitly checking for errors in the result object before proceeding with the successful path.
packages/webapp/src/routes/deploy/+layout.ts (1)
13-17: Improved error handling for getStrategyDetails.The code now properly checks for errors in the result object and throws an error with a descriptive message if one is found. This is consistent with the error handling pattern implemented throughout the codebase.
packages/ui-components/src/lib/components/deployment/FieldDefinitionInput.svelte (3)
23-27: Good implementation of consistent error handling.The addition of error checking for GUI operations improves robustness and prevents silent failures.
41-45: Appropriate error handling for user interactions.The error checking pattern properly validates the response from the GUI operation, maintaining consistency with the error handling approach used throughout the component.
55-59: Consistent error handling implementation.The same error handling pattern is correctly applied here, completing the robust error checking across all GUI interactions in the component.
packages/ui-components/src/lib/components/deployment/DepositInput.svelte (1)
41-45: Well-implemented error handling with user feedback.This approach properly checks for errors and provides user feedback through the UI, which is good practice.
packages/ui-components/src/__tests__/getDeploymentTransactionArgs.test.ts (4)
16-17: Improved testing approach using real instances.Good change from using a mock object to instantiating the actual class, which makes the tests more realistic.
21-30: Well-structured mock implementation for the GUI instance.The approach of instantiating the real class and then mocking specific methods on the prototype is a better testing practice than creating a complete mock object. The return structure with a
valueproperty properly matches the new error handling pattern in the components.
38-44: Consistent mocking approach for all GUI methods.The mocking structure is consistently applied across multiple methods, properly wrapping return values in the expected format with the
valueproperty.
57-59: Tests properly updated to use the real instance.The test cases have been correctly updated to use the instantiated class instead of the mock object.
crates/common/src/dotrain_order/mod.rs (2)
12-12: Appropriate import for default document function.The import has been correctly updated to include the
default_documentfunction used in the new dummy method.
127-135: Useful addition of a default constructor method.The
dummy()method provides a convenient way to create default instances ofDotrainOrder. This is particularly useful for testing and initialization in the wasm context.packages/ui-components/src/__tests__/DeploymentsSection.test.ts (3)
2-2: Type imports for better type checking.Good addition of the
type Mockimport to properly type-cast the mocked function.
19-19: Updated mock structure to match the new API return type.The mock now returns an object with a
valueproperty containing the deployments, which aligns with the updated API contract ofDotrainOrderGui.getDeploymentDetails.
37-39: Error handling updated to match the new wasm API pattern.The error handling approach has been updated from a Promise rejection to returning an object with an
errorproperty. This change aligns with the WebAssembly API pattern where methods return structured responses containing either value or error.packages/ui-components/src/lib/components/deployment/StrategyPage.svelte (1)
30-34: Enhanced error handling for the WebAssembly API.The code now properly checks for errors in the result returned by
DotrainOrderGui.getStrategyDetails, extracting the error message when present and throwing it as an Error. This improves error handling and aligns with the WebAssembly API pattern.packages/ui-components/src/lib/components/deployment/SelectToken.svelte (2)
22-26: Improved error handling for WebAssembly API calls.The code now properly checks for errors in the result returned by
gui.getTokenInfo(), extracting the error message when present and throwing it as an Error. This is a necessary change to handle the updated API return structure.
38-42: Consistent error handling pattern applied.The same error handling pattern has been applied to the
getInfoForSelectedTokenfunction, ensuring consistent error handling throughout the component. This change maintains consistency with the WebAssembly API pattern.packages/ui-components/src/__tests__/TokenIOInput.test.ts (5)
5-5: Updated imports and reduced export scope.The
DotrainOrderGuiimport has been added, and theTokenIOInputComponentPropstype is now local rather than exported. This is a good practice to limit the public API surface area when types are only needed within the test file.Also applies to: 7-7
10-13: Switched to using actual DotrainOrderGui instance.The test now uses an actual instance of
DotrainOrderGuiinstead of a mock object, which better represents real-world usage and ensures that the tests validate against the actual API structure.
22-27: Updated mock structure to match the new API return type.The
mockTokenInfostructure now includes avalueproperty that wraps the token information, aligning with the updated WebAssembly API pattern.
29-64: Comprehensive test setup with DotrainOrderGui instance.The
beforeEachsetup has been significantly improved:
- It now creates a real
DotrainOrderGuiinstance- Mocks are properly initialized and cleared
- Method mocks now follow the WebAssembly return structure with
valueproperty- Test props are consistently setup with the instance
This change ensures that tests accurately reflect the component's interaction with the actual API.
88-88: Updated assertions to target the DotrainOrderGui instance.The assertions are now correctly targeting method calls on the
guiInstancerather than a mock object, which properly validates that the component interacts with theDotrainOrderGuiinstance as expected.Also applies to: 95-95, 125-125
packages/ui-components/src/__tests__/DepositInput.test.ts (5)
6-6: Good import addition for DotrainOrderGui class.The explicit import of
DotrainOrderGuiinstead of using a mock object aligns with the PR's objective to implement wasm macro on DotrainOrderGui methods.
12-12: Improved test setup with an actual DotrainOrderGui instance.Transitioning from a mock object to using a real
DotrainOrderGuiinstance is a better testing approach. This allows for direct testing against the actual class implementation with the new wasm macro functionality.Also applies to: 20-22
24-29: Proper method mocking on the prototype.Mocking methods directly on the
DotrainOrderGui.prototypeis cleaner than using a separate mock object. This approach ensures your tests properly reflect the behavior of the actual implementation.
33-39: Well-structured test mock implementation.The mock implementation of
getTokenInfowith a properly structured promise response aligns with how the actual wasm-exported method would behave.
43-43: Consistent usage of the DotrainOrderGui instance.All test cases now consistently use the
guiInstanceproperty, ensuring uniform interaction with the wasm-exported methods across the tests.Also applies to: 58-58, 68-68, 72-72, 80-80
Cargo.toml (2)
79-80: Formatting improvement for rain_orderbook_math dependency.The addition of a newline at the end of the file is a good practice for source files and improves consistency.
56-56:Details
✅ Verification successful
Updated dependency to use specific wasm-bindgen-utils commit.
The change from a version specification to a specific Git repository reference ensures compatibility with the wasm macro implementation. This is a good practice when depending on features from a specific commit.
🏁 Script executed:
#!/bin/bash # Check if the referenced commit exists and is accessible curl -s -o /dev/null -w "%{http_code}" https://github.com/rainlanguage/rain.wasm/commit/939b59f7b52798e04a988cef435030d78eb11e91Length of output: 134
Approved: wasm-bindgen-utils dependency update verified
The change in
Cargo.tomlon line 56—updating the dependency to a specific commit (939b59f7b52798e04a988cef435030d78eb11e91) from therain.wasmrepository—has been confirmed. The commit is accessible (HTTP 200), ensuring compatibility with the wasm macro implementation.packages/ui-components/src/__tests__/FieldDefinitionInput.test.ts (4)
8-8: Refactored to use actual DotrainOrderGui instance.Similar to other test file changes, this properly instantiates a real
DotrainOrderGuiobject and mocks its methods on the prototype. This is a better approach for testing wasm-exported methods.Also applies to: 23-28
35-36: Consistent usage of guiInstance in component props.All test cases now consistently pass the
guiInstanceto the component under test, ensuring uniform behavior across test scenarios.Also applies to: 47-48, 59-60, 76-77, 99-100, 114-115, 143-144
65-68: Updated expectations to validate interactions with DotrainOrderGui instance.The test assertions now correctly verify that methods are called on the actual
guiInstanceobject, which better represents how the component would interact with the wasm-exported methods in real usage.Also applies to: 83-86, 123-126, 152-155
129-134: Well-structured mock response format.The mock implementation correctly reflects the expected structure of responses from wasm-exported methods, making the tests more accurate and reliable.
packages/webapp/src/__tests__/handleGuiInitialization.test.ts (7)
1-4: Updated imports for better type checking.Added the
WasmEncodedResulttype import to properly type the mock responses, which aligns with the wasm macro implementation requirements.
7-7: Improved test setup with an actual DotrainOrderGui instance.Creating a real instance of
DotrainOrderGuiin the test setup provides a more accurate representation of the runtime behavior with wasm-exported methods.Also applies to: 14-15
18-18: Correct mocking of static method.The test now correctly mocks the static
deserializeStatemethod on theDotrainOrderGuiclass itself rather than on an instance, which matches how it would be used in actual code.Also applies to: 23-28
32-35: Properly typed mock implementation.The mock rejection and resolution now use proper typing with
WasmEncodedResult<void>, ensuring type compatibility with the wasm-exported methods.
39-45: Updated result validation for actual instance.The test now correctly validates that the result contains the actual
guiInstanceand verifies method calls on both the class and instance, which better represents the real behavior.
49-49: Consistent approach for instance method mocking.The instance method
chooseDeploymentis properly mocked onguiInstancedirectly, and the assertions correctly verify its usage, maintaining consistency with how wasm-exported methods would be called.Also applies to: 53-59
63-63: Proper error handling test.The test correctly mocks a rejection from the
chooseDeploymentmethod to verify the error handling behavior of the function under test, which is important for robust error handling with wasm-exported methods.packages/webapp/src/lib/services/handleGuiInitialization.ts (4)
1-1: Good addition of WasmEncodedResult type importThis import supports the new error handling pattern, providing better type safety for function results.
10-11: Improved GUI initialization patternChanging from a null initialization to direct instantiation of DotrainOrderGui creates a more consistent initialization flow and enables instance method usage.
21-24: Enhanced error handling for chooseDeploymentThe new error handling pattern with WasmEncodedResult provides more robust error management by explicitly checking for errors in the result.
27-30: Consistent implementation of error handling patternThis applies the same error handling pattern as the previous block, maintaining consistency throughout the function.
packages/ui-components/src/lib/stores/transactionStore.ts (1)
56-56: Simplified type for deploymentCalldataThe type change from a complex object to a string suggests that serialization/deserialization is now handled internally by the DotrainOrderGui WASM implementation, which is a cleaner separation of concerns.
packages/ui-components/src/__tests__/StrategyPage.test.ts (12)
4-4: Improved typing with explicit Mock importAdding the Mock type from vitest provides better type safety when mocking functions.
22-28: Updated mock structure for wasm encoded resultsThe new structure with a nested 'value' object properly mirrors the WasmEncodedResult pattern used throughout the codebase.
29-29: Type-safe mocking approachUsing type assertion with Mock ensures type safety when mocking the getStrategyDetails method.
45-51: Consistent mock structureThis mock maintains the same structure pattern as the previous one, ensuring consistency throughout the tests.
59-59: Consistent type assertion for mockingThe type assertion approach is consistently applied here as well.
83-87: Updated error handling in test mocksThe mock now correctly returns an object with an error property containing a message, matching the WasmEncodedResult pattern for error cases.
105-111: Consistent mock structure in fetch failure testThe mock structure with the value property is consistent with the other tests.
122-123: Updated error message expectationsTest expectations have been updated to match the new error messages displayed to the user.
129-135: Consistent mock structure for markdown URL testThis maintains the same mock structure pattern as the other tests.
150-150: Consistent mocking approachThe type assertion approach is consistently applied across all tests.
171-176: Consistent mock structure in fallback testThe mock maintains the same structure with a nested value object.
188-188: Type-safe mocking consistently appliedThe type assertion with Mock is consistently used throughout all tests.
packages/ui-components/src/lib/components/deployment/getDeploymentTransactionArgs.ts (3)
3-3: Updated import for DotrainOrderGuiThe import is now properly typed as a type import, which is more accurate.
16-16: Simplified type for deploymentCalldataChanging the type from a complex object to a string aligns with the changes in transactionStore.ts and suggests that serialization is now handled by the WASM implementation.
34-38: Robust error handling implementationThe new implementation properly captures the result from gui.getDeploymentTransactionArgs and checks for errors before proceeding. This pattern is consistent with the WASM result handling seen throughout the codebase.
packages/orderbook/test/js_api/gui.test.ts (5)
353-363: Great addition of error handling utility function!This new
extractWasmEncodedDatafunction provides a consistent way to handle errors and extract values from WebAssembly encoded results throughout the tests. It properly fails assertions when errors are detected and handles undefined values correctly.
365-370: Improved error handling for deployment key retrievalThe code now properly checks for errors when retrieving deployment keys, aligning with the more robust error handling pattern established by the
extractWasmEncodedDatautility function.
382-387: Enhanced class instantiation and error handlingThe code now explicitly creates a new DotrainOrderGui instance and properly handles potential errors during initialization and configuration retrieval. This is more robust and aligns with the overall error handling improvements.
407-413: Improved error handling for strategy detailsError checking has been properly implemented for the strategy details retrieval, ensuring that any errors are caught before proceeding with assertions.
454-463: Better token info retrieval with error handlingThe refactored approach for retrieving token information properly checks for errors at each step, providing more resilient test behavior when testing token-related functionality.
packages/ui-components/src/lib/components/deployment/DeploymentSteps.svelte (8)
8-8: Updated import to include TokenInfo typeThe import statement now correctly includes the TokenInfo type, which is used for the allTokenInfos array.
51-51: Type change for better type safetyChanged allTokenInfos from a generic type to a more specific TokenInfo[] array, providing better type checking and clarity about the expected data structure.
69-79: Improved error handling in getAllFieldDefinitionsThe implementation now properly checks for errors in the getAllFieldDefinitions result before assigning values, making the component more robust against potential errors from the underlying API.
87-91: Enhanced error handling in getAllDepositFieldsAdded proper error checking when retrieving the current deployment, ensuring that errors are caught and handled before attempting to access deposit fields.
102-106: Improved error handling in getAllTokenInputsNow properly checks for errors when retrieving the current deployment before accessing the inputs, preventing potential runtime errors.
114-118: Enhanced error handling in getAllTokenOutputsAdded proper error checking when retrieving token outputs from the current deployment, ensuring that errors are caught and handled appropriately.
149-157: Improved error handling in onSelectTokenSelectThe function now properly checks for errors when retrieving token information, ensuring that any issues with token data are caught and handled before updating the UI.
220-224: Enhanced error handling in areAllTokensSelectedThe function now properly checks for errors when retrieving token information, providing better error reporting and preventing potential runtime issues.
packages/ui-components/src/__tests__/DeploymentSteps.test.ts (4)
1-1: Added Mock type import for better type safetyIncluding the Mock type from vitest ensures better type safety when mocking methods, making it clearer what's being mocked and reducing potential type errors.
635-641: Improved test setup with real DotrainOrderGui instanceInstead of using a completely mocked object, the tests now instantiate a real DotrainOrderGui instance and apply mocks to it. This approach better reflects how the component will interact with the actual class.
643-651: Enhanced mock implementation for better testingThe setGui function now properly mocks methods on the DotrainOrderGui prototype, providing more realistic testing behavior and better type safety with explicit Mock typing.
795-810: Updated mock result format for getAllTokenInfosThe mock implementation now returns objects wrapped in a value property, aligning with the new error handling pattern where methods return objects with either a value or error property.
crates/js_api/src/gui/deposits.rs (5)
12-33: Added deposit validation methodThe new
check_depositsmethod provides important validation for deposits, ensuring that all required tokens are selected and properly configured before operations that depend on them. This improves error handling and user experience by catching issues early.
35-35: Updated WebAssembly export annotationChanging from
#[wasm_bindgen]to#[wasm_export]aligns with the PR's goal of updating the WebAssembly binding approach for the GUI components.
47-47: Enhanced return type specificationThe addition of
unchecked_return_type = "TokenDeposit[]"provides explicit type information for JavaScript, improving the integration between Rust and JavaScript code.
83-83: Improved type annotation for saveDepositThe explicit annotation of
unchecked_return_type = "void"clarifies that this method doesn't return a value to JavaScript, improving API clarity.
143-146: Enhanced error handling in has_any_depositUpdated the method to return a
Result<bool, GuiError>instead of a simple boolean, aligning its error handling pattern with other methods. This ensures consistency in how errors are propagated.crates/js_api/src/gui/mod.rs (13)
21-24: Imports look appropriate.
No concerns about adding these imports; they are necessary for the newly introducedHashMapusage and WASM export attributes.
51-51: No content change detected.
This single-line addition seems inconsequential (likely an extra blank line).
66-69: Request verification of custom export macro usage.
Using#[wasm_export]withunchecked_return_typecan be powerful but risky if actual data shapes differ from the declared JS type. Please verify the bridging in your integration tests to avoid runtime mismatches.
71-73: Parsing logic is concise and correct.
The immediate return of parsed deployment keys is clear.
76-82: Method signature update looks consistent.
Switching from returning a new object to updating the existing one makes sense. Ensure any previous consumers of the old signature are updated.
90-94: Field assignments are straightforward.
Implementation for setting the fields and returningOk(())is correct.
97-97: Export annotation consistency.
Good practice maintaining consistent#[wasm_export]usage across methods.
107-110: Getter function is concise.
ReturningGuiDeploymentCfgmatches the newly simplified approach.
166-167: Return type change to a vector is cleaner.
Switching from a wrapper struct to a plainVec<TokenInfo>is more direct and reduces overhead.
206-216: Deployment details retrieval.
Returning aHashMapinstead of a dedicated struct dereferences unnecessary wrappers and simplifies usage.
234-234: WASM export for generating Dotrain text.
No issues with returning a string here; the logic is straightforward.
247-247: Scenario binding retrieval.
Generating Rainlang text with updated scenario bindings is consistent.
342-350: Error bridging to WasmEncodedError.
IntegratingFrom<GuiError>is a clean approach to unify error handling in WASM.crates/js_api/src/gui/field_values.rs (9)
25-48: Check field values logic appears sound.
This method ensures defaults are assigned if present; otherwise, an error is returned for missing fields. This is a robust approach to validating user input or prefilled data.
53-73: Saving individual field values.
Includes preset verification and triggers a state update callback. Looks correct.
75-81: Batch-save field values.
Appropriately reusessave_field_value. This helps maintain DRY principles.
83-88: Removing field value.
Straightforward removal from the map followed by a callback trigger.
90-115: Retrieving a single field value.
Handles the preset vs. non-preset logic cleanly and returns aGuiPresetCfg.
117-130: Listing all field values.
Wrapping each binding inAllFieldValuesResultis consistent with your data model.
132-144: Field definition lookup.
The implementation checks the current deployment’s fields. Looks correct.
146-164: Filtering field definitions by default values.
Allowing optional filtering for default fields or non-default fields is a nice convenience.
166-177: Reporting missing field values.
This simple aggregator helps highlight incomplete user input.crates/js_api/src/gui/order_operations.rs (11)
89-90: WASM export for order operations.
Exposing these methods to JavaScript is consistent with your overall design.
187-224: Allowance checks for deposit tokens.
Logic flows well: it compiles a list of tokens needing deposit and checks allowances for each.
229-265: Generate approval calldatas if needed.
You handle the case of insufficient allowance to produce approval calls. That ensures minimal extraneous transactions.
291-332: Deposit calldatas generation.
Skips zero amount deposits, which is efficient. Implementation is clear.
335-347: Add order calldata generation.
Refactors away the wrapper struct in favor of plainBytes—simple and direct.
349-376: Combined deposit and add order calldatas.
Multicall usage looks correct. Good job merging deposit + order creation steps in a single transaction.
378-393: Set vault ID.
Allows dynamic vault assignments. The callback is invoked for reactivity.
395-424: Vault IDs retrieval.
Returning a map of input/output vault IDs is straightforward.
426-430: Check for any vault ID.
Simplifies checking if at least one vault ID is set, which helps in conditional UI actions.
432-437: Scenario binding updates.
Ties updated field values to scenario configuration—no issues specified.
439-492: Aggregated deployment transaction arguments.
Combines approvals, deposit, and add order calldatas into a single structure for easier transaction submission flow. Implementation is well-organized.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/orderbook/test/js_api/gui.test.ts (1)
353-363: Consider improving the undefined check in extractWasmEncodedDataThe condition
typeof void 0 === typeof result.valueis an unusual way to check ifresult.valueis undefined. For improved clarity and reliability, consider using a more standard approach:- if (typeof void 0 === typeof result.value) { + if (result.value === undefined) { return result.value as T; }or
- if (typeof void 0 === typeof result.value) { + if (typeof result.value === 'undefined') { return result.value as T; }crates/js_api/src/gui/mod.rs (1)
54-64: Added proper constructor for DotrainOrderGuiAdding a constructor method for
DotrainOrderGuiimproves initialization by providing default values. However, it initializes with a dummyDotrainOrderwhich may not be ideal for all use cases.Consider making the
dotrain_orderfield anOption<DotrainOrder>that's initiallyNoneuntil explicitly set, preventing potential issues with dummy objects:-pub fn new() -> DotrainOrderGui { - Self { - dotrain_order: DotrainOrder::dummy(), - selected_deployment: "".to_string(), - field_values: BTreeMap::new(), - deposits: BTreeMap::new(), - state_update_callback: None, - } -} +pub fn new() -> DotrainOrderGui { + Self { + dotrain_order: DotrainOrder::dummy(), // Maintains backward compatibility + selected_deployment: "".to_string(), + field_values: BTreeMap::new(), + deposits: BTreeMap::new(), + state_update_callback: None, + } +} + +// Alternative constructor that accepts an Option +pub fn with_order(order: Option<DotrainOrder>) -> DotrainOrderGui { + Self { + dotrain_order: order.unwrap_or_else(DotrainOrder::dummy), + selected_deployment: "".to_string(), + field_values: BTreeMap::new(), + deposits: BTreeMap::new(), + state_update_callback: None, + } +}crates/js_api/src/gui/field_values.rs (1)
25-48: Added robust field value validationThe new
check_field_valuesmethod properly validates that all required fields have values, falling back to defaults when available. This prevents runtime errors due to missing field values.Consider collecting all missing fields before returning an error to provide more comprehensive feedback:
pub fn check_field_values(&mut self) -> Result<(), GuiError> { let deployment = self.get_current_deployment()?; + let mut missing_fields = Vec::new(); for field in deployment.fields.iter() { if self.field_values.contains_key(&field.binding) { continue; } match &field.default { Some(default_value) => { self.save_field_value( field.binding.clone(), PairValue { is_preset: false, value: default_value.clone(), }, )?; } - None => return Err(GuiError::FieldValueNotSet(field.name.clone())), + None => missing_fields.push(field.name.clone()), } } + + if !missing_fields.is_empty() { + // Either return the first one as before + return Err(GuiError::FieldValueNotSet(missing_fields[0].clone())); + // Or consider extending the error type to support multiple missing fields + // return Err(GuiError::MultipleFieldValuesNotSet(missing_fields)); + } Ok(()) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
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 (31)
Cargo.toml(2 hunks)crates/common/src/dotrain_order/mod.rs(2 hunks)crates/js_api/src/gui/deposits.rs(5 hunks)crates/js_api/src/gui/field_values.rs(6 hunks)crates/js_api/src/gui/mod.rs(8 hunks)crates/js_api/src/gui/order_operations.rs(10 hunks)packages/orderbook/test/js_api/gui.test.ts(48 hunks)packages/ui-components/src/__tests__/DeploymentSteps.test.ts(8 hunks)packages/ui-components/src/__tests__/DeploymentsSection.test.ts(3 hunks)packages/ui-components/src/__tests__/DepositInput.test.ts(2 hunks)packages/ui-components/src/__tests__/FieldDefinitionInput.test.ts(11 hunks)packages/ui-components/src/__tests__/StrategyPage.test.ts(10 hunks)packages/ui-components/src/__tests__/TokenIOInput.test.ts(4 hunks)packages/ui-components/src/__tests__/getDeploymentTransactionArgs.test.ts(2 hunks)packages/ui-components/src/lib/components/deployment/ComposedRainlangModal.svelte(1 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/DepositInput.svelte(4 hunks)packages/ui-components/src/lib/components/deployment/FieldDefinitionInput.svelte(3 hunks)packages/ui-components/src/lib/components/deployment/SelectToken.svelte(2 hunks)packages/ui-components/src/lib/components/deployment/StrategyPage.svelte(1 hunks)packages/ui-components/src/lib/components/deployment/TokenIOInput.svelte(2 hunks)packages/ui-components/src/lib/components/deployment/getDeploymentTransactionArgs.ts(3 hunks)packages/ui-components/src/lib/stores/transactionStore.ts(1 hunks)packages/ui-components/src/lib/types/transaction.ts(1 hunks)packages/ui-components/test-setup.ts(2 hunks)packages/webapp/src/__tests__/handleGuiInitialization.test.ts(1 hunks)packages/webapp/src/lib/services/handleGuiInitialization.ts(3 hunks)packages/webapp/src/routes/deploy/+layout.ts(1 hunks)packages/webapp/src/routes/deploy/[strategyName]/[deploymentKey]/+layout.ts(1 hunks)packages/webapp/test-setup.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (16)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-legal)
- GitHub Check: standard-tests (ubuntu-latest, test-js-bindings)
- GitHub Check: standard-tests (ubuntu-latest, rainix-wasm-test)
- 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-artifacts)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-static)
- 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, ob-rs-test, true)
- GitHub Check: test
- GitHub Check: build-tauri (ubuntu-22.04, true)
- GitHub Check: test
- GitHub Check: git-clean
- GitHub Check: test
- GitHub Check: Deploy-Preview
🔇 Additional comments (94)
packages/webapp/src/routes/deploy/[strategyName]/[deploymentKey]/+layout.ts (1)
12-16: Improved error handling with result objectsThis change enhances error handling by properly checking the result object returned from the API call before using its values. The code now explicitly checks for error conditions and throws with appropriate error messages, making debugging easier and preventing potential runtime errors from undefined values.
This implementation aligns with the codebase-wide refactoring effort to standardize error handling as mentioned in the AI summary.
packages/ui-components/src/lib/types/transaction.ts (1)
7-7: Type simplification for better maintainabilityChanging the
deploymentCalldatatype from a complex object type to a simplestringaligns with the PR's goal of removing wrapper types in favor of simpler return types. This simplification makes the interface cleaner and more straightforward to use.packages/ui-components/src/lib/components/deployment/DeploymentsSection.svelte (1)
13-17: Consistent error handling pattern implementedThis change follows the same error handling pattern implemented throughout the codebase, checking the result object for errors before accessing the value. The implementation properly throws meaningful errors that will be caught by the surrounding try/catch block, improving error reporting to the user.
This approach is consistent with the changes seen in other files and ensures robust error handling across the application.
packages/ui-components/src/lib/components/deployment/ComposedRainlangModal.svelte (1)
14-19: Improved error handling for getComposedRainlangThe changes properly implement error handling by checking for
result.errorbefore proceeding, which is a significant improvement over the previous implementation. This ensures that any issues with retrieving the composed Rainlang are properly captured and reported instead of being silently ignored.crates/common/src/dotrain_order/mod.rs (2)
127-134: Good addition of thedummy()constructor methodThe new
dummy()method provides a convenient way to create a default instance ofDotrainOrderwith minimal initialization, which is useful for testing and default state handling. The implementation correctly initializes the struct with an empty string fordotrainand aDotrainYamlwith a single default document.
12-12: Updated imports to support the new dummy methodThe import statement has been correctly updated to include
default_document, which is now used in the newdummy()method implementation.packages/ui-components/src/lib/components/deployment/FieldDefinitionInput.svelte (3)
23-27: Enhanced error handling in onMount lifecycleThe code now properly checks for errors when retrieving field values, which improves error reporting and prevents silently ignoring potential issues. This is part of a broader pattern of improved error handling throughout the application.
41-45: Improved error handling in handlePresetClickError handling has been consistently implemented for the field value retrieval after saving a preset selection. This ensures that any errors during this operation are properly caught and reported.
55-59: Added error checking in handleCustomInputChangeThe implementation now properly validates the result of
getFieldValueafter saving a custom input value, following the same pattern of error checking used elsewhere in the component. This consistency in error handling improves the overall robustness of the application.packages/ui-components/src/lib/components/deployment/StrategyPage.svelte (1)
30-34: Enhanced error handling in getStrategy functionThe changes properly implement error checking for the result of
DotrainOrderGui.getStrategyDetails(). By explicitly checking forresult.errorand throwing an appropriate Error with the error message, the code ensures that errors are properly propagated and can be caught in the catch block where they're displayed to the user.packages/ui-components/src/lib/components/deployment/DepositInput.svelte (4)
27-31: Improved error handling in setCurrentDeposit functionThe changes add proper error checking for the result of
gui.getDeposits(), making the component more robust by explicitly handling error cases instead of relying on the catch block.
41-45: Better error handling for token info retrievalThe updated code properly checks if the result contains an error before proceeding, which provides more specific error information and makes debugging easier.
60-64: Consistent error handling in handlePresetClickThis change maintains consistency with the error handling pattern implemented throughout the component, ensuring errors from
getDeposits()are properly handled.
75-79: Enhanced error checking in handleInput functionThe addition of error checking for the deposits result ensures that any API errors are properly propagated rather than silently failing.
packages/webapp/src/routes/deploy/+layout.ts (1)
13-17: Improved error handling for strategy details retrievalThe updated code now correctly checks for errors in the result from
DotrainOrderGui.getStrategyDetails()and throws an error with a specific error message when needed, making debugging easier and providing more reliable error handling.packages/ui-components/src/lib/components/deployment/SelectToken.svelte (2)
22-26: Enhanced error handling in onMount lifecycleThe code now properly checks for errors in the result from
gui.getTokenInfo()before proceeding with setting the token information, improving component reliability.
38-42: Improved error handling in getInfoForSelectedToken functionSimilar to the changes in onMount, this function now properly checks for errors in the API response, providing more specific error information and consistent error handling.
packages/ui-components/src/__tests__/DeploymentsSection.test.ts (3)
2-2: Added Mock type importThe import statement now includes the Mock type from vitest, which is used to properly type-cast the mocked methods.
19-19: Updated mock implementation to match new API return structureThe mock for
getDeploymentDetailsnow returns an object with avalueproperty containing the mock data, which aligns with the updated API response structure.
37-39: Updated error handling test to match new error response formatRather than rejecting the promise, the mock now returns an object with an
errorproperty, which better reflects how the actual API now handles and communicates errors.packages/ui-components/src/lib/components/deployment/TokenIOInput.svelte (2)
21-26: Enhanced error handling implemented for vault ID retrievalThe code now properly captures and handles errors from the
getVaultIdsmethod by checking the result object for an error property before proceeding, which improves the component's robustness.
37-42: Improved error handling for token info retrievalSimilar to the vault ID retrieval, this change implements proper error checking before processing the token information result. This consistent error handling pattern prevents undefined behavior and enhances the component's reliability.
packages/ui-components/src/__tests__/FieldDefinitionInput.test.ts (4)
8-8: Test refactoring uses actual DotrainOrderGui instanceThe tests now use a real instance of DotrainOrderGui instead of a completely mocked object, which better reflects the class's actual behavior including potential WASM interactions. This approach is more aligned with how the component would behave in a real environment.
Also applies to: 23-25
25-28: Improved method mocking approachThe test now mocks specific methods on the DotrainOrderGui prototype rather than using a fully mocked object. This approach allows the test to leverage the actual class implementation while controlling the behavior of specific methods.
35-36: Updated mockGui references to guiInstanceAll references to the previous
mockGuihave been consistently updated to useguiInstancethroughout the test file, ensuring the tests remain coherent with the new implementation approach.Also applies to: 47-48, 59-60, 76-77, 99-100, 114-115, 143-144, 152-155
129-134: Updated mock return value structureThe mock implementation for
getFieldValuenow returns an object structure that matches the expected result type with error handling support, aligning with the changes made to the DotrainOrderGui API.packages/ui-components/src/lib/stores/transactionStore.ts (1)
56-56: Simplified deploymentCalldata type from complex object to stringThe type of
deploymentCalldatahas been changed from a complex object type to a simple string. This simplification is consistent with the implementation of WASM exports, which typically prefer simpler data types when crossing the WASM boundary.Cargo.toml (2)
56-56: Updated wasm-bindgen-utils dependency to use specific Git commitThe dependency has been changed from a version specification to a Git repository with a specific commit hash. This ensures consistency and reproducibility of the build, particularly important for WASM bindings which need to be compatible with the JavaScript API.
80-80: Minor formatting adjustment to dependency pathThis is a cosmetic change that doesn't affect functionality but maintains consistent formatting in the dependency declarations.
packages/ui-components/src/__tests__/DepositInput.test.ts (3)
6-6: Approve the switch to using a real DotrainOrderGui instanceThe change from using a mock object to a real instance of DotrainOrderGui improves test fidelity by more accurately representing how the component interacts with the GUI in production.
Also applies to: 12-12, 21-22
24-29: Well-structured mocking implementationThe updated mocking approach that returns objects with a
valueproperty is consistent with the PR's goal of standardizing error handling and result structures across the DotrainOrderGui API. This makes the tests more accurate and better aligned with how the API behaves in production.Also applies to: 33-38, 67-67
63-63: Properly updated test assertionsThe assertions have been correctly updated to verify interactions with the DotrainOrderGui instance, ensuring the tests continue to validate the component's behavior accurately.
Also applies to: 80-81
packages/orderbook/test/js_api/gui.test.ts (3)
366-371: Approve consistent error handling with extractWasmEncodedDataThe consistent use of the extractWasmEncodedData utility function throughout the file ensures uniform error handling for all API calls. This makes the code more maintainable and follows the PR's goal of standardizing how results and errors are processed.
Also applies to: 382-387, 395-401, 407-412
556-558: Approve improved error checking patternThe updated approach for testing error cases is more robust and explicit. Now the tests properly verify both the presence of errors and their specific messages, which enhances test coverage and makes debugging easier when failures occur.
Also applies to: 589-591
382-387: Approve uniform approach to testing DotrainOrderGuiThe test file now consistently instantiates DotrainOrderGui instances and handles their results in a uniform way across all test cases. This approach improves maintainability and better represents how the API should be used in production code.
Also applies to: 395-401, 454-457, 494-497, 513-513, 595-595
packages/ui-components/src/__tests__/getDeploymentTransactionArgs.test.ts (1)
8-8: Approve consistent approach to testing with structured result objectsThe test now properly uses a real DotrainOrderGui instance and mocks that return structured results with a
valueproperty. This approach aligns with the PR's goal of standardizing error handling and result structures, making the tests more closely mirror production code behavior.Also applies to: 16-17, 21-22, 23-30, 38-42, 44-44
packages/ui-components/src/__tests__/StrategyPage.test.ts (1)
4-4: Approve standardized result structure for strategy detailsThe test has been updated to use a consistent structure for both success results (with a
valueproperty) and error results (with anerrorproperty containing a message). This standardized approach aligns with the PR's goal of improving error handling and makes the tests more accurately reflect the API's behavior.Also applies to: 22-28, 83-87, 105-111
packages/webapp/src/lib/services/handleGuiInitialization.ts (4)
1-1: Good addition of WasmEncodedResult type importAdding the WasmEncodedResult import allows proper typing of WASM function results, improving type safety throughout the code.
10-11: Improved initialization and error handlingCreating a new DotrainOrderGui instance directly and using a properly typed result variable demonstrates better code practice than the previous implementation. This approach ensures the GUI object exists before use and provides typed error handling.
21-24: Enhanced error handling with proper WASM result checkingThe implementation now correctly checks for errors in the WASM operation result and throws with the specific error message from the result object. This provides more accurate error information than the previous implementation.
27-30: Consistent error handling patternThe same error handling pattern is applied consistently here, which is a good practice. The code properly checks for errors in the WASM result and throws with the specific error message.
packages/ui-components/src/__tests__/TokenIOInput.test.ts (6)
5-7: Good direct import of DotrainOrderGuiUsing a direct import of the DotrainOrderGui class and changing TokenIOInputComponentProps to a local type improves code organization and modularity.
10-14: Enhanced test fidelity with actual DotrainOrderGui instanceUsing an actual DotrainOrderGui instance rather than a mock object improves test fidelity and better reflects real-world usage patterns.
22-27: Proper structure for WASM result objectsThe updated mockTokenInfo structure with a value property correctly reflects the WASM function return pattern, making the tests more accurate.
29-51: Improved test setup with proper method mockingThe beforeEach implementation now correctly sets up mocks on the DotrainOrderGui prototype, which is the proper approach for mocking instance methods. The use of nested value objects in mock returns accurately reflects the WASM return structure.
53-64: Better test props setup with real instanceThe updated mockProps and outputMockProps now use the actual guiInstance, which makes the tests more realistic and better reflects how the component will be used in production.
88-88: Updated assertions to use the actual instanceThe assertions now correctly reference guiInstance instead of the previous mockGui, maintaining consistency with the updated test approach.
Also applies to: 95-95, 125-125
packages/ui-components/src/lib/components/deployment/getDeploymentTransactionArgs.ts (3)
3-3: Refined import to use only type informationGood practice to import just the type when only type information is needed, improving code quality and potentially reducing bundle size.
16-16: Simplified deploymentCalldata typeChanged deploymentCalldata type from a complex object to a simple string, which likely simplifies the interface between JavaScript and WASM.
34-38: Robust error handling for WASM operationsThe implementation now properly checks for errors in the WASM result and throws with the specific error message. This pattern is consistent with other files and provides better error handling.
- const { approvals, deploymentCalldata, orderbookAddress, chainId } = await gui.getDeploymentTransactionArgs(address); + const result = await gui.getDeploymentTransactionArgs(address); + if (result.error) { + throw new Error(result.error.msg); + } + const { approvals, deploymentCalldata, orderbookAddress, chainId } = result.value;packages/webapp/src/__tests__/handleGuiInitialization.test.ts (6)
1-4: Updated imports to use actual DotrainOrderGui classGood improvement to import the actual DotrainOrderGui class and WasmEncodedResult type, making the tests more accurate.
7-7: Enhanced test fidelity with actual DotrainOrderGui instanceUsing a real instance of DotrainOrderGui and initializing it in beforeEach improves test fidelity and better represents real-world usage patterns.
Also applies to: 14-14
18-18: Proper mocking of static and instance methodsThe implementation now correctly distinguishes between mocking static methods (DotrainOrderGui.deserializeState) and instance methods (DotrainOrderGui.prototype.chooseDeployment), with proper typing for the mock returns.
Also applies to: 33-35
39-45: Updated assertion to check the actual instanceThe assertion now correctly expects guiInstance in the result and verifies that the instance methods were called with the right parameters.
49-49: Consistent mocking pattern for instance methodsThe test consistently uses the same pattern for mocking instance methods and verifying their calls, which is a good practice for maintainable tests.
Also applies to: 53-59
63-63: Proper error handling test caseThe test correctly checks the error handling path by mocking a rejected promise, ensuring that the error handling behavior works as expected.
packages/ui-components/test-setup.ts (2)
15-19: Improved code formatting for better readabilityBreaking the
pageobject into multiple lines improves code readability while maintaining the same functionality.
56-87: Comprehensive mock implementation for DotrainOrderGuiThe addition of this mock for the
DotrainOrderGuiclass provides a complete testing foundation for components that depend on this API. The mock covers all necessary methods including static methods and prototype methods with proper TypeScript error suppression where needed.This implementation aligns well with the changes in the actual
DotrainOrderGuiimplementation where methods likegetAllTokenInfosandgetCurrentDeploymentnow return result objects that need to be checked for errors.packages/ui-components/src/lib/components/deployment/DeploymentSteps.svelte (8)
8-8: Added required TokenInfo type importAdded TokenInfo type import to match the updated return type of GUI methods.
51-51: Updated variable type to match API changesThe
allTokenInfosvariable type has been updated fromAllTokenInfostoTokenInfo[]to align with the changes in the underlying API.
69-79: Improved error handling in getAllFieldDefinitionsEnhanced error handling by properly checking for errors in the return value from
getAllFieldDefinitionscalls. This makes the component more robust when dealing with potential errors from the API.
87-91: Improved error handling in getAllDepositFieldsEnhanced error handling by checking for errors in the return value from
getCurrentDeploymentbefore accessing its properties.
102-106: Improved error handling in getAllTokenInputsEnhanced error handling by checking for errors in the return value from
getCurrentDeploymentbefore accessing its properties.
114-118: Improved error handling in getAllTokenOutputsEnhanced error handling by checking for errors in the return value from
getCurrentDeploymentbefore accessing its properties.
149-153: Improved error handling in onSelectTokenSelectEnhanced error handling by checking for errors in the return value from
getAllTokenInfosbefore using the result.
220-224: Improved error handling in areAllTokensSelectedEnhanced error handling by checking for errors in the return value from
getAllTokenInfosbefore using the result.packages/ui-components/src/__tests__/DeploymentSteps.test.ts (4)
1-1: Added Mock type import for better type safetyThe addition of the Mock type import improves type safety when working with mocked functions.
635-641: Improved test setup with proper mocking approachThe test setup now creates a real instance of
DotrainOrderGuiand assigns it todefaultProps.gui, making the tests more realistic and maintainable.
643-651: Simplified mock configuration with direct method mockingThe
setGuifunction has been simplified to directly mock the methods on theDotrainOrderGuiprototype rather than creating a new object. This approach is more maintainable and aligns better with how the component interacts with the API.
709-751: Improved mock implementations for complex methodsThe mock implementations for complex methods now use the Mock type and provide structured return objects that match the actual API's result format with value/error properties.
crates/js_api/src/gui/deposits.rs (8)
13-32: Added robust deposit validation methodA new
check_depositsmethod has been added to validate the deposits in the current deployment. It performs thorough validation by checking if each deposit has a token selected and verifies that the token exists in the deposits map.This validation is critical for ensuring the deployment process can proceed without unexpected errors related to missing deposit information.
35-35: Updated WebAssembly export mechanismChanged from
#[wasm_bindgen]to#[wasm_export]for the implementation block, which aligns with the WASM macro implementation mentioned in the PR objective.
47-47: Enhanced method export with type informationUpdated
getDepositsmethod withwasm_exportattribute and specified return type information (unchecked_return_type = "TokenDeposit[]"), improving the JavaScript interoperability.
83-83: Enhanced method export with type informationUpdated
saveDepositmethod withwasm_exportattribute and specified return type information (unchecked_return_type = "void"), improving the JavaScript interoperability.
115-115: Enhanced method export with type informationUpdated
removeDepositmethod withwasm_exportattribute and specified return type information (unchecked_return_type = "void"), improving the JavaScript interoperability.
122-122: Enhanced method export with type informationUpdated
getDepositPresetsmethod withwasm_exportattribute and specified return type information (unchecked_return_type = "string[]"), improving the JavaScript interoperability.
128-128: Enhanced method export with type informationUpdated
getMissingDepositsmethod withwasm_exportattribute and specified return type information (unchecked_return_type = "string[]"), improving the JavaScript interoperability.
143-146: Improved error handling in hasAnyDeposit methodUpdated
hasAnyDepositmethod with proper error handling by changing the return type frombooltoResult<bool, GuiError>, which aligns with the overall error handling strategy in the codebase.crates/js_api/src/gui/order_operations.rs (7)
89-90: Upgrade fromwasm_bindgentowasm_exportmacroThe shift from
#[wasm_bindgen]to#[wasm_export]aligns with the PR objective to implement WASM macros for DotrainOrderGui operations. This enables better JavaScript interoperability with clearer type definitions.
187-194: Return type simplification improves API usabilityThe
check_allowancesmethod now directly returnsResult<Vec<TokenAllowance>, GuiError>instead of wrapping it in a custom type. This simplification makes the API more intuitive while theunchecked_return_typeattribute ensures proper JavaScript type information.
223-224: Direct result returns improve error handlingRemoving wrapper types in favor of direct result returns allows for better error propagation and simplified error handling on both Rust and JavaScript sides.
349-376: Simplified Bytes encoding for deposit and add order operationsThe
generate_deposit_and_add_order_calldatasmethod now returns a direct Bytes result rather than a custom wrapper type, streamlining the contract interaction interface.
395-424: Direct HashMap return simplifies vault ID handlingThe
get_vault_idsmethod now returnsResult<HashMap<String, Vec<Option<U256>>>, GuiError>directly rather than wrapping it in a custom type, making the API more consistent with Rust idioms while preserving type safety for JavaScript.
426-430: Boolean check simplification improves readabilityThe
has_any_vault_idimplementation has been simplified to use a more idiomatic approach with theanycombinator, making the code more concise and readable.
336-347:Details
✅ Verification successful
Bytes return type simplifies calldata handling
Using
Bytesdirectly as the return type rather than a custom wrapper forgenerate_add_order_calldatasimplifies the contract interaction layer. Verify the JavaScript side properly handles this Bytes type.Run this script to verify how Bytes types are handled on the JavaScript side:
🏁 Script executed:
#!/bin/bash # Find instances of handling Bytes return types in JavaScript code rg -A 5 "generateAddOrderCalldata" --glob "*.ts" --glob "*.js"Length of output: 2300
Verified Bytes Handling in JavaScript
The test output confirms that the JavaScript side properly handles the
Bytestype returned bygenerateAddOrderCalldata. In the tests (inpackages/orderbook/test/js_api/gui.test.ts), the value obtained fromgui.generateAddOrderCalldata()is immediately processed withextractWasmEncodedDataand subsequently validated (e.g., checking its length). The use ofBytesdirectly simplifies the contract interaction layer as expected.No further modifications are needed.
crates/js_api/src/gui/mod.rs (6)
24-25: Important dependency change for WebAssembly utilitiesSwitched from direct
wasm-bindgentowasm_bindgen_utilswith updated imports. This change supports the WASM macro implementation and ensures consistent export patterns across the codebase.
66-74: Simplified deployment keys handlingThe
get_deployment_keysmethod now returns a direct vector of strings rather than a custom wrapper type, making the API more intuitive.
76-95: Improved deployment assignment with better error handlingThe
choose_deploymentmethod now modifies the instance's fields directly rather than returning a new instance, following a more idiomatic approach to stateful APIs. This also clarifies the ownership model.
166-193: Simplified token info handling with direct return typeThe
get_all_token_infosmethod now returns a direct vector of token information rather than a custom wrapper type, making the API more consistent and easier to use.
206-217: Improved deployment details handling with HashMap returnDirectly returning a
HashMap<String, NameAndDescriptionCfg>fromget_deployment_detailsimproves API clarity and follows Rust idioms better than a custom wrapper type.
343-350: Added proper WasmEncodedError conversion for better error handlingImplementing
From<GuiError> for WasmEncodedErrorensures consistent error handling in the WebAssembly context. This is crucial for proper error propagation to JavaScript.crates/js_api/src/gui/field_values.rs (3)
51-54: Updated WASM export annotations for field value operationsThe updated attribute for
save_field_valuenow uses#[wasm_export]with explicitunchecked_return_type, improving JavaScript interoperability.
117-130: Improved field values retrieval with clearer type annotationsThe
get_all_field_valuesmethod now uses explicit type annotations viaunchecked_return_type, making the JavaScript interface more predictable.
146-164: Enhanced field definitions retrieval with better type safetyThe
get_all_field_definitionsmethod's WASM export now includes explicit return type information, improving type safety when used from JavaScript.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
packages/ui-components/src/lib/components/deployment/DepositInput.svelte (1)
54-82:⚠️ Potential issueException handling in event handlers
Both
handlePresetClickandhandleInputfunctions throw errors but don't have try-catch blocks to handle those errors, which could lead to unhandled exceptions and poor user experience.Add try-catch blocks to handle the errors properly:
function handlePresetClick(preset: string) { if (deposit.token?.key) { inputValue = preset; gui?.saveDeposit(deposit.token?.key, preset); gui = gui; + try { const deposits = gui.getDeposits(); if (deposits.error) { throw new Error(deposits.error.msg); } currentDeposit = deposits.value.find((d) => d.token === deposit.token?.key); + } catch (e) { + error = e instanceof Error ? e.message : 'Error retrieving deposits'; + } } } function handleInput(e: Event) { if (deposit.token?.key) { if (e.currentTarget instanceof HTMLInputElement) { inputValue = e.currentTarget.value; gui?.saveDeposit(deposit.token.key, e.currentTarget.value); gui = gui; + try { const deposits = gui.getDeposits(); if (deposits.error) { throw new Error(deposits.error.msg); } currentDeposit = deposits.value.find((d) => d.token === deposit.token?.key); + } catch (e) { + error = e instanceof Error ? e.message : 'Error retrieving deposits'; + } } } }
🧹 Nitpick comments (9)
packages/ui-components/src/lib/components/deployment/getDeploymentTransactionArgs.ts (1)
16-16: Type change for deploymentCalldataThe type has been changed from a complex type
DepositAndAddOrderCalldataResultto a simplestring. This simplification is good for maintainability but ensure all downstream consumers are updated to handle this new string format correctly.packages/webapp/test-setup.ts (1)
12-20: Added mock for DotrainOrderGuiGood addition of test mocks for the
DotrainOrderGuiclass, which will improve testing of components that depend on this module.However, there's a TypeScript error suppression comment that should be addressed properly rather than suppressed with
@ts-expect-error.Instead of using a TypeScript error suppression, consider creating a proper interface for your mock:
- // @ts-expect-error - this is a mock - DotrainOrderGui.deserializeState = vi.fn(); + interface DotrainOrderGuiConstructor { + new (): any; + deserializeState: ReturnType<typeof vi.fn>; + } + (DotrainOrderGui as unknown as DotrainOrderGuiConstructor).deserializeState = vi.fn();packages/ui-components/src/lib/components/deployment/DepositInput.svelte (2)
60-64: Error handling in handlePresetClickAdding error checking for the getDeposits method ensures that any API errors are properly handled.
However, consider moving this error handling logic into a separate utility function since it's repeated in multiple places.
- const deposits = gui.getDeposits(); - if (deposits.error) { - throw new Error(deposits.error.msg); - } - currentDeposit = deposits.value.find((d) => d.token === deposit.token?.key); + currentDeposit = getDepositsWithErrorHandling(gui, deposit.token?.key); // Add this helper function at the top of the script + const getDepositsWithErrorHandling = (gui: DotrainOrderGui, tokenKey?: string) => { + const deposits = gui.getDeposits(); + if (deposits.error) { + throw new Error(deposits.error.msg); + } + return deposits.value.find((d) => d.token === tokenKey); + };
75-79: Error handling in handleInputThe error handling implementation here is identical to that in handlePresetClick.
As suggested above, consider refactoring this repeated error handling logic into a helper function to avoid code duplication.
- const deposits = gui.getDeposits(); - if (deposits.error) { - throw new Error(deposits.error.msg); - } - currentDeposit = deposits.value.find((d) => d.token === deposit.token?.key); + currentDeposit = getDepositsWithErrorHandling(gui, deposit.token?.key);packages/orderbook/test/js_api/gui.test.ts (1)
1177-1179: Consider adding a comment explaining the expected calldata format.The test verifies the length of the calldata (2314) but doesn't document what this represents or why this specific length is expected. A brief comment explaining what a valid calldata should contain would improve test readability.
crates/js_api/src/gui/order_operations.rs (2)
346-346: Efficient memory usage consideration.Ok(Bytes::copy_from_slice(&calldata))This is correct, though if
calldatais already owned, consider passing ownership without copying to reduce allocations, if feasible.
395-399: Export annotation and final return value forget_vault_ids.
The return type alignment with JavaScript’s map structure is reasonable; watch out for correct handling of optional vault IDs in JS code.Also applies to: 423-423
crates/js_api/src/gui/field_values.rs (2)
25-49: Newcheck_field_valuesmethod.
Ensures all fields required by the deployment have corresponding values, or applies defaults. This improves robustness. Watch out for whether changing the UI state behind the scenes might surprise end users.
90-91:getFieldValueannotation.
Exposes field retrieval logic to JS. Confirm performance is acceptable for repeated lookups in large field sets.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
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 (31)
Cargo.toml(2 hunks)crates/common/src/dotrain_order/mod.rs(2 hunks)crates/js_api/src/gui/deposits.rs(5 hunks)crates/js_api/src/gui/field_values.rs(6 hunks)crates/js_api/src/gui/mod.rs(8 hunks)crates/js_api/src/gui/order_operations.rs(10 hunks)packages/orderbook/test/js_api/gui.test.ts(48 hunks)packages/ui-components/src/__tests__/DeploymentSteps.test.ts(8 hunks)packages/ui-components/src/__tests__/DeploymentsSection.test.ts(3 hunks)packages/ui-components/src/__tests__/DepositInput.test.ts(2 hunks)packages/ui-components/src/__tests__/FieldDefinitionInput.test.ts(11 hunks)packages/ui-components/src/__tests__/StrategyPage.test.ts(10 hunks)packages/ui-components/src/__tests__/TokenIOInput.test.ts(4 hunks)packages/ui-components/src/__tests__/getDeploymentTransactionArgs.test.ts(2 hunks)packages/ui-components/src/lib/components/deployment/ComposedRainlangModal.svelte(1 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/DepositInput.svelte(4 hunks)packages/ui-components/src/lib/components/deployment/FieldDefinitionInput.svelte(3 hunks)packages/ui-components/src/lib/components/deployment/SelectToken.svelte(2 hunks)packages/ui-components/src/lib/components/deployment/StrategyPage.svelte(1 hunks)packages/ui-components/src/lib/components/deployment/TokenIOInput.svelte(2 hunks)packages/ui-components/src/lib/components/deployment/getDeploymentTransactionArgs.ts(3 hunks)packages/ui-components/src/lib/stores/transactionStore.ts(1 hunks)packages/ui-components/src/lib/types/transaction.ts(1 hunks)packages/ui-components/test-setup.ts(2 hunks)packages/webapp/src/__tests__/handleGuiInitialization.test.ts(1 hunks)packages/webapp/src/lib/services/handleGuiInitialization.ts(3 hunks)packages/webapp/src/routes/deploy/+layout.ts(1 hunks)packages/webapp/src/routes/deploy/[strategyName]/[deploymentKey]/+layout.ts(1 hunks)packages/webapp/test-setup.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (13)
- GitHub Check: standard-tests (ubuntu-latest, test-js-bindings)
- GitHub Check: standard-tests (ubuntu-latest, rainix-wasm-test)
- 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-artifacts)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-static)
- 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, ob-rs-test, true)
- GitHub Check: test
- GitHub Check: build-tauri (ubuntu-22.04, true)
- GitHub Check: test
- GitHub Check: Deploy-Preview
🔇 Additional comments (116)
packages/ui-components/src/lib/components/deployment/getDeploymentTransactionArgs.ts (1)
34-38: Improved error handling with result patternThis is a great improvement in error handling. The code now checks for errors before processing the result, which makes the function more robust and prevents unexpected behavior when errors occur.
packages/webapp/src/routes/deploy/[strategyName]/[deploymentKey]/+layout.ts (1)
12-16: Enhanced error handling in load functionExcellent improvement to the error handling in the
loadfunction. Now the code properly checks for errors fromgetDeploymentDetailbefore attempting to use the result.This makes the function more robust and provides clearer error messages when something goes wrong.
packages/ui-components/src/lib/components/deployment/DeploymentsSection.svelte (1)
13-17: Improved error handling implementationThe code now properly checks for errors from the API response before proceeding with data processing. This is a good practice that prevents silent failures and makes debugging easier.
The updated approach of storing the result in a variable, checking for errors explicitly, and then accessing the value property is consistent with best practices for handling asynchronous operations that can return errors.
packages/ui-components/src/lib/components/deployment/StrategyPage.svelte (1)
30-34: Enhanced error handling implementationThe error handling implementation has been improved to explicitly check for errors from the API response before proceeding, which aligns with the implementation in other components.
This improvement ensures errors are consistently handled across the application, making the codebase more robust and easier to maintain.
packages/ui-components/src/lib/components/deployment/ComposedRainlangModal.svelte (1)
14-20: Robust error handling implementationThe error handling implementation has been enhanced to check for errors from the API response explicitly before proceeding with setting the rainlang text and opening the modal.
This change makes the code more reliable by ensuring errors are properly caught and handled, preventing potential issues where errors might be silently ignored.
packages/ui-components/src/lib/components/deployment/DepositInput.svelte (2)
27-31: Improved error checking in setCurrentDeposit methodThe code now properly checks for errors from the getDeposits method before proceeding with finding the current deposit.
This is a good improvement that ensures errors are handled properly and not ignored.
41-45: Enhanced error handling in getTokenSymbol methodThe error handling has been improved to explicitly check for errors from the getTokenInfo method before proceeding with setting the token info.
This enhancement ensures consistent error handling across the component.
crates/common/src/dotrain_order/mod.rs (1)
126-135: Useful convenience method for creating a default instance.The new
dummy()method provides a simple way to create a defaultDotrainOrderinstance without requiring detailed parameters, which can be valuable for testing or initializing a basic structure before further configuration.packages/ui-components/src/__tests__/getDeploymentTransactionArgs.test.ts (7)
8-8: Improved import to use actual implementation.The test now imports the concrete
DotrainOrderGuiclass instead of using a complete mock, which better aligns with real usage patterns.
16-22: Better test setup using real class instance.Replacing the mock object with an actual instance of
DotrainOrderGuimakes the tests more realistic and closer to production code, improving test reliability.
23-30: Updated mock to match new error handling structure.The mock now returns an object with a
valueproperty, reflecting the new API structure that supports better error handling.
38-42: Consistent error handling pattern in mock.The mock for
generateApprovalCalldatasfollows the same pattern with avalueproperty, maintaining consistency with the updated API structure.
44-44: Updated function call to use the real instance.Function call updated to use the actual
DotrainOrderGuiinstance, which better represents how the code would be used in production.
57-58: Consistent use of the actual instance in error test.Error test for missing config now uses the actual instance, maintaining consistency throughout the test file.
64-65: Consistent use of the actual instance in wallet error test.Error test for missing wallet address now uses the actual instance, ensuring all test cases follow the same pattern.
packages/webapp/src/routes/deploy/+layout.ts (1)
13-17: Enhanced error handling with explicit checks.This change improves error handling by explicitly checking for errors from the
getStrategyDetailsmethod and throwing them with clear messages. The updated return structure properly extracts data fromresult.value, which aligns with the new API structure.packages/ui-components/src/lib/stores/transactionStore.ts (1)
56-56: Simplified deploymentCalldata type.Changed the type of
deploymentCalldatafrom a complex object to a simple string, which simplifies the API and likely makes it more straightforward to use. This change is consistent with the overall trend toward better error handling and simpler types across the codebase.packages/ui-components/src/lib/components/deployment/FieldDefinitionInput.svelte (3)
23-27: Improved error handling for getFieldValue in onMount.The error handling enhancement for
gui.getFieldValueis a good improvement. This structured approach ensures errors are properly captured and reported.
41-45: Consistent error handling in handlePresetClick.The same error handling pattern has been applied consistently to the
handlePresetClickfunction, properly capturing and handling potential errors from the API call.
55-59: Consistent error handling in handleCustomInputChange.The error handling implementation follows the same pattern as in other functions, ensuring consistency throughout the component.
packages/ui-components/src/lib/components/deployment/TokenIOInput.svelte (2)
21-26: Enhanced error handling for getVaultIds.Good implementation of structured error handling for the
getVaultIdsmethod. This approach properly captures errors and stores them in theerrorvariable for display.
37-42: Improved error handling in handleGetTokenInfo.The error handling has been properly implemented for the
getTokenInfomethod call, following the same pattern as in other parts of the codebase.packages/ui-components/src/__tests__/DeploymentsSection.test.ts (3)
2-2: Updated import to include Mock type.Good addition of the
Mocktype import from vitest, which provides better type safety when mocking the DotrainOrderGui methods.
19-19: Updated mock to match new response structure.The mock has been correctly updated to return an object with a
valueproperty containing the mock deployments, which aligns with the new API response structure.
37-39: Updated error mock to match new error response structure.The error mock now correctly returns an object with an
errorproperty containing a message, which matches the new error handling approach in the application.packages/ui-components/src/__tests__/StrategyPage.test.ts (9)
4-4: Updated import to include Mock type.The import has been correctly updated to include the
Mocktype from vitest for better type safety when mocking.
22-28: Updated mock structure to match new API response format.The mock strategy details object has been correctly updated to include a
valueproperty, which aligns with the new API response structure.
29-30: Updated mock implementation to use Mock type.The mocking of
getStrategyDetailshas been correctly updated to use theMocktype for better type safety.
45-51: Updated mock structure for consistency.The mock strategy details object structure has been consistently updated to match the new API response format.
59-59: Updated mock implementation with correct typing.The mock implementation has been updated to use the correct type casting.
83-87: Updated error mock to match new error response structure.The error mock now correctly returns an object with an
errorproperty containing a message, which aligns with the new error handling approach in the application.
105-111: Consistently updated all mock objects to match new response structure.All instances of mock strategy details have been updated to include the
valueproperty, ensuring consistency throughout the test file.Also applies to: 129-135, 170-176
122-123: Updated error message expectation.The expectation for error messages has been updated to match the new error handling approach and display format.
150-150: Consistent mock implementation across tests.The mock implementation for
getStrategyDetailshas been consistently updated across all test cases.Also applies to: 188-188
packages/ui-components/src/lib/components/deployment/SelectToken.svelte (2)
22-26: Good improvement to error handling in onMount lifecycle.The updated implementation properly checks for errors in the API response before processing the result, which enhances robustness. This matches the error handling pattern used elsewhere in the codebase.
38-42: Consistent error handling implementation in getInfoForSelectedToken.The error checking pattern has been correctly applied here too, ensuring consistent error handling throughout the component.
packages/orderbook/test/js_api/gui.test.ts (2)
353-363: Well-designed utility function for handling encoded result responses.This utility function centralizes error handling for WASM encoded results, improving code consistency and reducing duplication throughout the test file. The type parameter ensures proper return typing, making the tests more type-safe.
366-368: Good application of the utility function.Refactoring to use the
extractWasmEncodedDatahelper simplifies the test code and provides consistent error handling. The type annotation makes it clear what data structure is expected.Cargo.toml (1)
56-56:Details
❓ Verification inconclusive
Verify stability of the Git-based dependency.
The dependency has been updated from a versioned package to a specific Git commit. While this gives access to the latest features needed for the WASM macro changes, ensure this repository and commit are stable for production use.
It would be helpful to check if there are plans to eventually move back to a versioned release:
🌐 Web query:
What are best practices for using Git dependencies in Rust Cargo.toml instead of versioned crates?💡 Result:
When using Git dependencies in Rust's
Cargo.toml, follow these best practices to maintain stability, security, and reproducibility:Specifying Git Dependencies
Basic syntax: Declare dependencies with
git, and optionally includebranch,tag, orrevfor precise control:[dependencies] my_crate = { git = "https://github.com/user/repo.git", branch = "main" }This ensures Cargo uses the latest commit on the specified branch[1][4][7].
Explicit references: Prefer
rev(commit hash) ortagover branches for immutable references:my_crate = { git = "https://github.com/user/repo.git", tag = "v1.2.0" }Branches like
maincan introduce breaking changes unexpectedly[1][3][7].Version Validation
- Combine
gitwithversionto enforce semver compatibility:Cargo checks the crate's declared version against this constraint[7][13].my_crate = { git = "https://github.com/user/repo.git", version = "1.2" }Workspace and Local Development
- Workspace dependencies: For multi-crate repositories, ensure the Git URL points to a workspace root. Cargo automatically locates sub-crates[5].
- Local overrides: Temporarily override dependencies during development using
.cargo/config:This avoids modifying[paths] my_crate = "../local/path"Cargo.tomlfor local testing[1][7].Security and Maintenance
- Audit dependencies: Use tools like
cargo-auditandcargo-crevto review dependency trees, especially for private repositories[14].- Avoid wildcards: Never use
version = "*"with Git dependencies, as it bypasses version validation[4][13].Publishing Considerations
- Registry fallback: For publishable crates, combine Git and registry sources:
This allows local Git usage while ensuring registry compatibility[7][10].[dependencies] my_crate = { version = "1.2", git = "https://github.com/user/repo.git" }Common Pitfalls to Avoid
- Mixed dependency sources: Ensure
build-dependenciesand regular dependencies use identical Git references[5].- Workspace structure: Verify Git repositories containing workspaces have proper crate paths[5][12].
- CI/CD compatibility: Use
--lockedin CI pipelines to prevent unexpected Git dependency updates:[8][9]cargo build --lockedFor long-term stability, prefer crates.io versions when possible. Git dependencies work best for active development or private crates, but introduce deployment complexity for published packages[10][11]. When using them, prioritize explicit commit/tag references and implement regular dependency audits.
Citations:
- 1: https://web.mit.edu/rust-lang_v1.25/arch/amd64_ubuntu1404/share/doc/rust/html/cargo/reference/specifying-dependencies.html
- 2: https://users.rust-lang.org/t/cargo-toml-dependencies-and-git-branches/61234
- 3: https://internals.rust-lang.org/t/make-version-selection-work-on-repository-dependencies/17138
- 4: https://dev.to/alexmercedcoder/introduction-to-cargo-and-cargotoml-2l86
- 5: https://users.rust-lang.org/t/how-can-i-specify-a-git-dependency-from-a-workspace/63999
- 6: rust-lang/cargo#7497
- 7: https://doc.rust-lang.org/cargo/reference/specifying-dependencies.html
- 8: https://doc.rust-lang.org/cargo/commands/cargo-add.html
- 9: https://users.rust-lang.org/t/best-practices-for-bumping-versions-in-cargo-toml/111565
- 10: https://users.rust-lang.org/t/help-cargo-package-with-github-dependencies/96275
- 11: https://docs.scrt.network/secret-network-documentation/development/readme-1/setting-up-your-environment/cargo.toml-best-practices-crates-vs-dependencies
- 12: rust-lang/cargo#13121
- 13: https://users.rust-lang.org/t/best-practice-in-specifying-dependency-versions-in-cargo-toml/54007
- 14: https://users.rust-lang.org/t/cargo-toml-dependencies-security-best-practices/29399
Ensure Git Dependency Stability and Maintainability
The use of an explicit commit hash (
rev = "939b59f7b52798e04a988cef435030d78eb11e91") forwasm-bindgen-utilsaligns with current best practices for reproducible builds. However, please verify that this commit is stable and that the repository remains well-maintained for production use. Additionally, consider whether there's a plan to transition back to a versioned release or to combine the Git dependency with a version constraint once a stable release is available, as this approach can simplify long-term dependency management.
- Confirm that the repository state at the specified commit meets production standards.
- Evaluate future options for using a versioned dependency to maintain semver compatibility and reduce maintenance overhead.
packages/ui-components/src/__tests__/DepositInput.test.ts (2)
20-29: Good refactoring to use real DotrainOrderGui instance.Using a real instance with mocked methods on its prototype provides more realistic testing than a completely mocked object. This approach better captures actual component behavior.
Consider using a more explicit mocking approach like:
- (DotrainOrderGui.prototype.getDeposits as Mock).mockReturnValue({ + vi.spyOn(DotrainOrderGui.prototype, 'getDeposits').mockReturnValue({This would make the mocking intention clearer and provide better type safety.
63-63: Properly updated assertion to match new implementation.Assertion correctly verifies interaction with the instance rather than a mock function.
packages/webapp/src/lib/services/handleGuiInitialization.ts (4)
1-1: Good update to include WasmEncodedResult typeIncluding the
WasmEncodedResulttype in the import statement properly reflects the changes in implementation that use this type for improved error handling.
10-11: Improved initialization patternGood change to initialize
guidirectly as a new instance rather than starting with null. This ensures that the instance exists before calling methods on it, and the newresultvariable provides proper typing for the WASM operation results.
21-24: Enhanced error handling with more detailThis improved error handling approach correctly checks for errors in the result and provides more detailed error information by throwing the actual error message from the WASM result.
27-30: Consistent error handling patternThe error handling pattern is consistently applied in both branches of the conditional, which is good for maintainability and predictable behavior.
packages/ui-components/src/__tests__/TokenIOInput.test.ts (7)
5-5: Good shift to using actual DotrainOrderGui classImporting the actual class instead of using a mock object improves test fidelity by ensuring tests are aligned with the real implementation.
7-7: Simplified type definitionChanged from an exported type to a local type alias, which is appropriate since this type is only used within this test file.
10-14: Improved test setup with actual GUI instanceUsing a real instance of
DotrainOrderGuiinstead of a mock object improves test fidelity and makes the tests more resistant to interface changes.
21-27: Updated mock structure to match implementationThe mock token info structure now correctly includes a nested
valueproperty, which matches the actual implementation's return structure.
29-52: Comprehensive test setup with proper method mockingThe updated setup correctly creates a real instance and mocks its prototype methods. The
getVaultIdsmock now also follows the proper structure with a nestedvalueproperty.
53-64: Updated props to use real GUI instanceBoth
mockPropsandoutputMockPropsnow correctly use the realguiInstancereference, ensuring proper test interaction with the mocked methods.
88-88: Updated assertions to use instance methodsTest assertions now correctly reference the instance methods on
guiInstanceinstead of the previous mock object, maintaining consistency with the updated setup.Also applies to: 95-95, 125-125
packages/webapp/src/__tests__/handleGuiInitialization.test.ts (7)
1-4: Updated imports to support new testing approachThe imports now correctly include the
Mocktype and theDotrainOrderGuiandWasmEncodedResulttypes from the actual module, supporting the shift to using real instances in tests.
7-7: Switch to real DotrainOrderGui instanceUsing a real
DotrainOrderGuiinstance in tests rather than a custom mock object improves test fidelity and makes tests more robust against interface changes.Also applies to: 14-14
18-18: Correct mocking of static vs instance methodsThe test now properly distinguishes between static methods (deserializeState) and instance methods (chooseDeployment), correctly mocking each and verifying their calls appropriately.
Also applies to: 23-28
32-35: Improved mock implementation with proper typingThe mock for
chooseDeploymentnow correctly returns an object of typeWasmEncodedResult<void>, matching the actual implementation's return type.
39-45: Updated assertions to align with new instance-based approachAssertions now correctly check that the result contains the
guiInstanceand properly verify calls to both static and instance methods.
49-49: Consistent instance method mocking patternThe test for the case with no state URL follows the same pattern, correctly mocking the instance method and verifying its calls, maintaining consistency throughout the test file.
Also applies to: 53-59
63-63: Updated error handling testThe error test case now correctly mocks the rejection on the instance method rather than the static method, aligning with the implementation changes.
packages/ui-components/test-setup.ts (2)
15-19: Improved code formatting for readabilityThe page object definition has been reformatted for better readability, using multiple lines for a clearer structure.
56-87: Comprehensive mock implementation for DotrainOrderGuiExcellent addition of a complete mock for the
DotrainOrderGuiclass, including both static and instance methods. This centralized mock definition ensures consistency across all tests and makes it easier to maintain as the interface evolves.packages/ui-components/src/lib/components/deployment/DeploymentSteps.svelte (8)
8-8: Import statement updated to include TokenInfo type.The import statement has been updated to include the TokenInfo type, which supports the type change on line 51.
51-51: Type definition updated from AllTokenInfos to TokenInfo array.The type of
allTokenInfoshas been changed fromAllTokenInfostoTokenInfo[], which is more specific and aligns with the return value fromgui.getAllTokenInfos().
69-79: Improved error handling for field definitions retrieval.The implementation now properly checks for errors in the result from
gui.getAllFieldDefinitions()and throws an appropriate error message if one is found. This enhancement applies to both default and non-default field definitions.
87-91: Enhanced error handling for deposit fields retrieval.The implementation now properly checks for errors in the result from
gui.getCurrentDeployment()and throws an appropriate error message if one is found.
102-106: Improved error handling for token inputs retrieval.The implementation now properly checks for errors in the result from
gui.getCurrentDeployment()and throws an appropriate error message if one is found.
114-118: Enhanced error handling for token outputs retrieval.The implementation now properly checks for errors in the result from
gui.getCurrentDeployment()and throws an appropriate error message if one is found.
149-153: Improved error handling for token info retrieval in onSelectTokenSelect.The implementation now properly checks for errors in the result from
gui.getAllTokenInfos()and throws an appropriate error message if one is found.
220-224: Enhanced error handling for token info retrieval in areAllTokensSelected.The implementation now properly checks for errors in the result from
gui.getAllTokenInfos()and throws an appropriate error message if one is found.packages/ui-components/src/__tests__/FieldDefinitionInput.test.ts (3)
8-8: Changed from mock object to real instance with mocked methods.The test now uses a real instance of
DotrainOrderGuiinstead of a completely mocked object, which better represents how the component would interact with the class in production.
23-28: Improved test setup with more realistic instance usage.The test setup has been improved to:
- Create a real instance of DotrainOrderGui
- Mock specific methods directly on the prototype
- Use more realistic return value structures
This approach ensures the tests are more closely aligned with production usage patterns.
129-134: Updated mock implementation with proper result structure.The mock implementation of
getFieldValuenow returns a properly structured object withvalueproperty containingisPresetandvalue, which aligns with the expected data structure in the component.packages/ui-components/src/__tests__/DeploymentSteps.test.ts (3)
1-1: Added Mock type import for improved type safety in tests.The import statement has been updated to include the Mock type from vitest, which helps ensure type safety when mocking methods.
643-651: Streamlined test setup with direct prototype method mocking.The
setGuifunction has been simplified to mock DotrainOrderGui prototype methods directly rather than creating a complex mock object. This approach is more maintainable and better represents how the component interacts with the class.
795-810: Updated mock implementation with proper result structure.The mock implementation of
getAllTokenInfosnow returns a properly structured object with avalueproperty containing the token array, which aligns with the expected data structure in the component.crates/js_api/src/gui/deposits.rs (5)
13-32: Added comprehensive deposit validation method.A new
check_depositsmethod has been added that thoroughly validates:
- Each deposit has a token selected
- The token exists in the deposits map
This improves error handling by providing specific error messages for different validation failures, which will help with debugging and user feedback.
35-35: Updated export macro from wasm_bindgen to wasm_export.The export annotation has been changed from
#[wasm_bindgen]to#[wasm_export], which is likely part of a broader refactoring of the WASM binding approach.
47-47: Enhanced method export with explicit return type information.The
getDepositsmethod now includes explicit return type information usingunchecked_return_type = "TokenDeposit[]", which improves the TypeScript type definitions generated for JavaScript consumers.
83-83: Standardized method exports with explicit return type information.All exported methods now include explicit return type information using the
unchecked_return_typeattribute, which improves the TypeScript type definitions generated for JavaScript consumers.Also applies to: 115-115, 122-122, 128-128, 143-143
144-145: Improved error handling in has_any_deposit method.The
has_any_depositmethod now returns aResult<bool, GuiError>instead of just abool, which aligns with the error handling pattern used throughout the codebase and allows for proper error propagation.crates/js_api/src/gui/order_operations.rs (13)
89-89: Confirm usage of#[wasm_export]macro.
This macro replacement is consistent with the rest of the file’s changes. No issues found.
187-223: Reviewcheck_allowanceslogic and potential randomization concern.
The function loops through deposits, uses a randomly generated vault ID (rand::random()), and returns a list ofTokenAllowance. Ensure that using a random vault ID is intended and won't introduce reproducibility or consistency problems in production. If you need deterministic handling, consider a stable unique ID instead.
229-232: Exported function annotation looks good.
Definingunchecked_return_type = "ApprovalCalldataResult"is consistent with similar methods.
291-294: Export annotation forgenerateDepositCalldatas.
No logic changes here, just the updated export macro and unchecked return type. Looks correct.
335-336: Export annotation forgenerateAddOrderCalldata.
Switching to#[wasm_export]and specifying the return type as"string"aligns with the new approach.
349-352: Export annotation forgenerateDepositAndAddOrderCalldatas.
No concerns; reusing the pattern for#[wasm_export].
353-353: Check the method signature alignment.
pub async fn generate_deposit_and_add_order_calldatas(...)is consistent with the doc comment above. Looks good.
373-375: Concise multicall assembly.
Constructing the full multicall data withabi_encode()is solid; returningBytes::copy_from_sliceis consistent with your pattern.
378-378: Export annotation forsetVaultId.
No other changes. Implementation remains the same.
426-426: Export annotation forhasAnyVaultId.
Usingmap.values().any(|ids| …)is a concise check for at least one vault ID. No issues found.Also applies to: 429-429
432-432: Export annotation forupdateScenarioBindings.
Straightforward macro update. No additional remarks.
439-442: Export annotation forgetDeploymentTransactionArgs.
unchecked_return_typeensures JS sees the correct structure; everything else is standard.
482-482: Ensure clarity ondeposit_and_add_order_calldatausage.
This line sets the final deployment calldata. Confirm that combining deposit and add order remains valid if no deposits were needed.crates/js_api/src/gui/mod.rs (16)
21-21: Imports updated to align with macros.
ImportingHashMap,BTreeMap, andwasm_exportmatches the revised code approach.Also applies to: 24-24
51-64: New constructor forDotrainOrderGui.
Initializes dummy order and default states for fields. Initialization logic looks straightforward.
66-67: Begin#[wasm_export] impl DotrainOrderGui.
Switching from#[wasm_bindgen]to#[wasm_export]is consistent with the rest of the module.
68-74:getDeploymentKeysfunction.
Defines the JS name and setsunchecked_return_type = "string[]". Implementation calls intoDotrainOrder. No immediate concerns.
76-82:chooseDeploymentmethod changes.
Now returningResult<(), GuiError>instead of returning a re-instantiatedDotrainOrderGui. This appears simpler and clarifies side effects.
90-94: Assigningself.dotrain_order,self.selected_deployment, andself.state_update_callback.
Straightforward updates of internal state. Ensure the callback reference remains valid in downstream usage.
97-98: Added#[wasm_export(js_name = "getGuiConfig", unchecked_return_type = "GuiCfg")].
No logic changes, just macro usage for JS integration.
107-110:getCurrentDeploymentmacro annotation.
Again, consistent use of#[wasm_export]. Implementation remains the same.
126-127: Added#[wasm_export(js_name = "getTokenInfo", unchecked_return_type = "TokenInfo")].
Exposes token info retrieval to JS. Implementation looks unchanged.
166-167: Added#[wasm_export(js_name = "getAllTokenInfos", unchecked_return_type = "TokenInfo[]")].
Provides a list of tokens to JS. Implementation reads fromget_select_tokens. LGTM.
195-205:get_strategy_detailsannotation.
This method is newly exported with#[wasm_export]. The logic for parsing strategy details remains intact.
206-217:get_deployment_detailsannotation and return type.
ReturningMap<string, NameAndDescriptionCfg>in JS clarifies usage. No issues found.
219-232:get_deployment_detailannotation.
Expands the newly introduced deployment details mapping to retrieve a single detail. Straightforward.
234-245:generate_dotrain_textannotation.
No logic changes beyond export macro. Composes finaldotraintext.
246-255:get_composed_rainlangannotation.
Method runsupdate_scenario_bindingsthen composes the final Rainlang text. Looks consistent.
342-350: Wasm-encoded error conversion.
Implements bridging fromGuiErrortoWasmEncodedError. Replacing#[wasm_bindgen]is consistent with the changes.crates/js_api/src/gui/field_values.rs (7)
51-51:#[wasm_export]block start forDotrainOrderGui.
Moves field value methods into the new export macro block. No concerns.
53-54:saveFieldValueannotation.
Method signature and logic remain unchanged; now exported with#[wasm_export].
75-75:saveFieldValuesannotation.
Applies the same pattern to bulk saving. No issues noted.
83-83:removeFieldValueannotation.
Method logic is unchanged. Just adapts the macro.
117-120:getAllFieldValuesannotation.
Generates a vector of field bindings mapped to their user- or preset-based values. Straightforward.
146-146:getAllFieldDefinitionsand filtering logic.
The new annotation ensures correct exposure. The filtering by default presence is well-structured.Also applies to: 148-150
166-167:getMissingFieldValuesannotation.
Returns a list of missing field names. This neatly supports UI checks for incomplete configurations.
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
packages/orderbook/test/js_api/gui.test.ts (1)
1-1797: 🧹 Nitpick (assertive)Consider breaking this large test file into smaller modules.
This file is quite long (~1800 lines) which can make it difficult to maintain. Consider splitting it into smaller, more focused test files organized by functionality (e.g., deposit tests, vault ID tests, token selection tests) for better maintainability.
🧰 Tools
🪛 Biome (1.9.4)
[error] 1-1: A Node.js builtin module should be imported with the node: protocol.
Using the node: protocol is more explicit and signals that the imported module belongs to Node.js.
Unsafe fix: Add the node: protocol.(lint/style/useNodejsImportProtocol)
[error] 444-444: This let declares a variable that is only assigned once.
'deploymentDetail' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
[error] 535-535: This let declares a variable that is only assigned once.
'gui' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
[error] 617-617: This let declares a variable that is only assigned once.
'gui' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
[error] 803-803: This let declares a variable that is only assigned once.
'gui' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
[error] 856-856: This let declares a variable that is only assigned once.
'serializedState' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
[error] 907-907: This let declares a variable that is only assigned once.
'gui' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
[error] 931-931: This let declares a variable that is only assigned once.
'result' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
[error] 938-938: This let declares a variable that is only assigned once.
'testDotrain' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
[error] 983-983: This let declares a variable that is only assigned once.
'testDotrain' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
[error] 1006-1006: This let declares a variable that is only assigned once.
'gui' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
[error] 1026-1026: This let declares a variable that is only assigned once.
'dotrain2' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
[error] 1168-1168: This let declares a variable that is only assigned once.
'result' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
[error] 1203-1203: This let declares a variable that is only assigned once.
'result' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
[error] 1248-1248: This let declares a variable that is only assigned once.
'result' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
[error] 1291-1291: This let declares a variable that is only assigned once.
'result' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
[error] 1315-1315: This let declares a variable that is only assigned once.
'testDotrain' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
[error] 1364-1364: This let declares a variable that is only assigned once.
'testDotrain' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
[error] 1402-1402: This let declares a variable that is only assigned once.
'guiConfig' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
[error] 1426-1426: This let declares a variable that is only assigned once.
'testDotrain' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
[error] 1442-1442: This let declares a variable that is only assigned once.
'missingFieldValues' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
[error] 1448-1448: This let declares a variable that is only assigned once.
'stateUpdateCallback' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
[error] 1456-1456: This let declares a variable that is only assigned once.
'testDotrain' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
[error] 1613-1613: This let declares a variable that is only assigned once.
'gui' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
[error] 1617-1617: This let declares a variable that is only assigned once.
'dotrain3' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
[error] 1749-1749: This let declares a variable that is only assigned once.
'dotrain3' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
packages/orderbook/test/js_api/gui.test.ts(16 hunks)packages/ui-components/test-setup.ts(1 hunks)packages/webapp/src/__tests__/DeployModal.test.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: git-clean
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-artifacts)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-test)
- GitHub Check: test
- GitHub Check: test
🔇 Additional comments (7)
packages/ui-components/test-setup.ts (1)
82-83: LGTM: New mock methods added correctlyThese new mock methods (
getDeploymentTransactionArgsandgenerateApprovalCalldatas) have been properly added to theDotrainOrderGuiprototype, consistent with the pattern used for other mock methods in this test setup file. They're correctly implemented usingvi.fn()for test mocking.packages/webapp/src/__tests__/DeployModal.test.ts (1)
31-31: Simplified deploymentCalldata structureThe
deploymentCalldataproperty has been simplified from an object structure to a simple string value ('0x'). This change aligns with the broader refactoring mentioned in the PR summary where WASM export attributes were changed and function signatures updated to return results directly.packages/orderbook/test/js_api/gui.test.ts (5)
353-363: Great addition of the extractWasmEncodedData utility!The new helper function effectively centralizes error handling logic and improves code organization by providing a consistent way to handle WASM encoded results throughout the tests.
1046-1048: LGTM: Good error handling refactor.The code now correctly uses the extract utility function for processing WASM results.
1370-1383: Good validation of error messages for token selection.The test properly verifies that appropriate error messages are shown when required tokens aren't selected, which helps ensure a good user experience.
1469-1487: Well-structured vault ID handling.The code appropriately uses the extract utility for the vault ID operations and contains proper assertions for verifying behavior.
1519-1522: Good error validation for invalid vault IDs.The test correctly verifies that the API properly rejects invalid vault IDs, which is important for data integrity.
| ) -> Result<DepositAndAddOrderCalldataResult, GuiError> { | ||
| #[wasm_export( | ||
| js_name = "generateDepositAndAddOrderCalldatas", | ||
| unchecked_return_type = "string" |
There was a problem hiding this comment.
wasn't the more explicit type we had before better?
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 (1)
packages/orderbook/test/js_api/gui.test.ts(16 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
packages/orderbook/test/js_api/gui.test.ts
[error] 1153-1153: This let declares a variable that is only assigned once.
'dotrain2' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: test
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-test)
- GitHub Check: test
- GitHub Check: Deploy-Preview
🔇 Additional comments (14)
packages/orderbook/test/js_api/gui.test.ts (14)
353-363: Great addition of a central error handling utility!The
extractWasmEncodedDatautility function centralizes error handling and value extraction from WASM results, reducing code duplication across the test file. This improves maintainability and readability.
1046-1048: Code refactored to use the new error handling pattern.The call to
checkAllowancesnow uses theextractWasmEncodedDatautility for error handling, making the test more robust and consistent with the error handling pattern.
1065-1067: Updated error handling pattern while retaining type annotations.The code now uses the
extractWasmEncodedDatautility while keeping the necessary@ts-expect-errorcomments to handle type mismatches between TypeScript definitions and actual implementation.Also applies to: 1069-1075
1115-1117: Consistent error handling for deposit calldata generation.The deposit calldata generation now follows the same error handling pattern as other WASM operations, while maintaining the existing type workarounds.
Also applies to: 1119-1124
1189-1190: Simplified order calldata generation with consistent error handling.Now using the
extractWasmEncodedDatautility for handling errors fromgenerateAddOrderCalldata, making the error handling consistent across the codebase.
1267-1270: Consistent error handling for multicall calldata generation.The multicall calldata generation now follows the standardized error handling pattern used throughout the test suite.
1310-1313: Standardized error handling for multicall operations.This is another example of improved error handling consistency for multicall operations with missing field values.
1374-1377: Unified error handling for multicall with vault ID handling.The test handling for multicalls without vault IDs now follows the standard error handling pattern.
1493-1511: Improved vault ID state verification with centralized error handling.The code now uses the
extractWasmEncodedDatautility to safely extract vault ID information from Map objects, ensuring consistent error handling when working with complex data structures.
1522-1541: Consistent vault ID management with robust error handling.Further improvements to vault ID management with the centralized error handling approach, maintaining the same pattern across the codebase.
1544-1546: Added validation for vault ID format errors.The code now properly handles and tests for error cases when invalid vault ID formats are provided, improving the robustness of the test suite.
1572-1576: Refactored zero-amount deposit handling with consistent error extraction.The test for skipping deposits with zero amounts now uses the standard error handling utility, maintaining consistency in how errors are processed throughout the code.
1610-1628: Standardized error handling for deployment transaction arguments.The deployment transaction argument generation tests now use the centralized error handling utility, continuing the consistent pattern across the codebase.
1397-1407: Robust error validation for token selection requirements.The test suite now properly validates error cases when required tokens are not selected, providing better coverage of error conditions.
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 (4)
crates/js_api/src/gui/order_operations.rs(10 hunks)packages/orderbook/test/js_api/gui.test.ts(16 hunks)packages/ui-components/src/lib/components/deployment/getDeploymentTransactionArgs.ts(1 hunks)packages/ui-components/src/lib/types/transaction.ts(1 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
packages/orderbook/test/js_api/gui.test.ts (1)
packages/ui-components/src/lib/stores/transactionStore.ts (1)
DeploymentTransactionArgs(53-61)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-static)
- GitHub Check: test
🔇 Additional comments (38)
packages/ui-components/src/lib/types/transaction.ts (1)
3-7: Improved import structureConsolidating related imports into a single import statement improves code organization and readability.
packages/ui-components/src/lib/components/deployment/getDeploymentTransactionArgs.ts (1)
37-41: Enhanced error handlingThe new implementation properly checks for errors in the result before attempting to destructure the value. This prevents potential runtime exceptions if the result contains an error.
packages/orderbook/test/js_api/gui.test.ts (24)
353-363: Good utility function for centralized error handlingThe
extractWasmEncodedDatautility function centralizes error handling logic for WASM result objects, making the code more maintainable and less error-prone.
1046-1048: Improved return value handling with error checkingUpdated to use the new
extractWasmEncodedDatautility function for proper error handling of thecheckAllowancesresult.
1065-1067: Consistent error handling pattern appliedThis change follows the same pattern as other function calls by using
extractWasmEncodedDatafor thegenerateApprovalCalldatasresult.
1115-1117: Consistent error handling pattern appliedUpdated to use the new
extractWasmEncodedDatautility function for proper error handling of thegenerateDepositCalldatasresult.
1119-1125: Another type definition mismatchThis is another instance where
@ts-expect-erroris used due to a type mismatch between the actual structure returned and the TypeScript type definition.Similar to the previous comment, consider updating the TypeScript type definitions to match the actual structure returned by the WebAssembly functions to eliminate the need for
@ts-expect-errorcomments.
1130-1133: Consistent error handling pattern appliedUpdated to use the new
extractWasmEncodedDatautility function for proper error handling of thegenerateDepositCalldatasresult.
1165-1166: Consistent error handling pattern appliedUpdated to use the new
extractWasmEncodedDatautility function for proper error handling of thegenerateAddOrderCalldataresult.
1200-1201: Consistent error handling pattern appliedUpdated to use the new
extractWasmEncodedDatautility function for proper error handling of thegenerateAddOrderCalldataresult.
1243-1245: Consistent error handling pattern appliedUpdated to use the new
extractWasmEncodedDatautility function for proper error handling of thegenerateDepositAndAddOrderCalldatasresult.
1286-1288: Consistent error handling pattern appliedUpdated to use the new
extractWasmEncodedDatautility function for proper error handling of thegenerateDepositAndAddOrderCalldatasresult.
1350-1352: Consistent error handling pattern appliedUpdated to use the new
extractWasmEncodedDatautility function for proper error handling of thegenerateDepositAndAddOrderCalldatasresult.
1369-1371: Improved consistency in async function result handlingUpdated to use asynchronous patterns consistently with the new error handling approach.
1373-1375: Direct error checkingNow checking the error property directly from the result objects, which is more explicit and aligns with the error handling pattern throughout the file.
1376-1378: Direct error checkingConsistent error checking pattern applied to the
generateDepositCalldatasresult.
1379-1381: Direct error checkingConsistent error checking pattern applied to the
generateAddOrderCalldataresult.
1382-1384: Direct error checkingConsistent error checking pattern applied to the
generateDepositAndAddOrderCalldatasresult.
1430-1431: Consistent error handling pattern appliedUpdated to use the new
extractWasmEncodedDatautility function for proper error handling of thechooseDeploymentresult.
1436-1438: Direct error checking for improved clarityUsing the error property from the result object directly improves readability and aligns with the error handling pattern throughout the file.
1439-1441: Direct error checking for consistencyThe same consistent error checking pattern applied to the
generateDepositAndAddOrderCalldatasresult.
1469-1470: Improved use of extraction utilityUsing the
extractWasmEncodedDatautility function to properly handle potential errors from thehasAnyVaultIdresult.
1473-1486: Enhanced type safety with proper error handlingThis segment demonstrates the proper use of the extraction utility to safely access nested values with type checking.
1497-1499: Improved type handling with extraction utilityUsing the extraction utility to properly handle the
getVaultIdsresult with appropriate type parameters.
1586-1588: Consistent error handling with extraction utilityUpdated to use the
extractWasmEncodedDatautility function for proper error handling of thegetDeploymentTransactionArgsresult with appropriate type parameters.
1601-1603: Consistent error handling pattern appliedUpdated to use the new
extractWasmEncodedDatautility function for proper error handling of thegetDeploymentTransactionArgsresult.crates/js_api/src/gui/order_operations.rs (12)
63-65: Improved struct definition locationThe
WithdrawCalldataResultstruct has been moved to a more appropriate location in the file, improving code organization.
107-107: Updated WASM export attributeChanged from
#[wasm_bindgen]to#[wasm_export]for the implementation block, which aligns with the project's updated export strategy.
205-208: Enhanced WASM export with explicit return typeThe
#[wasm_export]attribute withunchecked_return_typeprovides more explicit type information for JavaScript consumers than the previous#[wasm_bindgen]attribute.
244-247: Enhanced WASM export with explicit return typeUsing
#[wasm_export]withunchecked_return_typeprovides better type information for JavaScript consumers.
306-309: Enhanced WASM export with explicit return typeUsing
#[wasm_export]withunchecked_return_typeprovides better type information for JavaScript consumers.
350-353: Enhanced WASM export with explicit return typeUsing
#[wasm_export]withunchecked_return_typeprovides better type information for JavaScript consumers.
369-372: Enhanced WASM export with explicit return typeUsing
#[wasm_export]withunchecked_return_typeprovides better type information for JavaScript consumers.
400-400: Enhanced WASM export with explicit return typeUsing
#[wasm_export]withunchecked_return_typeprovides better type information for JavaScript consumers.
417-417: Enhanced WASM export with explicit return typeUsing
#[wasm_export]withunchecked_return_typeprovides better type information for JavaScript consumers.
445-445: Enhanced WASM export with explicit return typeUsing
#[wasm_export]withunchecked_return_typeprovides better type information for JavaScript consumers.
451-451: Enhanced WASM export with explicit return typeUsing
#[wasm_export]withunchecked_return_typeprovides better type information for JavaScript consumers.
458-461: Enhanced WASM export with explicit return typeUsing
#[wasm_export]withunchecked_return_typeprovides better type information for JavaScript consumers.
2 participants
Caution
Do not merge before #1457
Motivation
Solution
Checks
By submitting this for review, I'm confirming I've done the following:
fix #1404
Summary by CodeRabbit
Refactor
deploymentCalldatain mock properties for easier handling.Tests
DotrainOrderGuiinstance.Chore