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

Draft: Network filter asset list integration #28095

Conversation

gambinish
Copy link
Contributor

@gambinish gambinish commented Oct 25, 2024

Description

Opening this PR in the hopes that it will be helpful in getting us on the same page. My proposal is that we use this as an integration branch for the multi chain asset list view. As patterns emerge, and backend polling updates are integrated, we can begin incrementally introducing this onto develop I think this is a really key feature that touches a lot of different things that are in flight.

This branch includes several things:

  1. Filter updates from here: feat: Token Network Filter UI [Mobile] metamask-mobile#11808 (nearly ready to merge)
  2. Some controller work from @bergeron that begins to introduce some polling here: chore: bump asset controllers to 39 + polling API #28025
  3. Links @salimtb updates to marketData here: fix: should not reset market data after switch network core#4832 to provide marketData across chains

@jonybur You'll notice that we still are not including the native tokens in the lists. I think it would be great if we could find a good way to do this. We are currently only supporting a single native token on develop. I think we should have all the data we need to get this working and integrated into the list and would be a great next step.

You'll also notice that only the selected network icons are rendering. I think that may partially be fixed with this PR: #27724 but I haven't look too much into it.

There's also some weirdness when rendering token list after switching networks, but this might be related to how we are mocking the token balances.

Open in GitHub Codespaces

How to run:

You will need to have core checked out to following commit: 401ba30f
Run yarn && yarn build

In extension, resolve @metamask/assets-controllers to file:../core/packages/assets-controllers and yarn && FILTER_TOKENS_TOGGLE=1 yarn start

⏰ A couple of things to keep in mind:

marketData should now get selected and appended as networks are changed. Note however, that we still are not preloading this market data for all chains when app first loads. This will be introduced with the polling updates. Workaround is to switch between your networks to load the required marketData

You'll notice that erc20 balances are being mocked. This is because we are moving away from useTokenTracker. We will be introducing a new piece of state tokenBalances that will represent the account => chainId => balance relationship for us. Until then, we are mocking this in the selector to unblock.

Related issues

Manual testing steps

  1. Go to asset-list Page, switch between your networks to preload market data (this is a workaround). Then, you should be able to filter by network, and sort. Mocked balances should render.

Screenshots/Recordings

Screen.Recording.2024-10-25.at.2.33.58.PM.mov

Pre-merge author checklist

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.

gambinish and others added 30 commits October 11, 2024 10:03
…om:MetaMask/metamask-extension into feat/mmassets-432_network-filter-extension
…om:MetaMask/metamask-extension into feat/mmassets-432_network-filter-extension
@@ -68,6 +68,7 @@ type AssetPickerModalProps = {
header: JSX.Element | string | null;
isOpen: boolean;
onClose: () => void;
action?: 'send' | 'receive';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's make these constants

@gambinish
Copy link
Contributor Author

Closing this in favor of this: #28386

@gambinish gambinish closed this Nov 12, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Nov 12, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
DO-NOT-MERGE Pull requests that should not be merged portfolio-view Used for PRs and issues related to Q4 2024 portfolio view team-assets
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants