feat: add rate limiting support to controllers leveraging the accounts API #6066
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
|
|
@@ -33,6 +33,7 @@ import type { | |||
| PreferencesControllerGetStateAction, | ||||
| PreferencesControllerStateChangeEvent, | ||||
| } from '@metamask/preferences-controller'; | ||||
| import type { AuthenticationControllerGetBearerToken } from '@metamask/profile-sync-controller/auth'; | ||||
| import type { TransactionControllerTransactionConfirmedEvent } from '@metamask/transaction-controller'; | ||||
| import type { Hex } from '@metamask/utils'; | ||||
| import { hexToNumber } from '@metamask/utils'; | ||||
|
|
@@ -93,6 +94,7 @@ export const STATIC_MAINNET_TOKEN_LIST = Object.entries<LegacyToken>( | |||
|
|
||||
| /** | ||||
| * Function that takes a TokensChainsCache object and maps chainId with TokenListMap. | ||||
| * | ||||
| * @param tokensChainsCache - TokensChainsCache input object | ||||
| * @returns returns the map of chainId with TokenListMap | ||||
| */ | ||||
|
|
@@ -129,7 +131,8 @@ export type AllowedActions = | |||
| | KeyringControllerGetStateAction | ||||
| | PreferencesControllerGetStateAction | ||||
| | TokensControllerGetStateAction | ||||
| | TokensControllerAddDetectedTokensAction | ||||
| | AuthenticationControllerGetBearerToken; | ||||
|
|
||||
| export type TokenDetectionControllerStateChangeEvent = | ||||
| ControllerStateChangeEvent<typeof controllerName, TokenDetectionState>; | ||||
|
|
@@ -162,13 +165,20 @@ type TokenDetectionPollingInput = { | |||
|
|
||||
| /** | ||||
| * Controller that passively polls on a set interval for Tokens auto detection | ||||
| * | ||||
| * intervalId - Polling interval used to fetch new token rates | ||||
| * | ||||
| * selectedAddress - Vault selected address | ||||
| * | ||||
| * networkClientId - The network client ID of the current selected network | ||||
| * | ||||
| * disabled - Boolean to track if network requests are blocked | ||||
| * | ||||
| * isUnlocked - Boolean to track if the keyring state is unlocked | ||||
| * | ||||
| * isDetectionEnabledFromPreferences - Boolean to track if detection is enabled from PreferencesController | ||||
| * | ||||
| * isDetectionEnabledForNetwork - Boolean to track if detected is enabled for current network | ||||
| */ | ||||
| export class TokenDetectionController extends StaticIntervalPollingController<TokenDetectionPollingInput>()< | ||||
| typeof controllerName, | ||||
|
|
@@ -179,7 +189,7 @@ export class TokenDetectionController extends StaticIntervalPollingController<To | |||
|
|
||||
| #selectedAccountId: string; | ||||
|
|
||||
| readonly #networkClientId: NetworkClientId; | ||||
|
|
||||
| #tokensChainsCache: TokensChainsCache = {}; | ||||
|
|
||||
|
|
@@ -189,7 +199,7 @@ export class TokenDetectionController extends StaticIntervalPollingController<To | |||
|
|
||||
| #isDetectionEnabledFromPreferences: boolean; | ||||
|
|
||||
| readonly #isDetectionEnabledForNetwork: boolean; | ||||
|
|
||||
| readonly #getBalancesInSingleCall: AssetsContractController['getBalancesInSingleCall']; | ||||
|
|
||||
|
|
@@ -199,20 +209,24 @@ export class TokenDetectionController extends StaticIntervalPollingController<To | |||
| properties: { | ||||
| tokens: string[]; | ||||
| // TODO: Either fix this lint violation or explain why it's necessary to ignore. | ||||
|
|
||||
| token_standard: string; | ||||
| // TODO: Either fix this lint violation or explain why it's necessary to ignore. | ||||
|
|
||||
| asset_type: string; | ||||
| }; | ||||
| }) => void; | ||||
|
|
||||
| readonly #accountsAPI = { | ||||
| isAccountsAPIEnabled: true, | ||||
| supportedNetworksCache: null as number[] | null, | ||||
| platform: '' as 'extension' | 'mobile', | ||||
|
|
||||
| async getSupportedNetworks(options: { | ||||
| getAuthenticationControllerBearerToken: () => ReturnType< | ||||
| AuthenticationControllerGetBearerToken['handler'] | ||||
| >; | ||||
| }) { | ||||
| /* istanbul ignore next */ | ||||
| if (!this.isAccountsAPIEnabled) { | ||||
| throw new Error('Accounts API Feature Switch is disabled'); | ||||
|
|
@@ -223,7 +237,11 @@ export class TokenDetectionController extends StaticIntervalPollingController<To | |||
| return this.supportedNetworksCache; | ||||
| } | ||||
|
|
||||
| const { getAuthenticationControllerBearerToken } = options; | ||||
|
|
||||
| const result = await fetchSupportedNetworks({ | ||||
| getAuthenticationControllerBearerToken, | ||||
| }).catch(() => null); | ||||
| this.supportedNetworksCache = result; | ||||
| return result; | ||||
| }, | ||||
|
|
@@ -232,7 +250,13 @@ export class TokenDetectionController extends StaticIntervalPollingController<To | |||
| address: string, | ||||
| chainIds: Hex[], | ||||
| supportedNetworks: number[] | null, | ||||
| options: { | ||||
| getAuthenticationControllerBearerToken: () => ReturnType< | ||||
| AuthenticationControllerGetBearerToken['handler'] | ||||
| >; | ||||
| }, | ||||
| ) { | ||||
| const { getAuthenticationControllerBearerToken } = options; | ||||
| const chainIdNumbers = chainIds.map((chainId) => hexToNumber(chainId)); | ||||
|
|
||||
| if ( | ||||
|
|
@@ -249,6 +273,7 @@ export class TokenDetectionController extends StaticIntervalPollingController<To | |||
| address, | ||||
| { | ||||
| networks: chainIdNumbers, | ||||
| getAuthenticationControllerBearerToken, | ||||
| }, | ||||
| this.platform, | ||||
| ); | ||||
|
|
@@ -287,10 +312,10 @@ export class TokenDetectionController extends StaticIntervalPollingController<To | |||
| properties: { | ||||
| tokens: string[]; | ||||
| // TODO: Either fix this lint violation or explain why it's necessary to ignore. | ||||
|
|
||||
| token_standard: string; | ||||
| // TODO: Either fix this lint violation or explain why it's necessary to ignore. | ||||
|
|
||||
| asset_type: string; | ||||
| }; | ||||
| }) => void; | ||||
|
|
@@ -439,7 +464,8 @@ export class TokenDetectionController extends StaticIntervalPollingController<To | |||
|
|
||||
| /** | ||||
| * Internal isActive state | ||||
| * | ||||
| * @returns boolean indicating if the controller is active | ||||
| */ | ||||
| get isActive(): boolean { | ||||
| return !this.#disabled && this.#isUnlocked; | ||||
|
|
@@ -485,6 +511,7 @@ export class TokenDetectionController extends StaticIntervalPollingController<To | |||
|
|
||||
| /** | ||||
| * Compares current and previous tokensChainsCache object focusing only on the data object. | ||||
| * | ||||
| * @param tokensChainsCache - current tokensChainsCache input object | ||||
| * @param previousTokensChainsCache - previous tokensChainsCache input object | ||||
| * @returns boolean indicating if the two objects are equal | ||||
|
|
@@ -711,7 +738,10 @@ export class TokenDetectionController extends StaticIntervalPollingController<To | |||
|
|
||||
| let supportedNetworks; | ||||
| if (this.#accountsAPI.isAccountsAPIEnabled) { | ||||
| supportedNetworks = await this.#accountsAPI.getSupportedNetworks({ | ||||
| getAuthenticationControllerBearerToken: | ||||
| this.#getAuthenticationControllerBearerToken.bind(this), | ||||
| }); | ||||
| } | ||||
| const { chainsToDetectUsingRpc, chainsToDetectUsingAccountAPI } = | ||||
| this.#getChainsToDetect(clientNetworks, supportedNetworks); | ||||
|
|
@@ -812,6 +842,7 @@ export class TokenDetectionController extends StaticIntervalPollingController<To | |||
|
|
||||
| /** | ||||
| * This adds detected tokens from the Accounts API, avoiding the multi-call RPC calls for balances | ||||
| * | ||||
| * @param options - method arguments | ||||
| * @param options.selectedAddress - address to check against | ||||
| * @param options.chainIds - array of chainIds to check tokens for | ||||
|
|
@@ -830,7 +861,15 @@ export class TokenDetectionController extends StaticIntervalPollingController<To | |||
| return await safelyExecute(async () => { | ||||
| // Fetch balances for multiple chain IDs at once | ||||
| const tokenBalancesByChain = await this.#accountsAPI | ||||
| .getMultiNetworksBalances( | ||||
| selectedAddress, | ||||
| chainIds, | ||||
| supportedNetworks, | ||||
| { | ||||
| getAuthenticationControllerBearerToken: | ||||
| this.#getAuthenticationControllerBearerToken.bind(this), | ||||
| }, | ||||
| ) | ||||
| .catch(() => null); | ||||
|
|
||||
| if (tokenBalancesByChain === null) { | ||||
|
|
@@ -879,10 +918,10 @@ export class TokenDetectionController extends StaticIntervalPollingController<To | |||
| properties: { | ||||
| tokens: eventTokensDetails, | ||||
| // TODO: Either fix this lint violation or explain why it's necessary to ignore. | ||||
|
There was a problem hiding this comment. Should we remove this comment too?
Suggested change
|
||||
|
|
||||
| token_standard: ERC20, | ||||
| // TODO: Either fix this lint violation or explain why it's necessary to ignore. | ||||
|
There was a problem hiding this comment. Should we remove this comment too?
Suggested change
|
||||
|
|
||||
| asset_type: ASSET_TYPES.TOKEN, | ||||
| }, | ||||
| }); | ||||
|
|
@@ -904,6 +943,7 @@ export class TokenDetectionController extends StaticIntervalPollingController<To | |||
|
|
||||
| /** | ||||
| * Helper function to filter and build token data for detected tokens | ||||
| * | ||||
| * @param options.tokenCandidateSlices - these are tokens we know a user does not have (by checking the tokens controller). | ||||
| * We will use these these token candidates to determine if a token found from the API is valid to be added on the users wallet. | ||||
| * It will also prevent us to adding tokens a user already has | ||||
|
|
@@ -1009,10 +1049,10 @@ export class TokenDetectionController extends StaticIntervalPollingController<To | |||
| properties: { | ||||
| tokens: eventTokensDetails, | ||||
| // TODO: Either fix this lint violation or explain why it's necessary to ignore. | ||||
|
There was a problem hiding this comment. Should we remove this comment too?
Suggested change
|
||||
|
|
||||
| token_standard: ERC20, | ||||
| // TODO: Either fix this lint violation or explain why it's necessary to ignore. | ||||
|
There was a problem hiding this comment. Should we remove this comment too?
Suggested change
|
||||
|
|
||||
| asset_type: ASSET_TYPES.TOKEN, | ||||
| }, | ||||
| }); | ||||
|
|
@@ -1041,6 +1081,14 @@ export class TokenDetectionController extends StaticIntervalPollingController<To | |||
| ); | ||||
| return account?.address || ''; | ||||
| } | ||||
|
|
||||
| async #getAuthenticationControllerBearerToken(): ReturnType< | ||||
| AuthenticationControllerGetBearerToken['handler'] | ||||
| > { | ||||
| return await this.messagingSystem.call( | ||||
| 'AuthenticationController:getBearerToken', | ||||
|
There was a problem hiding this comment. If all these controllers ultimately call the same action / method, why can't this just be the default where needed in a central accounts API service to avoid all these breaking changes and new dependencies? Maybe in Or even a util such as Or could the clients just implement a proxy to inject this token automatically to avoid any core changes at all and decouple all this from the controllers? There was a problem hiding this comment. Great callout. Do we only use the Accounts API in the accounts controller? There was a problem hiding this comment. I think multiple controllers have their own partial abstractions over the accounts API. My fear is if we keep casually adding peer dependencies between the controllers, we make the release process harder, breaking changes more common, and eventually we'll just have one interdependent bundle meaning no single controller can have a major release without it cascading through the others. There was a problem hiding this comment. @matthewwalsh0 I'm not sure I understand your comment correctly 😄 AFAIK, LMK if that clarifies it - or if I completely missed your point! There was a problem hiding this comment. Apologies, I initially mentioned the AccountsController by mistake, but updated it to clarify how we could abstract this complexity in various ways. There was a problem hiding this comment. Exactly, right now it's a lot of duplication and new dependencies on three new controllers, to ultimately do the same logic. So a central mechanism would be great, either client proxy, util, or shared service. There was a problem hiding this comment. Thanks everyone for your input! It's just unfortunate that I'll need to juggle around 3 different fetch implementations 😅
There was a problem hiding this comment. How do we feel about the proxy / hook idea, to automatically inject this from any client request, meaning no controller changes needed at all? And rather the clients themselves are conceptually "authorised" and so any requests they generate are automatically authorised also? There was a problem hiding this comment. What would automatically injecting the token from any client request look like? Why would it mean no controller changes? There was a problem hiding this comment. Instead of modifying individual service/controller functions. We can also use a rule-based fetch interceptor that automatically adds authentication headers to matching URLs. Note: The interceptor below only works with globalThis.fetch, so it has limitations when used with other HTTP clients. Additionally, it may need to be adapted to support both extension and mobile environments. We should also consider addressing known issues related to differences in polyfill timing, global scope variations, and Metro bundler behavior. The example is only for idea sharing. // packages/controller-utils/src/fetch-interceptor.ts
type AuthRule = {
pattern: string | RegExp;
getToken: () => Promise<string>;
headerName?: string;
enabled?: boolean;
};
class FetchInterceptor {
private authRules: AuthRule[] = [];
private originalFetch: typeof globalThis.fetch;
private isSetup = false;
constructor() {
this.originalFetch = globalThis.fetch;
}
setup() {
if (this.isSetup) return;
globalThis.fetch = async (input: RequestInfo | URL, init?: RequestInit) => {
const url = this.extractUrl(input);
const matchedRule = this.findMatchingRule(url);
if (matchedRule && matchedRule.enabled !== false) {
try {
const token = await matchedRule.getToken();
const headerName = matchedRule.headerName || 'Authorization';
const headerValue = headerName === 'Authorization' ? `Bearer ${token}` : token;
init = {
...init,
headers: {
...init?.headers,
[headerName]: headerValue,
},
};
} catch (error) {
console.warn(`Failed to get auth token for ${url}:`, error);
// Continue with original request
}
}
return this.originalFetch(input, init);
};
this.isSetup = true;
}
addAuthRule(rule: AuthRule) {
this.authRules.push(rule);
}
teardown() {
if (this.isSetup) {
globalThis.fetch = this.originalFetch;
this.isSetup = false;
}
}
private extractUrl(input: RequestInfo | URL): string {
if (typeof input === 'string') return input;
if (input instanceof URL) return input.href;
return input.url || '';
}
private findMatchingRule(url: string): AuthRule | null {
return this.authRules.find(rule => {
if (typeof rule.pattern === 'string') {
return url.includes(rule.pattern);
}
return rule.pattern.test(url);
}) || null;
}
}
export const fetchInterceptor = new FetchInterceptor();That we can use in the clients side import { fetchInterceptor } from '@metamask/controller-utils';
export async function initializeControllers() {
// Initialize auth controller first
const authController = new AuthenticationController({...});
// Setup authentication rules
fetchInterceptor.addAuthRule({
pattern: 'accounts.api.cx.metamask.io',
getToken: () => authController.getBearerToken(),
});
// Activate the interceptor
fetchInterceptor.setup();
// All subsequent fetch calls are automatically authenticated
const tokenDetectionController = new TokenDetectionController({...});
} |
||||
| ); | ||||
| } | ||||
| } | ||||
|
|
||||
| export default TokenDetectionController; | ||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was this a mistake? It looks like the line below would still violate this lint rule.
If the rule doesn't apply for some reason, we should ensure the accompanying TODO comment is also removed (as well as this empty line).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
autofixOnSavewas the culprit here 🕵️ Fixed with 0ce01be