fix: disable polling when MM closed #24162

sahar-fehri · 2024-04-22T11:09:21Z

Description

Once nftDetection is enabled by the user, the nftDetection controller will start polling data in the background every 3 min even if the user closes the metamask popup or the tab.

We want to stop unnecessary polling when MM is closed in an attempt to reduce traffic.

Related issues

Related to: MetaMask/core#4178

Manual testing steps

Go to this page...

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.

github-actions · 2024-04-22T11:09:34Z

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.

darkwing · 2024-04-22T18:13:16Z

E2E failures look legit

…-closed-v2

app/scripts/metamask-controller.js

metamaskbot · 2024-04-22T22:51:28Z

Builds ready [78f47bc]

builds: chrome, firefox
builds (beta): chrome
builds (flask): chrome, firefox
builds (MMI): chrome, firefox
builds (test): chrome, firefox
builds (test-flask): chrome, firefox
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5, 6
- common: 0, 1, 2, 3, 4, 5, 6, 7, 8
- content-script: 0
- ui: 0, 1, 10, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1198 ± 600 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	63	229	123	46	22
		domContentLoaded	11	100	28	20	10
		load	51	2911	1198	1249	600
		domInteractive	11	100	28	20	10

Bundle size diffs [🚨 Warning! Bundle size has increased!]

background: 386 Bytes (0.01%)
ui: 0 Bytes (0.00%)
common: 27 Bytes (0.00%)

codecov · 2024-04-23T14:24:29Z

Codecov Report

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

Project coverage is 67.44%. Comparing base (b3c8d32) to head (f49cd66).
Report is 18 commits behind head on develop.

Files	Patch %	Lines
app/scripts/metamask-controller.js	66.67%	3 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop   #24162      +/-   ##
===========================================
- Coverage    67.47%   67.44%   -0.02%     
===========================================
  Files         1257     1257              
  Lines        49232    49242      +10     
  Branches     12822    12816       -6     
===========================================
- Hits         33216    33211       -5     
- Misses       16016    16031      +15

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

app/scripts/metamask-controller.js

metamaskbot · 2024-04-23T19:30:00Z

Builds ready [f49cd66]

builds: chrome, firefox
builds (beta): chrome
builds (flask): chrome, firefox
builds (MMI): chrome, firefox
builds (test): chrome, firefox
builds (test-flask): chrome, firefox
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5, 6
- common: 0, 1, 2, 3, 4, 5, 6, 7, 8
- content-script: 0
- ui: 0, 1, 10, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (858 ± 582 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	59	287	112	53	25
		domContentLoaded	9	78	28	20	10
		load	47	3280	858	1213	582
		domInteractive	9	78	28	20	10

Bundle size diffs [🚨 Warning! Bundle size has increased!]

background: 311 Bytes (0.01%)
ui: 0 Bytes (0.00%)
common: 27 Bytes (0.00%)

## Explanation This PR goes together with this extension PR MetaMask/metamask-extension#24162 We want to reduce third party calls for nft detection when a user has closed the MM popup/window. To do that we are leveraging `stopNetworkRequests` and `triggerNetworkrequests` functions inside extension to either `stop()` or `start()` the nft detection. While doing this fixed the following scenario: 1- User imports MM wallet 2- User enables nft detection toggle 3- User closes MM tab or popup 4- We are no longuer making calls to third party because we called stop() function on nftDetectionController. It does not fix this scenario: 1- User imports MM wallet 2- User enabled nft detection toggle 3- User closes chrome all together 4- User reopens chrome without opening MM, and we notice that it is making calls in the background to third party even though MM is not open. This behavior was happening because of a couple of reasons; the property `disabled` being set to `true` in `defaultConfig` ``` this.defaultConfig = { interval: DEFAULT_INTERVAL, chainId: initialChainId, selectedAddress: '', disabled: true, }; ``` made the check on this line pass https://github.com/MetaMask/core/blob/15518442513fbeaade83e4993851acdc1f038e4a/packages/assets-controllers/src/NftDetectionController.ts#L480 and triggered the `start()` function (because the useNftDetection is true). I fixed this by having extension pass the `disabled` in the contructor rather than having the `true` value hardcoded in the controller. I have also refactored the `onPreferencesStateChange` to have it make this check ``` if(selectedAddress !== previouslySelectedAddress){ this.configure({ selectedAddress }); } ``` Separately. When the user first opens the selectedAddress is '' because its also set in default config which, because of the `OR` statement triggered the code to also go into the `start()` ## References  ## Changelog  ### `@metamask/assets-controllers` - **ADDED**: Added disabled inside contructor of nftDetectionController - **CHANGED**: Pass new disabled value instead of true in the default config - **CHANGED**: Made the check for the selected address separate from the check of useNftDetection value ## Checklist - [ ] I've updated the test suite for new or updated code as appropriate - [ ] I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate - [ ] I've highlighted breaking changes using the "BREAKING" category above as appropriate

fix: disable polling when MM closed

0482e38

sahar-fehri requested a review from a team as a code owner April 22, 2024 11:09

sahar-fehri mentioned this pull request Apr 22, 2024

fix: trigger start/stop nft polling if client is open/closed #24026

Closed

7 tasks

sahar-fehri 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 22, 2024

sahar-fehri mentioned this pull request Apr 22, 2024

fix: fix detecting NFTs after closing browser MetaMask/core#4178

Merged

3 tasks

fix: start/stop nft detection on unlock/lock event

622e75a

sahar-fehri added the team-assets label Apr 22, 2024

Merge branch 'develop' into fix/stop-polling-nft-controller-if-client…

78f47bc

…-closed-v2

bergeron reviewed Apr 22, 2024

View reviewed changes

app/scripts/metamask-controller.js Show resolved Hide resolved

gauthierpetetin added the needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) label Apr 23, 2024

