core icon indicating copy to clipboard operation
core copied to clipboard

feat: Implement Daily Percentage and Amount Change Feature by Chain ID

Open salimtb opened this issue 10 months ago • 4 comments

Explanation

This pull request introduces a new feature that enhances our application's capability to display daily percentage and amount changes for various assets. By extending our state structure, we now include two key pieces of data: priceChange1d and contractPercentChange1d, both indexed by chainId. This addition allows users to gain insights into the daily fluctuations of asset values across different blockchain networks, directly addressing the need for more granular and actionable financial information.

so, the TokenRatesController state has been updated to incorporate priceChange1d and contractPercentChange1d. These fields represent the daily amount change and daily percentage change, respectively, and are organized by chainId to facilitate easy access to chain-specific data.

References

Changelog

Daily Percentage and Amount Change Display by Chain ID: Introduced a feature that allows users to view the daily percentage increase/decrease (contractPercentChange1d) and the daily amount increase/decrease (priceChange1d) for assets, organized by chainId. This enhancement aims to provide users with actionable insights into the short-term performance of assets across different blockchain networks.

@metamask/package-assets-controllers

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

Checklist

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

salimtb avatar Apr 23 '24 11:04 salimtb

@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

bergeron avatar May 03 '24 18:05 bergeron

@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

hey @bergeron , this is a much cleaner solution indeed, but requires a lot more changes on the UI extension part. for mobile, we can also apply the changes to v18 of the assets controller, which is still feasible because we have a version of the controller for mobile. i'll also talk to tomas and try to update the asset controller in mobile ( progressively ). I'll make the changes on core + extension for now, and keep you posted. for mobile i'll discuss with the team to find the best solution

salimtb avatar May 06 '24 08:05 salimtb

Hey @salimtb 👋 should the changelog explicitly indicate the removal of contractExchangeRates, contractExchangeRatesByChainId and adding the marketData instead in the default state? Maybe: Removed : Removed contractExchangeRates from TokenRatesState Removed : Removed contractExchangeRatesByChainId from TokenRatesState Added : Added marketData to TokenRatesState

sahar-fehri avatar May 16 '24 12:05 sahar-fehri

LGTM

sahar-fehri avatar May 16 '24 13:05 sahar-fehri

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.

bergeron avatar May 17 '24 20:05 bergeron