feat: show all tokens for all chains in the token list #27785

jonybur · 2024-10-11T00:45:40Z

Description

Related issues

Fixes:

Manual testing steps

Go to this page...

Screenshots/Recordings

Before

After

Pre-merge author checklist

I've followed MetaMask Contributor Docs and MetaMask Extension Coding Standards.
I've completed the PR template to the best of my ability
I’ve included tests if applicable
I’ve documented my code using JSDoc format if applicable
I’ve applied the right labels on the PR (see labeling guidelines). 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.

github-actions · 2024-10-11T00:45:52Z

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

darkwing · 2024-10-11T17:41:34Z

Thank you for posting this WIP Jony. One approach that may make this easier is starting with storybook and adding an AssetList story with mock data across all chains and ensuring it looks good there. That will likely make the rest easier.

jonybur · 2024-10-14T12:27:37Z

@darkwing That's a good tip, thanks! Renderisation seems to be working well, I'm figuring out how to fetch the correct values for each chain's balances at the moment. The views seem to be fine at the present moment.

gambinish · 2024-10-17T16:49:03Z

Hey @jonybur nice work getting this started! I requested myself as reviewer here since we'll be working in the same area. I'll request you on my PRs shortly. I know this is still pretty early on, but a couple things I noticed to put on your radar. You are probably already investigating these things!

We'll want to make sure that we include the native tokens in the Sort by function to avoid a regression there:

Screen.Recording.2024-10-17.at.6.31.26.AM.mov

Does the TokenBalanceController need to get updated with balances across networks, in order to display the balances correctly for each token, on each network? From what I can tell, I think the controller currently accepts a single chainId in it's constructor (the selectedNetwork). I think that ideally, it should accept an array of chainId so that we can have a mapping of chainId => tokenBalances for each chain. If I am missing something here, please lmk!

Additionally, we'll also need to consider how we're handling the native token balances for each chain, since that's being handled a bit differently than the erc20 tokens. Currently when I'm on Ethereum mainnet, it's calculating the POL balance at the exchange rate of ETH on mainnet, rather than POL on Polygon (and vice versa). I think this also happens at the CurrencyRateController

Screen.Recording.2024-10-17.at.6.30.31.AM.mov

We should also take into consideration the showZeroBalances flag if you haven't already 👍

I think the main blocker with this task to get fully functional, is that marketData is only provided for the selected network. Since marketData is used to get the conversion rate, I don't see how we can update the balances across networks as we'd need to without this backend change.

Has something already been updated to handle this?

bergeron · 2024-10-17T17:06:40Z

ui/selectors/multichain.ts

