Feat/add config registry controller #7668
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
No dependency changes detected. Learn more about Socket for GitHub. 👍 No dependency changes detected in pull request |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
|
@metamaskbot publish-previews |
|
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions. |
| const stateMetadata = { | ||
| configs: { | ||
| persist: true, | ||
| anonymous: false, | ||
| includeInStateLogs: false, | ||
| includeInDebugSnapshot: true, | ||
| usedInUi: true, | ||
| }, | ||
| version: { | ||
| persist: true, | ||
| anonymous: false, | ||
| includeInStateLogs: true, | ||
| includeInDebugSnapshot: true, | ||
| usedInUi: false, | ||
| }, | ||
| lastFetched: { | ||
| persist: true, | ||
| anonymous: false, | ||
| includeInStateLogs: true, | ||
| includeInDebugSnapshot: true, | ||
| usedInUi: false, | ||
| }, | ||
| fetchError: { | ||
| persist: true, | ||
| anonymous: false, | ||
| includeInStateLogs: true, | ||
| includeInDebugSnapshot: true, | ||
| usedInUi: false, | ||
| }, | ||
| etag: { | ||
| persist: true, | ||
| anonymous: false, | ||
| includeInStateLogs: false, | ||
| includeInDebugSnapshot: false, | ||
| usedInUi: false, | ||
| }, | ||
| }; |
There was a problem hiding this comment.
We can use satisfies StateMetadata<ConfigRegistryState> to ensure that the stateMetadata object conforms to the expected structure for state metadata. As an example, anonymous is not accepted as a valid key anymore:
| const stateMetadata = { | |
| configs: { | |
| persist: true, | |
| anonymous: false, | |
| includeInStateLogs: false, | |
| includeInDebugSnapshot: true, | |
| usedInUi: true, | |
| }, | |
| version: { | |
| persist: true, | |
| anonymous: false, | |
| includeInStateLogs: true, | |
| includeInDebugSnapshot: true, | |
| usedInUi: false, | |
| }, | |
| lastFetched: { | |
| persist: true, | |
| anonymous: false, | |
| includeInStateLogs: true, | |
| includeInDebugSnapshot: true, | |
| usedInUi: false, | |
| }, | |
| fetchError: { | |
| persist: true, | |
| anonymous: false, | |
| includeInStateLogs: true, | |
| includeInDebugSnapshot: true, | |
| usedInUi: false, | |
| }, | |
| etag: { | |
| persist: true, | |
| anonymous: false, | |
| includeInStateLogs: false, | |
| includeInDebugSnapshot: false, | |
| usedInUi: false, | |
| }, | |
| }; | |
| const stateMetadata = { | |
| configs: { | |
| persist: true, | |
| includeInStateLogs: false, | |
| includeInDebugSnapshot: true, | |
| usedInUi: true, | |
| }, | |
| version: { | |
| persist: true, | |
| includeInStateLogs: true, | |
| includeInDebugSnapshot: true, | |
| usedInUi: false, | |
| }, | |
| lastFetched: { | |
| persist: true, | |
| includeInStateLogs: true, | |
| includeInDebugSnapshot: true, | |
| usedInUi: false, | |
| }, | |
| fetchError: { | |
| persist: true, | |
| includeInStateLogs: true, | |
| includeInDebugSnapshot: true, | |
| usedInUi: false, | |
| }, | |
| etag: { | |
| persist: true, | |
| includeInStateLogs: false, | |
| includeInDebugSnapshot: false, | |
| usedInUi: false, | |
| }, | |
| } satisfies StateMetadata<ConfigRegistryState>; |
|
|
||
| const DEFAULT_FALLBACK_CONFIG: Record<string, RegistryConfigEntry> = {}; | ||
|
|
||
| type ConfigRegistryPollingInput = Record<string, never>; |
There was a problem hiding this comment.
I don't think consumers should be able to arbitrarily set different polling cycles. We can probably remove this type and always (and only) accept null as polling input
| startPolling(input: ConfigRegistryPollingInput = {}): string { | ||
| return super.startPolling(input); | ||
| } | ||
|
|
||
| stopPolling(): void { | ||
| super.stopAllPolling(); | ||
| } |
There was a problem hiding this comment.
Do you think it would make sense to start and stop the polling autonomously by listening to KeyringController:unlock and KeyringController:lock events?
This way we would simplify the controller API, and make the client implementation easier
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| (state.configs as any) = { networks: {} }; |
There was a problem hiding this comment.
This cast to any seems unnecessary:
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | |
| (state.configs as any) = { networks: {} }; | |
| state.configs = { networks: {} }; |
| }); | ||
| } | ||
|
|
||
| removeConfig(key: string): void { |
There was a problem hiding this comment.
Curious: why should the client arbitrarily choose to remove config keys from the registry?
| clearConfigs(): void { | ||
| this.update((state) => { | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| (state.configs as any) = { networks: {} }; | ||
| }); | ||
| } |
There was a problem hiding this comment.
Similarly to removeConfig, I'm curious to know in what scenarios should the consumer clear all configurations
There was a problem hiding this comment.
You're right. The controller should be read-only for consumers, registry configs should be managed internally via polling. I'll remove these methods
|
|
||
| export type ConfigRegistryState = { | ||
| configs: { | ||
| networks?: Record<string, RegistryConfigEntry>; |
There was a problem hiding this comment.
If we already know that this configurations is for networks only, do you think it would make sense to make types more strict here, instead of generic RegistryConfigEntry?
Using specific types for network configurations would greately enhance type safety downstream, and help catch potential issues by types alone
|
|
||
| const controllerName = 'ConfigRegistryController'; | ||
|
|
||
| export const DEFAULT_POLLING_INTERVAL = 24 * 60 * 60 * 1000; |
There was a problem hiding this comment.
We have a cool function that we can use from @metamask/utils:
import { Duration, inMilliseconds } from '@metamask/utils';
| export const DEFAULT_POLLING_INTERVAL = 24 * 60 * 60 * 1000; | |
| export const DEFAULT_POLLING_INTERVAL = inMilliseconds(1, Duration.Day); |
There was a problem hiding this comment.
Going back at this, I think that having a default polling that is so long is equivalent to not having polling at all, because most sessions won't last that long so we would execute the polling cycle at init time only. Perhaps we should reduce this to something like 5 or 10 minutes?
| state.etag = result.etag ?? null; | ||
| }); | ||
| } catch (error) { | ||
| this.#handleFetchError(error); |
There was a problem hiding this comment.
We can intercept errors if we need to perform actions over them, but we should never suppress them. Errors should always be bubbled up to the caller unless there's a very good reason not to.
There was a problem hiding this comment.
Hmm, going back at this, there is a good reason to suppress the error in this case. Though, perhaps we can record the event with messenger.captureException?(Error)
| state = {}, | ||
| pollingInterval = DEFAULT_POLLING_INTERVAL, | ||
| fallbackConfig = DEFAULT_FALLBACK_CONFIG, | ||
| apiService = new ConfigRegistryApiService(), |
There was a problem hiding this comment.
We could, alternatively, initialize this service in the client, allowing the client to inject custom configurations (e.g. during testing environments).
The controller would use the service via messenger
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| export type AbstractConfigRegistryApiService = Partial< | ||
| Pick<ServicePolicy, 'onBreak' | 'onDegraded'> | ||
| > & { | ||
| fetchConfig(options?: FetchConfigOptions): Promise<FetchConfigResult>; | ||
| }; |
There was a problem hiding this comment.
What's the purpose of having an abstract class? Do we plan to add multiple services?
| * @param comparisonOptions - Options for comparing with existing networks. | ||
| * @returns Result containing networks to add and existing chain IDs. | ||
| */ | ||
| export function processNetworkConfigs( |
There was a problem hiding this comment.
I see a lot of logic in this file that supports this function, but I don't see this used anywhere. What do we need this for?
| const response = await this.#policy.execute(async () => { | ||
| const res = await fetchWithTimeout(); | ||
|
|
||
| if (res.status === 304) { | ||
| return { | ||
| status: 304, | ||
| headers: res.headers, | ||
| } as unknown as Response; | ||
| } | ||
|
|
||
| if (!res.ok) { | ||
| throw new Error( | ||
| `Failed to fetch config: ${res.status} ${res.statusText}`, | ||
| ); | ||
| } | ||
|
|
||
| return res; | ||
| }); |
There was a problem hiding this comment.
Thoughts on simply returning the fetchWithTimeout promise directly instead of returning another object in case 304 is received? I feel like it adds unnecessary complexity here, but maybe I'm missing something
| this.#apiBaseUrl = apiBaseUrl; | ||
| this.#endpointPath = endpointPath; |
There was a problem hiding this comment.
Thoughs on using something similar to ProfileMetricsService for configuring the environment?
It would make the fetchConfig method way easier as we wouldn't have to validate an arbitrary string passed to the constructor
There was a problem hiding this comment.
Moreover, the service is currently constructed by the controller only, so these constructor options would never really differ from the default ones, making the added complexity probably not worth it
| const fetchWithTimeout = async (): Promise<Response> => { | ||
| const controller = new AbortController(); | ||
| const timeoutId = setTimeout(() => controller.abort(), this.#timeout); | ||
|
|
||
| try { | ||
| const response = await this.#fetch(url.toString(), { | ||
| headers, | ||
| signal: controller.signal, | ||
| }); | ||
| clearTimeout(timeoutId); | ||
| return response; | ||
| } catch (error) { | ||
| clearTimeout(timeoutId); | ||
| if (error instanceof Error && error.name === 'AbortError') { | ||
| throw new Error(`Request timeout after ${this.#timeout}ms`); | ||
| } | ||
| throw error; | ||
| } | ||
| }; |
There was a problem hiding this comment.
I see that other services in the core packages don't handle directly timeouts, leaving it to the service policy execution. Was there a specific reason to handle timeouts directly in this service?
| return { | ||
| data, | ||
| etag, | ||
| notModified: false, |
There was a problem hiding this comment.
Do you think we can avoid the double negation on this parameter and call it modified (inverting the semantic)? It would probably be easier to read that way.
| } | ||
|
|
||
| const etag = response.headers.get('ETag') ?? undefined; | ||
| const data = (await response.json()) as RegistryConfigApiResponse; |
There was a problem hiding this comment.
We are using a wide type on the controller where we should know the type of data we have, while we cast a specific type in the service where we suppose to receive data in that shape. We should probably have a validation here, and we can use @metamask/superstruct for that - so we can avoid the type cast and give a guarantee that the data has the expected shape
| /** | ||
| * Checks if the config registry API feature flag is enabled. | ||
| * | ||
| * @param messenger - The controller messenger. | ||
| * @returns True if the feature flag is enabled, false otherwise. | ||
| */ | ||
| export function isConfigRegistryApiEnabled( | ||
| messenger: ConfigRegistryMessenger, | ||
| ): boolean { | ||
| try { | ||
| const state = messenger.call('RemoteFeatureFlagController:getState'); | ||
| const featureFlags = state.remoteFeatureFlags; | ||
|
|
||
| const flagValue = featureFlags[FEATURE_FLAG_KEY]; | ||
|
|
||
| if (typeof flagValue === 'boolean') { | ||
| return flagValue; | ||
| } | ||
|
|
||
| return DEFAULT_FEATURE_FLAG_VALUE; | ||
| } catch { | ||
| return DEFAULT_FEATURE_FLAG_VALUE; | ||
| } | ||
| } |
There was a problem hiding this comment.
Clients may want to use different feature flags in different environments. Perhaps it would be easier to simply have a function constructor parameter passed to the controller, maybe even with this same name, and have the controller call that function to establish if it should continue the polling loop?
| fetchError: { | ||
| persist: true, | ||
| anonymous: false, | ||
| includeInStateLogs: true, | ||
| includeInDebugSnapshot: true, | ||
| usedInUi: false, | ||
| }, | ||
| etag: { | ||
| persist: true, | ||
| anonymous: false, | ||
| includeInStateLogs: false, | ||
| includeInDebugSnapshot: false, | ||
| usedInUi: false, | ||
| }, |
There was a problem hiding this comment.
Could you clarify what these two properties are used for? In what scenarios would they be publicly inspected?
There was a problem hiding this comment.
fetchError: Stores API fetch error messages. Updated to includeInStateLogs: false and includeInDebugSnapshot: false to avoid exposing error details.
etag: Stores the HTTP ETag header for caching. Already configured to not be exposed.
They remain persisted for internal use but are excluded from public logs and error reports
| if (hasNoConfigs) { | ||
| this.useFallbackConfig(errorMessage); |
There was a problem hiding this comment.
Instead of handling this case this way, have you considered assigning the fallback config to the state at controller construction time? This way, you can ensure that the state is always initialized with a valid config, and you won't need to handle the undefined case later in the code.
|
It seems to me that we are relying on the polling cycle only, without considering how much time has passed since the last fetch. Though this means that we'll fetch configs everytime the user opens the app instead of every 24 hours |
| let clock: sinon.SinonFakeTimers; | ||
| let messenger: ConfigRegistryMessenger; | ||
| let rootMessenger: RootMessenger; | ||
| let apiService: AbstractConfigRegistryApiService; | ||
| let mockRemoteFeatureFlagGetState: jest.Mock; | ||
|
|
||
| beforeEach(() => { | ||
| clock = useFakeTimers(); | ||
| const messengers = getConfigRegistryControllerMessenger(); | ||
| messenger = messengers.messenger; | ||
| rootMessenger = messengers.rootMessenger; | ||
| apiService = buildMockApiService(); | ||
|
|
||
| mockRemoteFeatureFlagGetState = jest.fn().mockReturnValue({ | ||
| remoteFeatureFlags: { | ||
| configRegistryApiEnabled: true, | ||
| }, | ||
| cacheTimestamp: Date.now(), | ||
| }); | ||
|
|
||
| rootMessenger.registerActionHandler( | ||
| 'RemoteFeatureFlagController:getState', | ||
| mockRemoteFeatureFlagGetState, | ||
| ); | ||
| }); |
There was a problem hiding this comment.
We usually try to avoid using shared variables and encoding test initialization logic in beforeEach blocks, as it can make tests harder to read and maintain. Usually this can be easily done by using wrapper functions like withController:
There was a problem hiding this comment.
+1 for using a function instead of beforeEach. The rationale is explained here: https://github.com/MetaMask/contributor-docs/blob/main/docs/testing/unit-testing.md#avoid-the-use-of-beforeeach
See the SampleGasFeeController tests for a quick example on using withController. This pattern also allows you to automatically call destroy on your controller after each test. In this case it might be useful to ensure that polling is stopped so that Jest doesn't give you warnings about handles being open after tests end.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
packages/config-registry-controller/src/ConfigRegistryController.ts
Outdated
Show resolved
Hide resolved
packages/config-registry-controller/src/ConfigRegistryController.ts
Outdated
Show resolved
Hide resolved
packages/config-registry-controller/src/config-registry-api-service/transformers.ts
Outdated
Show resolved
Hide resolved
...-registry-controller/src/config-registry-api-service/abstract-config-registry-api-service.ts
Outdated
Show resolved
Hide resolved
|
|
||
| export type ConfigRegistryState = { | ||
| configs: { | ||
| networks?: Record<string, NetworkConfigEntry>; |
There was a problem hiding this comment.
Why is this property optional? Since it's a collection, would it be better to have this be required, but be empty if there is nothing configured? This you don't have to deal with undefined.
| metadata?: Json; | ||
| }; | ||
|
|
||
| export type ConfigRegistryState = { |
There was a problem hiding this comment.
Nit: It would be good to provide JSDoc which describes what these properties do and how they are used. In particular the version and etag properties — what's the difference?
| return; | ||
| } | ||
|
|
||
| // Validate API response structure to prevent runtime crashes |
There was a problem hiding this comment.
Why are we validating the API response structure here? It seems that fetchConfig should perform response validation, or throw if the response is not what it expects.
| } | ||
| } | ||
|
|
||
| protected useFallbackConfig(errorMessage?: string): void { |
There was a problem hiding this comment.
Is there a reason why this is marked as protected? Can we use a #private method instead?
|
|
||
| protected useFallbackConfig(errorMessage?: string): void { | ||
| this.update((state) => { | ||
| (state.configs as ConfigRegistryState['configs']) = { |
There was a problem hiding this comment.
Why are we typecasting state.configs here?
|
|
||
| const { url, type, networkClientId, failoverUrls } = endpoint; | ||
|
|
||
| if (!url || typeof url !== 'string') { |
There was a problem hiding this comment.
Is this the right place to perform runtime checks? It feels like a function whose goal is to convert from one data structure to another should already know what it's working with.
| } | ||
|
|
||
| return networks.filter((network) => { | ||
| if (!network || typeof network !== 'object') { |
There was a problem hiding this comment.
Why do all of these runtime checks? Isn't this what types are for?
| expect(result).not.toBeNull(); | ||
| expect(result?.chainId).toBe('0x1'); | ||
| expect(result?.name).toBe('Ethereum Mainnet'); | ||
| expect(result?.nativeCurrency).toBe('ETH'); | ||
| expect(result?.rpcEndpoints).toHaveLength(1); | ||
| expect(result?.rpcEndpoints[0].type).toBe(RpcEndpointType.Infura); | ||
| expect(result?.rpcEndpoints[0].networkClientId).toBe('mainnet'); | ||
| expect(result?.rpcEndpoints[0].failoverUrls).toStrictEqual([ | ||
| 'https://backup.infura.io/v3/{infuraProjectId}', | ||
| ]); | ||
| expect(result?.blockExplorerUrls).toStrictEqual(['https://etherscan.io']); | ||
| expect(result?.defaultRpcEndpointIndex).toBe(0); | ||
| expect(result?.defaultBlockExplorerUrlIndex).toBe(0); |
There was a problem hiding this comment.
Nit: If you use assert instead of expect(...), you can drop all of the ?.'s, e.g.:
| expect(result).not.toBeNull(); | |
| expect(result?.chainId).toBe('0x1'); | |
| expect(result?.name).toBe('Ethereum Mainnet'); | |
| expect(result?.nativeCurrency).toBe('ETH'); | |
| expect(result?.rpcEndpoints).toHaveLength(1); | |
| expect(result?.rpcEndpoints[0].type).toBe(RpcEndpointType.Infura); | |
| expect(result?.rpcEndpoints[0].networkClientId).toBe('mainnet'); | |
| expect(result?.rpcEndpoints[0].failoverUrls).toStrictEqual([ | |
| 'https://backup.infura.io/v3/{infuraProjectId}', | |
| ]); | |
| expect(result?.blockExplorerUrls).toStrictEqual(['https://etherscan.io']); | |
| expect(result?.defaultRpcEndpointIndex).toBe(0); | |
| expect(result?.defaultBlockExplorerUrlIndex).toBe(0); | |
| assert(result, "Expected result to not be null"); | |
| expect(result.chainId).toBe('0x1'); | |
| expect(result.name).toBe('Ethereum Mainnet'); | |
| expect(result.nativeCurrency).toBe('ETH'); | |
| expect(result.rpcEndpoints).toHaveLength(1); | |
| expect(result.rpcEndpoints[0].type).toBe(RpcEndpointType.Infura); | |
| expect(result.rpcEndpoints[0].networkClientId).toBe('mainnet'); | |
| expect(result.rpcEndpoints[0].failoverUrls).toStrictEqual([ | |
| 'https://backup.infura.io/v3/{infuraProjectId}', | |
| ]); | |
| expect(result.blockExplorerUrls).toStrictEqual(['https://etherscan.io']); | |
| expect(result.defaultRpcEndpointIndex).toBe(0); | |
| expect(result.defaultBlockExplorerUrlIndex).toBe(0); |
|
|
||
| describe('transformers', () => { | ||
| describe('transformNetworkConfig', () => { | ||
| it('should transform valid network config with infura endpoint', () => { |
There was a problem hiding this comment.
Thoughts on not using "should"? Jest doesn't use it for its tests, it makes tests read like a list of specifications rather than a list of requirements, and it's shorter:
| it('should transform valid network config with infura endpoint', () => { | |
| it('transforms valid network config with infura endpoint', () => { |
| let clock: sinon.SinonFakeTimers; | ||
| let messenger: ConfigRegistryMessenger; | ||
| let rootMessenger: RootMessenger; | ||
| let apiService: AbstractConfigRegistryApiService; | ||
| let mockRemoteFeatureFlagGetState: jest.Mock; | ||
|
|
||
| beforeEach(() => { | ||
| clock = useFakeTimers(); | ||
| const messengers = getConfigRegistryControllerMessenger(); | ||
| messenger = messengers.messenger; | ||
| rootMessenger = messengers.rootMessenger; | ||
| apiService = buildMockApiService(); | ||
|
|
||
| mockRemoteFeatureFlagGetState = jest.fn().mockReturnValue({ | ||
| remoteFeatureFlags: { | ||
| configRegistryApiEnabled: true, | ||
| }, | ||
| cacheTimestamp: Date.now(), | ||
| }); | ||
|
|
||
| rootMessenger.registerActionHandler( | ||
| 'RemoteFeatureFlagController:getState', | ||
| mockRemoteFeatureFlagGetState, | ||
| ); | ||
| }); |
There was a problem hiding this comment.
+1 for using a function instead of beforeEach. The rationale is explained here: https://github.com/MetaMask/contributor-docs/blob/main/docs/testing/unit-testing.md#avoid-the-use-of-beforeeach
See the SampleGasFeeController tests for a quick example on using withController. This pattern also allows you to automatically call destroy on your controller after each test. In this case it might be useful to ensure that polling is stopped so that Jest doesn't give you warnings about handles being open after tests end.
packages/config-registry-controller/src/ConfigRegistryController.test.ts
Show resolved
Hide resolved
|
@metamaskbot publish-previews |
|
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions. |
Explanation
Core Package (@metamask/config-registry-controller)
What was delivered:
Business value:
References
Checklist
Note
Introduces a new controller package to source network configurations remotely and keep them updated.
@metamask/config-registry-controllerwith persisted state (configs,version,lastFetched,etag) and 24h polling viaStaticIntervalPollingControllerConfigRegistryApiService(ETag/If-None-Match, retries, circuit breaker, degraded handling) and network filtering utilitiesconfigRegistryApiEnabled) with fallback config when disabled/unavailableKeyringControllerlock/unlock for polling lifecycleWritten by Cursor Bugbot for commit 839e62c. This will update automatically on new commits. Configure here.