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.

Sign up for GitHub

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

Jump to bottom

[Bug]: After swapping tokens don't show up in the wallet #23612

Closed
davibroc opened this issue Mar 20, 2024 · 7 comments
Closed

[Bug]: After swapping tokens don't show up in the wallet #23612

davibroc opened this issue Mar 20, 2024 · 7 comments
Assignees
@bergeron
Labels
regression-RC DEPRECATED: Please use "regresssion-RC-x.y.z" label instead regression-RC-11.13.0 release-11.14.0 Issue or pull request that will be included in release 11.14.0 release-blocker This bug is blocking the next release Sev1-high High severity; partial loss of service with severe impact upon users, with no workaround. team-assets type-bug

Comments

@davibroc
Copy link
Contributor

Describe the bug

After swapping I should end up with the swapped token in the wallet but it's not showing up. I am seeing this not only on mainnet but also other popular networks and hw wallets. See videos below.

Expected behavior

No response

Screenshots/Recordings

mainnet.mov
BNB.mov
Arbitrum.mov
erc20-erc20.mov

Steps to reproduce

  1. Swap a token
  2. Wait for completion
  3. Check the token is in the wallet

Error messages or log output

No response

Version

develop

Build type

None

Browser

Chrome

Operating system

MacOS

Hardware wallet

No response

Additional context

No response

Severity

No response
@github-project-automation github-project-automation bot moved this to To be fixed in Bugs by severity Mar 20, 2024
@gauthierpetetin gauthierpetetin added team-assets Sev2-normal Normal severity; minor loss of service or inconvenience. labels Mar 21, 2024
@sleepytanya
Copy link
Contributor

Might be related: when we initially start the wallet the prompt to import the existing tokens for current account is not displayed.

@bergeron
Copy link
Contributor

I'm working on a detection bug introduced by #22928 that may be the cause / related to this. But haven't tested swaps specifically. Assigning to myself.

@bergeron bergeron self-assigned this Mar 21, 2024
@davibroc davibroc added release-blocker This bug is blocking the next release Sev1-high High severity; partial loss of service with severe impact upon users, with no workaround. and removed Sev2-normal Normal severity; minor loss of service or inconvenience. labels Mar 27, 2024
@davibroc
Copy link
Contributor Author

@bergeron this has to be fixed before 11.14 goes out.

@davibroc davibroc added release-11.4.0 Issue or pull request that will be included in release 11.4.0 release-11.14.0 Issue or pull request that will be included in release 11.14.0 and removed release-11.4.0 Issue or pull request that will be included in release 11.4.0 labels Mar 27, 2024
@metamaskbot metamaskbot added the regression-RC DEPRECATED: Please use "regresssion-RC-x.y.z" label instead label Mar 27, 2024
@bergeron bergeron mentioned this issue Mar 29, 2024
Merged
7 tasks
@sleepytanya
Copy link
Contributor

NFTs displayed as expected, but ERC20 tokens are not detected when token auto-detection is on, tokens also are not displayed after importing them manually:

Screen.Recording.2024-03-28.at.9.14.57.PM.mov

