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 detected tokens added to wrong network #22814

Merged
merged 5 commits into from
Feb 13, 2024

Conversation

bergeron
Copy link
Contributor

@bergeron bergeron commented Feb 5, 2024

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.

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 bergeron requested a review from a team as a code owner February 5, 2024 16:29
Copy link
Contributor

github-actions bot commented Feb 5, 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.

Copy link

codecov bot commented Feb 5, 2024

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%.

❗ 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 ⚠️
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     

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

@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 Feb 8, 2024
@bergeron
Copy link
Contributor Author

bergeron commented Feb 8, 2024

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.

@metamaskbot
Copy link
Collaborator

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%)

Comment on lines -104 to -112
tokensController?.subscribe(
({ tokens = [], ignoredTokens = [], detectedTokens = [] }) => {
this.tokenAddresses = tokens.map((token) => {
return token.address;
});
this.hiddenTokens = ignoredTokens;
this.detectedTokens = detectedTokens;
},
);
Copy link
Contributor Author

@bergeron bergeron Feb 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tokensController?.subscribe was typically the last event to fire during a chain switch. Therefore tokenAddresses, hiddenTokens, and detectedTokens were still from the old chain during detection, even when (most) everything else is pointing to the new chain. Since tokensController has state per-chain, we can just grab the tokens for the correct chain instead of waiting to be updated via subscription.

Comment on lines +245 to +246
const chainIdAgainstWhichToDetect =
chainId ?? this.getChainIdFromNetworkStore();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

During a network switch, this.chainId is from the old chain until NetworkController:stateChange fires and updates it. But TokenListController:stateChange can fire first, leaving it stale at this point. This change gets the updated chain id from the network controller, as happens elsewhere in this file.

Comment on lines +187 to +189
this.network.findNetworkClientIdByChainId(
chainIdAgainstWhichToDetect,
),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels like maybe the sketchiest one. We weren't passing an explicit NetworkClientId here, which means AssetsContractController falls back to the chain in its state. But that controller's onNetworkDidChange has not always fired yet, so its still on the old chain, and we detect the new token addresses on the old chain.

This was the only way I found to go from chain id -> NetworkClientId since that's what getBalancesInSingleCall accepts.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey Brian, yeah so this is the best option available for right now. You've correctly identified an issue with this flow. We have implemented an alternative polling solution that we will be leveraging shortly but you've highlighted one piece that we're missing. @shanejonas we will need to pass along the networkClientId in the _executePoll method.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

@adonesky1 adonesky1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks for the fix! And totally agree with your comment that we should, in a subsequent PR, make sure the restartTokenDetection only fires once after network changes!

@bergeron bergeron merged commit b0bd096 into develop Feb 13, 2024
76 of 77 checks passed
@bergeron bergeron deleted the brian/token-detection-network-switch branch February 13, 2024 17:39
@github-actions github-actions bot locked and limited conversation to collaborators Feb 13, 2024
@metamaskbot metamaskbot added the release-11.12.0 Issue or pull request that will be included in release 11.12.0 label Feb 13, 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-11.12.0 Issue or pull request that will be included in release 11.12.0 team-assets
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants