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: cherry-pick fix check for undefined marketData (#28870) #28950

Conversation

gambinish
Copy link
Contributor

@gambinish gambinish commented Dec 5, 2024

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

Related issues

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

Manual testing steps

Manual steps are also described in the github
issue. 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

  8. Screenshots/Recordings

Before

After

Pre-merge author checklist

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.

Description

Open in GitHub Codespaces

Related issues

Fixes:

Manual testing steps

  1. Go to this page...

Screenshots/Recordings

Before

After

Pre-merge author checklist

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.

## **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]>
Copy link
Contributor

github-actions bot commented Dec 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.

@gambinish gambinish marked this pull request as ready for review December 5, 2024 03:01
@gambinish gambinish requested a review from a team as a code owner December 5, 2024 03:01
@gambinish gambinish changed the title fix: fix check for undefined marketData (#28870) fix: cherry-pick fix check for undefined marketData (#28870) Dec 5, 2024
@metamaskbot
Copy link
Collaborator

Builds ready [35a2b3d]
Page Load Metrics (1999 ± 94 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint16522348199719594
domContentLoaded16442324197518589
load16522348199919594
domInteractive24413252
backgroundConnect766252010
firstReactRender15322152
getState98148128126
initialActions01000
loadScripts12261855150017383
setupStore711811
uiStartup187527492291232111
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 289 Bytes (0.01%)
  • ui: 47 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

@gambinish gambinish merged commit e8a5d50 into Version-v12.9.0 Dec 5, 2024
76 of 79 checks passed
@gambinish gambinish deleted the fix/cherry-pick-79a8f57d2ac22bd36b09bfe63084f6ee8936785d branch December 5, 2024 16:08
@github-actions github-actions bot locked and limited conversation to collaborators Dec 5, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants