Skip to content

Generalize polling abstraction #3636

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

Merged
merged 29 commits into from
Dec 12, 2023
Merged
Show file tree
Hide file tree
Changes from 18 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
5064689
PollingController: add option to poll by blockTracker
adonesky1 Dec 6, 2023
394fcdd
fix
adonesky1 Dec 6, 2023
242252b
adding tests
adonesky1 Dec 6, 2023
4a74704
another test + cleanup
adonesky1 Dec 6, 2023
6698ebd
add protection against multiple sources of polling interval
adonesky1 Dec 6, 2023
8906fb8
cleanup
adonesky1 Dec 6, 2023
8edcf25
small cleanups
adonesky1 Dec 7, 2023
f688425
move tests
adonesky1 Dec 7, 2023
f72c570
reorganize new tests
adonesky1 Dec 7, 2023
28a98fe
WIP
adonesky1 Dec 7, 2023
1c297c2
moving toward pure template pattern
adonesky1 Dec 8, 2023
9f0efe9
wip template pattern complete for BlockTrackerPollingController, now …
adonesky1 Dec 8, 2023
6404305
StaticIntervalPollingController wip
adonesky1 Dec 8, 2023
d4992ff
trying a different PollingTokenSetId type
adonesky1 Dec 8, 2023
4b52a02
change all instances over to new class
adonesky1 Dec 8, 2023
b932365
cleanup
adonesky1 Dec 8, 2023
76449d6
cleanup
adonesky1 Dec 8, 2023
5ccf0d3
break when pollingToken found whether or not its the last
adonesky1 Dec 8, 2023
997c806
add IPollingController type
adonesky1 Dec 11, 2023
ac31ea0
Merge branch 'main' into generalize-polling-abstraction
adonesky1 Dec 11, 2023
781c967
Merge branch 'main' into generalize-polling-abstraction
adonesky1 Dec 11, 2023
823aaa1
address feedback + cleanup
adonesky1 Dec 11, 2023
d7e9ba2
Merge branch 'main' into generalize-polling-abstraction
adonesky1 Dec 11, 2023
8a684fb
Merge branch 'main' into generalize-polling-abstraction
adonesky1 Dec 11, 2023
071f73b
use StaticIntervalPollingController for PendingUserOperationTracker
adonesky1 Dec 11, 2023
8d5ff1e
Merge branch 'main' into generalize-polling-abstraction
adonesky1 Dec 11, 2023
d4ce6ac
Merge branch 'main' into generalize-polling-abstraction
adonesky1 Dec 12, 2023
77f3d2e
update AccountTrackerController to use StaticIntervalPollingControllerV1
adonesky1 Dec 12, 2023
34234c6
Merge branch 'main' into generalize-polling-abstraction
adonesky1 Dec 12, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions packages/assets-controllers/src/CurrencyRateController.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import type {
NetworkClientId,
NetworkControllerGetNetworkClientByIdAction,
} from '@metamask/network-controller';
import { PollingController } from '@metamask/polling-controller';
import { StaticIntervalPollingController } from '@metamask/polling-controller';
import { Mutex } from 'async-mutex';

import { fetchExchangeRate as defaultFetchExchangeRate } from './crypto-compare';
Expand Down Expand Up @@ -82,7 +82,7 @@ const defaultState = {
* Controller that passively polls on a set interval for an exchange rate from the current network
* asset to the user's preferred currency.
*/
export class CurrencyRateController extends PollingController<
export class CurrencyRateController extends StaticIntervalPollingController<
typeof name,
CurrencyRateState,
CurrencyRateMessenger
Expand Down
4 changes: 2 additions & 2 deletions packages/assets-controllers/src/NftDetectionController.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import type {
NetworkState,
NetworkClient,
} from '@metamask/network-controller';
import { PollingControllerV1 } from '@metamask/polling-controller';
import { StaticIntervalPollingControllerV1 } from '@metamask/polling-controller';
import type { PreferencesState } from '@metamask/preferences-controller';
import type { Hex } from '@metamask/utils';

Expand Down Expand Up @@ -147,7 +147,7 @@ export interface NftDetectionConfig extends BaseConfig {
/**
* Controller that passively polls on a set interval for NFT auto detection
*/
export class NftDetectionController extends PollingControllerV1<
export class NftDetectionController extends StaticIntervalPollingControllerV1<
NftDetectionConfig,
BaseState
> {
Expand Down
4 changes: 2 additions & 2 deletions packages/assets-controllers/src/TokenDetectionController.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import type {
NetworkController,
NetworkState,
} from '@metamask/network-controller';
import { PollingControllerV1 } from '@metamask/polling-controller';
import { StaticIntervalPollingControllerV1 } from '@metamask/polling-controller';
import type { PreferencesState } from '@metamask/preferences-controller';
import type { Hex } from '@metamask/utils';

Expand Down Expand Up @@ -44,7 +44,7 @@ export interface TokenDetectionConfig extends BaseConfig {
/**
* Controller that passively polls on a set interval for Tokens auto detection
*/
export class TokenDetectionController extends PollingControllerV1<
export class TokenDetectionController extends StaticIntervalPollingControllerV1<
TokenDetectionConfig,
BaseState
> {
Expand Down
4 changes: 2 additions & 2 deletions packages/assets-controllers/src/TokenListController.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import type {
NetworkState,
NetworkControllerGetNetworkClientByIdAction,
} from '@metamask/network-controller';
import { PollingController } from '@metamask/polling-controller';
import { StaticIntervalPollingController } from '@metamask/polling-controller';
import type { Hex } from '@metamask/utils';
import { Mutex } from 'async-mutex';

Expand Down Expand Up @@ -91,7 +91,7 @@ const defaultState: TokenListState = {
/**
* Controller that passively polls on a set interval for the list of tokens from metaswaps api
*/
export class TokenListController extends PollingController<
export class TokenListController extends StaticIntervalPollingController<
typeof name,
TokenListState,
TokenListMessenger
Expand Down
4 changes: 2 additions & 2 deletions packages/assets-controllers/src/TokenRatesController.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import type {
NetworkController,
NetworkState,
} from '@metamask/network-controller';
import { PollingControllerV1 } from '@metamask/polling-controller';
import { StaticIntervalPollingControllerV1 } from '@metamask/polling-controller';
import type { PreferencesState } from '@metamask/preferences-controller';
import type { Hex } from '@metamask/utils';

Expand Down Expand Up @@ -135,7 +135,7 @@ async function getCurrencyConversionRate({
* Controller that passively polls on a set interval for token-to-fiat exchange rates
* for tokens stored in the TokensController
*/
export class TokenRatesController extends PollingControllerV1<
export class TokenRatesController extends StaticIntervalPollingControllerV1<
TokenRatesConfig,
TokenRatesState
> {
Expand Down
4 changes: 2 additions & 2 deletions packages/gas-fee-controller/src/GasFeeController.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ import type {
NetworkState,
ProviderProxy,
} from '@metamask/network-controller';
import { PollingController } from '@metamask/polling-controller';
import { StaticIntervalPollingController } from '@metamask/polling-controller';
import type { Hex } from '@metamask/utils';
import { v1 as random } from 'uuid';

Expand Down Expand Up @@ -253,7 +253,7 @@ const defaultState: GasFeeState = {
/**
* Controller that retrieves gas fee estimate data and polls for updated data on a set interval
*/
export class GasFeeController extends PollingController<
export class GasFeeController extends StaticIntervalPollingController<
typeof name,
GasFeeState,
GasFeeMessenger
Expand Down
111 changes: 111 additions & 0 deletions packages/polling-controller/src/AbstractPollingController.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,111 @@
import type { NetworkClientId } from '@metamask/network-controller';
import type { Json } from '@metamask/utils';
import stringify from 'fast-json-stable-stringify';
import { v4 as random } from 'uuid';

export const getKey = (
networkClientId: NetworkClientId,
options: Json,
): PollingTokenSetId => `${networkClientId}:${stringify(options)}`;

export type PollingTokenSetId = `${NetworkClientId}:${string}`;

type Constructor = new (...args: any[]) => object;

/**
* AbstractPollingControllerBaseMixin
*
* @param Base - The base class to mix onto.
* @returns The composed class.
*/
export function AbstractPollingControllerBaseMixin<TBase extends Constructor>(
Base: TBase,
) {
abstract class AbstractPollingControllerBase extends Base {
Copy link
Contributor

@legobeat legobeat Dec 9, 2023

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done here 997c806

readonly #pollingTokenSets: Map<PollingTokenSetId, Set<string>> = new Map();

#callbacks: Map<
PollingTokenSetId,
Set<(PollingTokenSetId: PollingTokenSetId) => void>
> = new Map();

abstract _executePoll(
networkClientId: NetworkClientId,
options: Json,
): Promise<void>;

abstract _startPollingByNetworkClientId(
Copy link
Contributor

Choose a reason for hiding this comment

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

What are your thoughts on making this protected? This way we don't have to prepend it with an underscore. Same goes for _stopPollingByPollingTokenSetId.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The mixin pattern where we expose this class via function doesn't seem to like any private or protected methods in the exposed class...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems like maybe there are ways to get around it but I'm not sure if we want to pursue them if its not recommended.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, strange. Okay, no problem, then.

networkClientId: NetworkClientId,
options: Json,
): void;

abstract _stopPollingByPollingTokenSetId(key: PollingTokenSetId): void;

startPollingByNetworkClientId(
networkClientId: NetworkClientId,
options: Json = {},
): string {
const pollToken = random();
const key = getKey(networkClientId, options);
const pollingTokenSet =
this.#pollingTokenSets.get(key) || new Set<string>();
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: ?? would be safer:

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done here: 823aaa1

pollingTokenSet.add(pollToken);
this.#pollingTokenSets.set(key, pollingTokenSet);

if (pollingTokenSet.size === 1) {
// Start new polling only if it's the first token for this key
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: This comment repeats what the code already says:

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done here: 823aaa1

this._startPollingByNetworkClientId(networkClientId, options);
}

return pollToken;
}

stopAllPolling() {
this.#pollingTokenSets.forEach((tokenSet, _key) => {
tokenSet.forEach((token) => {
this.stopPollingByPollingToken(token);
});
});
}

stopPollingByPollingToken(pollingToken: string) {
if (!pollingToken) {
throw new Error('pollingToken required');
}

let keyToDelete: PollingTokenSetId | null = null;
for (const [key, tokenSet] of this.#pollingTokenSets) {
if (tokenSet.delete(pollingToken)) {
if (tokenSet.size === 0) {
keyToDelete = key;
}
break;
}
}

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();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

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();
}
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

gotcha! Thanks for clearing that up!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done here: 823aaa1

}

onPollingCompleteByNetworkClientId(
networkClientId: NetworkClientId,
callback: (networkClientId: NetworkClientId) => void,
options: Json = {},
) {
const key = getKey(networkClientId, options);
const callbacks = this.#callbacks.get(key) || new Set<typeof callback>();
Copy link
Contributor

Choose a reason for hiding this comment

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

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>();

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done here: 823aaa1

callbacks.add(callback);
this.#callbacks.set(key, callbacks);
}
}
return AbstractPollingControllerBase;
}
Loading