feat: handling swap error scenarios #1270
Conversation
arlert-armin
left a comment
arlert-armin
left a comment
There was a problem hiding this comment.
Thanks for your effort on this. I've just left some comments.
widget/embedded/src/components/InsufficientFeeModal/InsufficientFeeModal.tsx
Outdated
Show resolved
Hide resolved
| if (requiredChains.length === 1) { | ||
| const walletAddressError = | ||
| !selectedWallets.customDestination && | ||
| selectedWallets.sourceWallet?.address !== | ||
| selectedWallets.destinationWallet?.address; | ||
|
|
||
| if (walletAddressError) { | ||
| return { | ||
| title: swapButtonTitles().swap, | ||
| action: 'show-wallet-address-error', | ||
| disabled: false, | ||
| }; | ||
| } | ||
| } |
There was a problem hiding this comment.
Why do we need nested ifs? would this also work?
| if (requiredChains.length === 1) { | |
| const walletAddressError = | |
| !selectedWallets.customDestination && | |
| selectedWallets.sourceWallet?.address !== | |
| selectedWallets.destinationWallet?.address; | |
| if (walletAddressError) { | |
| return { | |
| title: swapButtonTitles().swap, | |
| action: 'show-wallet-address-error', | |
| disabled: false, | |
| }; | |
| } | |
| } | |
| if ( | |
| requiredChains.length === 1 && | |
| !selectedWallets.customDestination && | |
| selectedWallets.sourceWallet?.address !== selectedWallets.destinationWallet?.address | |
| ) { | |
| return { | |
| title: swapButtonTitles().swap, | |
| action: 'show-wallet-address-error', | |
| disabled: false, | |
| }; | |
| } |
Also please add a comment explaining what are you checking in here.
There was a problem hiding this comment.
I don’t agree with you. This error occurs only when executing a simple on-chain swap, so we should check for it only in that specific case. I also think extracting the long condition into a variable improves readability — but I’ve added a comment for clarity as well.
There was a problem hiding this comment.
I think you are missing my point I didn't say you extract condition into a variable I did the exact opposite to reduce nesting.
arlert-armin
left a comment
arlert-armin
left a comment
There was a problem hiding this comment.
Just add a few comments.
| import { useQuoteStore } from '../../store/quote'; | ||
| import { useConfirmSwap } from '../useConfirmSwap'; | ||
|
|
||
| export function useHandleConfirmSwap() { |
There was a problem hiding this comment.
Names are too similar to each other; please change the other hook to something more meaningful since currently it's not only responsible for confirming swaps.
| if (requiredChains.length === 1) { | ||
| const walletAddressError = | ||
| !selectedWallets.customDestination && | ||
| selectedWallets.sourceWallet?.address !== | ||
| selectedWallets.destinationWallet?.address; | ||
|
|
||
| if (walletAddressError) { | ||
| return { | ||
| title: swapButtonTitles().swap, | ||
| action: 'show-wallet-address-error', | ||
| disabled: false, | ||
| }; | ||
| } | ||
| } |
There was a problem hiding this comment.
I think you are missing my point I didn't say you extract condition into a variable I did the exact opposite to reduce nesting.
Summary
Please include a summary of the changes and the related issue. Please also include relevant motivation and context. List any dependencies that are required for this change.
Fixes # (issue)
How did you test this change?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Checklist: