Skip to content

fix: token rates controller start / stop logic#24021

Merged
bergeron merged 2 commits intodevelopfrom
brian/start-stop-token-rates-v3
Apr 16, 2024
Merged

fix: token rates controller start / stop logic#24021
bergeron merged 2 commits intodevelopfrom
brian/start-stop-token-rates-v3

Conversation

@bergeron
Copy link
Copy Markdown
Contributor

@bergeron bergeron commented Apr 15, 2024

Description

Problem: Users reporting missing fiat prices for some erc20 tokens, even when price setting is on:

Screenshot 2024-03-14 at 2 35 41 PM

Diagnosis:

(1) The TokenRatesController gets stopped when metamask is closed. But it is not started again when metamask re-opens, preventing any future token price updates.

(2) If you have metamask installed but you never open/unlock it (within a browser session), the TokenRatesController is still polling in the background every 3 minutes.

The fix for both is to start the controller in triggerNetworkrequests instead of on construction.

Open in GitHub Codespaces

Related issues

Fixes:

Manual testing steps

  1. Restore a wallet where multiple accounts have erc20 tokens
  2. Enable token autodetection
  3. Visit the first account with tokens, verify their prices
  4. Close metamask
  5. Re-open metamask
  6. Visit the second account
  7. Verify its tokens have prices

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 15, 2024 04:49
@github-actions
Copy link
Copy Markdown
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.


if (useCurrencyRateCheck) {
this.currencyRateController.stopAllPolling();
this.tokenRatesController.stop();
Copy link
Copy Markdown
Contributor Author

@bergeron bergeron Apr 15, 2024

Choose a reason for hiding this comment

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

This line is moved because I believe useCurrencyRateCheck is the proper guard, as occurs elsewhere. It was previously guarded by the autodetect feature, but pricing and autodetection are separate settings.

@bergeron bergeron requested review from sahar-fehri and salimtb April 15, 2024 05:02
@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 15, 2024
@codecov
Copy link
Copy Markdown

codecov bot commented Apr 16, 2024

Codecov Report

Attention: Patch coverage is 50.00000% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 67.59%. Comparing base (257e9f9) to head (dd6bd5f).

Files Patch % Lines
app/scripts/metamask-controller.js 50.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop   #24021      +/-   ##
===========================================
- Coverage    67.60%   67.59%   -0.01%     
===========================================
  Files         1247     1247              
  Lines        48921    48925       +4     
  Branches     12772    12766       -6     
===========================================
- Hits         33071    33067       -4     
- Misses       15850    15858       +8     

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

@metamaskbot
Copy link
Copy Markdown
Collaborator

Builds ready [dd6bd5f]
Page Load Metrics (986 ± 575 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint723921367435
domContentLoaded893302512
load6028859861197575
domInteractive893302512
Bundle size diffs [🚀 Bundle size reduced!]
  • background: -64 Bytes (-0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

@bergeron bergeron merged commit e322bc8 into develop Apr 16, 2024
@bergeron bergeron deleted the brian/start-stop-token-rates-v3 branch April 16, 2024 18:14
@github-actions github-actions bot locked and limited conversation to collaborators Apr 16, 2024
@metamaskbot metamaskbot added the release-11.16.0 Issue or pull request that will be included in release 11.16.0 label Apr 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.16.0 Issue or pull request that will be included in release 11.16.0 team-assets

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

5 participants