metamask-extension icon indicating copy to clipboard operation
metamask-extension copied to clipboard

Published 20 hours ago • verified publisher icon MetaMask

Replace useSafeGasEstimatePolling with usePolling

Open jiexi opened this issue 1 year ago • 2 comments

Description

Related issues

Fixes:

Manual testing steps

  1. Go to this page...

Screenshots/Recordings

Before

After

Pre-merge author checklist

  • [ ] I’ve followed MetaMask Coding Standards.
  • [ ] I've clearly explained what problem this PR is solving and how it is solved.
  • [ ] I've linked related issues
  • [ ] I've included manual testing steps
  • [ ] I've included screenshots/recordings if applicable
  • [ ] 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.
  • [ ] I’ve properly set the pull request status:
    • [ ] In case it's not yet "ready for review", I've set it to "draft".
    • [ ] In case it's "ready for review", I've changed it from "draft" to "non-draft".

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.

jiexi avatar Feb 16 '24 17:02 jiexi

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.

github-actions[bot] avatar Feb 16 '24 17:02 github-actions[bot]

Codecov Report

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

Project coverage is 68.77%. Comparing base (6c810aa) to head (d7b1e5e).

Files Patch % Lines
ui/ducks/metamask/metamask.js 90.48% 2 Missing :warning:
ui/store/actions.ts 77.78% 2 Missing :warning:
app/scripts/metamask-controller.js 0.00% 1 Missing :warning:
ui/pages/swaps/prepare-swap-page/review-quote.js 0.00% 1 Missing :warning:
Additional details and impacted files 
@@             Coverage Diff             @@
##           develop   #23010      +/-   ##
===========================================
+ Coverage    68.70%   68.77%   +0.07%     
===========================================
  Files         1120     1120              
  Lines        43509    43527      +18     
  Branches     11636    11654      +18     
===========================================
+ Hits         29891    29933      +42     
+ Misses       13618    13594      -24

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Feb 16 '24 18:02 codecov[bot]

No dependency changes detected. Learn more about Socket for GitHub ↗︎

👍 No dependency changes detected in pull request

socket-security[bot] avatar Feb 23 '24 22:02 socket-security[bot]

