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

fix: reduce size of token list fetch #24316

Merged
merged 31 commits into from
May 16, 2024
Merged

Conversation

bergeron
Copy link
Contributor

@bergeron bergeron commented Apr 30, 2024

Description

When switching to a network for the first time, MM checks whether a token list is available for the network. If not, the 3rd item is shown here:

Screenshot 2024-04-30 at 10 29 29 AM

But we don't need the entire token list, since we're only checking whether a list is available. Adding query parameters to reduce the response size. On mainnet for example, 1.5MB (gzipped) to < 1KB.

Open in GitHub Codespaces

Related issues

Manual testing steps

  1. Open metamask
  2. Visit a chain that has token lists, like linea
  3. The popup should not warn about missing tokens
  4. Visit a chain that does not have token lists, like filecoin
  5. The popup should warn about missing tokens

Screenshots/Recordings

Before

After

Pre-merge author checklist

  • I’ve followed MetaMask 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.

@bergeron bergeron requested a review from a team as a code owner April 30, 2024 18:13
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.

@bergeron bergeron added the needs-assets-ux-review A shared label between the Assets and UX team to flag PRs ready for consolidated team review. label Apr 30, 2024
hurricanenara
hurricanenara previously approved these changes Apr 30, 2024
@bergeron bergeron marked this pull request as draft April 30, 2024 20:13
@bergeron bergeron marked this pull request as ready for review April 30, 2024 20:20
Copy link

codecov bot commented May 6, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 67.34%. Comparing base (44e29aa) to head (acc46e3).

Additional details and impacted files
@@           Coverage Diff            @@
##           develop   #24316   +/-   ##
========================================
  Coverage    67.34%   67.34%           
========================================
  Files         1282     1282           
  Lines        50065    50065           
  Branches     12986    12986           
========================================
  Hits         33716    33716           
  Misses       16349    16349           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@bergeron
Copy link
Contributor Author

bergeron commented May 6, 2024

Hmmm the e2e tests failing in CI are passing locally

@bergeron
Copy link
Contributor Author

bergeron commented May 15, 2024

I'm confused by the e2e test failures. They keep passing locally in both browsers, and should be unrelated to the changes.

  • Success on testcase: 'Wallet Created Events @no-mmi are not sent when onboarding user who chooses to opt out metrics'
  • Success on testcase: 'Connections page should disconnect when click on Disconnect button in connections page'
  • Success on testcase: 'Permissions Page should redirect users to connections page when users click on connected permission'
  • ...etc

But not in CI for some reason?

@metamaskbot
Copy link
Collaborator

Builds ready [da392d5]
Page Load Metrics (1229 ± 614 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint711771032914
domContentLoaded106021157
load58291012291278614
domInteractive106021157
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 46 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

@bergeron bergeron merged commit b856338 into develop May 16, 2024
71 of 72 checks passed
@bergeron bergeron deleted the brian/token-list-exclusions branch May 16, 2024 15:58
@github-actions github-actions bot locked and limited conversation to collaborators May 16, 2024
@metamaskbot metamaskbot added the release-11.18.0 Issue or pull request that will be included in release 11.18.0 label May 16, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
needs-assets-ux-review A shared label between the Assets and UX team to flag PRs ready for consolidated team review. release-11.18.0 Issue or pull request that will be included in release 11.18.0 team-assets
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants