metamask-extension
metamask-extension copied to clipboard
Fix detected tokens added to wrong network
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):
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.
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.
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.
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.
Builds ready [5e6b428]
- builds: chrome, firefox
- builds (beta): chrome
- builds (flask): chrome, firefox
- builds (MMI): chrome, firefox
- builds (test): chrome, firefox
- builds (test-flask): chrome, firefox
- build viz: Build System
- mv3: Background Module Init Stats
- mv3: UI Init Stats
- mv3: Module Load Stats
- mv3: Bundle Size Stats
- mv2: E2e Actions Stats
- code coverage: Report
- storybook: Storybook
- typescript migration: Dashboard
- all artifacts
Page Load Metrics (816 ± 20 ms)
Platform | Page | Metric | Min (ms) | Max (ms) | Average (ms) | StandardDeviation (ms) | MarginOfError (ms) |
---|---|---|---|---|---|---|---|
Chrome | Home | firstPaint | 85 | 133 | 112 | 18 | 8 |
domContentLoaded | 9 | 49 | 19 | 11 | 5 | ||
load | 732 | 898 | 816 | 43 | 20 | ||
domInteractive | 9 | 49 | 19 | 11 | 5 |
Bundle size diffs [🚀 Bundle size reduced!]
- background: -54 Bytes (-0.00%)
- ui: 0 Bytes (0.00%)
- common: 0 Bytes (0.00%)