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

Fix detected tokens added to wrong network

Open bergeron opened this issue 1 year ago • 4 comments

Description

Fixes a race condition during token detection after switching networks.

During the network switch, some controllers state are still on the old chain id, and others are on the new chain id. This can cause different issues depending which controllers win the race.

The worst case is that detected tokens are added to the wrong network (see related issues):

image

In that ^ screenshot there are 2 mainnet tokens that we have a balance of, but they incorrectly appear under linea.

Some lesser (but still incorrect) behavior include using TokensController.(tokens|ignoredTokens|detectedTokens) from the wrong chain, and AssetsContractController.getBalancesInSingleCall() querying the wrong chain. These outcomes of the race as less bad, since querying the wrong chain likely returns 0 balances. And not excluding tokens already ignored / detected is just extra work.

There's a fix for each of the relevant controllers (DetectTokensController, TokensController, AssetsContractController) ensuring we use the chain ID being switched to.

Related issues

Auto token detection list collision with other networks #22512

Autodetect tokens display Mainnet tokens on another network #7587

Manual testing steps

It takes a few minutes of switching networks back and forth to reproduce the bug. But basically keep doing that and we should not see tokens hop from 1 network to the other.

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.

bergeron avatar Feb 05 '24 16:02 bergeron

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 05 '24 16:02 github-actions[bot]

Codecov Report

Attention: 8 lines in your changes are missing coverage. Please review.

Comparison is base (3fa4cd6) 68.48% compared to head (6550633) 68.46%.

:exclamation: Current head 6550633 differs from pull request most recent head 5e6b428. Consider uploading reports for the commit 5e6b428 to get more accurate results

Files Patch % Lines
app/scripts/controllers/detect-tokens.js 11.11% 8 Missing :warning:
Additional details and impacted files
@@             Coverage Diff             @@
##           develop   #22814      +/-   ##
===========================================
- Coverage    68.48%   68.46%   -0.02%     
===========================================
  Files         1088     1088              
  Lines        42886    42826      -60     
  Branches     11418    11395      -23     
===========================================
- Hits         29370    29319      -51     
+ Misses       13516    13507       -9     

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

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

A related thing I want to fix separately. We run detection 3 times when switching chains:

  • On network controller switching chains
  • On token list changing 1st time (on chain switch to cached version)
  • On token list changing 2nd time (after token list is updated via HTTP get)

Only needs to run once.

bergeron avatar Feb 08 '24 19:02 bergeron

Builds ready [5e6b428]
Page Load Metrics (816 ± 20 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint85133112188
domContentLoaded94919115
load7328988164320
domInteractive94919115
Bundle size diffs [🚀 Bundle size reduced!]
  • background: -54 Bytes (-0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

metamaskbot avatar Feb 08 '24 19:02 metamaskbot