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: fix fetch price #3687

Merged
merged 3 commits into from
Dec 21, 2023
Merged

fix: fix fetch price #3687

merged 3 commits into from
Dec 21, 2023

Conversation

sahar-fehri
Copy link
Contributor

Explanation

This used to throw an error;
Noticed that on the import flow, if you try to import two tokens exp (USDC and STEELO) where it should succeed to fetch prices for USDC and not for STEELO. It will throw for STEELO preventing fetching for USDC; hence you end up on the home page where you do not see the user token amount for USDC.

Steps to REPRODUCE:
1- Go to home page on extension (IDEALLY remove the extension and import again so u don't have cached rate values)
2- search for STEELO and select it
3- search for USDC and select it
4- Click next and click import
5- go to home page and notice you do not see the USDC token amount in fiat

References

Changelog

@metamask/assets-controllers

  • Fixed: removed the throw error in codefi-v2.ts file and changed it to just console the error

Checklist

  • I've updated the test suite for new or updated code as appropriate
  • I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate
  • I've highlighted breaking changes using the "BREAKING" category above as appropriate

@sahar-fehri sahar-fehri requested a review from a team as a code owner December 19, 2023 23:12
@sahar-fehri sahar-fehri self-assigned this Dec 19, 2023
@sahar-fehri sahar-fehri mentioned this pull request Dec 19, 2023
3 tasks
@sahar-fehri sahar-fehri force-pushed the fix/fix-fetch-price branch 2 times, most recently from cd65d96 to 81281cc Compare December 19, 2023 23:47
Copy link
Contributor

@mcmire mcmire left a comment

Choose a reason for hiding this comment

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

Had a suggestion for what we could possibly do instead of setting the value to undefined.

Copy link
Contributor

@mcmire mcmire left a comment

Choose a reason for hiding this comment

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

Okay, this looks good!

@sahar-fehri sahar-fehri merged commit 8259340 into main Dec 21, 2023
136 checks passed
@sahar-fehri sahar-fehri deleted the fix/fix-fetch-price branch December 21, 2023 16:39
sethkfman pushed a commit to MetaMask/metamask-mobile that referenced this pull request Jan 29, 2024
## **Description**

Patches MetaMask/core#3687

Fixes issue where importing 1 un-pricable token causes all tokens to
lose prices.

## **Related issues**

Fixes:

## **Manual testing steps**

1. Open a wallet containing erc20 tokens
2. Import a token for which there is no pricing data (e.g. manually
import `0xc7ad37edae28d8cd04bbe4a6ecf072314faed1be`)
3. Switch to a different chain, then switch back
4. Tokens with pricing information should still show fiat $ value

## **Screenshots/Recordings**

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

### **Before**
<img width="765" alt="Screenshot 2024-01-28 at 11 55 56 AM"
src="https://github.com/MetaMask/metamask-mobile/assets/3500406/decada89-2b14-4997-a52a-02ae5091db02">

### **After**

<!-- [screenshots/recordings] -->
<img width="673" alt="Screenshot 2024-01-28 at 12 09 09 PM"
src="https://github.com/MetaMask/metamask-mobile/assets/3500406/4383b746-1b24-41be-9196-f4ed66dbc3c5">


## **Pre-merge author checklist**

- [ ] I’ve followed [MetaMask Coding
Standards](https://github.com/MetaMask/metamask-mobile/blob/main/.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-mobile/blob/main/.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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants