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

[Bug]: Auto token detection list collision with other networks #22512

Closed
tmashuang opened this issue Jan 12, 2024 · 2 comments
Closed

[Bug]: Auto token detection list collision with other networks #22512

tmashuang opened this issue Jan 12, 2024 · 2 comments
Assignees
Labels
area-tokens regression-prod-11.8.0 Regression bug that was found in production in release 11.8.0 Sev1-high High severity; partial loss of service with severe impact upon users, with no workaround. team-assets type-bug

Comments

@tmashuang
Copy link
Contributor

Describe the bug

When having auto detection setting turned on, multiple networks, and multiple tokens associated with an account I find that the list for "found tokens to add" has other from another network on the list to add.

Expected behavior

Should only detect tokens based on network to curate the "found tokens to add" list.

Screenshots/Recordings

Screenshot 2024-01-12 at 7 59 13 PM
Screenshot 2024-01-12 at 7 55 34 PM

Steps to reproduce

Turn on Auto Detection of tokens list.
Add networks where the account has tokens. (I used Linea and Matic)
Wait for token detection list notification to add found tokens.
Lists should have extra tokens from Mainnet.

Error messages or log output

No response

Version

11.8.0

Build type

Beta

Browser

Chrome

Operating system

MacOS

Hardware wallet

No response

Additional context

No response

Severity

No response

@tmashuang tmashuang added type-bug area-tokens release-blocker This bug is blocking the next release labels Jan 12, 2024
@metamaskbot metamaskbot added the regression-prod-11.8.0 Regression bug that was found in production in release 11.8.0 label Jan 12, 2024
@adonesky1
Copy link
Contributor

@tmashuang, neither @jiexi or I have have been able to reproduce this issue.

@tmashuang tmashuang removed the release-blocker This bug is blocking the next release label Jan 15, 2024
@gauthierpetetin gauthierpetetin added the Sev1-high High severity; partial loss of service with severe impact upon users, with no workaround. label Jan 19, 2024
@bergeron bergeron self-assigned this Feb 1, 2024
bergeron added a commit that referenced this issue Feb 13, 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](https://github.com/MetaMask/metamask-extension/assets/3500406/c4dc4a21-9234-44a5-a915-7d2910a28a60)

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](#22512)

[Autodetect tokens display Mainnet tokens on another network
#7587](MetaMask/metamask-mobile#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**

<!-- If applicable, add screenshots and/or recordings to visualize the
before and after of your change. -->

### **Before**

<!-- [screenshots/recordings] -->

### **After**

<!-- [screenshots/recordings] -->

## **Pre-merge author checklist**

- [ ] I’ve followed [MetaMask Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md).
- [ ] 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](https://jsdoc.app/) format
if applicable
- [ ] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)).
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.
sahar-fehri pushed a commit that referenced this issue Feb 14, 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](https://github.com/MetaMask/metamask-extension/assets/3500406/c4dc4a21-9234-44a5-a915-7d2910a28a60)

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](#22512)

[Autodetect tokens display Mainnet tokens on another network
#7587](MetaMask/metamask-mobile#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**

<!-- If applicable, add screenshots and/or recordings to visualize the
before and after of your change. -->

### **Before**

<!-- [screenshots/recordings] -->

### **After**

<!-- [screenshots/recordings] -->

## **Pre-merge author checklist**

- [ ] I’ve followed [MetaMask Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md).
- [ ] 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](https://jsdoc.app/) format
if applicable
- [ ] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)).
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
Copy link
Contributor

Fixed in #22814

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-tokens regression-prod-11.8.0 Regression bug that was found in production in release 11.8.0 Sev1-high High severity; partial loss of service with severe impact upon users, with no workaround. team-assets type-bug
Projects
None yet
Development

No branches or pull requests

6 participants