[composable-controller] Enable handling of non-controller input #4943
Conversation
MajorLift
added
team-wallet-framework
team-tiger
|
@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. |
|
@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. |
|
@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. |
| @@ -16,8 +15,18 @@ import type { Patch } from 'immer'; | |||
|
|
|||
| export const controllerName = 'ComposableController'; | |||
|
|
|||
| export const INVALID_CONTROLLER_ERROR = | |||
| 'Invalid controller: controller must have a `messagingSystem` or be a class inheriting from `BaseControllerV1`.'; | |||
| // TODO: Replace with type guard once superclass for messageable non-controllers has been defined. | |||
There was a problem hiding this comment.
If the goal is to figure out which controllers do not have state, can we check for the absence of the state property?
There was a problem hiding this comment.
Unfortunately only AssetsContractController lacks a state property.
NftDetectionController and TokenDetectionController both have a state property which is assigned an empty object of type Record<never, never>, because both inherit from BaseController.
Checking for an empty state object is not useful because there could be controllers with non-empty state that are initialized with empty state objects.
These inconsistencies make the changes in this PR less type-safe than they could be. This ties in to one of the motivations for the MessengerClient/Messageable ADR. which is to align inheritance for stateless non-controllers.
There was a problem hiding this comment.
Oh, I see. My thought is that we could clean this up by assuming that the non-controllers do not inherit from BaseController. I forgot that we will do that through the MessengerClient/Messageable ADR though.
There was a problem hiding this comment.
There's currently no common supertype for the stateless non-controllers that is narrower than an object with a 'name' property.
Ideally, we would be able to exclude all classes with properties 'name', 'messagingSystem' and without 'state', 'metadata', but since that's currently not an option until the ADR is implemented, I went for just explicitly specifying the non-controllers, so we wouldn't have to deal with edge cases in the input.
I refactored the logic a bit. Hopefully this makes the intention a bit clearer: 85389bb
TODO: move to `@metamask/base-controller` once we have a type and type guard for stateless non-controllers
|
The issue suggests two solutions: supporting controllers with no state, or rejecting them. Why not reject instead, what advantage is there to passing in a "controller" with no state? |
|
@Gudahtt The motivation for this PR is to make the ComposableController API more accommodating rather than require code changes downstream with stricter guardrails, but maybe that's the wrong direction to take. My initial inclination was also to reject, which is reflected in the current state of the ComposableController. This had become a blocker for the update to v9 in mobile because the Engine class uses a single One concern I had regarding this was performance impacts on mobile Engine initialization time, since we would need to create a new filtered copy of the array that excludes non-controllers to pass into ComposableController. However, I've implemented a refactor here 5d0807f05c40fd667dfef28a1ae1233e9bc95ef7 that removes a costly If this doesn't cause any additional issues, then we could close this PR, and stricter safeguards on ComposableController could be preserved that enable it to avoid handling non-controllers altogether. |
|
Closing as wont-fix. Downstream consumers will need to ensure that only stateful controllers are present in the input array they pass into the Concerns over imposing performance penalty to mobile resolved here: MetaMask/metamask-mobile#12374 |
References
ComposableControllerAPI updates that fix broken functionality metamask-mobile#10441Changelog
@metamask/composable-controllerChanged
ComposableControllercontroller optioncontrollersand generic type parameterChildControllersto accept all objects with a 'name' property. Passing in non-controller input no longer produces a runtime error.stateChangeevent subscription operation will exclude non-controller input.ComposableControllerStateonly accepts controllers with a 'state' property.isBaseController,isBaseControllerV1fromControllerInstancetounknown.Checklist