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

feat: Upgrade assets controllers to 43 with multichain polling for token lists + detection #28447

Merged
merged 27 commits into from
Nov 15, 2024

Conversation

bergeron
Copy link
Contributor

@bergeron bergeron commented Nov 13, 2024

Description

This upgrades assets controllers to version 43. It allows us to poll for token lists and token detection across chains.

Open in GitHub Codespaces

Related issues

Fixes:

Manual testing steps

  1. Onboard to metamask
  2. Verify your erc20 tokens are detected
  3. Click a token
  4. Verify data on token details page
  5. Switch networks
  6. Repeat steps 2-4

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.

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
Copy link
Contributor Author

@metamaskbot update-policies

@metamaskbot
Copy link
Collaborator

Policies updated.
👀 Please review the diff for suspicious new powers.

🧠 Learn how: https://lavamoat.github.io/guides/policy-diff/#what-to-look-for-when-reviewing-a-policy-diff

@@ -7205,14 +7222,14 @@ export default class MetamaskController extends EventEmitter {

this.tokenListController.updatePreventPollingOnNetworkRestart(!newEnabled);
Copy link
Contributor Author

@bergeron bergeron Nov 13, 2024

Choose a reason for hiding this comment

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

I've never understood the purpose of preventPollingOnNetworkRestart, so I don't know whether it still makes sense. When it's set to true, all it seems to do is reset the state when switching chains. Keeping it around for now.

Copy link
Contributor

Choose a reason for hiding this comment

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

I saw this same thing and had similar thoughts. Sounds good

Copy link

socket-security bot commented Nov 13, 2024

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

Package New capabilities Transitives Size Publisher
npm/@metamask/controller-utils@11.4.3 network 0 264 kB metamaskbot

View full report↗︎

@bergeron
Copy link
Contributor Author

@metamaskbot update-policies

@metamaskbot
Copy link
Collaborator

Policy update failed. You can review the logs or retry the policy update here

@bergeron
Copy link
Contributor Author

@metamaskbot update-policies

@metamaskbot
Copy link
Collaborator

Policies updated.
👀 Please review the diff for suspicious new powers.

🧠 Learn how: https://lavamoat.github.io/guides/policy-diff/#what-to-look-for-when-reviewing-a-policy-diff

@bergeron
Copy link
Contributor Author

fixing e2e

Comment on lines +72 to +74
await dispatch(
toggleExternalServices(externalServicesOnboardingToggleState),
);
Copy link
Contributor Author

@bergeron bergeron Nov 14, 2024

Choose a reason for hiding this comment

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

There was a race condition here.

When you disable basic functionality during onboarding, we don't actually flip the setting until this final page. But the toggle was not awaited, so there was a brief period where:

  • completedOnboarding == true
  • useExternalServices == true

And then it would quickly flip to false. But with this change useExternalServices will be false immediately upon leaving onboarding.

Accompanying changes in actions.ts

@metamaskbot
Copy link
Collaborator

Builds ready [1411c2a]
Page Load Metrics (1939 ± 77 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint33822691841369177
domContentLoaded16932324191215776
load17062330193916077
domInteractive248842168
backgroundConnect887312211
firstReactRender502121014622
getState641921073517
initialActions01000
loadScripts12201750140412862
setupStore686162311
uiStartup199930402321262126
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: -75 Bytes (-0.00%)
  • ui: 2.71 KiB (0.04%)
  • common: 3.42 KiB (0.04%)

diff --git a/dist/TokenDetectionController.cjs b/dist/TokenDetectionController.cjs
index ab23c95d667357db365f925c4c4acce4736797f8..8fd5efde7a3c24080f8a43f79d10300e8c271245 100644
--- a/dist/TokenDetectionController.cjs
+++ b/dist/TokenDetectionController.cjs
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bergeron bergeron marked this pull request as ready for review November 14, 2024 23:25
@bergeron bergeron requested review from a team as code owners November 14, 2024 23:25
@metamaskbot
Copy link
Collaborator

Builds ready [979a3af]
Page Load Metrics (2213 ± 91 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint19722615223118890
domContentLoaded19442533218017885
load19612599221318991
domInteractive18100502010
backgroundConnect785292010
firstReactRender563001348943
getState512091193818
initialActions01000
loadScripts14051938164415273
setupStore686202311
uiStartup225830322658238114
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: -75 Bytes (-0.00%)
  • ui: 2.8 KiB (0.04%)
  • common: 3.6 KiB (0.04%)

@bergeron bergeron added this pull request to the merge queue Nov 15, 2024
Merged via the queue into develop with commit a597a8e Nov 15, 2024
86 checks passed
@bergeron bergeron deleted the brian/upgrade-assets-controllers-43 branch November 15, 2024 18:01
@github-actions github-actions bot locked and limited conversation to collaborators Nov 15, 2024
@metamaskbot metamaskbot added the release-12.8.1 Issue or pull request that will be included in release 12.8.1 label Nov 15, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
release-12.8.1 Issue or pull request that will be included in release 12.8.1 team-assets
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants