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: prevent race condition for swap state #6624

Merged
merged 1 commit into from
May 22, 2023

Conversation

just-toby
Copy link
Contributor

@just-toby just-toby commented May 22, 2023

Description

This bug happens when the ConfirmSwapModal is dismissed before the swapCallback promise resolves.

When the bug happens, the state is set like this:

  1. user proceeds normally and opens the ConfirmSwapModal -> showConfirm is true (set on L783)
  2. handleSwap is called and triggers the swapCallback (L426). showConfirm is still true at this point
  3. handleConfirmDismiss is called and showConfirm is set to false
  4. swapCallback resolves and sets showConfirm to an outdated value (L428) which re-opens the modal

Test plan

Reproducing the error

  1. see the test results for the swap/errors.test.ts test suite - it's flaky because this race condition causes the modal to stay visible when it shouldn't

QA (ie manual testing)

  • this change totally fixes flakiness in certain swap e2e tests (tested in other branches)
  • manually verified that the ConfirmSwapModal still works in the new permit2 flow with these changes

Automated testing

  • Unit test N/A
  • Integration/E2E test (existing test flakiness in other PRs is fixed by this change)

@just-toby just-toby requested a review from zzmp May 22, 2023 22:20
@just-toby just-toby temporarily deployed to false May 22, 2023 22:20 — with GitHub Actions Inactive
@just-toby just-toby temporarily deployed to false May 22, 2023 22:20 — with GitHub Actions Inactive
@just-toby just-toby temporarily deployed to false May 22, 2023 22:20 — with GitHub Actions Inactive
@vercel
Copy link

vercel bot commented May 22, 2023

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

Name Status Preview Updated (UTC)
interface ✅ Ready (Inspect) Visit Preview May 22, 2023 10:20pm
interface-node18 ✅ Ready (Inspect) Visit Preview May 22, 2023 10:20pm

@just-toby just-toby temporarily deployed to false May 22, 2023 22:20 — with GitHub Actions Inactive
@just-toby just-toby temporarily deployed to false May 22, 2023 22:26 — with GitHub Actions Inactive
@just-toby just-toby temporarily deployed to false May 22, 2023 22:26 — with GitHub Actions Inactive
@just-toby just-toby temporarily deployed to false May 22, 2023 22:26 — with GitHub Actions Inactive
@just-toby just-toby temporarily deployed to false May 22, 2023 22:26 — with GitHub Actions Inactive
@codecov
Copy link

codecov bot commented May 22, 2023

Codecov Report

Patch coverage: 80.00% and project coverage change: -0.13 ⚠️

Comparison is base (f3a80c6) 60.18% compared to head (9e37cb7) 60.05%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6624      +/-   ##
==========================================
- Coverage   60.18%   60.05%   -0.13%     
==========================================
  Files         719      719              
  Lines       21143    21143              
  Branches     6974     6974              
==========================================
- Hits        12724    12697      -27     
- Misses       8342     8369      +27     
  Partials       77       77              
Flag Coverage Δ
e2e-tests 60.49% <80.00%> (-0.14%) ⬇️
unit-tests 21.11% <0.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/pages/Swap/index.tsx 89.62% <80.00%> (ø)

... and 17 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@cypress
Copy link

cypress bot commented May 22, 2023

1 flaky tests on run #11211 ↗︎

0 69 8 0 Flakiness 1

Details:

fix: prevent race condition for swap state
Project: Uniswap Interface Commit: 9e37cb7a29
Status: Passed Duration: 06:05 💡
Started: May 22, 2023 10:28 PM Ended: May 22, 2023 10:34 PM
Flakiness  cypress/e2e/swap/swap.test.ts • 1 flaky test • e2e

View Output Video

Test Artifacts
Swap > Swap on main page > swaps ETH for USDC Output Screenshots Video

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings.

@just-toby just-toby requested a review from vm May 22, 2023 22:37
@just-toby just-toby merged commit 95814e3 into main May 22, 2023
23 of 26 checks passed
@just-toby just-toby deleted the fix/swap-modal-race-condition branch May 22, 2023 22:57
@tinaszheng
Copy link
Contributor

nice fix!

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.

None yet

4 participants