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. |
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/ConfigRegistryController.ts
Outdated
Show resolved
Hide resolved
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/ConfigRegistryController.ts
Outdated
Show resolved
Hide resolved
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/ConfigRegistryController.ts
Outdated
Show resolved
Hide resolved
packages/config-registry-controller/src/ConfigRegistryController.ts
Outdated
Show resolved
Hide resolved
| apiNetworks.forEach((registryConfig) => { | ||
| const { chainId } = registryConfig; | ||
| newConfigs[chainId] = registryConfig; | ||
| }); |
There was a problem hiding this comment.
Duplicate chainId handling relies on array order not priority
Low Severity
The network deduplication logic in _executePoll simply overwrites newConfigs[chainId] with each iteration, meaning the last array element with a given chainId wins regardless of priority. The test titled "handles duplicate chainIds by keeping highest priority network" passes only because the high-priority entry happens to appear last in the mock array. If the API returns duplicates in a different order, the lower-priority entry would silently win. No priority-based sorting or comparison is performed.
| // Skip fetch when API is disabled; client uses static config. | ||
| if (!isApiEnabled) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Fallback config not stored for runtime revert
Medium Severity
When the feature flag is disabled, _executePoll returns early without modifying state. The fallbackConfig is only used during construction (to set initial state) and is never stored as a class field. This means if the flag transitions from enabled→disabled after a successful fetch, previously fetched API data persists in state instead of reverting to the fallback. Since configs has persist: true, even across app restarts the persisted API data takes precedence over fallbackConfig in the constructor's state.configs?.networks ?? { ...fallbackConfig } check. The PR claims "Fallback to static network list when disabled" but this only holds when no API data was ever fetched and persisted.
Additional Locations (1)
| symbol: string(), | ||
| decimals: number(), | ||
| coingeckoCoinId: string(), | ||
| }); |
There was a problem hiding this comment.
Duplicate identical schemas for asset validation
Low Severity
NativeAssetSchema and GovernanceAssetSchema are byte-for-byte identical — both define the exact same six fields (assetId, imageUrl, name, symbol, decimals, coingeckoCoinId). Having two copies means any future field change (e.g., from an API contract update) needs to be applied in both places, creating a risk that one is updated while the other is missed, causing silent validation mismatches.
| // Skip fetch when API is disabled; client uses static config. | ||
| if (!isApiEnabled) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Disabling flag keeps remote configs
Medium Severity
When isConfigRegistryApiEnabled returns false, _executePoll returns early without restoring fallbackConfig, so previously fetched remote state.configs.networks can remain active even though the feature is disabled. This conflicts with the “fallback to static network list when disabled” behavior and can leave users seeing remote networks after turning the flag off.
There was a problem hiding this comment.
We will handle this in client side (extension and mobile)
|
@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. |
packages/config-registry-controller/src/ConfigRegistryController.ts
Outdated
Show resolved
Hide resolved
packages/config-registry-controller/src/config-registry-api-service/filters.ts
Outdated
Show resolved
Hide resolved
packages/config-registry-controller/src/config-registry-api-service/index.ts
Outdated
Show resolved
Hide resolved
...es/config-registry-controller/src/config-registry-api-service/config-registry-api-service.ts
Show resolved
Hide resolved
packages/config-registry-controller/src/ConfigRegistryController.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
packages/config-registry-controller/src/ConfigRegistryController.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
packages/config-registry-controller/src/ConfigRegistryController.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
There was a problem hiding this comment.
This looks a lot better! The only thing I really noticed was the extra export in index.ts but everything else is minor.
(Also, I know there are some comments that Cursor has made. I haven't taken a look at these, although I'll comment on the one you pinged me on.)
|
|
||
| const controllerName = 'ConfigRegistryController'; | ||
|
|
||
| export const DEFAULT_POLLING_INTERVAL = inMilliseconds(1, Duration.Day); |
There was a problem hiding this comment.
Nice! I forgot we have this utility.
| ...args: Parameters<ServicePolicy['onDegraded']> | ||
| ): ReturnType<ServicePolicy['onDegraded']> { | ||
| return this.#policy.onDegraded(...args); | ||
| } |
There was a problem hiding this comment.
I don't see this comment, but that's a fair question. These are standard methods on data service classes. They allow consumers to respond to when the API is degraded or unavailable.
...es/config-registry-controller/src/config-registry-api-service/config-registry-api-service.ts
Outdated
Show resolved
Hide resolved
packages/config-registry-controller/src/ConfigRegistryController.test.ts
Outdated
Show resolved
Hide resolved
packages/config-registry-controller/src/ConfigRegistryController.test.ts
Outdated
Show resolved
Hide resolved
packages/config-registry-controller/src/ConfigRegistryController.test.ts
Outdated
Show resolved
Hide resolved
packages/config-registry-controller/src/ConfigRegistryController.test.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
3 participants
Explanation
Core Package (@metamask/config-registry-controller)
What was delivered:
Business value:
References
Checklist
Note
Medium Risk
Introduces a new remotely-fetched network configuration source with polling, feature-flag gating, and ETag caching; failures or schema mismatches could affect how clients derive network lists when enabled. Scope is mostly additive and isolated to a new package with extensive tests.
Overview
Adds a new
@metamask/config-registry-controllerpackage that polls a remote config-registry API (daily by default) to populate persisted network configuration state, with ETag-based cache validation and fallback to a provided static config when theconfigRegistryApiEnabledremote feature flag is disabled/unavailable.Includes a
ConfigRegistryApiService(env-aware endpoints, retry/circuit-break policy integration, response validation viasuperstruct) plus selectors/filters (e.g.selectFeaturedNetworks) and comprehensive unit tests. Repo wiring updates include CODEOWNERS/teams mapping, TS project references, README package list/graph, Lavamoat allowlist, and workspace lockfile.Written by Cursor Bugbot for commit e6575ef. This will update automatically on new commits. Configure here.