Builds ready [45f960e]
Page Load Metrics (1208 ± 399 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint662151274120
domContentLoaded10106362813
load5721351208832399
domInteractive10105362813
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 278 Bytes (0.01%)
  • ui: 2.08 KiB (0.03%)
  • common: 0 Bytes (0.00%)

metamaskbot avatar Feb 26 '24 21:02 metamaskbot

Builds ready [7d1eca7]
Page Load Metrics (1240 ± 465 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint612351315325
domContentLoaded977332211
load4623181240969465
domInteractive976332211
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 278 Bytes (0.01%)
  • ui: 2.08 KiB (0.03%)
  • common: 0 Bytes (0.00%)

metamaskbot avatar Feb 27 '24 19:02 metamaskbot

Builds ready [3f6f272]
Page Load Metrics (1324 ± 439 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint602561364019
domContentLoaded107333199
load4824491324913439
domInteractive107333199
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 278 Bytes (0.01%)
  • ui: 2.08 KiB (0.03%)
  • common: 0 Bytes (0.00%)

metamaskbot avatar Feb 28 '24 17:02 metamaskbot

Overall no issues with behavior from my first pass. Although there are styling issues that came up on a certain network.

Network name: Ganache 8545 1337 New RPC URL: HTTP://127.0.0.1:8545 Chain ID: 1337 Currency symbol: g-eth

ganache-send-tx-gas-styling-issue

tmashuang avatar Feb 29 '24 00:02 tmashuang

I will update this same comment with the findings during the testing phase.

QA Findings

1. Unresponsive network handling behaviour

Whenever I am on a network which is unresponsive, I see errors on

  • StaticIntervalPollingController
  • PollingBlockTracker
  • RPC errors

Could those be handled somehow? This way we would avoid having too much noise in Sentry. Then, after I switch to another responsive network, I continue to see errors and requests being made in the old network.

https://github.com/MetaMask/metamask-extension/assets/54408225/aab0c11e-abc8-4920-861e-29600013a964

https://github.com/MetaMask/metamask-extension/assets/54408225/c520e259-ef0d-4a93-8fa1-f08885e47666

2. Can't perform a React state update on an unmounted component

I've encountered the following warning . Can't perform a React state update on an unmounted component. This is a no-op, but it indicates a memory leak in your application for 2 actions:

  • When trying to Edit the gas, I saw this one in useGasFeeEstimates:

Screenshot from 2024-03-03 13-39-29

  • When clicking Edit transaction, I saw this one in gasTiming.component:

https://github.com/MetaMask/metamask-extension/assets/54408225/59757ce5-66fb-48e0-9683-0c3e2303a083

3. Performance Degradation

Not sure if it's exclusive to this PR, since I've seen this in other builds recently. However, thought it is worth mentioning (might be related to the issue 2?). When starting a transaction, I'm experiencing a significant performance degradation and I see these warnings on the background console. This one seems particularly related to gas:

Screenshot from 2024-03-03 13-34-40

These seem more generic. Though not sure if somehow related to the multichain work

Screenshot from 2024-03-03 13-35-58

4. Couldn't find networkClientId for chainId

I have encountered the background error: Couldn't find networkClientId for chainId but still not sure yet how to repro. I was using a local network.

Screenshot from 2024-03-03 12-48-44

seaona avatar Mar 03 '24 12:03 seaona

Item 2: Can't perform a React state update on an unmounted component. This is a no-op, but it indicates a memory leak in your application should be addressed by https://github.com/MetaMask/metamask-extension/pull/23010/commits/d0d702f3f21b4be3ea6fa596205495eca65d1f05

jiexi avatar Mar 04 '24 18:03 jiexi

Builds ready [d0d702f]
Page Load Metrics (1379 ± 373 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint672201424220
domContentLoaded9103452613
load5423851379778373
domInteractive9103452612
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 278 Bytes (0.01%)
  • ui: 2.11 KiB (0.03%)
  • common: 0 Bytes (0.00%)

metamaskbot avatar Mar 04 '24 18:03 metamaskbot

  1. Unresponsive network handling behaviour Whenever I am on a network which is unresponsive, I see errors on
  • StaticIntervalPollingController
  • PollingBlockTracker
  • RPC errors

Could those be handled somehow? This way we would avoid having too much noise in Sentry.

So on this branch I'm only seeing the errors for PollingBlockTracker (which are expected though should probably be formatted better) since polling for new block info is indeed failing.

I'm also seeing these errors on develop if you stop a local testnet while its selected. So I'd say we should handle them in a separate PR if at all.

Then, after I switch to another responsive network, I continue to see errors and requests being made in the old network.

I wasn't able to reproduce this on this branch 🤔 Could you describe any other steps you took before you saw this happening? I'm assuming you just started a local testnet and then turned it off and then looked for errors and then switched networks to sepolia?

adonesky1 avatar Mar 04 '24 21:03 adonesky1

Item 3: Seems like this is caused by having a very large state in MetaMask, likely from having many transactions in state and/or a slow machine. IMO not related to these particular set of changes.

Item 4: This is a DetectTokensController issue that I've brought to the attention of the appropriate individuals.

That should be all the items. Thank you for this thoroughly detailed list, @seaona!

jiexi avatar Mar 04 '24 22:03 jiexi

:green_circle: 1. Unresponsive network handling behaviour --> out of scope

I wasn't able to reproduce this on this branch 🤔 Could you describe any other steps you took before you saw this happening? I'm assuming you just started a local testnet and then turned it off and then looked for errors and then switched networks to sepolia?

I am able to repro, I see errors both from StaticIntervalPollingController and PolliognBlockTracker after I switch networks. However, as you pointed then those seem to be independent from this PR, so it can be left out of scope if that sounds good. I marked is as green, but feel free to disagree

Steps:

  1. Add ganache (server is on)
  2. Perform a tx
  3. Switch to sepolia
  4. See errors continue --only stopped if I reload the wallet with the reload button from the Extension Chrome page

:yellow_circle: 2. Can't perform a React state update on an unmounted component

  • :green_circle: I see the useGasFeeEstimates fixed
  • :yellow_circle: I still see the gas-timing-component when editing a transaction

https://github.com/MetaMask/metamask-extension/assets/54408225/30035069-b128-46e4-9d6a-1ec8baa935f8

:green_circle: 3. Performance Degradation

It seems that this might be also out of scope of this PR. I've seen that this happened to other branches with multichain flag enabled. Another QA engineer from Confirmations (@sleepytanya ) also experienced this in other branches.

:green_circle: 4. Couldn't find networkClientId for chainId

This is out of scope as per this comment:

Item 4: This is a DetectTokensController issue that I've brought to the attention of the appropriate individuals.

seaona avatar Mar 05 '24 10:03 seaona

  1. Can't perform a React state update on an unmounted component
  • I still see the gas-timing-component when editing a transaction

This is a bug that currently exists in develop. I have manually checked on develop to confirm. We will address as part of this PR though https://github.com/MetaMask/metamask-extension/pull/23010/commits/1080c91aa5f0077229bf270b5b2916b61899d305

jiexi avatar Mar 05 '24 17:03 jiexi

I am able to repro, I see errors both from StaticIntervalPollingController and PolliognBlockTracker after I switch networks.

Steps:

Add ganache (server is on) Perform a tx Switch to sepolia

I agree this is should be addressed separately but I'm confused why I'm not seeing the errors you describe. I followed the steps above and this is what I'm seeing:

https://github.com/MetaMask/metamask-extension/assets/34557516/546a8592-3237-44d1-af49-27d02e8d6a20

adonesky1 avatar Mar 05 '24 18:03 adonesky1

Builds ready [37ef9ac]
Page Load Metrics (857 ± 451 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint862381364320
domContentLoaded158634199
load772761857939451
domInteractive158534199
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 278 Bytes (0.01%)
  • ui: 2.14 KiB (0.03%)
  • common: 0 Bytes (0.00%)

metamaskbot avatar Mar 06 '24 17:03 metamaskbot

Builds ready [974acba]
Page Load Metrics (1093 ± 393 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint842111333316
domContentLoaded107132168
load8020071093818393
domInteractive97132168
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 278 Bytes (0.01%)
  • ui: 2.14 KiB (0.03%)
  • common: 0 Bytes (0.00%)

metamaskbot avatar Mar 06 '24 20:03 metamaskbot

Builds ready [d7b1e5e]
Page Load Metrics (1058 ± 431 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint752931385627
domContentLoaded107427157
load5824081058897431
domInteractive107427157
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 278 Bytes (0.01%)
  • ui: 2.14 KiB (0.03%)
  • common: 0 Bytes (0.00%)

metamaskbot avatar Mar 08 '24 14:03 metamaskbot