Divide DepositOrWithdrawModal into Deposit and Withdraw#1870
Merged
hardingjam merged 8 commits intomainfrom May 27, 2025
Merged
Divide DepositOrWithdrawModal into Deposit and Withdraw#1870hardingjam merged 8 commits intomainfrom
hardingjam merged 8 commits intomainfrom
Conversation
Contributor
## Walkthrough
This change refactors the handling of deposit and withdraw actions by splitting the combined modal components, types, handlers, and tests into separate deposit and withdraw versions. Types and props are renamed and adjusted accordingly, and all related logic, UI, and tests are updated to use the new structure.
## Changes
| File(s) | Change Summary |
|-------------------------------------------------------------------------------------------------|--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
| packages/ui-components/src/lib/index.ts<br>packages/ui-components/src/lib/types/modal.ts<br>packages/ui-components/src/lib/types/transaction.ts | Renamed types: `DepositOrWithdrawArgs` → `VaultActionArgs`, `DepositOrWithdrawModalProps` → `VaultActionModalProps`. Updated property names and removed the `action` property. Adjusted exports and prop types to match the new naming and structure. |
| packages/webapp/src/lib/components/DepositModal.svelte | Refactored to handle only deposit actions. Updated prop type to `VaultActionArgs`, removed all withdrawal-related logic, and simplified UI and validation accordingly. |
| packages/webapp/src/lib/components/WithdrawModal.svelte | Added a new component to handle withdraw actions, with its own state management, validation, and transaction flow, mirroring the structure of the deposit modal but focused on withdrawals. |
| packages/webapp/src/lib/services/modal.ts | Split the combined modal handler into `handleDepositModal` and `handleWithdrawModal`, each invoking their respective modal component. Updated prop types and imports to match the new structure. |
| packages/webapp/src/lib/services/handleVaultAction.ts | Renamed parameter interface and updated function signature to include `queryClient` and `queryKey`, removed `vaultId`. Replaced single modal handler call with conditional calls to `handleDepositModal` or `handleWithdrawModal` based on action. Updated callback and argument properties accordingly. |
| packages/webapp/src/routes/orders/[network]-[orderHash]/+page.svelte<br>packages/webapp/src/routes/vaults/[network]-[id]/+page.svelte | Updated to use the new separate modal handlers and types. Removed the `action` prop, updated callback property names, and improved safety in accessing configuration values. Logic now chooses the appropriate handler based on action type. |
| packages/webapp/src/__tests__/DepositModal.test.ts | Refactored tests to focus exclusively on deposit functionality. Removed all withdraw-related tests, props, and logic. Updated test IDs, button references, and mocks for deposit-only scenarios. |
| packages/webapp/src/__tests__/WithdrawModal.test.ts | Added a new test suite for the withdraw modal component, covering rendering, user interactions, error handling, validation, and transaction flow for withdrawal actions. |
| packages/ui-components/src/__tests__/awaitTransactionIndexing.test.ts | Modified mocked error response format in fetch error test and removed assertion on number of fetch calls. |
## Sequence Diagram(s)
```mermaid
sequenceDiagram
participant User
participant Page
participant ModalService
participant DepositModal
participant WithdrawModal
participant TransactionStore
User->>Page: Clicks "Deposit" or "Withdraw"
Page->>ModalService: handleDepositModal(args) or handleWithdrawModal(args)
ModalService->>DepositModal: Open (for deposit)
ModalService->>WithdrawModal: Open (for withdraw)
User->>DepositModal: Enter amount, submit
DepositModal->>TransactionStore: Initiate deposit transaction
TransactionStore-->>DepositModal: Transaction result
User->>WithdrawModal: Enter amount, submit
WithdrawModal->>TransactionStore: Initiate withdraw transaction
TransactionStore-->>WithdrawModal: Transaction resultPossibly related PRs
Suggested labels
Suggested reviewers
Learnt from: hardingjam Learnt from: hardingjam |
hardingjam
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 11
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (10)
packages/ui-components/src/lib/index.ts(1 hunks)packages/ui-components/src/lib/types/modal.ts(1 hunks)packages/ui-components/src/lib/types/transaction.ts(1 hunks)packages/webapp/src/__tests__/DepositModal.test.ts(18 hunks)packages/webapp/src/__tests__/WithdrawModal.test.ts(1 hunks)packages/webapp/src/lib/components/DepositModal.svelte(6 hunks)packages/webapp/src/lib/components/WithdrawModal.svelte(1 hunks)packages/webapp/src/lib/services/modal.ts(2 hunks)packages/webapp/src/routes/orders/[network]-[orderHash]/+page.svelte(3 hunks)packages/webapp/src/routes/vaults/[network]-[id]/+page.svelte(2 hunks)
🧰 Additional context used
🧠 Learnings (4)
packages/ui-components/src/lib/types/modal.ts (1)
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1744
File: packages/webapp/src/lib/components/DepositOrWithdrawModal.svelte:73-76
Timestamp: 2025-05-09T05:30:02.520Z
Learning: In Rain.orderbook, `VaultCalldataResult` is a wrapper type that contains both `error` and `value` properties. When using this in the `DepositOrWithdrawModal` component, only the inner `value` field is passed to the `handleTransaction` function, not the complete wrapper.
packages/webapp/src/lib/components/DepositModal.svelte (1)
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1744
File: packages/webapp/src/lib/components/DepositOrWithdrawModal.svelte:73-76
Timestamp: 2025-05-09T05:30:02.520Z
Learning: In Rain.orderbook, `VaultCalldataResult` is a wrapper type that contains both `error` and `value` properties. When using this in the `DepositOrWithdrawModal` component, only the inner `value` field is passed to the `handleTransaction` function, not the complete wrapper.
packages/webapp/src/routes/vaults/[network]-[id]/+page.svelte (1)
Learnt from: hardingjam
PR: rainlanguage/rain.orderbook#1504
File: tauri-app/src/routes/orders/[network]-[orderHash]/+page.svelte:33-37
Timestamp: 2025-04-08T16:35:15.127Z
Learning: In the Rain OrderBook project, the onDeposit and onWithdraw functions in page components are kept simple (just calling modal handlers and revalidating queries) because error handling for deposit and withdraw actions (including user cancellations and failed transactions) is handled within the modal components themselves.
packages/webapp/src/routes/orders/[network]-[orderHash]/+page.svelte (1)
Learnt from: hardingjam
PR: rainlanguage/rain.orderbook#1504
File: tauri-app/src/routes/orders/[network]-[orderHash]/+page.svelte:33-37
Timestamp: 2025-04-08T16:35:15.127Z
Learning: In the Rain OrderBook project, the onDeposit and onWithdraw functions in page components are kept simple (just calling modal handlers and revalidating queries) because error handling for deposit and withdraw actions (including user cancellations and failed transactions) is handled within the modal components themselves.
🧬 Code Graph Analysis (2)
packages/webapp/src/lib/services/modal.ts (1)
packages/ui-components/src/lib/types/modal.ts (1)
VaultActionModalProps(4-7)
packages/ui-components/src/lib/types/modal.ts (2)
packages/ui-components/src/lib/index.ts (2)
VaultActionModalProps(80-80)VaultActionArgs(78-78)packages/ui-components/src/lib/types/transaction.ts (1)
VaultActionArgs(14-21)
🔇 Additional comments (23)
packages/ui-components/src/lib/types/transaction.ts (1)
14-16: Type signature change improves modularityThe change from
DepositOrWithdrawArgstoVaultActionArgsand updating the callback fromonDepositOrWithdrawto the more genericonSuccessis well-structured. Removing theactionproperty moves the action responsibility to the component level rather than being determined by a property.packages/ui-components/src/lib/types/modal.ts (1)
2-7: Modal props update aligns with component splitting strategyThe renamed
VaultActionModalPropstype and updated import properly reflect the changes to separate deposit and withdraw functionality. This creates a more focused type that works well with the new modal component architecture.packages/ui-components/src/lib/index.ts (1)
78-80: Export updates ensure API consistencyThe exports have been properly updated to use the new type names, maintaining a consistent public API for consumers of the library.
packages/webapp/src/routes/vaults/[network]-[id]/+page.svelte (2)
5-5: Import reflects modal component separationThe import change from a combined handler to separate
handleDepositModalandhandleWithdrawModalfunctions aligns with the PR's goal of separating these concerns.
19-34: Modal handler selection logic is clean and maintainableThe refactored
handleVaultActionfunction elegantly selects the appropriate modal handler based on action type. The callback rename fromonDepositOrWithdrawtoonSuccessmakes the code more intuitive and aligns with the PR objectives.packages/webapp/src/lib/services/modal.ts (3)
3-4: Clean implementation of split modal components.The previously combined DepositOrWithdrawModal has been properly split into dedicated DepositModal and WithdrawModal components, aligning with the PR objective to facilitate incremental migration to the new TransactionManager.
7-7: Type rename is consistent.Good replacement of DepositOrWithdrawModalProps with VaultActionModalProps, maintaining type safety while reflecting the component restructuring.
17-23: Well-structured modal handlers.The split into separate handlers follows the established pattern in the file. Each handler instantiates its respective component with the appropriate props structure, maintaining consistency with other modal handlers.
packages/webapp/src/lib/components/DepositModal.svelte (6)
6-6: Proper type import update.Correctly updated import to use the new VaultActionArgs type to reflect the separation of concerns.
35-37: Clean props destructuring.The component now correctly uses VaultActionArgs and removes action-related conditionals, focusing solely on deposit functionality.
72-107: Simplified transaction handling.The handleContinue function is now cleaner and more focused by removing withdrawal-specific logic. The VaultCalldataResult handling correctly extracts and passes the inner value to handleTransaction, consistent with the project's established pattern.
122-124: Streamlined validation.Validation logic is simplified by directly using userBalance without conditional checks for action type.
130-130: Updated UI text for clarity.Modal title now clearly indicates this is exclusively for deposit actions.
175-182: Clearer button labeling.Button text and test ID are updated to be deposit-specific, improving clarity and test stability.
packages/webapp/src/routes/orders/[network]-[orderHash]/+page.svelte (3)
10-14: Clean import structure.Imports are properly updated to use the new separate modal handlers, using a more organized import syntax.
23-25: Improved error resilience.Good defensive programming with fallback values for subgraphUrl, rpcUrl, and chainId to prevent undefined values.
43-58: Well-structured modal selection.The handleVaultAction function now elegantly selects the appropriate modal handler based on the action type. The callback property rename from onDepositOrWithdraw to onSuccess is clearer and more consistent with typical event handler naming conventions.
packages/webapp/src/__tests__/DepositModal.test.ts (6)
3-3: Updated component import.Test file correctly imports the new DepositModal component instead of the previous combined modal.
7-8: Focused imports for deposit functionality.Import list has been properly updated to include only deposit-related functions.
19-21: Simplified mocks.Mock implementations are correctly updated to focus only on deposit-related functions.
39-39: Updated test suite name.Test suite name now accurately reflects that it's testing the DepositModal component.
71-74: Updated test rendering and assertions.Tests are properly updated to render the DepositModal component and check for deposit-specific elements using the correct test IDs.
228-246: Focused test cases.Test cases are properly updated to focus only on deposit-specific scenarios, maintaining good test coverage for the specific component responsibilities.
Comment on lines
+291
to
+303
| it('shows the cancel button that closes the modal', async () => { | ||
| render(WithdrawModal, defaultProps); | ||
|
|
||
| await waitFor(() => { | ||
| expect(screen.getByText('Cancel')).toBeInTheDocument(); | ||
| }); | ||
|
|
||
| const cancelButton = screen.getByText('Cancel'); | ||
| await fireEvent.click(cancelButton); | ||
| await waitFor(() => { | ||
| expect(transactionStore.reset).not.toHaveBeenCalled(); | ||
| }); | ||
| }); |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve cancel button test to verify modal closing.
The test verifies that the cancel button exists and that transactionStore.reset is not called, but it doesn't verify that the modal is actually closed. Consider updating this test to verify that the open prop is set to false when the cancel button is clicked.
it('shows the cancel button that closes the modal', async () => {
+ const mockProps = {
+ ...defaultProps,
+ open: true
+ };
- render(WithdrawModal, defaultProps);
+ const { component } = render(WithdrawModal, mockProps);
await waitFor(() => {
expect(screen.getByText('Cancel')).toBeInTheDocument();
});
const cancelButton = screen.getByText('Cancel');
await fireEvent.click(cancelButton);
await waitFor(() => {
expect(transactionStore.reset).not.toHaveBeenCalled();
+ expect(component.open).toBe(false);
});
});Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/webapp/src/__tests__/WithdrawModal.test.ts around lines 291 to 303,
the test for the cancel button currently checks its presence and that
transactionStore.reset is not called, but does not verify that the modal closes.
Update the test to simulate clicking the cancel button and then assert that the
modal's open state is set to false, confirming the modal closes as expected.
Contributor
Author
There was a problem hiding this comment.
the modal closing is native flowbite behaviour
Contributor
There was a problem hiding this comment.
@hardingjam Thanks for clarifying that the modal closing is handled by Flowbite's native behavior. That makes sense - since Flowbite manages the modal state internally, checking the component's open prop value wouldn't be an effective way to verify the closing functionality. The current test approach is appropriate given this architecture.
🐰
✏️ Learnings added
Learnt from: hardingjam
PR: rainlanguage/rain.orderbook#1870
File: packages/webapp/src/__tests__/WithdrawModal.test.ts:291-303
Timestamp: 2025-05-20T12:05:48.544Z
Learning: The modal closing behavior in the WithdrawModal component is handled by Flowbite's native implementation rather than through component props, making it unnecessary to test the open state changes directly.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
Comment on lines
+79
to
+94
| async function handleContinue() { | ||
| isCheckingCalldata = true; | ||
| try { | ||
| const withdrawCalldataResult = await getVaultWithdrawCalldata(vault, amount.toString()); | ||
| if (withdrawCalldataResult.error) { | ||
| errorMessage = withdrawCalldataResult.error.msg; | ||
| } else { | ||
| handleTransaction(withdrawCalldataResult.value); | ||
| } | ||
| currentStep = 2; | ||
| } catch { | ||
| errorMessage = 'Failed to get calldata.'; | ||
| } finally { | ||
| isCheckingCalldata = false; | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Fix error handling in handleContinue function.
There's an issue with the control flow in the handleContinue function. Even when an error occurs, the code still sets currentStep = 2, which moves the user to the next step. This could lead to a confusing user experience.
async function handleContinue() {
isCheckingCalldata = true;
try {
const withdrawCalldataResult = await getVaultWithdrawCalldata(vault, amount.toString());
if (withdrawCalldataResult.error) {
errorMessage = withdrawCalldataResult.error.msg;
+ return; // Don't proceed to step 2 if there's an error
} else {
handleTransaction(withdrawCalldataResult.value);
+ currentStep = 2; // Only move to step 2 on success
}
- currentStep = 2;
} catch {
errorMessage = 'Failed to get calldata.';
} finally {
isCheckingCalldata = false;
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| async function handleContinue() { | |
| isCheckingCalldata = true; | |
| try { | |
| const withdrawCalldataResult = await getVaultWithdrawCalldata(vault, amount.toString()); | |
| if (withdrawCalldataResult.error) { | |
| errorMessage = withdrawCalldataResult.error.msg; | |
| } else { | |
| handleTransaction(withdrawCalldataResult.value); | |
| } | |
| currentStep = 2; | |
| } catch { | |
| errorMessage = 'Failed to get calldata.'; | |
| } finally { | |
| isCheckingCalldata = false; | |
| } | |
| } | |
| async function handleContinue() { | |
| isCheckingCalldata = true; | |
| try { | |
| const withdrawCalldataResult = await getVaultWithdrawCalldata(vault, amount.toString()); | |
| if (withdrawCalldataResult.error) { | |
| errorMessage = withdrawCalldataResult.error.msg; | |
| return; // Don't proceed to step 2 if there's an error | |
| } else { | |
| handleTransaction(withdrawCalldataResult.value); | |
| currentStep = 2; // Only move to step 2 on success | |
| } | |
| } catch { | |
| errorMessage = 'Failed to get calldata.'; | |
| } finally { | |
| isCheckingCalldata = false; | |
| } | |
| } |
🤖 Prompt for AI Agents
In packages/webapp/src/lib/components/WithdrawModal.svelte around lines 79 to
94, the handleContinue function sets currentStep = 2 even when an error occurs,
causing incorrect progression in the UI. Modify the code so that currentStep = 2
is only set when there is no error in withdrawCalldataResult and no exception is
thrown. Move the currentStep assignment inside the else block where
handleTransaction is called, and avoid setting it in the catch block.
Contributor
Author
There was a problem hiding this comment.
good spot, thank you
Contributor
There was a problem hiding this comment.
@hardingjam You're welcome! I'm glad I could help identify that issue. Feel free to let me know if you need any more assistance with this PR.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
| export let open: boolean; | ||
| export let args: VaultActionArgs; | ||
|
|
||
| const { vault, chainId, subgraphUrl, account } = args; |
Contributor
There was a problem hiding this comment.
Missing callback invocation for transaction completion.
The component destructures args to access various properties, but there's no mention of the callback function (onDepositOrWithdraw or onSuccess) that should be called when a transaction is complete. If the component is supposed to notify its parent when a transaction is complete, this callback should be destructured and called at the appropriate time.
-const { vault, chainId, subgraphUrl, account } = args;
+const { vault, chainId, subgraphUrl, account, onSuccess } = args;
// Then call it after successful transaction
async function handleTransaction(transactionCalldata: VaultCalldataResult) {
transactionStore.handleDepositOrWithdrawTransaction({
config: $wagmiConfig,
transactionCalldata,
action: 'withdraw',
chainId,
vault,
subgraphUrl
});
+ // Call the callback when transaction is complete
+ if (onSuccess) {
+ onSuccess();
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const { vault, chainId, subgraphUrl, account } = args; | |
| const { vault, chainId, subgraphUrl, account, onSuccess } = args; | |
| async function handleTransaction(transactionCalldata: VaultCalldataResult) { | |
| transactionStore.handleDepositOrWithdrawTransaction({ | |
| config: $wagmiConfig, | |
| transactionCalldata, | |
| action: 'withdraw', | |
| chainId, | |
| vault, | |
| subgraphUrl | |
| }); | |
| // Call the callback when transaction is complete | |
| if (onSuccess) { | |
| onSuccess(); | |
| } | |
| } |
🤖 Prompt for AI Agents
In packages/webapp/src/lib/components/WithdrawModal.svelte at line 33, the
destructuring of args misses extracting the callback function (such as
onDepositOrWithdraw or onSuccess) that should be called upon transaction
completion. Update the destructuring to include the appropriate callback
function from args, then ensure this callback is invoked at the correct point in
the transaction flow to notify the parent component when the transaction
finishes.
Contributor
Author
There was a problem hiding this comment.
the TransactionModal handles success and the listener handles adding toasts
This behaviour is refactored as we move the transactions to the TransactionManager
Contributor
There was a problem hiding this comment.
@hardingjam Thanks for clarifying the architecture. I understand now that success handling is delegated to the TransactionModal component and listeners rather than directly calling the callback. This makes sense as part of your incremental migration to the new TransactionManager mentioned in the PR description.
✏️ Learnings added
Learnt from: hardingjam
PR: rainlanguage/rain.orderbook#1870
File: packages/webapp/src/lib/components/WithdrawModal.svelte:33-33
Timestamp: 2025-05-20T12:01:28.354Z
Learning: In the rain.orderbook project, transaction success handling in modal components is managed by the TransactionModal component and listeners that handle adding toasts, rather than directly invoking callbacks. This is part of the architecture being refactored as transactions are moved to the TransactionManager.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
findolor
reviewed
May 21, 2025
Collaborator
findolor
left a comment
findolor
left a comment
There was a problem hiding this comment.
do you think having a generic modal would work for both deposit and withdraw? i see that the ui elements are mostly the same aside from simple text changes. i think we can reduce the amount of template code by creating a shared component. curious about what you think
Contributor
Author
Issue #1794 will remove the need for separate modals, but I'm moving each transaction over one by one, so for now I need to keep the 'old' DepsoitModal until I migrate that transaction. Made an issue to re-merge these: #1879 |
findolor
approved these changes
May 21, 2025
4 tasks
hardyjosh
approved these changes
May 26, 2025
4 tasks
This was referenced Jul 1, 2025
Merged
This was referenced Jul 11, 2025
This was referenced Jul 21, 2025
Merged
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
Separating the concerns of deposit/withdraw modals will make it easier to incrementally move the transactions into the new TransactionManager (see #1797, #1796)
Solution
Divide DepositOrWithdrawModal into seperate components.
Checks
By submitting this for review, I'm confirming I've done the following:
Summary by CodeRabbit