bergeron added a commit that referenced this issue Apr 5, 2024
@bergeron
fix: token detection and import (#23798)
f327149 
## **Description**

Fixes issues with detecting and manually importing tokens, caused by
recent controller changes.

Hotfix for 11.14.0

This state publish had an undefined variable, and had the wrong function
signature:

<img width="597" alt="Screenshot 2024-03-28 at 6 04 41 PM"
src="https://github.com/MetaMask/metamask-extension/assets/3500406/15cd8341-725b-49d8-bd4d-bf8539185214">

Which prevented the controllers from processing account switches.

A test was failing because RPC requests were sent before onboarding.
Delayed the enabling of token detection controller to
`triggerNetworkrequests`.


Also patching this fix for detection during account switch on mainnet
when autodetection=off:
MetaMask/core#4133

[![Open in GitHub
Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/23798?quickstart=1)


## **Related issues**

Fixes:
- #23526
- #23612


## **Manual testing steps**


## **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 completed the PR template to the best of my ability
- [ ] 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.

## **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 added a commit that referenced this issue Apr 5, 2024
@bergeron
fix: token detection and import (#23798)
3e5ce75 
## **Description**

Fixes issues with detecting and manually importing tokens, caused by
recent controller changes.

Hotfix for 11.14.0

This state publish had an undefined variable, and had the wrong function
signature:

<img width="597" alt="Screenshot 2024-03-28 at 6 04 41 PM"
src="https://github.com/MetaMask/metamask-extension/assets/3500406/15cd8341-725b-49d8-bd4d-bf8539185214">

Which prevented the controllers from processing account switches.

A test was failing because RPC requests were sent before onboarding.
Delayed the enabling of token detection controller to
`triggerNetworkrequests`.


Also patching this fix for detection during account switch on mainnet
when autodetection=off:
MetaMask/core#4133

[![Open in GitHub
Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/23798?quickstart=1)


## **Related issues**

Fixes:
- #23526
- #23612


## **Manual testing steps**


## **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 completed the PR template to the best of my ability
- [ ] 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.

## **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.
@sleepytanya sleepytanya mentioned this issue Apr 7, 2024
Closed
@github-project-automation github-project-automation bot moved this to To be fixed in Bugs by team Apr 9, 2024
@bergeron
Copy link
Contributor

bergeron commented Apr 9, 2024

Should be fixed by #23798 and cherry picked to 11.14 in #23871

@github-project-automation github-project-automation bot moved this from To be fixed to Fixed in Bugs by team Apr 12, 2024
@github-project-automation github-project-automation bot moved this from To be fixed to Fixed in Bugs by severity Apr 12, 2024
@benjisclowder benjisclowder mentioned this issue May 27, 2024
Open
9 tasks
@benjisclowder
Copy link
Contributor

benjisclowder commented Jun 14, 2024 •
edited
Loading

Collaborative Effort Required for Root Cause Analysis (RCA) on Critical Issues

We are quickly approaching the end of the quarter and we encourage you once again to take some moments and perform RCA on this critical issue. You may do so by answering the questions below:

  1. What PR fixed the issue?
  2. Can you pinpoint the commit from which the issue originated?
  3. Write a short explanation of the technical cause of the bug
  4. How could we have avoided merging this bug? What would have had to be different about our code, tests or processes?
    4.1. Were there any missing unit, e2e or manual tests that could have preempted this issue?
    4.2. Were there any other elements lacking, such as typed code, comprehensive documentation, well-architected APIs, etc., that might have prevented this issue?
    4.3. If your answer to a and b is no, then is there anything at all that you can think of that, if it had been different before this bug was introduced, would have prevented it from being merged?

Please provide your answers as a reply to this comment and tag me as well.

You can read more about the initiative here. Thank you!

Tagging eng. who added the fix: @bergeron

@github-project-automation github-project-automation bot moved this to To be fixed in Bugs by team Jun 28, 2024
@benjisclowder
Copy link
Contributor

Collaborative Effort Required for Root Cause Analysis (RCA) on Critical Issues

We are quickly approaching the end of the quarter and we encourage you once again to take some moments and perform RCA on this critical issue. You may do so by answering the questions below:

  1. What PR fixed the issue?
  2. Can you pinpoint the commit from which the issue originated?
  3. Write a short explanation of the technical cause of the bug
  4. How could we have avoided merging this bug? What would have had to be different about our code, tests or processes?
    4.1. Were there any missing unit, e2e or manual tests that could have preempted this issue?
    4.2. Were there any other elements lacking, such as typed code, comprehensive documentation, well-architected APIs, etc., that might have prevented this issue?
    4.3. If your answer to a and b is no, then is there anything at all that you can think of that, if it had been different before this bug was introduced, would have prevented it from being merged?

Please provide your answers as a reply to this comment and tag me as well.

You can read more about the initiative here. Thank you!

Tagging eng. who added the fix: @bergeron

Hi, @bergeron , if you're not able to conduct RCA on this issue, is there anyone you can assign this to? I would very much appreciate it. Thank you!

@github-project-automation github-project-automation bot moved this to To be fixed in Bugs by team Nov 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@bergeron bergeron

Labels
regression-RC DEPRECATED: Please use "regresssion-RC-x.y.z" label instead regression-RC-11.13.0 release-11.14.0 Issue or pull request that will be included in release 11.14.0 release-blocker This bug is blocking the next release Sev1-high High severity; partial loss of service with severe impact upon users, with no workaround. team-assets type-bug
Projects
Bugs by severity
Archived in project
Bugs by team
Status: To be fixed
Milestone
No milestone
Development

No branches or pull requests
6 participants
@bergeron @davibroc @gauthierpetetin @metamaskbot @sleepytanya @benjisclowder