refactor!: simplify ConfigRegistryController event listeners#8230
Conversation
mikesposito
changed the title
ConfigRegistryController event listenersConfigRegistryController event listeners
| this.messenger.subscribe( | ||
| 'RemoteFeatureFlagController:stateChange', | ||
| this.#onFeatureFlagChange.bind(this), | ||
| (stateSelector) => stateSelector.remoteFeatureFlags[FEATURE_FLAG_KEY], |
There was a problem hiding this comment.
This subscription selector allows us to narrow down the events that will be captured by the subscription. This is useful for performance reasons, as it allows us to only capture the events that we are interested in.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| async _executePoll(_input: null): Promise<void> { | ||
| let isApiEnabled = false; | ||
| try { | ||
| isApiEnabled = isConfigRegistryApiEnabled( | ||
| this.messenger.call('RemoteFeatureFlagController:getState'), | ||
| ); | ||
| } catch { | ||
| // RemoteFeatureFlagController unavailable; skip fetch. | ||
| } | ||
|
|
||
| // Skip fetch when API is disabled; client uses static config. | ||
| if (!isApiEnabled) { | ||
| return; | ||
| } | ||
|
|
||
| try { |
There was a problem hiding this comment.
Given the event listeners set up by this controller, we can assume that polling will be disabled when the feature flag is turned off, making this code unreachable.
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.
|
|
||
| async _executePoll(_input: null): Promise<void> { | ||
| let isApiEnabled = false; | ||
| try { |
There was a problem hiding this comment.
Removed feature-flag guard from _executePoll allows unguarded API calls
Medium Severity
_executePoll previously checked isConfigRegistryApiEnabled before making API calls, acting as a defense-in-depth guard. This guard has been removed, but startPolling is publicly exposed via the messenger as ConfigRegistryController:startPolling. Any external caller invoking startPolling directly while the feature flag is disabled will now trigger API calls unconditionally, bypassing the kill-switch intent of the feature flag. The PR states "The controller functionality should be exactly the same," but this is a behavioral change for the direct-call path. Previously-passing tests for this scenario (e.g., "uses fallback config when feature flag is disabled") were removed rather than adapted.
| 'RemoteFeatureFlagController:getState', | ||
| ).remoteFeatureFlags[FEATURE_FLAG_KEY]; | ||
|
|
||
| if (typeof featureFlagValue !== 'boolean') { |
There was a problem hiding this comment.
We don't know the type returned? We need to be this defensive?
There was a problem hiding this comment.
We know the type, but it is a generic Json type that could be any serializable value, so this guard is just to narrow it down:
| /** | ||
| * Setup messenger event listeners necessary for the controller lifecycle | ||
| */ | ||
| #setupEventListeners(): void { |
There was a problem hiding this comment.
Ah great method to keep things organized
|
CI is failling due to some imports still being there ✌️ |
mikesposito
deleted the
mikesposito/refactor/simplify-dynamic-registry-listeners
branch
## Explanation <!-- Thanks for your contribution! Take a moment to answer these questions so that reviewers have the information they need to properly understand your changes: * What is the current state of things and why does it need to change? * What is the solution your changes offer and how does it work? * Are there any changes whose purpose might not obvious to those unfamiliar with the domain? * If your primary goal was to update one package but you found you had to update another one along the way, why did you do so? * If you had to upgrade a dependency, why did you do so? --> `ConfigRegistryController` is being refactored to simplify messenger event listeners attached on construction. The controller functionality should be exactly the same. ## References <!-- Are there any issues that this pull request is tied to? Are there other links that reviewers should consult to understand these changes better? Are there client or consumer pull requests to adopt any breaking changes? For example: * Fixes #12345 * Related to #67890 --> N/A ## Checklist - [ ] I've updated the test suite for new or updated code as appropriate - [ ] I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate - [ ] I've communicated my changes to consumers by [updating changelogs for packages I've changed](https://github.com/MetaMask/core/tree/main/docs/processes/updating-changelogs.md) - [ ] I've introduced [breaking changes](https://github.com/MetaMask/core/tree/main/docs/processes/breaking-changes.md) in this PR and have prepared draft pull requests for clients and consumer packages to resolve them <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Medium Risk** > Changes the controller’s polling lifecycle triggers and adds a new required messenger permission (`KeyringController:getState`), which could impact when config fetching starts/stops if consumers don’t update delegation or if event/flag semantics differ. > > **Overview** > Refactors `ConfigRegistryController` construction-time subscriptions into dedicated private handlers (`#onUnlock`, `#onLock`, `#onFeatureFlagChange`) and switches feature-flag change handling to use the event’s selected flag value plus `KeyringController:getState` to gate polling on wallet-unlocked state. > > Removes the `isConfigRegistryApiEnabled` utility and its tests, updates controller/tests accordingly, and documents a **breaking** messenger requirement: consumers must allow `KeyringController:getState` for `ConfigRegistryControllerMessenger`. > > <sup>Written by [Cursor Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit ec294ce. This will update automatically on new commits. Configure [here](https://cursor.com/dashboard?tab=bugbot).</sup> <!-- /CURSOR_SUMMARY -->
2 participants
Explanation
ConfigRegistryControlleris being refactored to simplify messenger event listeners attached on construction. The controller functionality should be exactly the same.References
N/A
Checklist
Note
Medium Risk
Changes the controller’s polling lifecycle triggers and adds a new required messenger permission (
KeyringController:getState), which could impact when config fetching starts/stops if consumers don’t update delegation or if event/flag semantics differ.Overview
Refactors
ConfigRegistryControllerconstruction-time subscriptions into dedicated private handlers (#onUnlock,#onLock,#onFeatureFlagChange) and switches feature-flag change handling to use the event’s selected flag value plusKeyringController:getStateto gate polling on wallet-unlocked state.Removes the
isConfigRegistryApiEnabledutility and its tests, updates controller/tests accordingly, and documents a breaking messenger requirement: consumers must allowKeyringController:getStateforConfigRegistryControllerMessenger.Written by Cursor Bugbot for commit ec294ce. This will update automatically on new commits. Configure here.