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.

Sign up for GitHub

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

Jump to bottom

feat: Integrate new TokenListController polling pattern [Core] #4878

Merged
merged 14 commits into from
Nov 5, 2024
Merged

feat: Integrate new TokenListController polling pattern [Core] #4878

Changes from 1 commit
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
3831038
feat: Init polling controller updates
gambinish Oct 31, 2024
6a93137
chore: Revive NetworkController subscriptions and chainId constructor…
gambinish Oct 31, 2024
f1d1588
fix: ensure that chainId is passed to fetchTokenList in order to upda…
gambinish Oct 31, 2024
31b15e6
chore: Cleanup single return update of fetchTokenList
gambinish Oct 31, 2024
0b7230c
chore: Cleanup
gambinish Oct 31, 2024
3a3393b
chore: Specify which methods should eventually be deprecated
gambinish Oct 31, 2024
4dca721
fix: Temporarily comment out failing unit tests to test CI
gambinish Oct 31, 2024
98ccc21
fix: Deprecate old startPolling method, inherit interval and set from…
gambinish Nov 1, 2024
3399c81
Merge branch 'main' into 3429--tokenlist-polling-controller
gambinish Nov 4, 2024
b5122cf
chore: Uncomment failed unit tests
gambinish Nov 4, 2024
94afe43
fix: Update unit tests
gambinish Nov 5, 2024
5003331
fix: Fix unit tests, remove uneccessary ones
gambinish Nov 5, 2024
22f474a
Merge branch 'main' into 3429--tokenlist-polling-controller
gambinish Nov 5, 2024
e3b867f
Merge branch 'main' into 3429--tokenlist-polling-controller
gambinish Nov 5, 2024
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
102 changes: 17 additions & 85 deletions packages/assets-controllers/src/TokenListController.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,7 @@ import type {
} from '@metamask/base-controller';
import { safelyExecute } from '@metamask/controller-utils';
import type {
NetworkClientId,
NetworkControllerStateChangeEvent,
NetworkState,
NetworkControllerGetNetworkClientByIdAction,
} from '@metamask/network-controller';
import { StaticIntervalPollingController } from '@metamask/polling-controller';
Expand Down Expand Up @@ -94,7 +92,7 @@ export const getDefaultTokenListState = (): TokenListState => {

/** The input to start polling for the {@link TokenListController} */
type TokenListPollingInput = {
networkClientId: NetworkClientId;
chainId: Hex;
};

/**
Expand All @@ -113,36 +111,26 @@ export class TokenListController extends StaticIntervalPollingController<TokenLi

private readonly cacheRefreshThreshold: number;

private chainId: Hex;
gambinish marked this conversation as resolved.
Show resolved Hide resolved

private abortController: AbortController;
private readonly abortController: AbortController;

/**
* Creates a TokenListController instance.
*
* @param options - The controller options.
* @param options.chainId - The chain ID of the current network.
* @param options.onNetworkStateChange - A function for registering an event handler for network state changes.
* @param options.interval - The polling interval, in milliseconds.
* @param options.cacheRefreshThreshold - The token cache expiry time, in milliseconds.
* @param options.messenger - A restricted controller messenger.
* @param options.state - Initial state to set on this controller.
* @param options.preventPollingOnNetworkRestart - Determines whether to prevent poilling on network restart in extension.
*/
constructor({
chainId,
preventPollingOnNetworkRestart = false,
onNetworkStateChange,
interval = DEFAULT_INTERVAL,
cacheRefreshThreshold = DEFAULT_THRESHOLD,
messenger,
state,
}: {
chainId: Hex;
preventPollingOnNetworkRestart?: boolean;
onNetworkStateChange?: (
listener: (networkState: NetworkState) => void,
) => void;
interval?: number;
cacheRefreshThreshold?: number;
messenger: TokenListControllerMessenger;
Expand All @@ -156,75 +144,26 @@ export class TokenListController extends StaticIntervalPollingController<TokenLi
});
this.intervalDelay = interval;
this.cacheRefreshThreshold = cacheRefreshThreshold;
this.chainId = chainId;
this.updatePreventPollingOnNetworkRestart(preventPollingOnNetworkRestart);
this.abortController = new AbortController();
if (onNetworkStateChange) {
// TODO: Either fix this lint violation or explain why it's necessary to ignore.
// eslint-disable-next-line @typescript-eslint/no-misused-promises
onNetworkStateChange(async (networkControllerState) => {
await this.#onNetworkControllerStateChange(networkControllerState);
});
} else {
this.messagingSystem.subscribe(
'NetworkController:stateChange',
// TODO: Either fix this lint violation or explain why it's necessary to ignore.
// eslint-disable-next-line @typescript-eslint/no-misused-promises
async (networkControllerState) => {
await this.#onNetworkControllerStateChange(networkControllerState);
},
);
}
}

/**
* Updates state and restarts polling on changes to the network controller
* state.
*
* @param networkControllerState - The updated network controller state.
*/
async #onNetworkControllerStateChange(networkControllerState: NetworkState) {
const selectedNetworkClient = this.messagingSystem.call(
'NetworkController:getNetworkClientById',
networkControllerState.selectedNetworkClientId,
);
const { chainId } = selectedNetworkClient.configuration;

if (this.chainId !== chainId) {
this.abortController.abort();
this.abortController = new AbortController();
this.chainId = chainId;
if (this.state.preventPollingOnNetworkRestart) {
this.clearingTokenListData();
} else {
// Ensure tokenList is referencing data from correct network
this.update(() => {
return {
...this.state,
tokenList: this.state.tokensChainsCache[this.chainId]?.data || {},
};
});
await this.restart();
}
}
gambinish marked this conversation as resolved.
Show resolved Hide resolved
}

/**
* Start polling for the token list.
*/
async start() {
if (!isTokenListSupportedForNetwork(this.chainId)) {
gambinish marked this conversation as resolved.
Show resolved Hide resolved
if (!isTokenListSupportedForNetwork(pollingInput.chainId)) {
return;
}
await this.#startPolling();
await this.#startPolling(pollingInput.chainId);
}

/**
* Restart polling for the token list.
*/
async restart() {
this.stopPolling();
await this.#startPolling();
await this.#startPolling(pollingInput.chainId);
}

/**
Expand All @@ -251,14 +190,16 @@ export class TokenListController extends StaticIntervalPollingController<TokenLi
}

/**
* Starts a new polling interval.
* Starts a new polling interval for a given chainId
* @param input - The input for the poll.
* @param input.chainId - The chainId of the chain to trigger the fetch.
*/
async #startPolling(): Promise<void> {
await safelyExecute(() => this.fetchTokenList());
async #startPolling({ chainId }: TokenListPollingInput): Promise<void> {
await safelyExecute(() => this._executePoll({ chainId }));
// TODO: Either fix this lint violation or explain why it's necessary to ignore.
// eslint-disable-next-line @typescript-eslint/no-misused-promises
this.intervalId = setInterval(async () => {
await safelyExecute(() => this.fetchTokenList());
await safelyExecute(() => this._executePoll({ chainId }));
}, this.intervalDelay);
}

Expand All @@ -267,30 +208,21 @@ export class TokenListController extends StaticIntervalPollingController<TokenLi
*
* @private
* @param input - The input for the poll.
* @param input.networkClientId - The ID of the network client triggering the fetch.
* @param input.chainId - The chainId of the chain to trigger the fetch.
* @returns A promise that resolves when this operation completes.
*/
async _executePoll({
networkClientId,
}: TokenListPollingInput): Promise<void> {
return this.fetchTokenList(networkClientId);
async _executePoll({ chainId }: TokenListPollingInput): Promise<void> {
return this.fetchTokenList(chainId);
}

/**
* Fetching token list from the Token Service API.
*
* @param networkClientId - The ID of the network client triggering the fetch.
* @param chainId - The chainId of the network client triggering the fetch.
*/
async fetchTokenList(networkClientId?: NetworkClientId): Promise<void> {
async fetchTokenList(chainId: Hex): Promise<void> {
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 continuing to take a network client ID until we remove the global network completely? My thought is that clients might find it easier to pass this instead of a chain ID. The selected network client ID can be easily obtained from NetworkController state as selectedNetworkClientId and accessed within a Redux selector, while the client must obtain the chain ID by either doing the same thing that's happening here (use the messenger to get the network client by its ID, then get the chain ID that corresponds to it), or if within a Redux selector, loop through the networkConfigurationsByChainId to find the network configuration that matches the selectedNetworkClientId and then get the chainId off of that.

Ultimately I guess it depends on what the clients are currently doing. If we already have a way of accessing the current chain ID in a more or les type-safe way, then no worries. So, maybe this is change is just fine, but I wanted to double-check?

Copy link
Contributor Author

@gambinish gambinish Nov 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I think that the move towards passing in the chain ID, rather than an networkClientId was an intentional one. You're right, it's one of the steps towards removing the global network selector entirely. Yes, the clients are currently being refactored to now execute a single poll per ChainId.

I'll defer to Brian here for more on these design decisions, but the pattern with the other AssetsControllers are also moving towards this same way of passing chainID instead of networkClientId.

Copy link
Contributor

Choose a reason for hiding this comment

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

We're going to fetch token lists across all chains rather than just the current chain, so from the perspective of token lists we already no longer care about the current network. It'll just do Object.keys(networkConfigurationsByChainId) and fetch for each chain id. So the chain id input is already more convenient.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh okay! Sounds good then. Thanks for the reply.

const releaseLock = await this.mutex.acquire();
let networkClient;
if (networkClientId) {
networkClient = this.messagingSystem.call(
'NetworkController:getNetworkClientById',
networkClientId,
);
}
const chainId = networkClient?.configuration.chainId ?? this.chainId;

try {
const { tokensChainsCache } = this.state;
let tokenList: TokenListMap = {};
Expand Down
Loading