Skip to content

Fix tokensControllers instantiation#22372

Merged
danjm merged 2 commits intodevelopfrom
asset-controller-fixes
Dec 22, 2023
Merged

Fix tokensControllers instantiation#22372
danjm merged 2 commits intodevelopfrom
asset-controller-fixes

Conversation

@danjm
Copy link
Copy Markdown
Contributor

@danjm danjm commented Dec 21, 2023

Description

This PR addresses some issues that were identified by @Gudahtt when reviewing the diff for v11.7.3 (#22354), although the comments were put on the v11.7.2 PR, because what is currently 11.7.3 was once 11.7.2: #22336

Screenshot from 2023-12-21 08-34-37
Screenshot from 2023-12-21 08-34-23
Screenshot from 2023-12-21 08-33-58

Manual testing steps

  1. The token testing scenarios should all pass https://github.com/MetaMask/metamask-extension/tree/develop/test/scenarios/4.%20tokens
  2. Fiat values for erc20 tokens should be correctly displayed
  3. For the scenarios in 1 and 2 related to autodetection and token display, the tests should also pass after switching networks and switching networks multiple times in short succession

Pre-merge author checklist

  • I’ve followed MetaMask Coding Standards.
  • I've clearly explained what problem this PR is solving and how it is solved.
  • I've linked related issues
  • I've included manual testing steps
  • I've included screenshots/recordings if applicable
  • 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.
  • I’ve properly set the pull request status:
    • In case it's not yet "ready for review", I've set it to "draft".
    • In case it's "ready for review", I've changed it from "draft" to "non-draft".

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.

@danjm danjm requested review from a team as code owners December 21, 2023 12:08
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Dec 21, 2023

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.

@danjm danjm mentioned this pull request Dec 21, 2023
Merged
@danjm danjm added the team-extension-platform Extension Platform team label Dec 21, 2023
@metamaskbot metamaskbot added the INVALID-PR-TEMPLATE PR's body doesn't match template label Dec 21, 2023
Gudahtt
Gudahtt previously approved these changes Dec 21, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

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

LGTM!

danjm
danjm commented Dec 21, 2023
View reviewed changes
app/scripts/metamask-controller.js
),
onNetworkDidChange: networkControllerMessenger.subscribe.bind(
networkControllerMessenger,
'NetworkController:stateChange',
Copy link
Copy Markdown
Contributor Author

@danjm danjm Dec 21, 2023

Choose a reason for hiding this comment

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

@Gudahtt There is a problem with this change. It requires MetaMask/core#3598, which has not been released yet.

Will leaving this as 'NetworkController:stateChange' be buggy? Or does it just mean that the listener will be called more than it needs to be?

Copy link
Copy Markdown
Contributor Author

@danjm danjm Dec 21, 2023

Choose a reason for hiding this comment

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

Actually, I addressed this here c65def9

Copy link
Copy Markdown
Member

@Gudahtt Gudahtt Dec 21, 2023

Choose a reason for hiding this comment

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

Good catch. We would have released the network controller alongside this had we noticed that.

@metamaskbot
Copy link
Copy Markdown
Collaborator

metamaskbot commented Dec 21, 2023

Builds ready [c65def9]
Page Load Metrics (1225 ± 129 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint973181907636
domContentLoaded10194857838
load81417561225268129
domInteractive10194857838
Bundle size diffs [🚀 Bundle size reduced!]
  • background: -44 Bytes (-0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

@codecov
Copy link
Copy Markdown

codecov bot commented Dec 21, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (c0f4258) 67.99% compared to head (c65def9) 68.00%.
Report is 5 commits behind head on develop.

Additional details and impacted files 
@@             Coverage Diff             @@
##           develop   #22372      +/-   ##
===========================================
+ Coverage    67.99%   68.00%   +0.01%     
===========================================
  Files         1075     1075              
  Lines        41577    41568       -9     
  Branches     11182    11184       +2     
===========================================
- Hits         28270    28266       -4     
+ Misses       13307    13302       -5

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

@Gudahtt Gudahtt removed the INVALID-PR-TEMPLATE PR's body doesn't match template label Dec 21, 2023
@danjm danjm merged commit c40dbb1 into develop Dec 22, 2023
@danjm danjm deleted the asset-controller-fixes branch December 22, 2023 08:25
@github-actions github-actions bot locked and limited conversation to collaborators Dec 22, 2023
@metamaskbot metamaskbot added the release-11.9.0 Issue or pull request that will be included in release 11.9.0 label Dec 22, 2023
@metamaskbot metamaskbot added release-11.7.3 Issue or pull request that will be included in release 11.7.3 and removed release-11.9.0 Issue or pull request that will be included in release 11.9.0 labels Jan 2, 2024
@metamaskbot
Copy link
Copy Markdown
Collaborator

metamaskbot commented Jan 2, 2024

Missing release label release-11.7.3 on PR. Adding release label release-11.7.3 on PR and removing other release labels(release-11.9.0), as PR was cherry-picked in branch 11.7.3.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@Gudahtt Gudahtt Gudahtt approved these changes

+1 more reviewer

@bergeron bergeron bergeron approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

release-11.7.3 Issue or pull request that will be included in release 11.7.3 team-extension-platform Extension Platform team

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@danjm @metamaskbot @Gudahtt @bergeron