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]: MetaMask crash when try to import token after onboarding with basic functionality off #28864

Closed
chloeYue opened this issue Dec 3, 2024 · 3 comments · Fixed by #28870
Closed

[Bug]: MetaMask crash when try to import token after onboarding with basic functionality off #28864

chloeYue opened this issue Dec 3, 2024 · 3 comments · Fixed by #28870
Assignees
@sahar-fehri @gambinish
Labels
regression-RC-12.9.0 Regression bug that was found in release candidate (RC) for release 12.9.0 release-12.10.0 Issue or pull request that will be included in release 12.10.0 release-blocker This bug is blocking the next release Sev2-normal Normal severity; minor loss of service or inconvenience. team-assets type-bug

Comments

@chloeYue
Copy link
Contributor

chloeYue commented Dec 3, 2024 •
edited
Loading

Describe the bug

Onboarding MM by importing a wallet which have ERC20 token asset, toggle off basic functionality on manage privacy settings page during onboarding, click import token on homepage after onboarding makes MM crashes.

Note: not reproducible on production

Expected behavior

MM should not crash

Screenshots/Recordings

Screen.Recording.2024-12-03.at.11.54.44.mov

Steps to reproduce

  1. Onboarding to MM by importing wallet with ERC20 asset
  2. Toggle off basic functionality on privacy settings during onboarding
  3. Click import token on homepage

Error messages or log output

No response

Detection stage

During release testing

Version

12.9.0

Build type

None

Browser

Chrome

Operating system

MacOS

Hardware wallet

No response

Additional context

No response

Severity

No response
@chloeYue chloeYue added type-bug release-blocker This bug is blocking the next release team-assets regression-RC-12.9.0 Regression bug that was found in release candidate (RC) for release 12.9.0 labels Dec 3, 2024
@github-project-automation github-project-automation bot moved this to To be fixed in Bugs by severity Dec 3, 2024
@github-project-automation github-project-automation bot moved this to To be fixed in Bugs by team Dec 3, 2024
@chloeYue chloeYue added the Sev2-normal Normal severity; minor loss of service or inconvenience. label Dec 3, 2024
@seaona
Copy link
Contributor

seaona commented Dec 3, 2024

Could this be related to the below Sentry issue? I see that the error is the same TypeError: Cannot convert undefined or null to object. Possibly it's a different way to reproduce it, given in this ticket it's not in prod, but closely related 🤔

The below case is reproduceable with the Token price feature disabled, and not when enabled. It happens both in the RC and in current prod. Possibly something changed and now the error can also be triggered with the above repro steps

#28019

@gambinish gambinish mentioned this issue Dec 3, 2024
Closed
7 tasks
@gambinish gambinish self-assigned this Dec 3, 2024
@gambinish
Copy link
Contributor

gambinish commented Dec 3, 2024 •
edited
Loading

I believe I have a PR fix here: #28904

Will cherry pick to RC once approved and merged.

When basic functionality is off, marketData doesn't exist, so fiat values aren't rendered. I verified that this behavior is consistent with prod. Is this expected?

@gambinish
Copy link
Contributor

Let's actually go with @sahar-fehri fix here: #28870

Same fix for a different bug, but should also resolve this issue here.

@gambinish gambinish mentioned this issue Dec 3, 2024
Merged
7 tasks
github-merge-queue bot pushed a commit that referenced this issue Dec 5, 2024
@sahar-fehri @gambinish
fix: fix check for undefined marketData (#28870)
79a8f57 
## **Description**

This PR fixes app crash after user removes a network then adds it back
and clicks on import token banner

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

## **Related issues**

Fixes:
#28019 (comment)
Fixes: #28882
Fixes: #28864

## **Manual testing steps**
Manual steps are also described in the github
[issue](#28019 (comment)).
However; I do not think that the Show native token as main balance needs
to be ONt o repro the initial issue. Also no need to add new RPC from
chainList;

Settings:
1. Show balance and token price OFF
2. Token autodetect ON

On main view

1. Select an account which has some ERC20 tokens in Polygon
2. Add Polygon default network
3. See tokens are autodetected and you can open the modal -> but don't
import the tokens!
4. Switch to another network
5. Delete Polygon network
6. Re-add Polygon default network
7. Click Import tokens --> Wallet should not crash

12. ## **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 Contributor
Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask
Extension Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/main/.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/main/.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.

---------

Co-authored-by: Nick Gambino <[email protected]>
@github-project-automation github-project-automation bot moved this from To be fixed to Fixed in Bugs by severity Dec 5, 2024
@github-project-automation github-project-automation bot moved this from To be fixed to Fixed in Bugs by team Dec 5, 2024
@metamaskbot metamaskbot added the release-12.10.0 Issue or pull request that will be included in release 12.10.0 label Dec 5, 2024
gambinish added a commit that referenced this issue Dec 5, 2024
@sahar-fehri @gambinish
fix: fix check for undefined marketData (#28870)
35a2b3d 
## **Description**

This PR fixes app crash after user removes a network then adds it back
and clicks on import token banner

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

## **Related issues**

Fixes:
#28019 (comment)
Fixes: #28882
Fixes: #28864

## **Manual testing steps**
Manual steps are also described in the github
[issue](#28019 (comment)).
However; I do not think that the Show native token as main balance needs
to be ONt o repro the initial issue. Also no need to add new RPC from
chainList;

Settings:
1. Show balance and token price OFF
2. Token autodetect ON

On main view

1. Select an account which has some ERC20 tokens in Polygon
2. Add Polygon default network
3. See tokens are autodetected and you can open the modal -> but don't
import the tokens!
4. Switch to another network
5. Delete Polygon network
6. Re-add Polygon default network
7. Click Import tokens --> Wallet should not crash

12. ## **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 Contributor
Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask
Extension Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/main/.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/main/.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.

---------

Co-authored-by: Nick Gambino <[email protected]>
@gambinish gambinish mentioned this issue Dec 5, 2024
Merged
14 tasks
gambinish added a commit that referenced this issue Dec 5, 2024
@gambinish @sahar-fehri
fix: cherry-pick fix check for undefined marketData (#28870) (#28950)
e8a5d50 
## **Description**

Cherry pick to 12.9 RC of:
#28870

This PR fixes app crash after user removes a network then adds it back
and clicks on import token banner

[![Open in GitHub

Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/28870?quickstart=1)

## **Related issues**

Fixes:

#28019 (comment)
Fixes: #28882
Fixes: #28864

## **Manual testing steps**
Manual steps are also described in the github

[issue](#28019 (comment)).
However; I do not think that the Show native token as main balance needs
to be ONt o repro the initial issue. Also no need to add new RPC from
chainList;

Settings:
1. Show balance and token price OFF
2. Token autodetect ON

On main view

1. Select an account which has some ERC20 tokens in Polygon
2. Add Polygon default network
3. See tokens are autodetected and you can open the modal -> but don't
import the tokens!
4. Switch to another network
5. Delete Polygon network
6. Re-add Polygon default network
7. Click Import tokens --> Wallet should not crash

12. ## **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**

- [x] I've followed [MetaMask Contributor
Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask
Extension Coding

Standards](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/CODING_GUIDELINES.md).
- [x] I've completed the PR template to the best of my ability
- [x] I’ve included tests if applicable
- [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [x] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-extension/blob/main/.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.

---------


<!--
Please submit this PR as a draft initially.
Do not mark it as "Ready for review" until the template has been
completely filled out, and PR status checks have passed at least once.
-->

## **Description**

<!--
Write a short description of the changes included in this pull request,
also include relevant motivation and context. Have in mind the following
questions:
1. What is the reason for the change?
2. What is the improvement/solution?
-->

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

## **Related issues**

Fixes:

## **Manual testing steps**

1. Go to this page...
2.
3.

## **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 Contributor
Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask
Extension Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/main/.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/main/.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.

Co-authored-by: sahar-fehri <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@sahar-fehri sahar-fehri

@gambinish gambinish

Labels
regression-RC-12.9.0 Regression bug that was found in release candidate (RC) for release 12.9.0 release-12.10.0 Issue or pull request that will be included in release 12.10.0 release-blocker This bug is blocking the next release Sev2-normal Normal severity; minor loss of service or inconvenience. team-assets type-bug
Projects
Bugs by severity
Archived in project
Milestone
No milestone
6 participants
@sahar-fehri @gambinish @metamaskbot @seaona @chloeYue and others