core
core copied to clipboard
feat: Implement Daily Percentage and Amount Change Feature by Chain ID
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 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 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
andcontractExchangeRatesByChainId
. 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
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
LGTM
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.