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 (cherry-pick): Multichain: UX: Check for transactions on all networks and QueuedRequestCount (#25614) #25641

Merged

Conversation

darkwing
Copy link
Contributor

@darkwing darkwing commented Jul 2, 2024

Description

There's a case where there could be a race condition between the QueuedRequestController's network switching and the Routes' network switching. The root issue is that the use of getUnapprovedTransactions only gets transactions on the current chain, thus triggering Routes to switch networks once the last transaction of the current chain is done, but there could be more transactions on other chains which we should allow QRC to handle network switching for.

Thus, this PR lets QRC control network switching until there are absolutely no transactions or queued requests left.

This PR includes an
**E2E
** for the scenario where there are multiple queued transactions but since this is a race issue, it's difficult to reproduce in a provable E2E

Open in GitHub Codespaces

Related issues

Possibly related:
#25596

Manual testing steps

  1. Open Tab 1, connect to Uniswap on Ethereum Mainnet
  2. Open Tab 2, connect to PancakeSwap on BNB Chain
  3. Open Tab 3, connect to Test Dapp on Sepolia
  4. Initiate a swap on Tab 1 and Tab 2 BUT DO NOT CONFIRM IT, JUST MOVE ON TO THE NEXT TAB
  5. Initiate a send on Tab 3 BUT DO NOT CONFIRM IT
  6. On the confirmation screen, you should still see the first confirmation from Tab 1 (Uniswap) on Ethereum Mainnet; confirm or reject it. See the confirmation window close
  7. A new confirmation popup should come up with the PancakeSwap/ Tab 2 confirmation on BNB chain; confirm or reject it. See the confirmation window close
  8. See one last confirmation screen pop up for the Tab 3 / Test Dapp send on Sepolia. Confirm or reject it.

Create transactions on different networks, reject and/or confirm them, click around between popup and notification window, ensure nothing unexpected occurs

Screenshots/Recordings

Before

After

Pre-merge author checklist

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.

Description

Open in GitHub Codespaces

Related issues

Fixes:

Manual testing steps

  1. Go to this page...

Screenshots/Recordings

Before

After

Pre-merge author checklist

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.

…dRequestCount (#25614)

## **Description**

There's a case where there could be a race condition between the
`QueuedRequestController`'s network switching and the Routes' network
switching. The root issue is that the use of `getUnapprovedTransactions`
only gets transactions on the `current` chain, thus triggering Routes to
switch networks once the last transaction of the current chain is done,
but there could be more transactions on other chains which we should
allow QRC to handle network switching for.

Thus, this PR lets QRC control network switching until there are
absolutely no transactions or queued requests left.

[This PR includes an
**E2E](#25536 for
the scenario where there are multiple queued transactions but since this
is a race issue, it's difficult to reproduce in a provable E2E

[![Open in GitHub
Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/25614?quickstart=1)

## **Related issues**

Possibly related:
#25596

## **Manual testing steps**

1. Open `Tab 1`, connect to `Uniswap` on `Ethereum Mainnet`
2. Open `Tab 2`, connect to `PancakeSwap` on `BNB Chain`
3. Open `Tab 3`, connect to `Test Dapp` on `Sepolia`
4. Initiate a swap on `Tab 1` and `Tab 2` *BUT DO NOT CONFIRM IT, JUST
MOVE ON TO THE NEXT TAB*
5. Initiate a send on `Tab 3` *BUT DO NOT CONFIRM IT*
6. On the confirmation screen, you should still see the first
confirmation from `Tab 1` (`Uniswap`) on `Ethereum Mainnet`; confirm or
reject it. See the confirmation window close
7. A new confirmation popup should come up with the `PancakeSwap`/ `Tab
2` confirmation on `BNB` chain; confirm or reject it. See the
confirmation window close
8. See one last confirmation screen pop up for the `Tab 3` / `Test Dapp`
send on `Sepolia`. Confirm or reject it.

Create transactions on different networks, reject and/or confirm them,
click around between popup and notification window, ensure nothing
unexpected occurs


## **Screenshots/Recordings**

<!-- If applicable, add screenshots and/or recordings to visualize the
before and after of your change. -->

### **Before**

<!-- [screenshots/recordings] -->

### **After**

<!-- [screenshots/recordings] -->

## **Pre-merge author checklist**

- [ ] I've followed [MetaMask Contributor
Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask
Extension Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md).
- [ ] 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](https://jsdoc.app/) format
if applicable
- [ ] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)).
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.
@darkwing darkwing requested a review from a team as a code owner July 2, 2024 20:44
@darkwing darkwing changed the title fix: Multichain: UX: Check for transactions on all networks and QueuedRequestCount (#25614) fix (cherry-pick): Multichain: UX: Check for transactions on all networks and QueuedRequestCount (#25614) Jul 2, 2024
Copy link
Contributor

github-actions bot commented Jul 2, 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.

@metamaskbot
Copy link
Collaborator

Builds ready [deb50c5]
Page Load Metrics (55 ± 5 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint7210789115
domContentLoaded9231131
load418055105
domInteractive9231131
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 70.27 KiB (2.03%)
  • ui: 10.2 KiB (0.15%)
  • common: 171.97 KiB (2.73%)

Copy link

codecov bot commented Jul 9, 2024

Codecov Report

Attention: Patch coverage is 82.75862% with 5 lines in your changes missing coverage. Please review.

Project coverage is 65.61%. Comparing base (5bce75c) to head (deb50c5).

Files Patch % Lines
ui/pages/routes/routes.component.js 0.00% 2 Missing ⚠️
ui/selectors/transactions.js 86.67% 2 Missing ⚠️
ui/selectors/util.js 87.50% 1 Missing ⚠️
Additional details and impacted files
@@               Coverage Diff                @@
##           Version-v12.0.0   #25641   +/-   ##
================================================
  Coverage            65.61%   65.61%           
================================================
  Files                 1363     1363           
  Lines                54352    54378   +26     
  Branches             14130    14138    +8     
================================================
+ Hits                 35658    35676   +18     
- Misses               18694    18702    +8     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@danjm danjm merged commit f391832 into Version-v12.0.0 Jul 9, 2024
76 of 77 checks passed
@danjm danjm deleted the Version-v12.0.0.0-cherrypick-redirect-simplification branch July 9, 2024 16:30
@github-actions github-actions bot locked and limited conversation to collaborators Jul 9, 2024
@metamaskbot metamaskbot added the release-12.0.0 Issue or pull request that will be included in release 12.0.0 label Jul 9, 2024
@metamaskbot
Copy link
Collaborator

No release label on PR. Adding release label release-12.0.0 on PR, as PR was cherry-picked in branch 12.0.0.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
release-12.0.0 Issue or pull request that will be included in release 12.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants