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

refactor: change CurrencyRateController Polling to be UI driven #23476

Merged
merged 44 commits into from
Apr 24, 2024

Conversation

jiexi
Copy link
Contributor

@jiexi jiexi commented Mar 13, 2024

Description

Currently currency rate polling is started and stopped based on active trusted connections to the MetaMaskController (i.e. whenever the wallet ui is opened). In the future we want polling to be tied to the actual components that rely on polled currency rate state. As a transitionary step, this PR moves currency rate polling from MetaMaskController into a hook that wraps the root level <Routes> component. Nothing changes except that currency rate polling goes from being MetaMaskController connection driven to actually wallet UI driven. Polling will still occur as long at least one wallet UI is open as it currently already does.

Open in GitHub Codespaces

Related issues

Fixes: https://github.com/MetaMask/MetaMask-planning/issues/2083

Manual testing steps

  1. Open background console - network tab
  2. Filter by min-api.cryptocompare.com
  3. Open wallet UI
  4. See request in background network tab
  5. Change network
  6. See new request
  7. Close wallet UI
  8. Open wallet UI right away
  9. See no new requests
  10. Close wallet UI
  11. Wait 3 minutes
  12. See no new requests
  13. Open wallet UI
  14. See new request
  15. Leave wallet UI open
  16. Wait 3 minutes
  17. See new request

Screenshots/Recordings

Before

After

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.

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

codecov bot commented Mar 13, 2024

Codecov Report

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

Project coverage is 67.46%. Comparing base (ec03702) to head (17af907).

Files Patch % Lines
ui/store/actions.ts 18.18% 9 Missing ⚠️
ui/hooks/useCurrencyRatePolling.ts 0.00% 7 Missing ⚠️
app/scripts/metamask-controller.js 0.00% 4 Missing ⚠️
ui/contexts/currencyRate.js 0.00% 4 Missing ⚠️
ui/hooks/usePolling.ts 50.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop   #23476      +/-   ##
===========================================
- Coverage    67.53%   67.46%   -0.06%     
===========================================
  Files         1261     1263       +2     
  Lines        49289    49327      +38     
  Branches     12856    12854       -2     
===========================================
- Hits         33283    33278       -5     
- Misses       16006    16049      +43     

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

@metamaskbot
Copy link
Collaborator

Builds ready [0c8eefb]
Page Load Metrics (1029 ± 420 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint732581274421
domContentLoaded108325178
load6024291029874420
domInteractive108325178
Bundle size diffs [🚀 Bundle size reduced!]
  • background: -619 Bytes (-0.02%)
  • ui: 2.43 KiB (0.03%)
  • common: 0 Bytes (0.00%)

@jiexi jiexi mentioned this pull request Mar 20, 2024
13 tasks
@jiexi jiexi marked this pull request as ready for review March 25, 2024 20:50
@jiexi jiexi requested a review from a team as a code owner March 25, 2024 20:50
@metamaskbot
Copy link
Collaborator

Builds ready [ad1c711]
Page Load Metrics (1272 ± 574 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint6140317110952
domContentLoaded107327178
load50273512721195574
domInteractive107327178
Bundle size diffs [🚀 Bundle size reduced!]
  • background: -445 Bytes (-0.01%)
  • ui: 2.14 KiB (0.03%)
  • common: 331 Bytes (0.01%)

adonesky1
adonesky1 previously approved these changes Apr 12, 2024
Copy link
Contributor

@adonesky1 adonesky1 left a comment

Choose a reason for hiding this comment

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

LGTM!

adonesky1
adonesky1 previously approved these changes Apr 17, 2024
shanejonas
shanejonas previously approved these changes Apr 17, 2024
@jiexi jiexi dismissed stale reviews from adonesky1 and shanejonas via c8ffbf8 April 23, 2024 18:23
@metamaskbot
Copy link
Collaborator

Builds ready [23b4931]
Page Load Metrics (1136 ± 627 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint711511012412
domContentLoaded10341973
load62306911361305627
domInteractive10341973
Bundle size diffs [🚀 Bundle size reduced!]
  • background: -389 Bytes (-0.01%)
  • ui: 2.14 KiB (0.03%)
  • common: 331 Bytes (0.01%)

@legobeat
Copy link
Contributor

legobeat commented Apr 24, 2024

In the future we want polling to be tied to the actual components that rely on polled currency rate state

Is this actually desired? By making the HTTP request timing depend on user action, user activity metadata leaking to price provider (and middle-parties) will increase.

Tangential but related: Why are we not fetching currency rates like ETH-USD via on-chain oracles instead (or in addition)? This would enable price lookups without any third-party connections, only relying on eth_call to oracle(s). Could be worth looking into as an alternative or parallel path to reduce non-chain API calls as part of this feature.

@adonesky1
Copy link
Contributor

adonesky1 commented Apr 24, 2024

Is this actually desired? By making the HTTP request timing depend on user action, user activity metadata leaking to price provider (and middle-parties) will increase.

not sure I follow? Are you saying that the metadata associated with http requests of when users have certain UI's open is a leak? If so I don't think thats really a reasonable boundary to set out for ourself, it a pretty common pattern for how to determine when or when not to make queries... but perhaps I'm misunderstanding. The goal here is to only query for what we need based on what actual data needs to be rendered and minimize any additional polling we're doing outside of that.

Why are we not fetching currency rates like ETH-USD via on-chain oracles instead (or in addition)?

Definitely worth considering but very much out of scope for this particular PR. I would raise this with @alfeng6 and the assets team!

@metamaskbot
Copy link
Collaborator

Builds ready [17af907]
Page Load Metrics (1789 ± 664 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint832381544923
domContentLoaded13117443316
load69360917891382664
domInteractive13117443316
Bundle size diffs [🚀 Bundle size reduced!]
  • background: -389 Bytes (-0.01%)
  • ui: 2.14 KiB (0.03%)
  • common: 331 Bytes (0.01%)

@jiexi jiexi merged commit 7e972ba into develop Apr 24, 2024
68 checks passed
@jiexi jiexi deleted the jl/mmp-2083/currency-rate-polling-hook-global branch April 24, 2024 21:28
@github-actions github-actions bot locked and limited conversation to collaborators Apr 24, 2024
@metamaskbot metamaskbot added the release-11.16.0 Issue or pull request that will be included in release 11.16.0 label Apr 24, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
release-11.16.0 Issue or pull request that will be included in release 11.16.0 team-wallet-api-platform
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants