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: UX: Multichain: Fix dead network problem when switching tabs #25425

Merged
merged 8 commits into from
Jul 3, 2024

Conversation

darkwing
Copy link
Contributor

@darkwing darkwing commented Jun 19, 2024

Description

This PR ensures that a connection to a given network does not block the UI when switching to a dapp tab that remembers that down network, thus preventing a non-respondent network from not displaying the MetaMask UI

Open in GitHub Codespaces

Related issues

Fixes: #25588

Manual testing steps

Scenario 1: Kill network, manually click MetaMask extension icon

  1. Start local ganache
  2. Add the local network to your network listing (Settings -> Networks -> Add custom network)
  3. In tab 1, connect to dapp 1 on Localhost
  4. In tab 2, connect to dapp 2 on Ethereum Mainnet
  5. Kill local ganache
  6. Click tab 1
  7. Click the MetaMask icon in the extension bar
  8. See UI pop up as expected

Scenario 2: Kill network, trigger transaction from dapp

  1. Start local ganache
  2. Add the local network to your network listing (Settings -> Networks -> Add custom network)
  3. In tab 1, connect to dapp 1 on Localhost
  4. In tab 2, connect to dapp 2 on Ethereum Mainnet
  5. Kill local ganache
  6. Click tab 1
  7. Trigger a transaction from the dapp
  8. See UI pop up quickly, with confirmation, as expected

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.

Copy link
Contributor

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.

@darkwing darkwing marked this pull request as ready for review June 24, 2024 20:32
@darkwing darkwing requested a review from a team as a code owner June 24, 2024 20:32
@darkwing
Copy link
Contributor Author

I noticed that this test was still taking a long time on Chrome so I did some more manual testing and it appears as though (assuming you kill local ganache):

  1. Click the send button in test dapp
  2. (random, sometimes long wait time)
  3. The confirmation eventually pops up

Spying on the service worker between clicking the send button and the confirmation finally coming up, it appears the confirmation doesn't appear until the following network request(s) complete:

  • https://tx-sentinel-ethereum-mainnet.api.cx.metamask.io/networks
  • https://gas.api.cx.metamask.io/networks/1337/suggestedGasFees
SCR-20240625-rfkn

@darkwing darkwing force-pushed the multichain-broken-network branch 2 times, most recently from dfea637 to 4635c11 Compare June 27, 2024 14:32
@bergeron
Copy link
Contributor

When trying to send a transaction on a network with dead RPC, I see 2 RPC calls that each take ~5 seconds to fail, which together cause the ~10 second delay in receiving the metamask popup:

  • Click send on test dapp
  • extension executes addTransactionWithController
  • Calls TransactionController.addTransaction
    • determineTransactionType runs the eth_getCode RPC query
    • updateGasProperties runs the eth_estimateGas RPC query

I don't know why the RPC queries take 5 seconds instead of failing fast. Perhaps somewhere lower level is doing retry. But that's the only code I see slowing down the popup.

Screen.Recording.2024-06-27.at.2.21.48.PM.mov

@seaona
Copy link
Contributor

seaona commented Jun 28, 2024

on the test environment I am seeing a great amount of requests for mostly eth_estimateGas, eth_gasPrice and eth_blockNumber. Those return very fast, but we keep making requests until ~10seconds.

testing-env-dead-network.mp4

and on the dev build from this branch, I am seeing a similar behaviour as I see in production: sometimes the popup is open fast, but sometimes it takes ~5 seconds

multichain-broken-network-branch.mp4

Copy link

codecov bot commented Jun 28, 2024

Codecov Report

Attention: Patch coverage is 0% with 6 lines in your changes missing coverage. Please review.

Project coverage is 69.56%. Comparing base (4a9c480) to head (84786eb).
Report is 25 commits behind head on develop.

Files Patch % Lines
ui/store/actions.ts 0.00% 4 Missing ⚠️
app/scripts/metamask-controller.js 0.00% 2 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop   #25425      +/-   ##
===========================================
- Coverage    69.57%   69.56%   -0.01%     
===========================================
  Files         1360     1360              
  Lines        48172    48177       +5     
  Branches     13296    13296              
===========================================
  Hits         33513    33513              
- Misses       14659    14664       +5     

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

@metamaskbot
Copy link
Collaborator

Builds ready [84786eb]
Page Load Metrics (263 ± 280 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint653551165928
domContentLoaded97530199
load412024263582280
domInteractive97530199
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 80 Bytes (0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 194 Bytes (0.00%)

@darkwing darkwing added the needs-assets-ux-review A shared label between the Assets and UX team to flag PRs ready for consolidated team review. label Jul 2, 2024
@darkwing darkwing merged commit 363a8f4 into develop Jul 3, 2024
63 checks passed
@darkwing darkwing deleted the multichain-broken-network branch July 3, 2024 21:51
@github-actions github-actions bot locked and limited conversation to collaborators Jul 3, 2024
@metamaskbot metamaskbot added the release-12.2.0 Issue or pull request that will be included in release 12.2.0 label Jul 3, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
needs-assets-ux-review A shared label between the Assets and UX team to flag PRs ready for consolidated team review. release-12.2.0 Issue or pull request that will be included in release 12.2.0 team-wallet-ux
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

[Bug]: UX: Multichain: Slow / Down Network Stalls UI
6 participants