feat: Implement market data by Chain ID

[salimtb]

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

[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

[sahar-fehri]

LGTM

[mcmire]

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

[bergeron]

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.

[mcmire]

Sorry, but I'm still not sure why we're doing this.

The fetchTokenPrices method always passes a zero address as the first argument. Therefore, it seems that even if tokenAddresses is empty, tokenPricesByTokenAddress will always contain a property which is the zero address. Therefore, it seems that we already have the functionality we need, and we don't need to do this explicitly. I would expect that this would be enough:

const tokenPricesByTokenAddress = await reduceInBatchesSerially<
  Hex,
  Awaited<ReturnType<AbstractTokenPricesService['fetchTokenPrices']>>
>({
  values: [...tokenAddresses].sort(),
  batchSize: TOKEN_PRICES_BATCH_SIZE,
  eachBatch: async (allTokenPricesByTokenAddress, batch) => {
    const tokenPricesByTokenAddressForBatch =
      await this.#tokenPricesService.fetchTokenPrices({
        tokenAddresses: batch,
        chainId,
        currency: nativeCurrency,
      });
    return {
      ...allTokenPricesByTokenAddress,
      ...tokenPricesByTokenAddressForBatch,
    };
  },
  initialResult: {},
});

return Object.entries(tokenPricesByTokenAddress).reduce(
  (obj, [tokenAddress, token]) => {
    return {
      ...obj,
      [tokenAddress.toLowerCase()]: token,
    };
  },
  {},
);

Is this true?

One other thing that's interesting to me is that in this second call, we are passing this.config.chainId. If we do truly want to keep this code, I feel that we should use chainId instead. this.config.chainId stores the globally selected chain ID, but chainId may be a different chain than the globally selected version (side note: this controller uses two different ways of polling, and eventually updateExchangeRates and this.config.chainId will go away; this is because dapps may want to connect to a different chain than the user has selected in MetaMask).

[salimtb]

hey @mcmire ,
sorry, maybe my explanation wasn't clear,
to elucidate why the fetch operation for the native token is maintained even when tokenAddresses is an empty array, let's delve into the specifics of your proposal and clarify why it falls short in scenarios where a user hasn't imported any tokens and only holds the native token.

The reduceInBatchesSerially function is designed to process a collection of values in batches, determined by the values parameter. This batching mechanism is crucial for handling large datasets efficiently by breaking them down into manageable chunks.

In the scenario at hand, we encounter a situation where tokenAddresses is an empty array. This directly impacts the reduceInBatchesSerially function because it relies on the values array to partition the data into batches. With tokenAddresses being empty, the function identifies zero batches to process. Consequently, it bypasses the loop entirely ( check the function reduceInBatchesSerially ), leading to the immediate return of the initialResult, which, in this context, is an empty object. As a result, our intended API call to fetch token prices is never executed.

This behavior underscores the necessity of retaining the fetch operation for the native token outside of the reduceInBatchesSerially function when tokenAddresses.length === 0. By doing so, we ensure that essential market data for the native token is still retrieved and processed, even in the absence of any imported tokens. This approach guarantees that users solely holding the native token are not overlooked and that their token data is accurately reflected in the application.

In summary, when the list of token addresses is empty, our function reduceInBatchesSerially skips processing and returns an empty result. This is why we must ensure that data for the native token is fetched separately, guaranteeing that users holding only the native token still receive relevant market information.

[salimtb]

@mcmire fixed for the chainId

[mcmire]

Ah, gotcha. Thanks for the explanation. It sounds like a better fix would be to move the batching code into the service object. This way we can ensure that token addresses contains the zero address first before attempting to iterate over them. This is something that I had meant to do when I introduced that code but didn't get around to it.

I guess we don't have to do that here, though, we can do that in another PR.

[mcmire]

LGTM, thanks for your changes.