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

fix(8667): trigger swap tx on approval tx confirmed #9173

Merged
merged 3 commits into from
Apr 17, 2024
Merged

Conversation

Akaryatrh
Copy link
Contributor

@Akaryatrh Akaryatrh commented Apr 9, 2024

Description

When swapping a token that needs approval (and requires a 2 step swap process), it's needed for the user, when using a hardware wallet, to manually sign transactions. Currently, second modal opening is triggered when approval transaction is finished (meaning when it has been submitted). But this would end up in a situation where the first modal was not unmounted before it was asked to be mounted again. This flow was creating an issue where the user could not confirm the second transaction (as second modal was never displayed)

To mitigate this issue, we choose to wait for first transaction completion before starting the second one. This is not perfect from user POV as a long delay can happen between approval and swap txs, but as discussed with @AlexJupiter and @gantunesr it's considered as a temporary solution until we figure out a long term one.

Related issues

Fixes: #8667

Manual testing steps

For ease the explanation, we are using AAVE and MATIC on Polygon network and will sign txs with a Ledger account. You also need blind signing to be enabled on your Ledger device.

  1. make sure AAVE token approval has been revoked here https://polygonscan.com/tokenapprovalchecker for your Ledger account
  2. select swap action from actions dropdown
  3. select your Ledger account
  4. select AAVE token as origin token
  5. type a value that you want to swap (0.0000001 is fine)
  6. select MATIC token as destination token
  7. get quotes (you may need to retry several times this step)
  8. Swipe to swap
  9. First modal should be displayed. Sign approval tx on your ledger device
  10. Once approval txs is confirmed, second modal is opening. Sign swap tx on your ledger device
  11. Swap should be completed

Screenshots/Recordings

Before

https://recordit.co/Vg7MLtediW

After

Record_2024-04-09-14-29-50.mp4

Pre-merge author checklist

  • I’ve followed MetaMask Coding Standards.
  • I've completed the PR template to the best of my ability
  • I’ve included tests if applicable
  • I’ve documented my code using JSDoc format if applicable
  • I’ve applied the right labels on the PR (see labeling guidelines). Not required for external contributors.

Pre-merge reviewer checklist

  • I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed).
  • I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

Copy link
Contributor

github-actions bot commented Apr 9, 2024

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

@Akaryatrh Akaryatrh marked this pull request as ready for review April 9, 2024 13:14
@Akaryatrh Akaryatrh requested a review from a team as a code owner April 9, 2024 13:14
Copy link

sonarcloud bot commented Apr 12, 2024

Copy link
Contributor

@MarioAslau MarioAslau left a comment

Choose a reason for hiding this comment

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

LGTM

@Akaryatrh Akaryatrh merged commit d9b53e5 into main Apr 17, 2024
31 checks passed
@Akaryatrh Akaryatrh deleted the fix/8667 branch April 17, 2024 08:22
@github-actions github-actions bot locked and limited conversation to collaborators Apr 17, 2024
@metamaskbot metamaskbot added the release-7.22.0 Issue or pull request that will be included in release 7.22.0 label Apr 17, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
release-7.22.0 Issue or pull request that will be included in release 7.22.0 team-hardware-wallets
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

[Bug]: ERC20 swap attempt does not succeed on Ledger account when token spending approval is required
3 participants