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

Draft
wants to merge 16 commits into
base: feat/mmassets-432_network-filter-extension
Choose a base branch
from

Conversation

gambinish
Copy link
Contributor

@gambinish gambinish commented Oct 25, 2024

Description

Opening this PR for some discussion on implementation strategies for network filter + cross chain asset list integrations in the hopes that it will be helpful in getting us on the same page. This PoC incorporates the marketData updates on core, along with some assets controller updates that integrate polling.

  1. This PR includes filter updates from here: feat: Token Network Filter UI [Mobile] metamask-mobile#11808 (nearly ready to merge)
  2. Additionally, it includes some controller work from Brian that is in PR here: chore: bump asset controllers to 39 + polling API #28025
  3. Links Salim's updates to marketData here: fix: should not reset market data after switch network core#4832

Open in GitHub Codespaces

To get this running, 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

This should enable marketData to get fetched and appended as networks are changed. Note however, that we still are not preloading this market data for all chains when app first loads. I think that we should, unless there is a strong reason not to @salimtb

Additionally, you'll notice that balances are only getting fetched for the selected chain. I believe this is something to do with our TokenTracker implementation, but I need to look into this further.

Related issues

TBD

Manual testing steps

  1. Go to asset-list Page, reload and view console log of data being returned

Screenshots/Recordings

Before

After

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 gambinish added the DO-NOT-MERGE Pull requests that should not be merged label Oct 25, 2024
Copy link
Contributor

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.

Copy link

New and removed dependencies detected. Learn more about Socket for GitHub ↗︎

Package New capabilities Transitives Size Publisher

🚮 Removed packages: npm/@metamask/controller-utils@11.3.0

View full report↗︎

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DO-NOT-MERGE Pull requests that should not be merged team-assets
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants