Added polling-controller #1703

shanejonas · 2023-09-22T14:48:11Z

Explanation

Originally was in #1673 but pulled out to get this in on its own.

Adds an abstract class (currently named PollingController. The start and stop methods are parameterized by networkClientId's and pollingToken's and polling intervals are stored in class variables by chainId so that multiple networkClients/chainIds can poll simultaneously. executePoll is an abstract method to be implemented by the controller itself so that this pattern could be generalized and be agnostic to what is being executed on the polling interval.

References

Related to #1673
Related to https://github.com/MetaMask/MetaMask-planning/issues/1314

socket-security · 2023-09-22T15:41:31Z

No top level dependency changes detected. Learn more about Socket for GitHub ↗︎

packages/polling-controller/src/PollingController.ts

packages/polling-controller/src/PollingController.test.ts

adonesky1 · 2023-09-22T19:56:33Z

A few small nits but otherwise lets gooo

packages/polling-controller/src/PollingController.ts

jiexi · 2023-09-22T20:26:54Z

packages/polling-controller/src/PollingController.ts

+    if (this.intervalIds[networkClientId]) {
+      clearTimeout(this.intervalIds[networkClientId]);
+      delete this.intervalIds[networkClientId];
+    }


i think this logic should be DRY'ed into it's own private method since we use it in the stop() method as well

packages/polling-controller/src/PollingController.ts

jiexi · 2023-09-22T20:31:27Z

packages/polling-controller/src/PollingController.ts

+    }
+    let found = false;
+    this.networkClientIdTokensMap.forEach((tokens, networkClientId) => {
+      if (tokens.has(pollingToken)) {


nit. feel free to ignore. I feel that this would be slightly cleaner if this was turned negated / turned into a guard.

packages/polling-controller/src/PollingController.test.ts

Gudahtt

Looks great!

On second thought, it does seem important for the interval to be configurable

packages/polling-controller/package.json

BelfordZ · 2023-09-25T17:54:11Z

packages/polling-controller/src/PollingController.ts

+   * @param networkClientId - The networkClientId to start polling for
+   * @returns void
+   */
+  start(networkClientId: NetworkClientId) {


Should we support polling controllers which may only have 1 polling loop active per networkClientId? Perhaps we could have an option passed into the constructor to indicate this? for example, if the blocktracker was a controller, we would only want to have 1 active block polling per network.

Good question. I can't think of any controllers where we'd want this offhand though.

BelfordZ · 2023-09-25T17:54:50Z

packages/polling-controller/src/PollingController.ts

+  /**
+   * Stops polling for all networkClientIds
+   */
+  stopAll() {


Should we have this optionally take a networkClientId, allowing all polling to be stopped for a given network?

We could do this but I'm not sure I can think of a situation where we'd want this behavior. In general PollingTokens should be disconnected individually so we can be sure that there are no lingering listeners depending on active polling. TBH I'm not even sure where we'll want to call stopAll?

stopAll is useful in tests but I don't think many other places will call it

BelfordZ · 2023-09-25T17:59:26Z

packages/polling-controller/src/PollingController.ts

+      try {
+        await this.executePoll(networkClientId);
+      } catch (error) {
+        console.error(error);


Should we be eating the errors? I suppose even if we let the error bubble up, since this is happening inside an interval, nothing else would be able to catch the error anyways. In this case, does it make sense to add an interface to subscribe to errors? IE how would a controller using this abstract controller be able to detect an error in its executePoll call, and react to it.

Should we keep polling when there are errors? Maybe we can have an option to configure an linear/exponential backoff when there are errors.

I had the same thought, this is definitely not ideal for error handling. But it's strictly better than the status quo, where typically the error is completely swallowed and not even logged. Something to consider later as an enhancement perhaps

…ixes

Co-authored-by: Alex Donesky <adonesky@gmail.com>

…k method

Gudahtt · 2023-09-26T12:38:34Z

packages/polling-controller/src/PollingController.ts

+} from '@metamask/base-controller';
+import type { NetworkClientId } from '@metamask/network-controller';
+import type { Json } from '@metamask/utils';
+import { v1 as random } from 'uuid';


Hmm, why not use v4? It's my understanding that v1 is not quite as random, and that v4 is the better choice in most situations

packages/polling-controller/package.json

Gudahtt

LGTM! Just one pending suggestion

socket-security · 2023-09-26T13:30:57Z

👍 Dependency issues cleared. Learn more about Socket for GitHub ↗︎

This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored.

Gudahtt

LGTM!

Unused dependencies have been removed

## Explanation > Originally was in #1673 but pulled out to get this in on its own. Adds an abstract class (currently named `PollingController`. The start and stop methods are parameterized by `networkClientId`'s and `pollingToken`'s and polling intervals are stored in class variables by `chainId` so that multiple `networkClients`/`chainIds` can poll simultaneously. `executePoll` is an abstract method to be implemented by the controller itself so that this pattern could be generalized and be agnostic to what is being executed on the polling interval. ## References Related to #1673 Related to MetaMask/MetaMask-planning#1314 --------- Co-authored-by: Alex Donesky <adonesky@gmail.com>

shanejonas requested a review from a team as a code owner September 22, 2023 14:48

shanejonas force-pushed the polling-controller branch from 3bce199 to db9ec11 Compare September 22, 2023 15:06

adonesky1 reviewed Sep 22, 2023

View reviewed changes

packages/polling-controller/src/PollingController.ts Outdated Show resolved Hide resolved

shanejonas force-pushed the polling-controller branch from 28cb1e8 to 28de47f Compare September 22, 2023 16:57

adonesky1 reviewed Sep 22, 2023

View reviewed changes

packages/polling-controller/src/PollingController.test.ts Outdated Show resolved Hide resolved

adonesky1 reviewed Sep 22, 2023

View reviewed changes

packages/polling-controller/src/PollingController.test.ts Outdated Show resolved Hide resolved

adonesky1 reviewed Sep 22, 2023

View reviewed changes

packages/polling-controller/src/PollingController.test.ts Outdated Show resolved Hide resolved

adonesky1 reviewed Sep 22, 2023

View reviewed changes

packages/polling-controller/src/PollingController.test.ts Outdated Show resolved Hide resolved

adonesky1 reviewed Sep 22, 2023

View reviewed changes

packages/polling-controller/src/PollingController.test.ts Outdated Show resolved Hide resolved

adonesky1 requested review from jiexi, BelfordZ and Gudahtt September 22, 2023 19:56

jiexi reviewed Sep 22, 2023

View reviewed changes

packages/polling-controller/src/PollingController.ts Outdated Show resolved Hide resolved

jiexi reviewed Sep 22, 2023

View reviewed changes

packages/polling-controller/src/PollingController.ts Show resolved Hide resolved

jiexi reviewed Sep 22, 2023

View reviewed changes

packages/polling-controller/src/PollingController.ts Outdated Show resolved Hide resolved

jiexi reviewed Sep 22, 2023

View reviewed changes

Gudahtt reviewed Sep 22, 2023

View reviewed changes

packages/polling-controller/src/PollingController.test.ts Outdated Show resolved Hide resolved

Gudahtt reviewed Sep 22, 2023

View reviewed changes

packages/polling-controller/src/PollingController.test.ts Outdated Show resolved Hide resolved

Gudahtt reviewed Sep 22, 2023

View reviewed changes

packages/polling-controller/src/PollingController.test.ts Show resolved Hide resolved

Gudahtt previously approved these changes Sep 22, 2023

View reviewed changes

legobeat previously requested changes Sep 22, 2023

View reviewed changes

packages/polling-controller/package.json Outdated Show resolved Hide resolved

BelfordZ reviewed Sep 25, 2023

View reviewed changes

shanejonas added 2 commits September 25, 2023 15:50

Added polling-controller

98efc47

Changed old names to PollingController in tests + other PR feedback f…

b3dafbf

…ixes

shanejonas and others added 10 commits September 25, 2023 15:50

Removed unused deps

7cf3c13

Fixed naming on PollingController

f663d29

Update packages/polling-controller/src/PollingController.test.ts

b6db670

Co-authored-by: Alex Donesky <adonesky@gmail.com>

Changed PollingController test beforeEach to new createExecutePollMoc…

58ab401

…k method

Added TICK_TIME constant for PollingController tests

4251790

Added deepmerge dep

c72f5d5

Fixed deepmerge issue

cb99a77

add interval setting in the constructor

0ee7b44

update test

fac9924

add intervallength test

911ff20

adonesky1 force-pushed the polling-controller branch from bb2371e to 911ff20 Compare September 25, 2023 20:51

adonesky1 requested review from jiexi, Gudahtt, BelfordZ and legobeat September 25, 2023 20:56

Gudahtt reviewed Sep 26, 2023

View reviewed changes

packages/polling-controller/package.json Outdated Show resolved Hide resolved

Gudahtt previously approved these changes Sep 26, 2023

View reviewed changes

Merge branch 'main' into polling-controller

43e9ed5

Fixed constraint issues

207e05c

shanejonas dismissed Gudahtt’s stale review via 207e05c September 26, 2023 13:51

shanejonas added 2 commits September 26, 2023 07:10

Merge branch 'main' into polling-controller

730fbd9

Fixed PollingController uuid to use v4

4f02231

Gudahtt approved these changes Sep 26, 2023

View reviewed changes

shanejonas merged commit 9698b0a into main Sep 26, 2023
107 checks passed

shanejonas deleted the polling-controller branch September 26, 2023 14:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added polling-controller #1703

Added polling-controller #1703

shanejonas commented Sep 22, 2023

socket-security bot commented Sep 22, 2023 •

edited

Loading

adonesky1 commented Sep 22, 2023

jiexi Sep 22, 2023

jiexi Sep 22, 2023

Gudahtt left a comment

BelfordZ Sep 25, 2023

Gudahtt Sep 25, 2023

BelfordZ Sep 25, 2023

adonesky1 Sep 25, 2023

shanejonas Sep 26, 2023

BelfordZ Sep 25, 2023

Gudahtt Sep 25, 2023

Gudahtt Sep 26, 2023

Gudahtt left a comment

socket-security bot commented Sep 26, 2023 •

edited

Loading

Gudahtt left a comment

Added polling-controller #1703

Added polling-controller #1703

Conversation

shanejonas commented Sep 22, 2023

Explanation

References

socket-security bot commented Sep 22, 2023 • edited Loading

adonesky1 commented Sep 22, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Gudahtt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Gudahtt left a comment

Choose a reason for hiding this comment

socket-security bot commented Sep 26, 2023 • edited Loading

Gudahtt left a comment

Choose a reason for hiding this comment

socket-security bot commented Sep 22, 2023 •

edited

Loading

socket-security bot commented Sep 26, 2023 •

edited

Loading