sahar-fehri added 2 commits April 23, 2024 09:35

fix: rm start/stop on unlock/lock

069ee7c

fix: add small delay

3972d07

salimtb previously approved these changes Apr 23, 2024

View reviewed changes

NidhiKJha previously approved these changes Apr 23, 2024

View reviewed changes

fix: update e2e test

172dffc

sahar-fehri dismissed stale reviews from NidhiKJha and salimtb via 172dffc April 23, 2024 12:58

fix: update e2e test

f5b3f42

sahar-fehri added 2 commits April 23, 2024 17:17

fix: refactor e2e test

6142899

fix: fix e2e test

f49cd66

bergeron reviewed Apr 23, 2024

View reviewed changes

app/scripts/metamask-controller.js Show resolved Hide resolved

bergeron approved these changes Apr 23, 2024

View reviewed changes

darkwing approved these changes Apr 23, 2024

View reviewed changes

sahar-fehri merged commit 2638466 into develop Apr 23, 2024
68 checks passed

sahar-fehri deleted the fix/stop-polling-nft-controller-if-client-closed-v2 branch April 23, 2024 20:19

github-actions bot removed the needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) label Apr 23, 2024

github-actions bot locked and limited conversation to collaborators Apr 23, 2024

metamaskbot added the release-11.16.0 Issue or pull request that will be included in release 11.16.0 label Apr 23, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: disable polling when MM closed #24162

fix: disable polling when MM closed #24162

sahar-fehri commented Apr 22, 2024

github-actions bot commented Apr 22, 2024

darkwing commented Apr 22, 2024

metamaskbot commented Apr 22, 2024

codecov bot commented Apr 23, 2024 •

edited

metamaskbot commented Apr 23, 2024

fix: disable polling when MM closed #24162

fix: disable polling when MM closed #24162

Conversation

sahar-fehri commented Apr 22, 2024

Description

Related issues

Manual testing steps

Screenshots/Recordings

Before

After

Pre-merge author checklist

Pre-merge reviewer checklist

github-actions bot commented Apr 22, 2024

darkwing commented Apr 22, 2024

metamaskbot commented Apr 22, 2024

codecov bot commented Apr 23, 2024 • edited

Codecov Report

metamaskbot commented Apr 23, 2024

codecov bot commented Apr 23, 2024 •

edited