Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor: Decompose swap form #200

Merged
merged 14 commits into from
Mar 3, 2025
Merged

Refactor: Decompose swap form #200

merged 14 commits into from
Mar 3, 2025

Conversation

chapati23
Copy link
Contributor

Description

While trying to debug, I got very annoyed with the unwieldiness of the SwapForm with lots of components and hooks inlined in the same file.

This PR is about decomposing the SwapForm into smaller parts. It extracts components and hooks into individual files and aims at making the DX for this feature better.

Other changes

I also upgraded yarn from 3.3.1 to 4.6.0 while I was at it, hence the yarn.lock update, but no dependencies where actually changed or updated.

Tested

I checked that swaps still work in both directions and with or without approvals.

How to review

Everything should work exactly as it does on production right now, no functionality should have changed.

  • Check out the preview URL
  • Do a few swaps in different directions and with and without approval TXs
  • Review the code

Copy link

vercel bot commented Feb 28, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
mento-web ✅ Ready (Inspect) Visit Preview 💬 Add feedback Mar 3, 2025 1:01pm

@Andrew718PLTS
Copy link
Contributor

Andrew718PLTS commented Feb 28, 2025

@chapati23 The 'Continue' button is disabled upon entering the swap-in amount; I can't actually do a swap with that.
image

It looks like the bug that I fixed already. Have you updated the main branch before checkout? If not, try to do a rebase first.

@Andrew718PLTS
Copy link
Contributor

Andrew718PLTS commented Mar 3, 2025

The changes look good, but there are a couple of cases we can improve here:
The 'Preparing Swap Transaction' text on the button. It's not the correct text all the time - we have two TXs, Approval and Swap, but show this text for both cases, which is incorrect because when we're waiting for approval tx, we show the text about the swap tx preparing. Here, I see two possible solutions:

  1. Show specified text for approval and swap TXs (Preparing Approval Transaction & Preparing Swap Transaction).
  2. Show simplified a generic text (Preparing Transaction).
    Please choose any of them or devise another approach (but I'd take the second one).
Screenshot 2025-03-03 at 11 11 24 Screenshot 2025-03-03 at 11 11 13

@Andrew718PLTS
Copy link
Contributor

When an allowance is less or equal to approveAmount, then we have the opportunity to swap directly without sending the approval tx - it doesn't send the swap tx for the first time. You should close the 'Performing Swap' popup and try to click the 'Swap' button again. I checked, and this reproduces on prod as well, but maybe you can resolve it here, or I will resolve it myself in my PR. What do you think?

@chapati23
Copy link
Contributor Author

@Andrew718PLTS i refactored the whole buttonText & disabled status logic into a useSwapState() hook because i got confused with all the possible states buried in that SwapConfirm component

can you re-test all cases? locally it all looked good to me now and all button text & disabled statuses made sense

@Andrew718PLTS Andrew718PLTS self-requested a review March 3, 2025 15:25
Copy link
Contributor

@Andrew718PLTS Andrew718PLTS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@chapati23 I re-tested it using the latest Vercel preview (your last changes have been deployed successfully) and locally.

  1. All the states display correctly.
  2. The 'Swap' tx is sent once you click the corresponding button on the 'Confirm Swap' page.

@chapati23 chapati23 merged commit 283d07b into main Mar 3, 2025
7 checks passed
@chapati23 chapati23 deleted the chore/decompose-swap-form branch March 3, 2025 16:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants