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

feat: Implement market data by Chain ID #4206

Merged
merged 8 commits into from
May 29, 2024
Merged

Conversation

salimtb
Copy link
Contributor

@salimtb salimtb commented Apr 23, 2024

Explanation

This pull request introduces a significant enhancement to our application's functionality by integrating a new feature that presents market data more comprehensively. This includes updates such as daily percentage changes and the fluctuation in amounts for a variety of assets. To accommodate this, we've expanded our state structure to encapsulate a broader array of data points. Below is a snippet illustrating the enriched data now available:

{
    "0x3845badade8e6dff049820680d1f14bd3903a5d0": {
        "id": "the-sandbox",
        "price": 0.00012453786505819405,
        "marketCap": 282288.57927502826,
        ...
        "pricePercentChange1y": -9.725813860523756
    },
    "0xdac17f958d2ee523a2206206994597c13d831ec7": {
        "id": "tether",
        "price": 0.00026894681330590396,
        "marketCap": 30060069.165317584,
        ...
        "pricePercentChange1y": -0.05851492115750438
    }
}

As a result, the TokenRatesController state has been meticulously updated to include this marketData, enriching our application with a more detailed and dynamic representation of asset performance. This enhancement not only broadens the scope of information we provide but also elevates the user experience by offering a granular view of market trends and asset valuations.

References

Changelog

We've introduced a new marketData property to the application's state, providing a comprehensive overview of market-related information for various assets. This enhancement brings a wealth of data directly into our application.
This update significantly enriches the data available within our application, offering users detailed insights into asset performance and market trends. The marketData property is meticulously structured to ensure easy access to a wide array of information, enhancing the overall user experience by providing a granular and dynamic view of the market.

@metamask/package-assets-controllers

  • ADDED: Added marketData to TokenRatesState
  • REMOVED: Removed contractExchangeRates from TokenRatesState
  • REMOVED: Removed contractExchangeRatesByChainId from TokenRatesState

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

@salimtb salimtb requested a review from a team as a code owner April 23, 2024 11:42
@salimtb salimtb force-pushed the add-daily-increase-token branch 4 times, most recently from f8009ff to 69ff1aa Compare April 23, 2024 13:38
@salimtb salimtb requested a review from bergeron April 23, 2024 14:26
@salimtb salimtb force-pushed the add-daily-increase-token branch 3 times, most recently from 295320e to 282e7b2 Compare April 26, 2024 09:12
@salimtb salimtb requested review from bergeron and a team May 2, 2024 08:12
@bergeron
Copy link
Contributor

bergeron commented May 3, 2024

@salimtb I spoke to the shared libraries devs at office hours. Their recommendation was to make a breaking change to move to the new structure, rather than maintain backwards compatibility with the old structure. Because having the same data in multiple places has been a source of bugs.

That would mean removing contractExchangeRates and contractExchangeRatesByChainId. This should simplify the code in core, but does force clients to access price from the new field. It would also require clients to run a migration. But since prices are ephemeral, we should just be able to clear this controllers state and let the next fetch populate the new structure.

I think it'd be good to keep marketData at the top level, just in case we need to introduce other top level fields.

This will probably be difficult to get on mobile, until it has upgraded to a more recent version of assets controllers. We may want to help accelerate this vs trying a tricky patch

@salimtb

This comment was marked as outdated.

@sahar-fehri

This comment was marked as outdated.

@sahar-fehri
Copy link
Contributor

LGTM

sahar-fehri
sahar-fehri previously approved these changes May 16, 2024
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.

Sorry for the delay in reviewing. I made some suggestions below, mostly having to do with naming and updating of JSDocs.

@salimtb salimtb force-pushed the add-daily-increase-token branch 4 times, most recently from 0d7b2f6 to 4dbb8a5 Compare May 17, 2024 15:36
@salimtb salimtb requested review from mcmire and GuiBibeau May 17, 2024 16:38
@bergeron
Copy link
Contributor

Be sure to update PR description/title to reflect the broader market data being added. Since it's more than just the daily % change now.

@salimtb salimtb requested a review from mcmire May 23, 2024 17:19
@salimtb salimtb changed the title feat: Implement Daily Percentage and Amount Change Feature by Chain ID feat: Implement market data by Chain ID May 23, 2024
@salimtb salimtb requested a review from mcmire May 27, 2024 15:12
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.

LGTM, thanks for your changes.

@salimtb salimtb merged commit c1a8b82 into main May 29, 2024
147 checks passed
@salimtb salimtb deleted the add-daily-increase-token branch May 29, 2024 08:52
bergeron added a commit that referenced this pull request Jun 6, 2024
## Explanation

When adding market data for tokens in
#4206, it also changed the token
addresses to be lowercased instead of checksum. This PR moves it back to
the checksum format, since some places on the client expected it.

## References

<!--
Are there any issues that this pull request is tied to? Are there other
links that reviewers should consult to understand these changes better?

For example:

* Fixes #12345
* Related to #67890
-->

## Changelog

<!--
If you're making any consumer-facing changes, list those changes here as
if you were updating a changelog, using the template below as a guide.

(CATEGORY is one of BREAKING, ADDED, CHANGED, DEPRECATED, REMOVED, or
FIXED. For security-related issues, follow the Security Advisory
process.)

Please take care to name the exact pieces of the API you've added or
changed (e.g. types, interfaces, functions, or methods).

If there are any breaking changes, make sure to offer a solution for
consumers to follow once they upgrade to the changes.

Finally, if you're only making changes to development scripts or tests,
you may replace the template below with "None".
-->

### `@metamask/assets-controllers`

- **CHANGED**: Token rates controller uses checksum instead of lowercase
format for token addresses

## 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
salimtb added a commit to MetaMask/metamask-extension that referenced this pull request Jun 21, 2024
<!--
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**

This pull request introduces a new feature to the MetaMask extension
that enhances user experience by displaying the percentage increase or
decrease for each token directly within the UI. This update aims to
provide users with immediate visual feedback on the performance of their
tokens, helping them make more informed decisions based on recent market
trends.

core PR: MetaMask/core#4206

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

## **Related issues**

Fixes:

## **Manual testing steps**

1. Go to the wallet view
2. You should see the percentage of increase/decrease

## **Screenshots/Recordings**

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

### **Before**
<img width="360" alt="Screenshot 2024-04-24 at 15 03 39"
src="https://github.com/MetaMask/metamask-extension/assets/26223211/636bdde4-eab2-40f8-8ed5-935007dd3cda">


### **After**

<img width="356" alt="Screenshot 2024-04-24 at 14 53 25"
src="https://github.com/MetaMask/metamask-extension/assets/26223211/06a22e59-3c5c-4079-a286-79caef75b801">

## **Pre-merge author checklist**

- [x] I’ve followed [MetaMask Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.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/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.
salimtb added a commit to MetaMask/metamask-mobile that referenced this pull request Jul 3, 2024
<!--
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**

This pull request introduces a new feature to the MetaMask mobile that
enhances user experience by displaying the percentage increase or
decrease for each token directly within the UI. This update aims to
provide users with immediate visual feedback on the performance of their
tokens, helping them make more informed decisions based on recent market
trends.

core PR: MetaMask/core#4206
figma:
https://www.figma.com/design/aMYisczaJyEsYl1TYdcPUL/Wallet-Assets?node-id=1620-23897&t=EJSzfKoFvTJ5LuK0-0

## **Related issues**

Fixes: #9635 

## **Manual testing steps**

1. Go to the wallet view
2. You should see the percentage of increase/decrease

## **Screenshots/Recordings**

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

### **Before**

<img width="487" alt="Screenshot 2024-05-15 at 16 54 25"
src="https://github.com/MetaMask/metamask-mobile/assets/26223211/5b165737-4150-4ca7-83a3-3cb667b6f4a6">

### **After**
![Screenshot 2024-05-15 at 16 39
31](https://github.com/MetaMask/metamask-mobile/assets/26223211/3792f1d2-6300-4cad-ad5a-6c49e1697773)


## **Pre-merge author checklist**

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

Successfully merging this pull request may close these issues.

5 participants