Skip to content
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

Generalize polling abstraction #3636

Merged
merged 29 commits into from
Dec 12, 2023
Merged
Show file tree
Hide file tree
Changes from 21 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
16 changes: 10 additions & 6 deletions packages/assets-controllers/src/CurrencyRateController.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,8 @@ import type {
NetworkClientId,
NetworkControllerGetNetworkClientByIdAction,
} from '@metamask/network-controller';
import { PollingController } from '@metamask/polling-controller';
import type { IPollingController } 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,11 +83,14 @@ 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<
typeof name,
CurrencyRateState,
CurrencyRateMessenger
> {
export class CurrencyRateController
extends StaticIntervalPollingController<
typeof name,
CurrencyRateState,
CurrencyRateMessenger
>
implements IPollingController
adonesky1 marked this conversation as resolved.
Show resolved Hide resolved
{
private readonly mutex = new Mutex();

private readonly fetchExchangeRate;
Expand Down
11 changes: 6 additions & 5 deletions packages/assets-controllers/src/NftDetectionController.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,8 @@ import type {
NetworkState,
NetworkClient,
} from '@metamask/network-controller';
import { PollingControllerV1 } from '@metamask/polling-controller';
import type { IPollingController } 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,10 +148,10 @@ export interface NftDetectionConfig extends BaseConfig {
/**
* Controller that passively polls on a set interval for NFT auto detection
*/
export class NftDetectionController extends PollingControllerV1<
NftDetectionConfig,
BaseState
> {
export class NftDetectionController
extends StaticIntervalPollingControllerV1<NftDetectionConfig, BaseState>
implements IPollingController
{
private intervalId?: ReturnType<typeof setTimeout>;

private getOwnerNftApi({
Expand Down
11 changes: 6 additions & 5 deletions packages/assets-controllers/src/TokenDetectionController.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,8 @@ import type {
NetworkController,
NetworkState,
} from '@metamask/network-controller';
import { PollingControllerV1 } from '@metamask/polling-controller';
import type { IPollingController } 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,10 +45,10 @@ export interface TokenDetectionConfig extends BaseConfig {
/**
* Controller that passively polls on a set interval for Tokens auto detection
*/
export class TokenDetectionController extends PollingControllerV1<
TokenDetectionConfig,
BaseState
> {
export class TokenDetectionController
extends StaticIntervalPollingControllerV1<TokenDetectionConfig, BaseState>
implements IPollingController
{
private intervalId?: ReturnType<typeof setTimeout>;

/**
Expand Down
16 changes: 10 additions & 6 deletions packages/assets-controllers/src/TokenListController.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,8 @@ import type {
NetworkState,
NetworkControllerGetNetworkClientByIdAction,
} from '@metamask/network-controller';
import { PollingController } from '@metamask/polling-controller';
import type { IPollingController } 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,11 +92,14 @@ const defaultState: TokenListState = {
/**
* Controller that passively polls on a set interval for the list of tokens from metaswaps api
*/
export class TokenListController extends PollingController<
typeof name,
TokenListState,
TokenListMessenger
> {
export class TokenListController
extends StaticIntervalPollingController<
typeof name,
TokenListState,
TokenListMessenger
>
implements IPollingController
{
private readonly mutex = new Mutex();

private intervalId?: ReturnType<typeof setTimeout>;
Expand Down
11 changes: 6 additions & 5 deletions packages/assets-controllers/src/TokenRatesController.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,8 @@ import type {
NetworkController,
NetworkState,
} from '@metamask/network-controller';
import { PollingControllerV1 } from '@metamask/polling-controller';
import type { IPollingController } from '@metamask/polling-controller';
import { StaticIntervalPollingControllerV1 } from '@metamask/polling-controller';
import type { PreferencesState } from '@metamask/preferences-controller';
import type { Hex } from '@metamask/utils';
import { isDeepStrictEqual } from 'util';
Expand Down Expand Up @@ -136,10 +137,10 @@ 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<
TokenRatesConfig,
TokenRatesState
> {
export class TokenRatesController
extends StaticIntervalPollingControllerV1<TokenRatesConfig, TokenRatesState>
implements IPollingController
{
private handle?: ReturnType<typeof setTimeout>;

#pollState = PollState.Inactive;
Expand Down
16 changes: 10 additions & 6 deletions packages/gas-fee-controller/src/GasFeeController.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,8 @@ import type {
NetworkState,
ProviderProxy,
} from '@metamask/network-controller';
import { PollingController } from '@metamask/polling-controller';
import type { IPollingController } 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,11 +254,14 @@ const defaultState: GasFeeState = {
/**
* Controller that retrieves gas fee estimate data and polls for updated data on a set interval
*/
export class GasFeeController extends PollingController<
typeof name,
GasFeeState,
GasFeeMessenger
> {
export class GasFeeController
extends StaticIntervalPollingController<
typeof name,
GasFeeState,
GasFeeMessenger
>
implements IPollingController
{
private intervalId?: ReturnType<typeof setTimeout>;

private readonly intervalDelay;
Expand Down
138 changes: 138 additions & 0 deletions packages/polling-controller/src/AbstractPollingController.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,138 @@
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 type IPollingController = {
startPollingByNetworkClientId(
networkClientId: NetworkClientId,
options: Json,
): string;

stopAllPolling(): void;

stopPollingByPollingToken(pollingToken: string): void;

onPollingCompleteByNetworkClientId(
networkClientId: NetworkClientId,
callback: (networkClientId: NetworkClientId) => void,
options: Json,
): void;

_executePoll(networkClientId: NetworkClientId, options: Json): Promise<void>;
_startPollingByNetworkClientId(
networkClientId: NetworkClientId,
options: Json,
): void;
_stopPollingByPollingTokenSetId(key: PollingTokenSetId): void;
};

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
implements IPollingController
{
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;
adonesky1 marked this conversation as resolved.
Show resolved Hide resolved
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
Loading