-
-
Notifications
You must be signed in to change notification settings - Fork 180
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.
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
[composable-controller] Fix incorrect behavior and improve type-level safeguards #4467
[composable-controller] Fix incorrect behavior and improve type-level safeguards #4467
Conversation
6457a28
to
1a452ef
Compare
1a452ef
to
7359c4b
Compare
7359c4b
to
63e3837
Compare
a65140e
to
604c2e9
Compare
d5343e5
to
a850f90
Compare
a32d745
to
ea7e75d
Compare
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
…r all child controllers
c081c50
to
f59de5e
Compare
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
1f29151
to
36f9cf4
Compare
edc11c3
to
7804ce6
Compare
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.
Notes for reviewers:
*/ | ||
export class ComposableController< | ||
ComposableControllerState extends LegacyComposableControllerStateConstraint, | ||
ChildControllers extends ControllerInstance = GetChildControllers<ComposableControllerState>, | ||
ChildControllers extends ControllerInstance, |
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.
The previous approach of generating mock controller classes from the ComposableControllerState
input was flawed, because the mock controllers would always be incomplete, complicating attempts at validation.
Instead, we now rely on the downstream consumer to provide both a composed type of state schemas (ComposableControllerState
) and a type union of the child controller instances (ChildControllers
).
For example, in mobile, we can use (with some adjustments) EngineState
for the former, and Controllers[keyof Controllers]
for the latter.
try { | ||
this.messagingSystem.subscribe( | ||
// TODO: Either fix this lint violation or explain why it's necessary to ignore. | ||
// False negative. `name` is a string type. | ||
// eslint-disable-next-line @typescript-eslint/restrict-template-expressions | ||
`${name}:stateChange`, | ||
(childState: Record<string, unknown>) => { | ||
(childState: LegacyControllerStateConstraint) => { | ||
this.update((state) => { | ||
Object.assign(state, { [name]: childState }); | ||
}); | ||
}, | ||
); | ||
} else if (isBaseControllerV1(controller)) { | ||
controller.subscribe((childState) => { | ||
} catch (error: unknown) { | ||
// False negative. `name` is a string type. | ||
// eslint-disable-next-line @typescript-eslint/restrict-template-expressions | ||
console.error(`${name} - ${String(error)}`); | ||
} |
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.
To handle edge cases due to mobile using older BaseControllerV1
versions, this method no longer attempts to limit the stateChange
event subscription to controllers that are defined with a messenger. Instead, it relies on the events allowlist of the ComposableController
messenger to ensure that only valid subscriptions are made.
The subscribe
call throws an error when invoked with a stateChange
event that is not in the allowlist. This prevents any incorrect subscriptions to child controllers without a messenger or a stateChange
event.
The catch block is intended to allow the logic to continue without terminating prematurely. We can remove or replace the console.error
call if it causes noisy messages downstream.
if (isBaseControllerV1(controller)) { | ||
controller.subscribe((childState: StateConstraintV1) => { |
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.
This subscription applies to all V1 child controllers. This may result in duplicate state updates for V1 child controllers with messengers, but the performance impact should be minimal, and it shouldn't affect the correctness of state.
Also, this issue will be automatically resolved as we phase out V1 controllers in our clients.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
b42590f
to
2ca9038
Compare
@metamaskbot publish-preview |
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions.
|
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.
Looks good! just one question
## Explanation This commit moves `BaseController`-related types and functions in `@metamask/composable-controller` to `@metamask/base-controller`. Because applying these changes requires a concurrent major update of `@metamask/base-controller`, this commit will be excluded from `@metamask/composable-controller@8.0.0` (#4467), so that complications can be avoided while applying `8.0.0` to mobile. ## References - Blocked by #4467 - Blocked by MetaMask/metamask-mobile#10441 ## Changelog ### `@metamask/base-controller` (minor) ### Added - Migrate from `@metamask/composable-controller@8.0.0` into `@metamask/base-controller`: types `LegacyControllerStateConstraint`, `RestrictedControllerMessengerConstraint` and type guard functions `isBaseController`, `isBaseControllerV1` ([#4581](#4581)) - Add and export types `ControllerInstance`, `BaseControllerInstance`, `StateDeriverConstraint`, `StateMetadataConstraint`, `StatePropertyMetadataConstraint`, `BaseControllerV1Instance`, `ConfigConstraintV1`, `StateConstraintV1` ([#4581](#4581)) ### `@metamask/composable-controller` (major) ### Removed - **BREAKING:** Remove exports for types `LegacyControllerStateConstraint`, `RestrictedControllerMessengerConstraint`, and type guard functions `isBaseController`, `isBaseControllerV1` ([#4467](#4467)) - These have been migrated to `@metamask/base-controller@6.2.0`. ## Checklist - [x] I've updated the test suite for new or updated code as appropriate - [x] I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate - [x] I've highlighted breaking changes using the "BREAKING" category above as appropriate
Overview
This commit fixes issues with the
ComposableController
class's interface, and its logic for validating V1 and V2 controllers.These changes will enable
ComposableController
to function correctly downstream in the Mobile Engine, and eventually the Wallet Framework POC.Explanation
The previous approach of generating mock controller classes from the
ComposableControllerState
input using theGetChildControllers
was flawed, because the mock controllers would always be incomplete, complicating attempts at validation.Instead, we now rely on the downstream consumer to provide both a composed type of state schemas (
ComposableControllerState
) and a type union of the child controller instances (ChildControllers
). For example, in mobile, we can use (with some adjustments)EngineState
for the former, andControllers[keyof Controllers]
for the latter.The validation logic for V1 controllers has also been updated. Due to breaking changes made to the private properties of
BaseControllerV1
(#3959), mobile V1 controllers relying on versions prior to these changes were introduced were incompatible with the up-to-dateBaseControllerV1
version that the composable-controller package references.In this commit, the validator type
BaseControllerV1Instance
filters out the problematic private properties by using thePublicInterface
type. Because the public API ofBaseControllerV1
has been relatively constant, this removes the type errors that previously occurred in mobile when passing V1 controllers intoComposableController
.References
BaseControllerV1Instance
type and makeChildControllers
a required type parameter #4448ComposableController
instantiation and functionality is broken metamask-mobile#10073@metamask/composable-controller
from^3.0.0
to^6.0.2
metamask-mobile#10011Changelog
@metamask/composable-controller
(major)Changed
ComposableController
class:ComposedControllerState
(constrained byLegacyComposableControllerStateConstraint
) andChildControllers
(constrained byControllerInstance
) (#4467).isBaseController
now validates that the input has an object-type property namedmetadata
in addition to its existing checks.isBaseControllerV1
now validates that the input has object-type propertiesconfig
,state
, and function-type propertysubscribe
, in addition to its existing checks.LegacyControllerStateConstraint
type fromBaseState | StateConstraint
toBaseState & object | StateConstraint
.ControllerName
to theRestrictedControllerMessengerConstraint
type, which extendsstring
and defaults tostring
.Fixed
ComposableController
class raises a type error if a non-controller with nostate
property is passed into theChildControllers
generic parameter or thecontrollers
constructor option.ComposableController
class is instantiated, its messenger now attempts to subscribe to all child controllerstateChange
events that are included in the messenger's events allowlist.@metamask/composable-controller@6.0.0
causedstateChange
event subscriptions to fail.isBaseController
andisBaseControllerV1
no longer return false negatives.instanceof
operator is no longer used to validate that the input is a subclass ofBaseController
orBaseControllerV1
.ChildControllerStateChangeEvents
type checks that the child controller's state extends from theStateConstraintV1
type instead of fromRecord<string, unknown>
. (#4467)interface
keyword, which are incompatible withRecord<string, unknown>
by default. This resulted inChildControllerStateChangeEvents
failing to generatestateChange
events for V1 controllers and returningnever
.Checklist