+  return `eip155:${chainIdDecimal}`;
+}
+
+export function getChains(state: MultichainState): MultichainNetwork[] {


Whats the reason for the changes in this file? I understood it was for non-evm support, so wondering why it needs to change vs just having a selector in selectors.js

I moved this to selectors.js

salimtb · 2024-10-17T18:04:29Z

Hi @jonybur ,

If I understand correctly, the process in this PR works like this:

You retrieve a list of all tokens along with their balances using accountsByChainId, which provides a list of token addresses and balances, organized by blockchain network (so this results in a very large amount of data).
Then, you filter the data to keep only the tokens that have a balance. This way, you get the list of "detected tokens" across all chains by looping through them (link to code here).
After that, you display the converted values.

This approach has a significant impact on performance because it handles a large dataset. To improve this, I suggest simplifying it by making the marketData multichain in the assetsController. The marketData already includes the detected tokens, tokens imported by the user, and all market-related information, but right now, it only handles data for the selected network.
you can find below the structure of market data:

{
  "0x1": {
    // Native token here ******
    "0x0000000000000000000000000000000000000000": {
      "tokenAddress": "0x0000000000000000000000000000000000000000",
      "currency": "ETH",
      "id": "ethereum",
      "price": 1.0000726178421582,
      "marketCap": 120374263.85039304,
      "allTimeHigh": 1.872695124126123,
      "allTimeLow": 0.00016621452365167183,
      "totalVolume": 5308887.015842034,
      "high1d": 1.0147677692591692,
      "low1d": 0.9936194962298293,
      "circulatingSupply": 120387362.762955,
      "dilutedMarketCap": 120374263.85039304,
      "marketCapPercentChange1d": -0.64964,
      "priceChange1d": -16.22303390018078,
      "pricePercentChange1h": -0.2999715552409102,
      "pricePercentChange1d": -0.618879494740698,
      "pricePercentChange7d": 9.728879673482494,
      "pricePercentChange14d": 12.175196996309179,
      "pricePercentChange30d": 10.175859028915708,
      "pricePercentChange200d": -28.044745794161667,
      "pricePercentChange1y": 64.93607348906181
    },
    // Detected/imported token here ******
    "0xA0b86991c6218b36c1d19D4a2e9Eb0cE3606eB48": {
      "tokenAddress": "0xA0b86991c6218b36c1d19D4a2e9Eb0cE3606eB48",
      "currency": "ETH",
      "id": "usd-coin",
      "price": 0.00038370084519404344,
      "marketCap": 13331889.134607311,
      "allTimeHigh": 0.00044914647747917576,
      "allTimeLow": 0.00033691628933347535,
      "totalVolume": 1574846.8115542484,
      "high1d": 0.0003850375358218917,
      "low1d": 0.00038302674159194645,
      "circulatingSupply": 34751452795.3628,
      "dilutedMarketCap": 13331829.76013019,
      "marketCapPercentChange1d": -0.06737,
      "priceChange1d": -0.000655467524627418,
      "pricePercentChange1h": -0.05174268708087084,
      "pricePercentChange1d": -0.06553537070828762,
      "pricePercentChange7d": 0.02156004626949369,
      "pricePercentChange14d": -0.03760749615660747,
      "pricePercentChange30d": 0.01603305301773797,
      "pricePercentChange200d": 0.05198102213414082,
      "pricePercentChange1y": -0.012088418136522984
    },
  }
}

In the meantime, I recommend using mock data until we can handle this properly on the backend (specifically in the assetsController).
we will also need to make allDetectedTokens multi-chain if this is not the case in order to retrieve information that is not linked to the market, such as name, symbol, etc.

bergeron · 2024-10-18T16:25:07Z

You retrieve a list of all tokens along with their balances using accountsByChainId, which provides a list of token addresses and balances

I believe accountsByChainId only stores native balances, so it would not deal with erc20 tokens. It's from the AccountTrackerController and is a mapping from chain id -> account address -> native balance

gambinish · 2024-10-18T16:59:10Z

ui/components/app/assets/token-list/token-list.tsx

-    return sortAssets(
-      [nativeTokenWithBalance, ...tokensWithBalances],
-      tokenSortConfig,
-    );


Why are we removing native tokens from the sorting functionality here? Was there a product request to change this behavior?

My understanding is that we will need to include the native tokens with their balances, along with all erc20's so that they can be sorted in the correct order by declining fiat balance (and potentially future sort criteria)

This is likely where our main integration point will be for filterAssets as well, and will probably get added before the sorting functionality higher in the tree.

Per my other comment, I think that we should move this up a level, so that we can do this data manipulation once before render, while still including nativeTokens in its calculation to maintain our sort functionality. Otherwise, we will end up with separate arbitrarily ordered sorted lists for each chain, with the native tokens pinned at the top.

This is a wip, I removed it to simplify my testing. I'm looking at hardcoding marketData now

gambinish · 2024-10-18T17:10:23Z

ui/components/multichain/asset-picker-amount/asset-picker-modal/AssetList.tsx

+type TokenListItemWithBalanceProps = {
+  chain: any; // Replace 'any' with the actual type of 'chain' if available
+  token: AssetWithDisplayData<ERC20Asset> | AssetWithDisplayData<NativeAsset>;
+  primaryCurrency: string;
+  secondaryCurrency: string;
+  primaryNumberOfDecimals: number;
+  secondaryNumberOfDecimals: number;
+  nativeCurrency: string;
+  isDisabled: boolean;
+};
+
+function TokenListItemWithBalance({
+  chain,
+  token,
+  primaryCurrency,
+  secondaryCurrency,
+  primaryNumberOfDecimals,
+  secondaryNumberOfDecimals,
+  nativeCurrency,
+  isDisabled,
+}: TokenListItemWithBalanceProps) {
+  const { chainId } = chain.network;
+  const balancesByChainId = useSelector(
+    getSelectedAccountCachedBalancesByChainId,
+  );
+  const balanceValue = balancesByChainId?.[chainId];
+
+  console.log({ balancesByChainId });
+
+  const [, primaryCurrencyProperties] = useCurrencyDisplay(balanceValue, {
+    numberOfDecimals: primaryNumberOfDecimals,
+    currency: primaryCurrency,
+  });
+  const [secondaryCurrencyDisplay] = useCurrencyDisplay(balanceValue, {
+    numberOfDecimals: secondaryNumberOfDecimals,
+    currency: secondaryCurrency,
+    hideLabel: true,
+  });
+
+  return (
+    <TokenListItem
+      title={nativeCurrency}
+      chain={chain}
+      primary={primaryCurrencyProperties.value}
+      tokenSymbol={primaryCurrency}
+      secondary={secondaryCurrencyDisplay}
+      tokenImage={token.image}
+      isOriginalTokenSymbol
+      tooltipText={isDisabled ? 'swapTokenNotAvailable' : undefined}
+    />
+  );
+}


Is there a reason why these changes can't be incorporated into the existing TokenListItem component? This seems to essentially wrap that component.

gambinish · 2024-10-18T17:36:48Z

ui/components/app/assets/token-list/token-list.tsx

+      <>{nativeToken}</>
+      {chains.map((chain) => (
+        <TokenListForChain
+          key={chain.network.chainId}
+          chain={chain}
+          onTokenClick={onTokenClick}
+          tokenSortConfig={tokenSortConfig}
+          selectedAccount={selectedAccount}
+          conversionRate={conversionRate}
+          contractExchangeRates={contractExchangeRates}
+          shouldHideZeroBalanceTokens={shouldHideZeroBalanceTokens}
+        />
+      ))}


Rather than mapping over chains, rendering a separate list for each chain, and then pinning the nativeTokens to the top, I would suggest doing some data manipulation first. I think it would make sense to move the sortedAssets useMemo function up to this level, before the return.

This will allow us to:

Make this into a flat array first, so that we don't have a separate TokenList for each chain. We should instead have a single TokenList, which is correctly sorted and filtered for all tokens for all chains

Allow us to include the native tokens for each chain in the sorted/filtered array, rather than pinning them as a fragment to the top of the list as we are here.

gambinish · 2024-10-18T17:43:32Z

ui/components/app/assets/token-list/token-list.tsx

-        if (tokenData?.isNative) {
-          // we need cloneElement so that we can pass the unique key
-          return React.cloneElement(nativeToken as React.ReactElement, {
-            key: `${tokenData.symbol}-${tokenData.address}`,
-          });
-        }


I think that if we include the nativeTokens as an array of react nodes in TokenList, rather than a fragment as we are now, we can retain this logic here to allow React to target them independently

The idea: when all tokens (including native tokens) get memoized into a sorted and filtered array together, each native token can be identified by the isNative prop. React can then render them as a NativeToken component, while still retaining it's sorted/filtered order within the list.

gambinish · 2024-10-18T17:49:44Z

ui/components/app/assets/asset-list/asset-list.tsx

+        nativeToken={
+          <>
+            {chains?.map((chain) => (
+              <NativeToken chain={chain} onClickAsset={onClickAsset} />
+            ))}
+          </>
+        }


Let's rename this prop to nativeTokens

Rather than wrapping them in a fragment, let's just pass them in as an array of NativeToken elements

Suggested change

nativeToken={

<>

{chains?.map((chain) => (

<NativeToken chain={chain} onClickAsset={onClickAsset} />

))}

</>

}

nativeTokens={chains?.map((chain) => (

<NativeToken chain={chain} onClickAsset={onClickAsset} />

))}

This will allow us to render each of them independently farther down the component tree, since they won't be bundled together in a single fragment. They will need to appear in different orders in the list depending on sort and filter criteria

Also, if you see an alternate approach to solve for this problem, I'm open to discussing that as well 👍

gambinish

Hey Jony, went over this again and I left a few suggestions mainly related to how we can avoid regressions to our sorting functionality. This sortedAssets useMemo looks like it will be our main integration point for our features, and I plan to incorporate filteredAssets there.

If we can do the data manipulation to covert all tokens for all chains into a flat array within/before we hit that useMemo hook, I think it should be a pretty straightforward integration.

I'd really like to see that hook get used higher up in the tree, so that it only gets called once, while keeping the nativeTokenBalances in its calculation to maintain it's behavior.

gambinish · 2024-10-18T18:23:58Z

As an aside, I synced with @bergeron and @salimtb and I think that there are indeed some changes that we should make in the TokenRatesController to provide you with marketData for all chains (rather than only the selected chain). This should unblock and allow you to make the balance calculations for each token across chains as you'd need to.

## **Description** Adds Token network filter controls. Note that this is not fully functional, and is currently blocked by two PRs before it can be fully integrated: 1. #27785 2. MetaMask/core#4832 In the meantime, this PR is set behind a feature flag `FILTER_TOKENS_TOGGLE` and can be run as follows: `FILTER_TOKENS_TOGGLE=1 yarn webpack --watch` Alternatively: `FILTER_TOKENS_TOGGLE=1 yarn start` [![Open in GitHub Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/27884?quickstart=1) Included in this PR: 1. Adds new `tokenNetworkFilter` preference to PreferencesController to manage which chains should be considered when filtering tokens 2. Adds an action to update this preference by All Networks (no filters) and Current Network `{ [chainId]: true) }` this is meant to be flexible enough to support multiple chains in the future. 3. Adds `filterAssets` function in a similar style to `sortAssets` it should be configuration based, and should be extensible enough to support filtering assets by deeply nested values (NFT traits), and to also support complex filter types (like price ranges). 4. Dropdown should show the balance for the selected network Not included in this PR: 1. Aggregated balance across chains. Blocked by MetaMask/core#4832 and currently hardcoded to $1000 2. Token lists will not be filtered in this PR. Blocked by #27785 ## **Related issues** https://github.com/orgs/MetaMask/projects/85/views/35?pane=issue&itemId=82217837 https://consensyssoftware.atlassian.net/browse/MMASSETS-430 ## **Manual testing steps** Token Filter selection should persist through refresh Current chain balance should reflect the balance of the current chain Should visibly match designs: https://www.figma.com/design/aMYisczaJyEsYl1TYdcPUL/Portfolio-View?node-id=5750-47217&node-type=canvas&t=EjOUPnqy7tWZE6sV-0 ## **Screenshots/Recordings** https://github.com/user-attachments/assets/4b132e47-0dcf-4e9c-8755-ccb2be1d5dc1 ## **Pre-merge author checklist** - [x] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Extension 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. --------- Co-authored-by: Salim TOUBAL <salim.toubal@outlook.com>

gambinish · 2024-11-12T22:25:24Z

Closing this in favor of #28386

Wip

e483c81

jonybur requested review from bergeron and sahar-fehri October 11, 2024 00:48

jonybur added 3 commits October 16, 2024 14:09

wip

6ed6cc0

wip

42d3c44

wip

5eb3ecb

darkwing added the portfolio-view Used for PRs and issues related to Q4 2024 portfolio view label Oct 17, 2024

wip

0579090

gambinish self-requested a review October 17, 2024 16:21

bergeron reviewed Oct 17, 2024

View reviewed changes

gambinish reviewed Oct 18, 2024

View reviewed changes

gambinish requested changes Oct 18, 2024

View reviewed changes

Revisions

588c547

gambinish mentioned this pull request Oct 22, 2024

feat: Token Network Filter UI [Extension] #27884

Merged

7 tasks

gambinish closed this Nov 12, 2024

github-actions bot locked and limited conversation to collaborators Nov 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: show all tokens for all chains in the token list #27785

feat: show all tokens for all chains in the token list #27785

jonybur commented Oct 11, 2024 •

edited by github-actions bot

Loading

github-actions bot commented Oct 11, 2024

darkwing commented Oct 11, 2024

jonybur commented Oct 14, 2024

gambinish commented Oct 17, 2024 •

edited

Loading

bergeron Oct 17, 2024

jonybur Oct 22, 2024

jonybur Oct 22, 2024

salimtb commented Oct 17, 2024 •

edited

Loading

bergeron commented Oct 18, 2024 •

edited

Loading

gambinish Oct 18, 2024 •

edited

Loading

jonybur Oct 21, 2024

gambinish Oct 18, 2024 •

edited

Loading

gambinish Oct 18, 2024 •

edited

Loading

gambinish Oct 18, 2024 •

edited

Loading

gambinish Oct 18, 2024 •

edited

Loading

gambinish Oct 18, 2024

gambinish left a comment

gambinish commented Oct 18, 2024 •

edited

Loading

gambinish commented Nov 12, 2024

feat: show all tokens for all chains in the token list #27785

feat: show all tokens for all chains in the token list #27785

Conversation

jonybur commented Oct 11, 2024 • edited by github-actions bot Loading

Description

Related issues

Manual testing steps

Screenshots/Recordings

Before

After

Pre-merge author checklist

Pre-merge reviewer checklist

github-actions bot commented Oct 11, 2024

darkwing commented Oct 11, 2024

jonybur commented Oct 14, 2024

gambinish commented Oct 17, 2024 • edited Loading

bergeron Oct 17, 2024

Choose a reason for hiding this comment

jonybur Oct 22, 2024

Choose a reason for hiding this comment

jonybur Oct 22, 2024

Choose a reason for hiding this comment

salimtb commented Oct 17, 2024 • edited Loading

bergeron commented Oct 18, 2024 • edited Loading

gambinish Oct 18, 2024 • edited Loading

Choose a reason for hiding this comment

jonybur Oct 21, 2024

Choose a reason for hiding this comment

gambinish Oct 18, 2024 • edited Loading

Choose a reason for hiding this comment

gambinish Oct 18, 2024 • edited Loading

Choose a reason for hiding this comment

gambinish Oct 18, 2024 • edited Loading

Choose a reason for hiding this comment

gambinish Oct 18, 2024 • edited Loading

Choose a reason for hiding this comment

gambinish Oct 18, 2024

Choose a reason for hiding this comment

gambinish left a comment

Choose a reason for hiding this comment

gambinish commented Oct 18, 2024 • edited Loading

gambinish commented Nov 12, 2024

jonybur commented Oct 11, 2024 •

edited by github-actions bot

Loading

gambinish commented Oct 17, 2024 •

edited

Loading

salimtb commented Oct 17, 2024 •

edited

Loading

bergeron commented Oct 18, 2024 •

edited

Loading

gambinish Oct 18, 2024 •

edited

Loading

gambinish Oct 18, 2024 •

edited

Loading

gambinish Oct 18, 2024 •

edited

Loading

gambinish Oct 18, 2024 •

edited

Loading

gambinish Oct 18, 2024 •

edited

Loading

gambinish commented Oct 18, 2024 •

edited

Loading