Generalize polling abstraction #3636

adonesky1 · 2023-12-07T21:24:01Z

~~WIP~~
Ready for review: I will add changelog entries when/if we decide to roll with this approach.

Alternative approach to refactoring the PollingController per this feedback on PollingTracker enhancement work

CHANGELOG ENTRIES

@metamask/polling-controller

Removed 

BREAKING: PollingController, PollingControllerV1 and PollingControllerOnly are all removed and replaced by StaticIntervalPollingController, StaticIntervalPollingControllerV1 and StaticIntervalPollingControllerOnly

Added 

BlockTrackerPollingController, BlockTrackerPollingControllerV1 and BlockTrackerPollingControllerOnly have been added and can be used by subclasses to poll with a blockTracker using the same API as the StaticIntervalPollingController versions - the only exception is the requirement to implement the abstract method _getNetworkClientById on subclasses.

packages/polling-controller/src/StaticIntervalPollingController.ts

jiexi · 2023-12-07T22:13:14Z

packages/polling-controller/src/BlockTrackerPollingController.ts

+    }
+
+    stopBlockTrackingPolling(key: string) {
+      const [networkClientId] = key.split(':');


do you think we should have a helper to get us from pollingKey back to networkClient and options?

jiexi · 2023-12-07T22:21:24Z

Main question is if PollingStrategy is necessary or not. Otherwise looking great

mcmire

I think there are plenty of opportunities for improving upon the architecture here — separating the pieces more and perhaps deciding on a better name that encapsulates the commonalities between the two classes. But that of course would take more time. In the meantime, I'm curious whether we ought to drop the strategy pattern and stick with abstract methods that get overridden in subclasses, since we seem to be following that pattern already. I tried to make some suggestions below.

packages/polling-controller/src/BlockTrackerPollingController.ts

mcmire · 2023-12-07T23:12:18Z

packages/polling-controller/src/BlockTrackerPollingController.ts

+      });
+    }
+
+    abstract getNetworkClientById(


Is this something that can be defined in the abstract PollingController? It seems like something that would be common to both.

This one is specific to the BlockTrackerPollingController so it wouldn't be great to require it on the StaticInterval interface

packages/polling-controller/src/BlockTrackerPollingController.ts

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

packages/polling-controller/src/StaticIntervalPollingController.ts

matthewwalsh0 · 2023-12-08T20:22:58Z

I'm not sure I see the benefit of having multiple controllers for each possible interval mechanism as that sounds like it would increase the complexity for the child controllers to adopt or switch between them, or even provide their own custom interval mechanism in rare cases.

Would an alternate architecture be to maintain a single PollingController with a fixed API that defaults to static intervals but provides a setIntervalStrategy(intervalStrategy: IntervalStrategy) that can be called only if we need to switch from the default?

The different interval strategies could be defined in classes inside the same package but with names such as StaticIntervalStrategy and BlockTrackerIntervalStrategy.

The fundamental logic and API and flow remain the same, so it feels natural to encapsulate all that in a single PollingController, we just want a way to alternate the interval types but in an extensible way.

Would that also simplify the upgrade process for all the controllers already using the PollingController?

adonesky1 · 2023-12-08T20:44:28Z

I will add changelog entries when/if we decide to roll with this approach.

packages/polling-controller/src/AbstractPollingController.ts

packages/polling-controller/src/BlockTrackerPollingController.ts

jiexi · 2023-12-08T21:39:09Z

packages/polling-controller/src/BlockTrackerPollingController.ts

+          networkClientId,
+          options,
+        );
+        networkClient.blockTracker.addListener('latest', updateOnNewBlock);


does it make sense to also call updateOnNewBlock with the current block number that blockTracker returns here?

…to StaticIntervalPollingController

legobeat · 2023-12-09T01:42:54Z

packages/polling-controller/src/AbstractPollingController.ts

+export function AbstractPollingControllerBaseMixin<TBase extends Constructor>(
+  Base: TBase,
+) {
+  abstract class AbstractPollingControllerBase extends Base {


Could we explicitly write an interface which captures the polling behavior, and this abstract class then implements? Would make integration less coupled to the partial implementation.

Done here 997c806

mcmire · 2023-12-11T16:28:55Z

packages/polling-controller/src/BlockTrackerPollingController.ts

+  ) {
+    #activeListeners: Record<string, (options: Json) => Promise<void>> = {};
+
+    abstract _getNetworkClientById(


Why is this abstract for BlockTrackerPollingController but not for StaticIntervalPollingController? How do we envision this being used?

This method is used just below on line 45 to retrieve the networkClient blockTracker. The StaticIntervalPollingController does not require the networkClient. The reason I don't have this as a constructor arg is because safely adding a required param to the constructor for mixins is tricky and generally not recommended since the children will have diverse constructor signatures

I see, okay.

mcmire · 2023-12-11T16:33:58Z

packages/polling-controller/src/AbstractPollingController.ts

+      this.#pollingTokenSets.set(key, pollingTokenSet);
+
+      if (pollingTokenSet.size === 1) {
+        // Start new polling only if it's the first token for this key


Nit: This comment repeats what the code already says:

Suggested change

// Start new polling only if it's the first token for this key

done here: 823aaa1

mcmire · 2023-12-11T16:37:25Z

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

+    clock.restore();
+  });
+
+  describe('Polling on block times', () => {


What is the purpose of this extra describe? I don't see another one in this file. It seems that this could be merged into the outer describe.

done here: 823aaa1

mcmire · 2023-12-11T16:39:17Z

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

+};
+
+let getNetworkClientByIdStub: jest.Mock;
+class MyGasFeeController extends BlockTrackerPollingController<any, any, any> {


Does it matter that this is a gas fee controller? Perhaps ChildBlockTrackerPollingController would be a more suitable name.

fixed here: 823aaa1

mcmire · 2023-12-11T16:42:37Z

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

+      }
+
+      private start(interval: number) {
+        setInterval(() => {


Does this test block tracker actually need to emit blocks on an interval? It doesn't seem like the class being tested cares about that — it just assumes that if it listens to latest for a block tracker, at some point that event will be emitted. Can we call emit manually in the test instead? It seems that this would remove the need to keep track of so many timers and simplify the math required to understand each test.

yeah this makes more sense!

Modified the tests to use this pattern here: 823aaa1

mcmire · 2023-12-11T17:02:15Z

packages/polling-controller/src/AbstractPollingController.ts

+      if (keyToDelete) {
+        // TODO figure out why this is necessary
+        const nonNullKey = keyToDelete;
+        this._stopPollingByPollingTokenSetId(nonNullKey);
+        this.#pollingTokenSets.delete(nonNullKey);
+        this.#callbacks.get(nonNullKey)?.forEach((callback) => {
+          // for some reason this typescript can't tell that keyToDelete is not null here
+          callback(nonNullKey);
+        });
+        this.#callbacks.get(nonNullKey)?.clear();
+      }


This happens because you're using forEach to iterate through the callbacks, and you're passing a function to forEach. Functions are, by their nature, not guaranteed to be run immediately, so keyToDelete could have changed back to null before it was called, so TypeScript can't ensure that it's not null. To fix this you can use a for...of loop:

Suggested change

if (keyToDelete) {

// TODO figure out why this is necessary

const nonNullKey = keyToDelete;

this._stopPollingByPollingTokenSetId(nonNullKey);

this.#pollingTokenSets.delete(nonNullKey);

this.#callbacks.get(nonNullKey)?.forEach((callback) => {

// for some reason this typescript can't tell that keyToDelete is not null here

callback(nonNullKey);

});

this.#callbacks.get(nonNullKey)?.clear();

}

if (keyToDelete) {

this._stopPollingByPollingTokenSetId(keyToDelete);

this.#pollingTokenSets.delete(keyToDelete);

const callbacks = this.#callbacks.get(keyToDelete);

if (callbacks) {

for (const callback of callbacks) {

// eslint-disable-next-line n/callback-return

callback(keyToDelete);

}

callbacks.clear();

}

}

gotcha! Thanks for clearing that up!

done here: 823aaa1

mcmire · 2023-12-11T17:03:13Z

packages/polling-controller/src/AbstractPollingController.ts

+      options: Json = {},
+    ) {
+      const key = getKey(networkClientId, options);
+      const callbacks = this.#callbacks.get(key) || new Set<typeof callback>();


Nit: ?? would be safer:

Suggested change

const callbacks = this.#callbacks.get(key) || new Set<typeof callback>();

const callbacks = this.#callbacks.get(key) ?? new Set<typeof callback>();

done here: 823aaa1

mcmire · 2023-12-11T17:03:28Z

packages/polling-controller/src/AbstractPollingController.ts

+      const pollToken = random();
+      const key = getKey(networkClientId, options);
+      const pollingTokenSet =
+        this.#pollingTokenSets.get(key) || new Set<string>();


Nit: ?? would be safer:

Suggested change

this.#pollingTokenSets.get(key) || new Set<string>();

this.#pollingTokenSets.get(key) ?? new Set<string>();

done here: 823aaa1

packages/assets-controllers/src/CurrencyRateController.ts

mcmire · 2023-12-11T17:06:41Z

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

+  return executePollMock;
+};
+
+class MyGasFeeController extends StaticIntervalPollingController<


Does it matter that this is a gas fee controller? Perhaps ChildStaticIntervalPollingController would be a more suitable name.

done here: 823aaa1

mcmire · 2023-12-11T17:10:43Z

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

+      expect(controller._executePoll).toHaveBeenCalledTimes(2);
+      controller.stopAllPolling();
+    });
+    it('should call _executePoll immediately and on interval if polling', async () => {


Hmm, that doesn't seem to be what this test is testing. It seems that _executePoll is called immediately regardless of whether polling was started, yes? If so, then I'm curious whether this test can be merged with the previous one?

Yep seems redundant!

done here: 823aaa1

mcmire · 2023-12-11T17:11:45Z

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

+      await advanceTime({ clock, duration: TICK_TIME * 2 });
+      expect(controller._executePoll).toHaveBeenCalledTimes(3);
+    });
+    it('should call _executePoll immediately once and continue calling _executePoll on interval when start is called again with the same networkClientId', async () => {


Nit: We know we are testing startPollingByNetworkClientId so we don't need to repeat the name of the method in the test:

Suggested change

it('should call _executePoll immediately once and continue calling _executePoll on interval when start is called again with the same networkClientId', async () => {

it('should call _executePoll immediately once and continue calling _executePoll on interval when called again with the same networkClientId', async () => {

done here: 823aaa1

mcmire · 2023-12-11T17:13:43Z

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

+    it('should error if no pollingToken is passed', () => {
+      controller.startPollingByNetworkClientId('mainnet');
+      expect(() => {
+        controller.stopPollingByPollingToken(undefined as unknown as any);


Instead of using any here (which doesn't say anything as to why this is necessary) what do you think about using @ts-expect-error:

Suggested change

controller.stopPollingByPollingToken(undefined as unknown as any);

// @ts-expect-error We're intentionally passing no polling token

controller.stopPollingByPollingToken();

done here: 823aaa1

mcmire · 2023-12-11T17:16:28Z

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

+      controller.stopAllPolling();
+    });
+
+    it('should publish "pollingComplete" when stop is called', async () => {


What are your thoughts on moving this test to a different describe for onPollingCompleteByNetworkClientId?

yep makes sense!

done here: 823aaa1

mcmire · 2023-12-11T17:18:11Z

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

+> {
+  _executePoll = createExecutePollMock();
+
+  _intervalLength = TICK_TIME;


There is no property called _intervalLength, so this has no effect. You can test this by changing TICK_TIME to something else like 500. Thoughts on overriding getIntervalLength instead?

yeah whoops this got left in by mistake. Why not just call the setIntervalLength?

Yeah, that would work too!

done here: 823aaa1

mcmire · 2023-12-11T17:18:55Z

packages/polling-controller/src/StaticIntervalPollingController.ts

+  {
+    readonly #intervalIds: Record<PollingTokenSetId, NodeJS.Timeout> = {};
+
+    intervalLength: number | undefined = 1000;


Since this property is currently public, you have two ways of accessing this property. This seems unnecessary. Thoughts on using #intervalLength instead of intervalLength?

(Note: if this is the way it was before, then fine, we can change this in another PR, but I thought I'd call it out anyway)

done here: 823aaa1

mcmire · 2023-12-11T17:19:29Z

packages/polling-controller/src/StaticIntervalPollingController.ts

+          }
+          this._startPollingByNetworkClientId(networkClientId, options);
+        },
+        existingInterval ? this.intervalLength : 0,


Thoughts on using getIntervalLength() so that when you mock this method in the tests it'll work across the board?

could do that... or just call setIntervalLength in the tests where you want o modify the default no?

Yes, that would work too.

done here: 823aaa1

mcmire

Looks good! Thanks for your patience on this.

adonesky1 mentioned this pull request Dec 7, 2023

PollingController: add option to poll by blockTracker #3623

Closed

jiexi reviewed Dec 7, 2023

View reviewed changes

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

jiexi reviewed Dec 7, 2023

View reviewed changes

mcmire reviewed Dec 7, 2023

View reviewed changes

MajorLift reviewed Dec 8, 2023

View reviewed changes

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

adonesky1 marked this pull request as ready for review December 8, 2023 20:44

adonesky1 requested a review from a team as a code owner December 8, 2023 20:44

jiexi reviewed Dec 8, 2023

View reviewed changes

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

jiexi reviewed Dec 8, 2023

View reviewed changes

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

jiexi reviewed Dec 8, 2023

View reviewed changes

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

jiexi reviewed Dec 8, 2023

View reviewed changes

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

jiexi reviewed Dec 8, 2023

View reviewed changes

adonesky1 added 15 commits December 8, 2023 15:49

PollingController: add option to poll by blockTracker

5064689

fix

394fcdd

adding tests

242252b

another test + cleanup

4a74704

add protection against multiple sources of polling interval

6698ebd

cleanup

8906fb8

small cleanups

8edcf25

move tests

f688425

reorganize new tests

f72c570

WIP

28a98fe

moving toward pure template pattern

1c297c2

wip template pattern complete for BlockTrackerPollingController, now …

9f0efe9

…to StaticIntervalPollingController

StaticIntervalPollingController wip

6404305

trying a different PollingTokenSetId type

d4992ff

change all instances over to new class

4b52a02

adonesky1 added 3 commits December 8, 2023 15:49

cleanup

b932365

cleanup

76449d6

break when pollingToken found whether or not its the last

5ccf0d3

adonesky1 force-pushed the generalize-polling-abstraction branch from 229f64a to 5ccf0d3 Compare December 8, 2023 21:49

legobeat reviewed Dec 9, 2023

View reviewed changes

add IPollingController type

997c806

adonesky1 requested review from mcmire, jiexi, MajorLift and legobeat December 11, 2023 16:24

adonesky1 added 2 commits December 11, 2023 10:32

Merge branch 'main' into generalize-polling-abstraction

ac31ea0

Merge branch 'main' into generalize-polling-abstraction

781c967

mcmire reviewed Dec 11, 2023

View reviewed changes

address feedback + cleanup

823aaa1

adonesky1 force-pushed the generalize-polling-abstraction branch from a9ea7e6 to 823aaa1 Compare December 11, 2023 19:01

adonesky1 requested a review from mcmire December 11, 2023 19:25

adonesky1 added 3 commits December 11, 2023 13:39

Merge branch 'main' into generalize-polling-abstraction

d7e9ba2

Merge branch 'main' into generalize-polling-abstraction

8a684fb

use StaticIntervalPollingController for PendingUserOperationTracker

071f73b

adonesky1 force-pushed the generalize-polling-abstraction branch from 14dde1c to 071f73b Compare December 11, 2023 23:09

adonesky1 added 4 commits December 11, 2023 17:09

Merge branch 'main' into generalize-polling-abstraction

8d5ff1e

Merge branch 'main' into generalize-polling-abstraction

d4ce6ac

update AccountTrackerController to use StaticIntervalPollingControllerV1

77f3d2e

Merge branch 'main' into generalize-polling-abstraction

34234c6

mcmire approved these changes Dec 12, 2023

View reviewed changes

adonesky1 merged commit 42d0b32 into main Dec 12, 2023
136 checks passed

adonesky1 deleted the generalize-polling-abstraction branch December 12, 2023 21:36

	const callbacks = this.#callbacks.get(key) \|\| new Set<typeof callback>();
	const callbacks = this.#callbacks.get(key) ?? new Set<typeof callback>();

	this.#pollingTokenSets.get(key) \|\| new Set<string>();
	this.#pollingTokenSets.get(key) ?? new Set<string>();

	it('should call _executePoll immediately once and continue calling _executePoll on interval when start is called again with the same networkClientId', async () => {
	it('should call _executePoll immediately once and continue calling _executePoll on interval when called again with the same networkClientId', async () => {

	controller.stopPollingByPollingToken(undefined as unknown as any);
	// @ts-expect-error We're intentionally passing no polling token
	controller.stopPollingByPollingToken();

Generalize polling abstraction #3636

Generalize polling abstraction #3636

Conversation

adonesky1 commented Dec 7, 2023 • edited Loading

@metamask/polling-controller

Removed

Added

Choose a reason for hiding this comment

jiexi commented Dec 7, 2023

mcmire left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthewwalsh0 commented Dec 8, 2023

adonesky1 commented Dec 8, 2023

jiexi Dec 8, 2023 • edited Loading

Choose a reason for hiding this comment

legobeat Dec 9, 2023 • edited Loading

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

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcmire Dec 11, 2023 • edited Loading

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

mcmire left a comment

Choose a reason for hiding this comment

adonesky1 commented Dec 7, 2023 •

edited

Loading

Removed 

Added 

jiexi Dec 8, 2023 •

edited

Loading

legobeat Dec 9, 2023 •

edited

Loading

mcmire Dec 11, 2023 •

edited

Loading