metamask-extension icon indicating copy to clipboard operation
metamask-extension copied to clipboard

Published 20 hours ago • verified publisher icon MetaMask

Replacing setInterval with chrome.alarm in Detect-Token controller

Open NiranjanaBinoy opened this issue 2 years ago • 9 comments

#15509 As a part of MV3, we need to replace the setTimeout and setInterval functions with chrome Alarms throughout the code base. While creating chrome alarms, we can define delayInMinutes to show how many minutes we should wait before the first time the alarm is triggered; we can also define periodInMinutes which is the number of minutes between alarms. Unlike setIntervals the time interval has to be mentioned in Minutes, not in milliseconds. Inside the onAlarm event listener, we can define the task to be executed during the alarm defined.

Currently, I have kept the interval as 0.2 purely for testing purposes, we can update that to 3 before merging the PR. Also, any interval below 1 will not be honored in production but it is allowed in the development environment .

NiranjanaBinoy avatar Aug 09 '22 20:08 NiranjanaBinoy

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.

github-actions[bot] avatar Aug 09 '22 20:08 github-actions[bot]

The unit test is failing for detect-token controller test and metamask-controller test when trying to differentiate between MV3 and MV2 using isManifestV3 from mv3.utils. I tried to mock both isManifestV3 and also browser.runtime.getManifest but both scenarios are working. I have commented out, and commited have the steps/ways I tired out to mock the functions. The error message we are getting: TypeError: _webextensionPolyfill.default.runtime.getManifest is not a function

NiranjanaBinoy avatar Aug 18 '22 19:08 NiranjanaBinoy

Builds ready [44d1c29]
Page Load Metrics (1820 ± 81 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint961846199378182
domContentLoaded16062241179216177
load16062264182016881
domInteractive16062241179216177

metamaskbot avatar Aug 19 '22 17:08 metamaskbot

Builds ready [55236c8]
Page Load Metrics (1844 ± 60 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint98164125199
domContentLoaded16422104182212158
load17062128184412560
domInteractive16422104182212158

metamaskbot avatar Aug 22 '22 21:08 metamaskbot

Builds ready [07181c7]
Page Load Metrics (1932 ± 153 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint1043951486833
domContentLoaded159725341909306147
load159726871932318153
domInteractive159725341909306147

metamaskbot avatar Aug 23 '22 16:08 metamaskbot

From QA point of view, a couple of comments/questions:

  • When I Lock MM, I still see that on the Service Worker console, the hasAlarm console log is triggered non-stop. I am wondering if this alarm should be cleared, when MM is locked

alarms-token-detection-locked.webm

  • When I disable the Token Detection toggle, I still see that the hasAlarm console log is triggered. I am wondering if this alarm should be cleared, once the functionality is disabled. This might be related to this other bug around the toggle: token detection still works while toggle is Disabled, so maybe it should be fixed separately.

token-detection-toggle.webm

  • When I am on the MM popup view, I see that no hasAlarm or in rescheduleTokenDetectionPollingInMV3 console.logs are ever triggered, even though Token Detection continues to work

popup-alarm-not-showing.webm

seaona avatar Sep 11 '22 10:09 seaona

Thanks for the QA @seaona ❤️ I have added some clarifications and queries below.

When I Lock MM, I still see that on the Service Worker console, the hasAlarm console log is triggered non-stop. I am wondering if this alarm should be cleared, when MM is locked

In MV2, the function seems to be getting triggered even when the MM is locked and pop-up is closed, but the scenario is handled inside the detectNewtoken() function, which returns if the pop is closed and the MM is locked.

When I disable the Token Detection toggle, I still see that the hasAlarm console log is triggered. I am wondering if this alarm should be cleared, once the functionality is disabled. This might be related to https://github.com/MetaMask/metamask-extension/issues/15790 around the toggle: token detection still works while toggle is Disabled, so maybe it should be fixed separately.

During token detection v2 development, a decision was made to keep token detection active for Mainnet using the static list even when the toggle is turned off to preserve the existing behavior in Mainnet(This scenario is handled inside the detectNewToken()). I will verify this with @danjm regarding this scenario and update accordingly.

When I am on the MM popup view, I see that no hasAlarm or in rescheduleTokenDetectionPollingInMV3 console.logs are ever triggered, even though Token Detection continues to work

Chrome alarm seems to be working in pop up view for me, Could you please let me know how often this is happening and if this is happening for a particular scenario?

NiranjanaBinoy avatar Sep 15 '22 23:09 NiranjanaBinoy

Builds ready [aeaa099]
Page Load Metrics (1311 ± 89 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint931206169238114
domContentLoaded11431977128417182
load11432076131118689
domInteractive11431977128417182

metamaskbot avatar Sep 20 '22 15:09 metamaskbot

Builds ready [92bee65]
Page Load Metrics (1347 ± 93 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint96141112147
domContentLoaded11172084133519995
load11372084134719593
domInteractive11172084133519995

metamaskbot avatar Sep 21 '22 20:09 metamaskbot

Builds ready [f789025]
Page Load Metrics (2389 ± 68 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint87134110147
domContentLoaded20502729236115876
load21692729238914268
domInteractive20502729236115876

highlights:

storybook

metamaskbot avatar Sep 25 '22 18:09 metamaskbot

@NiranjanaBinoy thank you v much for your comments! I've double-checked the pop up behavior and it is working fine!!

Just one last thing, about calling the function while MetaMask being locked, co you confirm is it okay in the MV3 context? I can see my token detected and address while logged out:

image

logout-alarm.webm

seaona avatar Sep 26 '22 10:09 seaona

@seaona Thanks for confirming the pop-up behavior. ❤️ When detectNewTokens() is the first line in the function, we are checking if the application is open and unlocked if not, the detectNewTokens function will be returned/exited in the second line. So even if the function is triggered while MM is locked, the function will not be executed. This is the same as in MV2 as well. The situation where we are having detectedTokens in the state while the user is locked out seems to exist in MV2 as well; the same goes for the other token details. We would probably have to deal with it as a separate issue unrelated to MV3.

NiranjanaBinoy avatar Sep 26 '22 19:09 NiranjanaBinoy

Builds ready [f86c190]
Page Load Metrics (2837 ± 209 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint893295293689331
domContentLoaded215134082795413198
load215135912837434209
domInteractive215134082795413198

highlights:

storybook

metamaskbot avatar Sep 26 '22 21:09 metamaskbot

Sounds good! Thank you @NiranjanaBinoy !! :heart:

seaona avatar Sep 27 '22 08:09 seaona

Builds ready [34f1f57]
Page Load Metrics (2773 ± 176 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint1011751452211
domContentLoaded218734132728344165
load218834392773367176
domInteractive218734132728344165

highlights:

storybook

metamaskbot avatar Sep 28 '22 15:09 metamaskbot

Builds ready [5f352b3]
Page Load Metrics (2480 ± 156 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint871801152110
domContentLoaded166128592442315151
load166129532480325156
domInteractive166128592442315151

metamaskbot avatar Oct 06 '22 16:10 metamaskbot

Builds ready [742dc40]
Page Load Metrics (2743 ± 134 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint1032550246529254
domContentLoaded178132372723293140
load184532372743279134
domInteractive178132362723293140

metamaskbot avatar Oct 11 '22 15:10 metamaskbot

Closing this PR because if the service worker restarts every 5 mins, it will not have much impact on intervals less than 5 mins if the function that is scheduled is called during the interval as well. Quoting related chat from slack here

NiranjanaBinoy avatar Oct 20 '22 15:10 NiranjanaBinoy