Implement wasm macro on DotrainOrderGui deposit methods#1456
Conversation
WalkthroughThis pull request introduces modifications to the Changes
Sequence Diagram(s)sequenceDiagram
participant JS as JavaScript Client
participant Gui as DotrainOrderGui
participant Validator as Deposit Validator
JS->>Gui: call check_deposits()
Gui->>Validator: Validate each deposit (check token selected & existence)
Validator-->>Gui: Return validation result or error
Gui-->>JS: Return Ok or GuiError
sequenceDiagram
participant Test as Test Suite
participant Extractor as extractWasmEncodedData
participant Gui as DotrainOrderGui
Test->>Gui: call getDeposits() / other methods
Gui-->>Test: Return raw result (value/error)
Test->>Extractor: Process raw result
Extractor-->>Test: Return extracted data or error details
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 (4)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
packages/ui-components/src/lib/components/deployment/DepositInput.svelte (1)
54-66: 🛠️ Refactor suggestionMissing error handling in handlePresetClick function
Similar to
handleInput, thehandlePresetClickfunction throws an error but doesn't have a try/catch block to handle it. This could cause unhandled exceptions in the UI.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 getting deposits'; + } } }
🧹 Nitpick comments (4)
packages/ui-components/src/lib/components/deployment/DepositInput.svelte (1)
60-64: Consider extracting error handling pattern to a utility functionThe error handling pattern is repeated multiple times in this file (and across other files). Consider creating a utility function to handle this common pattern to reduce duplication and improve maintainability.
+ // Add this to a utils file + function handleResult<T>(result: { error?: { msg: string }, value?: T }): T { + if (result.error) { + throw new Error(result.error.msg); + } + return result.value as T; + } // Then use it like: - const deposits = gui.getDeposits(); - if (deposits.error) { - throw new Error(deposits.error.msg); - } - currentDeposit = deposits.value.find((d) => d.token === deposit.token?.key); + const deposits = handleResult(gui.getDeposits()); + currentDeposit = deposits.find((d) => d.token === deposit.token?.key);crates/js_api/src/gui/deposits.rs (2)
12-33: Consider adding deposit context in error messages.The
check_depositsmethod appropriately checks for missing or unselected tokens. However, to aid in debugging, you might consider including more context—such as deposit index or deposit name—in the error message. This additional detail can be especially helpful when multiple deposits fail validation.
83-113: Validate amount format if necessary.The
save_depositmethod conditionally removes or updates deposits based on the provided amount string. Consider validating numeric formats if the deposit amount must be strictly numeric. Otherwise, code logic looks straightforward and consistent with project conventions.crates/js_api/src/gui/mod.rs (1)
166-193: Potential parallelization for retrieving many tokens.
get_all_token_infosprocesses each token key sequentially. Consider fetching token info concurrently if performance becomes a concern, especially with substantial token lists.
📜 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 (22)
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/mod.rs(8 hunks)packages/orderbook/test/js_api/gui.test.ts(38 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__/StrategyPage.test.ts(10 hunks)packages/ui-components/src/__tests__/TokenIOInput.test.ts(4 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/SelectToken.svelte(2 hunks)packages/ui-components/src/lib/components/deployment/StrategyPage.svelte(1 hunks)packages/ui-components/src/lib/components/deployment/TokenIOInput.svelte(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: test
- GitHub Check: build-tauri (ubuntu-22.04, true)
- GitHub Check: standard-tests (ubuntu-latest, ob-rs-test, true)
- GitHub Check: git-clean
- GitHub Check: test
- GitHub Check: Deploy-Preview
- GitHub Check: test
🔇 Additional comments (75)
packages/webapp/test-setup.ts (1)
11-20: Good update to the test mocks to align with the WASM implementation changesThe mocks correctly mirror the structure of the actual
DotrainOrderGuiclass, adding both a static methoddeserializeStateand an instance methodchooseDeployment. This will ensure the tests remain valid with the updated error handling pattern across the application.packages/webapp/src/routes/deploy/[strategyName]/[deploymentKey]/+layout.ts (1)
12-16: Improved error handling implementationThe updated code properly handles potential errors from the
getDeploymentDetailmethod by checking for the presence oferrorin the result object before destructuring thevalue. This change makes the code more robust by explicitly handling error conditions rather than assuming the call will always succeed.packages/ui-components/src/lib/components/deployment/DeploymentsSection.svelte (1)
13-17: Proper error handling pattern implementedThe updated code correctly checks for errors in the result object before accessing the value, which is consistent with the WASM export pattern now used in the DotrainOrderGui implementation. This ensures that errors are properly propagated and handled rather than causing runtime exceptions.
packages/ui-components/src/lib/components/deployment/DepositInput.svelte (2)
27-31: Error handling added to getDeposits methodThe code now properly checks for errors in the deposits result before accessing the value. This is a good improvement that aligns with the WASM export pattern.
41-45: Proper error handling for getTokenInfoAdding error checking before accessing the result value is the correct approach for handling the WASM export pattern. This change improves the robustness of the component.
packages/webapp/src/routes/deploy/+layout.ts (1)
13-17: Enhanced error handling for strategy details retrievalThe code now properly handles errors returned from
DotrainOrderGui.getStrategyDetails()by checking for the presence of an error in the result object and throwing it explicitly, rather than relying on the function to throw errors directly. This makes the error handling more explicit and consistent with other parts of the codebase.packages/ui-components/src/lib/components/deployment/SelectToken.svelte (2)
22-26: Improved error handling for token information retrievalThe code now properly checks for errors in the result returned from
gui.getTokenInfo()before proceeding, which ensures that any issues are properly propagated as exceptions.
38-42: Consistent error handling for token information retrievalThe same error handling pattern is applied here as in the previous token info retrieval, maintaining consistency throughout the component.
packages/ui-components/src/lib/components/deployment/StrategyPage.svelte (1)
30-34: Enhanced error handling for strategy detailsThe code now properly handles errors returned from
DotrainOrderGui.getStrategyDetails()by explicitly checking for the presence of an error in the result object and throwing it with a meaningful message. This is consistent with the error handling improvements made across the codebase.crates/common/src/dotrain_order/mod.rs (2)
12-12: Streamlined imports with additional functionalityThe import has been modified to include
default_documentalong withYamlErrorandYamlParsable, which is needed for the newdummy()method implementation.
126-135: Added convenience method for creating default DotrainOrder instancesA new
dummy()method has been added to create default instances ofDotrainOrder, which is useful for testing and initialization purposes. The implementation correctly sets up a minimal valid instance with an empty dotrain string and a default document in the yaml structure.packages/ui-components/src/lib/components/deployment/TokenIOInput.svelte (1)
32-37: Improved error handling with structured response formatThe code has been refactored to handle responses using a structured error pattern. This is a good enhancement that makes error handling more explicit and consistent.
Note that the optional chaining operator (
?.) in line 32 onvault.token?.keyis still used while it was removed from theguiobject, suggestingguiis now expected to be defined at this point.packages/ui-components/src/lib/components/deployment/ComposedRainlangModal.svelte (1)
14-19:Details
❓ Verification inconclusive
Enhanced error handling with structured result
The implementation now correctly handles the structured result object returned by
getComposedRainlang(). This follows the same pattern as other components in the codebase, ensuring consistent error handling.Note that this implementation throws an error rather than setting an error state variable like some other components do. Consider whether this is intentional or if it should follow the same pattern as other components by displaying errors to the user rather than throwing them.
Confirm whether this error handling approach is consistent with the application's error handling strategy:
🌐 Web query:
What is the best practice for handling errors in Svelte components - throwing errors vs displaying error messages?💡 Result:
When handling errors in Svelte components, best practices depend on error type and context. Here's a structured approach:
Expected vs. Unexpected Errors
Expected Errors (e.g., validation failures, API timeouts):
- Throw structured errors using SvelteKit’s
errorhelper:import { error } from '@sveltejs/kit'; throw error(404, 'Resource not found');
- SvelteKit automatically renders the nearest
+error.sveltepage with$page.errorcontaining your message[2][8].- Avoid
try...catchin actions for expected validation errors. Instead, usefailfor form submissions to maintain response flow[6]:import { fail } from '@sveltejs/kit'; return fail(400, { message: 'Invalid email' });Unexpected Errors (e.g., runtime bugs, unhandled exceptions):
- Use error boundaries to isolate component crashes:
<svelte:boundary onerror={(error, reset) => logError(error)}> <ComponentThatMightCrash /> </svelte:boundary>
- Prevents app-wide crashes and allows recovery via
reset[4].- Log errors globally via
handleErrorhook for monitoring:export function handleError({ error }) { console.error(error); return { message: 'Internal Error' }; // Generic user-facing message }
Best Practices
Approach Use Case Example errorhelperHTTP/expected errors throw error(404, 'Page not found')failmethodForm validation errors return fail(400, { email: 'Invalid format' })Error boundaries Component rendering errors <svelte:boundary onerror={...}>Global listeners Uncaught exceptions/promise rejections window.onerror = (msg, source) => { ... }
Key Recommendations
- Avoid exposing sensitive details: Unexpected errors show a generic message (e.g., "Internal Error") by default[12].
- Type safety: Define
App.PageErrorto standardize error structures[8].- Proactive monitoring: Use tools like Zipy for error tracking and session replay[1][3][10].
Example Flow
- Validation failure → Return
failwith user-facing message.- API error → Throw
errorwith status code.- Component crash → Catch with
<svelte:boundary>and log.- Global error → Log via
handleErrorand display fallback UI.By combining Svelte’s built-in tools with structured error boundaries, you balance user experience and debuggability.
Citations:
- 1: https://www.zipy.ai/blog/svelte-errors
- 2: https://svelte.dev/tutorial/kit/handleerror
- 3: https://www.zipy.ai/blog/debug-svelte-store-errors
- 4: https://joyofcode.xyz/catch-errors-during-rendering-with-svelte-error-boundaries
- 5: https://www.youtube.com/watch?v=xCzpZW-0cr4
- 6: https://dev.to/dev_michael/why-you-should-avoid-using-trycatch-in-sveltekit-actions-3dia
- 7: https://khromov.se/error-boundaries-in-svelte/
- 8: https://svelte.dev/docs/kit/errors
- 9: sveltejs/kit#9567
- 10: https://www.zipy.ai/blog/debug-svelte-component-lifecycle-errors
- 11: https://blog.openreplay.com/advanced-error-handling-in-svelte/
- 12: https://svelte.dev/tutorial/kit/error-basics
- 13: sveltejs/kit#6499
Confirm Consistent Error Handling Across Components
The updated implementation inpackages/ui-components/src/lib/components/deployment/ComposedRainlangModal.sveltenow handles the structured result fromgetComposedRainlang()correctly by throwing an error when one is returned. This approach aligns with Svelte’s best practices for handling certain error types—specifically, throwing structured errors (e.g., via SvelteKit’serrorhelper) for cases where a standardized error page is appropriate. However, note that other components display error messages inline by updating an error state instead of throwing errors.Please verify whether this modal’s error handling should continue throwing errors (to potentially leverage global error boundaries and SvelteKit’s mechanisms) or if it should be modified to display error messages inline for a more user-friendly experience.
- File:
packages/ui-components/src/lib/components/deployment/ComposedRainlangModal.svelte(Lines 14-19)- Diff Snippet Remains:
let result = await gui.getComposedRainlang(); if (result.error) { throw new Error(result.error.msg); } rainlangText = result.value; open = true;packages/ui-components/src/__tests__/DeploymentsSection.test.ts (3)
2-2: Added Mock type import for improved type safetyGood addition of the
type Mockimport from Vitest to ensure type safety when working with mocks.
19-19: Updated mock to match new result structureThe mock implementation has been updated to return a structured result with a
valueproperty, matching the new API contract. This ensures that tests align with the actual implementation.
37-39: Error case mock updated to use structured result patternThe error case mock has been updated to return an object with an
errorproperty instead of rejecting with an error. This aligns with the new error handling pattern implemented across the application.Cargo.toml (2)
80-80: Fixed formatting in path declarationMinor formatting improvement to the path declaration for
rain_orderbook_math.
56-56:Details
❓ Verification inconclusive
Updated wasm-bindgen-utils to use specific Git commit
The dependency has been changed from a version string to a Git repository reference with a specific commit hash. This ensures consistency and removes potential issues with version incompatibilities.
Make sure this specific commit has been thoroughly tested with your application, as pinning to a specific commit can make future upgrades more challenging.
🏁 Script executed:
#!/bin/bash # Check if this is the latest commit in the repo gh repo view rainlanguage/rain.wasm --json defaultBranchRef --jq '.defaultBranchRef.name' > branch.txt DEFAULT_BRANCH=$(cat branch.txt) COMMIT_HASH="939b59f7b52798e04a988cef435030d78eb11e91" LATEST_COMMIT=$(gh api repos/rainlanguage/rain.wasm/commits/$DEFAULT_BRANCH --jq '.sha') echo "Checking if commit $COMMIT_HASH is the latest in $DEFAULT_BRANCH branch..." echo "Latest commit: $LATEST_COMMIT" if [ "$COMMIT_HASH" = "$LATEST_COMMIT" ]; then echo "✅ The specified commit is the latest on the default branch." else echo "⚠️ The specified commit is NOT the latest on the default branch." echo "Checking when this commit was created..." COMMIT_DATE=$(gh api repos/rainlanguage/rain.wasm/commits/$COMMIT_HASH --jq '.commit.author.date') echo "Commit date: $COMMIT_DATE" echo "Checking for newer commits since then..." NEWER_COMMITS=$(gh api repos/rainlanguage/rain.wasm/commits --query="since=$COMMIT_DATE" --jq 'length') echo "Number of newer commits: $NEWER_COMMITS" fiLength of output: 2795
Action: Verify the chosen commit for wasm-bindgen-utils is intentional
It appears that the
wasm-bindgen-utilsdependency inCargo.tomlat line 56 now points to commit
939b59f7b52798e04a988cef435030d78eb11e91, which is not the latest commit on the defaultmainbranch (the latest isc8e1105dd5b524d57a8fdbc6545f493043e06d7c).
If you have deliberately pinned to this older commit, please ensure it has been thoroughly tested with your application. If not, consider whether upgrading to the latest commit might be beneficial to avoid potential issues arising from outdated functionality.
- File:
Cargo.toml(line 56)- Diff snippet:
wasm-bindgen-utils = { git = "https://github.com/rainlanguage/rain.wasm", rev = "939b59f7b52798e04a988cef435030d78eb11e91" }packages/webapp/src/lib/services/handleGuiInitialization.ts (4)
1-1: Good addition of the WasmEncodedResult type import.The explicit import of the WasmEncodedResult type improves type safety and readability by making it clear what kind of result structure we're dealing with.
10-11: Improved initialization with explicit type declaration.The change from initializing
guiasnullto creating a new instance directly is a good improvement. Adding the typedresultvariable enhances type safety for the error handling that follows.
21-24: Proper error handling implementation.The code now correctly checks for errors in the result object and throws a meaningful error message. This is an important improvement for robustness.
27-30: Consistent error handling pattern.The error handling pattern is consistently applied across different code paths, which is good for maintainability and debugging.
packages/ui-components/src/__tests__/TokenIOInput.test.ts (7)
5-5: Adding DotrainOrderGui import for actual instance testing.Good practice to import the actual class being tested rather than relying on mocks.
7-7: Type declaration changed to local scope.Changed from exported type to locally scoped type, which is appropriate since this type is only used within this test file.
10-10: Using actual DotrainOrderGui instance in tests.This change makes the tests more realistic by testing against an actual instance of the class rather than a mock object.
22-27: Updated mock structure with value property.The addition of the
valueproperty in the mock response aligns with the new WASM result pattern, making the tests consistent with the implementation.
29-51: Well-structured test setup with prototype mocking.The test setup is now more maintainable with clear mock implementations on the DotrainOrderGui prototype. The organization with clearing mocks first, then instantiating the class, and finally setting up mocks is a good practice.
53-64: Updated mock props to use the DotrainOrderGui instance.Props are correctly updated to use the actual instance, maintaining consistency throughout the test.
88-88: Updated method expectations for new instance-based approach.Test expectations are properly updated to check calls on the instance methods rather than static methods.
Also applies to: 95-95, 125-125
packages/ui-components/src/__tests__/DepositInput.test.ts (7)
6-6: Added DotrainOrderGui import for instance-based testing.Consistent with the changes in other test files, importing the actual class enhances test realism.
12-12: Using DotrainOrderGui instance in tests.Consistent with other test files, using an actual instance makes tests more realistic.
20-29: Improved test setup with instance and prototype mocking.The test setup now follows the same pattern as other test files, with clear initialization and mock implementations on the prototype.
33-38: Updated mock return structure with value property.The mock response now includes a
valueproperty, consistent with the WASM result pattern used throughout the codebase.
43-43: Updated component props to use DotrainOrderGui instance.All component renderings now consistently use the instance rather than a mock object.
Also applies to: 58-58, 72-72
63-63: Updated expectations to check instance methods.Test assertions now correctly verify calls on the instance methods.
Also applies to: 80-80
67-67: Consistent mocking approach using prototype.The mock for isDepositPreset follows the same pattern as other mocks, using the prototype approach.
packages/ui-components/src/lib/components/deployment/DeploymentSteps.svelte (5)
8-8: Updated imports to include TokenInfo type.Good practice to explicitly import the types being used, improving code readability and maintainability.
15-15: Simplified import by using type prefix.Using the
typeprefix in the import improves clarity about what's being imported.
51-51: Improved type specificity for allTokenInfos.Changed from a generic
AllTokenInfosto the more specificTokenInfo[], which enhances type safety.
78-82: Robust error handling for getCurrentDeployment.The code now properly checks for errors in the result object and throws appropriate exceptions with meaningful messages. This is implemented consistently across all calls to getCurrentDeployment.
Also applies to: 93-97, 105-109
140-144: Consistent error handling for getAllTokenInfos.The same error handling pattern is applied to getAllTokenInfos calls, maintaining consistency throughout the codebase and improving robustness.
Also applies to: 211-215
packages/ui-components/src/__tests__/StrategyPage.test.ts (3)
4-4: Updated import to support improved mocking approach.The addition of
type Mockfrom vitest enhances type safety when mocking the DotrainOrderGui methods.
22-29: Adjusted test structure to match new result handling pattern.The mockStrategyDetails object has been updated to include properties under a
valuefield, which reflects an improved API design pattern for handling results. This structured approach separates successful results from error conditions more clearly.
83-87: Enhanced error handling with structured error objects.The error handling has been improved to use a structured result with an
errorobject containing a message instead of rejecting with an error. This approach provides more consistent error handling and makes tests more predictable.packages/orderbook/test/js_api/gui.test.ts (5)
17-18: Updated imports to support structured result handling.Adding the
WasmEncodedResulttype import supports the new structured approach to result handling in the API.
354-364: Well-designed utility function for handling structured results.The new
extractWasmEncodedDatafunction provides a centralized and consistent way to handle the structured result pattern. It properly checks for errors and extracts values from theWasmEncodedResult<T>type, improving code readability and maintaining consistent error handling throughout the tests.
367-369: Consistent implementation of the new result handling pattern.This code segment exemplifies the new pattern used throughout the file - calling the method to get a WasmEncodedResult, then extracting the value using the utility function. This pattern is more robust for error handling while keeping the tests readable.
557-559: Direct testing of error cases without exceptions.Instead of expecting exceptions, the code now directly checks for error messages in the result object. This approach is more explicit and allows for more precise testing of error conditions.
1228-1230: Added method for removing field values.The addition of the
removeFieldValuemethod enhances the API's functionality by providing a way to clear field values. The test verifies that the method correctly empties the field values array.packages/webapp/src/__tests__/handleGuiInitialization.test.ts (4)
1-4: Enhanced imports for more accurate testing.Updated imports to include
type MockandWasmEncodedResulttypes, enabling more precise mocking and better type safety in tests.
7-14: Improved testing approach using actual class instance.Switching from a completely mocked object to a real
DotrainOrderGuiinstance with mocked methods provides more realistic testing. This approach better tests the actual integration between components and catches potential issues with method signatures or behavior.
32-36: Properly typed mock responses for consistent error handling.Using
WasmEncodedResult<void>as the return type for mocked methods ensures type consistency with the actual implementation. This approach maintains the structured result pattern throughout the codebase.
62-70: Comprehensive error handling test.The test properly verifies error handling behavior by mocking a rejection and checking that the handler returns the expected error message. This ensures robustness in error scenarios.
packages/ui-components/test-setup.ts (2)
15-19: Improved readability of mock store configuration.The page object has been reformatted for better readability, making it easier to understand the structure of the mocked data.
56-72: Comprehensive mock setup for DotrainOrderGui.Added a complete mock for DotrainOrderGui that includes both static and instance methods. This centralized mock setup ensures consistency across tests and simplifies individual test files by providing pre-mocked methods.
The comments explaining the TypeScript annotations are helpful for future maintainers.
crates/js_api/src/gui/deposits.rs (5)
47-81: Looks good; ensure all edge cases are tested.The
get_depositsmethod correctly transforms and returns deposit details, including preset validation. Ensure there is adequate test coverage for scenarios like invalid presets, empty strings, and missing tokens to confirm robust error handling.
115-120: Straightforward removal logic.
No issues found inremove_deposit. Removal is properly followed by a state update callback.
122-126: Return of presets is clear.
get_deposit_presetsgracefully returns either the stored presets or an empty vector. This is a clear and concise approach.
128-141: Appropriate missing-deposits check.
get_missing_depositsproperly identifies deposits that are not yet defined. No concerns here.
143-146: Consistent boolean check.
has_any_depositreturning aResult<bool, GuiError>is consistent with the rest of the API, although it does not appear to require error handling internally. This is fine for uniformity.crates/js_api/src/gui/mod.rs (15)
21-24: Import changes acknowledged.The updated imports for
HashMapand thewasm_exportmacro look fine.
51-51: No concerns regarding this change.The additional blank line or spacing is inconsequential and does not introduce any issues.
54-63: Constructor implementation.Providing a default constructor with
DotrainOrder::dummy()is valid for quick instantiation. Ensure that any fields requiring actual data are updated during further initialization steps.
68-74: Deployment keys retrieval looks correct.The
get_deployment_keysmethod properly parses deployment keys and returns them as a vector of strings. Code is straightforward and properly handles errors.
76-82: Deployment selection logic is solid.
choose_deploymentchecks for valid deployments before storing them, preventing users from selecting invalid deployments.
90-94: Assignment of state fields.Storing
dotrain_order,deployment_name, and an optional callback in this block appears consistent with the rest of the code. No issues found.
97-105: Retrieving GUI config.The
get_gui_configmethod properly fetches and returns the GUI configuration from the YAML structure. Error handling is well-defined.
107-110: Deployment retrieval.
get_current_deploymentdelegates toget_gui_configand validates the user’s selected deployment. Implementation looks correct.
126-164: Async token info retrieval.
get_token_infomethod checks for local token metadata and fetches missing fields on-chain if needed. This approach is well-structured; just be mindful of performance if large numbers of tokens are fetched repeatedly.
195-204: Strategy details retrieval.
get_strategy_detailsfetches metadata as expected. Implementation is straightforward.
207-217: Deployment details mapping.
get_deployment_detailsreturns aHashMapof name-and-description data, aligning with the updated approach. No issues found.
219-232: Deployment detail access.
get_deployment_detailusesget_deployment_detailsto extract a single entry, returning an error if it isn’t found. This flow is correct.
234-245: Dotrain text generation.
generate_dotrain_textconstructs a combined YAML body. Implementation appears robust and meets the straightforward requirement.
246-255: Composing rainlang.
get_composed_rainlangfinalizes scenario bindings and composes the deployment to a Rainlang representation. No concerns regarding this logic.
342-350: WasmEncodedError conversion.Converting
GuiErrorintoWasmEncodedErrorby embedding the error message is a suitable approach for propagating error details to JavaScript.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
packages/orderbook/test/js_api/gui.test.ts(6 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
packages/orderbook/test/js_api/gui.test.ts (2)
crates/js_api/src/gui/deposits.rs (1)
deposits(49-80)crates/settings/src/gui.rs (3)
presets(130-140)presets(499-516)presets(701-714)
🪛 Biome (1.9.4)
packages/orderbook/test/js_api/gui.test.ts
[error] 585-585: This let declares a variable that is only assigned once.
'deposits' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
[error] 589-589: This let declares a variable that is only assigned once.
'depositsAfterRemove' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
⏰ Context from checks skipped due to timeout of 90000ms (16)
- GitHub Check: standard-tests (ubuntu-latest, test-js-bindings)
- GitHub Check: standard-tests (ubuntu-latest, rainix-rs-static)
- GitHub Check: standard-tests (ubuntu-latest, rainix-wasm-artifacts)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-test)
- GitHub Check: standard-tests (ubuntu-latest, rainix-wasm-test)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-legal)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-artifacts)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-static)
- GitHub Check: standard-tests (ubuntu-latest, ob-rs-test, true)
- GitHub Check: standard-tests (ubuntu-latest, rainix-rs-artifacts, true)
- GitHub Check: Deploy-Preview
- GitHub Check: build-tauri (ubuntu-22.04, true)
- GitHub Check: test
- GitHub Check: git-clean
- GitHub Check: test
- GitHub Check: test
🔇 Additional comments (8)
packages/orderbook/test/js_api/gui.test.ts (8)
554-561: Good implementation of the new extractWasmEncodedData utility.The code now correctly uses the
extractWasmEncodedDatautility function to handle results fromhasAnyDeposit(), which simplifies error handling and aligns with the changes in the underlying implementation. This reflects the updated method signature in the Rust code where return types now useResult<bool, GuiError>.
569-571: LGTM: Consistent extraction of deposit data.The usage of
extractWasmEncodedDatafor processing the deposits result is consistent with the new error handling approach.
579-581: Proper error handling testing for non-existent tokens.The error handling approach correctly tests the specific error message for a deposit token that doesn't exist in the GUI config, which is appropriate for this test case.
592-595: Good inline usage of extractWasmEncodedData.The code correctly uses the utility function inline when only needing to access properties of the returned value, which keeps the code concise.
602-609: LGTM: Proper handling of deposit presets.The code correctly extracts and validates the deposit presets using the new utility function.
612-614: Proper error testing for invalid token preset.The test correctly verifies the expected error message when retrieving presets for a token that doesn't exist in the GUI config.
902-910: Consistent extraction in deserialization test.This change ensures consistent use of the data extraction pattern throughout the test suite, including after state deserialization.
930-933: LGTM: Consistent extraction after clearState.The code correctly uses the extraction utility to verify deposits are cleared.
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 (2)
packages/orderbook/test/js_api/gui.test.ts(6 hunks)packages/ui-components/src/lib/components/deployment/DepositInput.svelte(3 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
packages/orderbook/test/js_api/gui.test.ts (2)
crates/js_api/src/gui/deposits.rs (1)
deposits(49-80)crates/settings/src/gui.rs (3)
presets(130-140)presets(499-516)presets(701-714)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: standard-tests (ubuntu-latest, rainix-wasm-test)
- GitHub Check: standard-tests (ubuntu-latest, rainix-rs-static)
- GitHub Check: standard-tests (ubuntu-latest, rainix-rs-artifacts, true)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-test)
- GitHub Check: Deploy-Preview
- GitHub Check: test
- GitHub Check: test
- GitHub Check: test
- GitHub Check: build-tauri (ubuntu-22.04, true)
- GitHub Check: git-clean
🔇 Additional comments (15)
packages/ui-components/src/lib/components/deployment/DepositInput.svelte (2)
27-31: Improved error handling for deposit retrieval.The enhanced error handling now properly checks for errors from
getDeposits()and throws a meaningful error with the appropriate message.
75-83: Complete error handling implementation in handleInput.Good implementation of try-catch block to handle potential errors when retrieving deposits.
packages/orderbook/test/js_api/gui.test.ts (13)
555-556: Updated hasAnyDeposit call to use extractWasmEncodedData.The test now properly handles the new return type from the wasm-exported method.
558-559: Updated getDeposits call to use extractWasmEncodedData.The test now properly unwraps the result from the wasm-exported method.
561-561: Consistent use of extractWasmEncodedData for hasAnyDeposit.Good consistency in handling wasm-exported method results.
570-572: Updated getDeposits call to use extractWasmEncodedData in update deposit test.Now properly handles the WasmEncodedResult return type.
580-581: Improved error handling test for getDepositPresets.The test now correctly checks the error message in the result object instead of expecting a thrown exception.
585-586: Updated getDeposits call in remove deposit test.Consistently using the extractWasmEncodedData pattern.
589-590: Updated getDeposits call after removal.Using extractWasmEncodedData to handle the result consistently.
593-595: Consistent use of extractWasmEncodedData for getDeposits.Maintaining the pattern throughout the test file for deposit operations.
602-603: Updated getDepositPresets call to use extractWasmEncodedData.Now properly unwraps the structured result from the wasm-exported method.
612-614: Updated error test for getDepositPresets.Now properly checks the error in the returned result object.
903-911: Updated getDeposits call in deserializeState test.Consistently using the extractWasmEncodedData pattern for accessing the deposit data.
931-932: Updated getDeposits call in clearState test.Maintains consistency in handling WasmEncodedResult returns.
354-364: Well-implemented utility function for extracting data from WasmEncodedResult.This utility centralizes error handling for wasm-exported methods, making the tests more maintainable and consistent.
The function appropriately:
- Checks for errors in the result
- Uses a custom error message if provided
- Handles undefined values
- Returns the extracted value
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
packages/ui-components/src/lib/components/deployment/DepositInput.svelte(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (16)
- GitHub Check: build-tauri (ubuntu-22.04, true)
- GitHub Check: Deploy-Preview
- GitHub Check: test
- GitHub Check: standard-tests (ubuntu-latest, rainix-wasm-test)
- GitHub Check: standard-tests (ubuntu-latest, test-js-bindings)
- GitHub Check: standard-tests (ubuntu-latest, rainix-rs-static)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-legal)
- GitHub Check: standard-tests (ubuntu-latest, ob-rs-test, true)
- GitHub Check: standard-tests (ubuntu-latest, rainix-wasm-artifacts)
- 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: test
- GitHub Check: test
- GitHub Check: git-clean
🔇 Additional comments (3)
packages/ui-components/src/lib/components/deployment/DepositInput.svelte (3)
27-31: Great improvement to error handling in setCurrentDeposit!The error handling has been significantly improved by checking for errors in the deposits response and throwing appropriate errors. The error message in the catch block now properly extracts the message from the Error object.
Also applies to: 33-33, 35-35
61-69: Nice addition of consistent error handling for handlePresetClick.This addresses the previous review comment by adding proper error handling when getting deposits. The pattern matches the approach used in other functions, making the error handling consistent throughout the component.
80-88: Good implementation of error handling for handleInput.This successfully addresses the previous review comment by adding proper try-catch error handling for the getDeposits call. The error handling is now aligned with the rest of the component.
2 participants
Caution
Do not merge before #1452
Motivation
Solution
Checks
By submitting this for review, I'm confirming I've done the following:
fix #1402
Summary by CodeRabbit
New Features
DepositInputcomponent for better user experience when retrieving deposits.Bug Fixes