ComposableController usage in the mobile Engine is currently broken in two ways.
The first argument of the ComposableController's constructor has a type constraint that is incompatible with V1 members of the controllers input array.
Argument of type '(PhishingController | AccountsController | AccountTrackerController | AddressBookController | ... 22 more ... | SwapsController)[]' is not assignable to parameter of type 'ControllerList'.
Type 'PhishingController | AccountsController | AccountTrackerController | AddressBookController | ... 22 more ... | SwapsController' is not assignable to type 'BaseController<any, any> | { name: string; state: Record<string, unknown>; }'.
Type 'AccountTrackerController' is not assignable to type 'BaseController<any, any> | { name: string; state: Record<string, unknown>; }'.
Type 'AccountTrackerController' is not assignable to type 'BaseController<any, any>'.
Types have separate declarations of a private property 'initialConfig'.ts(2345)
The error message makes it clear that V1 controllers, not V2, are causing the issue.
The second argument passed into the ComposableController constructor has always been a ControllerMessenger class instance, when it should have been a RestrictedControllerMessenger.
This grants the ComposableController wider permissions than intended (we only want to allow stateChange events for all child controllers), and generally goes against our recommended usage pattern for messengers.
(However, it shouldn't prevent the controllerMessenger or ComposableController state from subscribing to state updates from child controllers)
Solution
Add tests to Engine.test.js file verifying that the datamodel has subscribed to state updates from all child controllers (both those with a messagingSystem, and V1 controllers with no messagingSystem).
const engine = Engine.init(initialState);
const { controllerMessenger } = engine;
test.each(Object.entries(engine.context))('controller-messenger subscribes to all controllers', ([name, controller]) => {
if (hasProperty(controller, 'messagingSystem')) {
const eventType = `${name}:stateChange`;
expect(controllerMessenger.unsubscribe(eventType)).toThrow(
`Subscription not found for event: ${eventType}`,
);
} else if (hasProperty(controller, 'unsubscribe')) {
expect(controller.unsubscribe((state) => {
this.update({ [name]: state });
})).toHaveReturned(true);
} else {
fail('not a valid controller');
}
});
Bump @metamask/composable-controller to the latest version which offers better type safety and quality-of-life improvements.
@metamask/composable-controller@3.0.0 features widespread any usage, which would unnecessarily complicate any debugging efforts.
For the issue of defining a type constraint that can accept V1 controllers for the first argument controllers, attempt to resolve by bumping V1 controllers from mobile.
All of the V1 controllers in Engine's context have a V2 release, except for
Stakeholder review needed before the work gets merged
[ ] Engineering (needed in most cases)
[ ] Design
[ ] Product
[ ] QA (automation tests are required to pass before merging PRs but not all changes are covered by automation tests - please review if QA is needed beyond automation tests)
What is this about?
Explanation
ComposableController
usage in the mobile Engine is currently broken in two ways.ComposableController
's constructor has a type constraint that is incompatible with V1 members of thecontrollers
input array.Up until now, this error has been suppressed with the following comment, which is incorrect:
ComposableController
has supported V2 controllers since before its first release (see https://github.com/MetaMask/core/pull/447).ComposableController
constructor has always been aControllerMessenger
class instance, when it should have been aRestrictedControllerMessenger
.This grants the
ComposableController
wider permissions than intended (we only want to allowstateChange
events for all child controllers), and generally goes against our recommended usage pattern for messengers.(However, it shouldn't prevent the
controllerMessenger
orComposableController
state from subscribing to state updates from child controllers)Solution
Engine.test.js
file verifying that thedatamodel
has subscribed to state updates from all child controllers (both those with amessagingSystem
, and V1 controllers with nomessagingSystem
).@metamask/composable-controller
to the latest version which offers better type safety and quality-of-life improvements.@metamask/composable-controller@3.0.0
features widespreadany
usage, which would unnecessarily complicate any debugging efforts.controllers
, attempt to resolve by bumping V1 controllers from mobile.context
have a V2 release, except forSwapsController
, which has a V2 upgrade being implemented by the accounts team, andAssetsContractController
, which will be upgraded into a non-controller, necessitating changes inComposableController
(https://github.com/MetaMask/core/issues/4444).If unsuccessful, wait until the issue is fixed from the
ComposableController
side: https://github.com/MetaMask/core/issues/4448Scenario
No response
Design
No response
Technical Details
No response
Threat Modeling Framework
No response
Acceptance Criteria
No response
Stakeholder review needed before the work gets merged
References
No response