[composable-controller] Enable handling of non-controller input

MajorLift commented 3 days ago

References

Closes https://github.com/MetaMask/core/issues/4444
Blocking https://github.com/MetaMask/metamask-mobile/pull/10441

Changelog

`@metamask/composable-controller`

Changed

Widen ComposableController controller option controllers and generic type parameter ChildControllers to accept all objects with a 'name' property. Passing in non-controller input no longer produces a runtime error.
- Note that the composed state output and stateChange event subscription operation will exclude non-controller input.
- Generic type parameter ComposableControllerState only accepts controllers with a 'state' property.
Widen input type for type guards isBaseController, isBaseControllerV1 from ControllerInstance to unknown.

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
[x] I've prepared draft pull requests for clients and consumer packages to resolve any breaking changes

MajorLift commented 3 days ago

@metamaskbot publish-preview

github-actions[bot] commented 3 days ago

Preview builds have been published. See these instructions for more information about preview builds.

Expand for full list of packages and versions.

``` { "@metamask-previews/accounts-controller": "19.0.0-preview-bb2f1fdd", "@metamask-previews/address-book-controller": "6.0.1-preview-bb2f1fdd", "@metamask-previews/announcement-controller": "7.0.1-preview-bb2f1fdd", "@metamask-previews/approval-controller": "7.1.1-preview-bb2f1fdd", "@metamask-previews/assets-controllers": "44.0.0-preview-bb2f1fdd", "@metamask-previews/base-controller": "7.0.2-preview-bb2f1fdd", "@metamask-previews/build-utils": "3.0.1-preview-bb2f1fdd", "@metamask-previews/chain-controller": "0.1.3-preview-bb2f1fdd", "@metamask-previews/composable-controller": "9.0.1-preview-bb2f1fdd", "@metamask-previews/controller-utils": "11.4.3-preview-bb2f1fdd", "@metamask-previews/ens-controller": "15.0.0-preview-bb2f1fdd", "@metamask-previews/eth-json-rpc-provider": "4.1.6-preview-bb2f1fdd", "@metamask-previews/gas-fee-controller": "22.0.1-preview-bb2f1fdd", "@metamask-previews/json-rpc-engine": "10.0.1-preview-bb2f1fdd", "@metamask-previews/json-rpc-middleware-stream": "8.0.5-preview-bb2f1fdd", "@metamask-previews/keyring-controller": "18.0.0-preview-bb2f1fdd", "@metamask-previews/logging-controller": "6.0.2-preview-bb2f1fdd", "@metamask-previews/message-manager": "11.0.1-preview-bb2f1fdd", "@metamask-previews/multichain": "0.0.0-preview-bb2f1fdd", "@metamask-previews/name-controller": "8.0.1-preview-bb2f1fdd", "@metamask-previews/network-controller": "22.0.2-preview-bb2f1fdd", "@metamask-previews/notification-controller": "7.0.0-preview-bb2f1fdd", "@metamask-previews/notification-services-controller": "0.13.0-preview-bb2f1fdd", "@metamask-previews/permission-controller": "11.0.3-preview-bb2f1fdd", "@metamask-previews/permission-log-controller": "3.0.1-preview-bb2f1fdd", "@metamask-previews/phishing-controller": "12.3.0-preview-bb2f1fdd", "@metamask-previews/polling-controller": "12.0.1-preview-bb2f1fdd", "@metamask-previews/preferences-controller": "14.0.0-preview-bb2f1fdd", "@metamask-previews/profile-sync-controller": "1.0.0-preview-bb2f1fdd", "@metamask-previews/queued-request-controller": "7.0.1-preview-bb2f1fdd", "@metamask-previews/rate-limit-controller": "6.0.1-preview-bb2f1fdd", "@metamask-previews/selected-network-controller": "19.0.0-preview-bb2f1fdd", "@metamask-previews/signature-controller": "22.0.0-preview-bb2f1fdd", "@metamask-previews/transaction-controller": "39.0.0-preview-bb2f1fdd", "@metamask-previews/user-operation-controller": "18.0.0-preview-bb2f1fdd" } ```

MajorLift commented 3 days ago

@metamaskbot publish-preview

github-actions[bot] commented 3 days ago

Preview builds have been published. See these instructions for more information about preview builds.

Expand for full list of packages and versions.

``` { "@metamask-previews/accounts-controller": "19.0.0-preview-3d44b965", "@metamask-previews/address-book-controller": "6.0.1-preview-3d44b965", "@metamask-previews/announcement-controller": "7.0.1-preview-3d44b965", "@metamask-previews/approval-controller": "7.1.1-preview-3d44b965", "@metamask-previews/assets-controllers": "44.0.0-preview-3d44b965", "@metamask-previews/base-controller": "7.0.2-preview-3d44b965", "@metamask-previews/build-utils": "3.0.1-preview-3d44b965", "@metamask-previews/chain-controller": "0.1.3-preview-3d44b965", "@metamask-previews/composable-controller": "9.0.1-preview-3d44b965", "@metamask-previews/controller-utils": "11.4.3-preview-3d44b965", "@metamask-previews/ens-controller": "15.0.0-preview-3d44b965", "@metamask-previews/eth-json-rpc-provider": "4.1.6-preview-3d44b965", "@metamask-previews/gas-fee-controller": "22.0.1-preview-3d44b965", "@metamask-previews/json-rpc-engine": "10.0.1-preview-3d44b965", "@metamask-previews/json-rpc-middleware-stream": "8.0.5-preview-3d44b965", "@metamask-previews/keyring-controller": "18.0.0-preview-3d44b965", "@metamask-previews/logging-controller": "6.0.2-preview-3d44b965", "@metamask-previews/message-manager": "11.0.1-preview-3d44b965", "@metamask-previews/multichain": "0.0.0-preview-3d44b965", "@metamask-previews/name-controller": "8.0.1-preview-3d44b965", "@metamask-previews/network-controller": "22.0.2-preview-3d44b965", "@metamask-previews/notification-controller": "7.0.0-preview-3d44b965", "@metamask-previews/notification-services-controller": "0.13.0-preview-3d44b965", "@metamask-previews/permission-controller": "11.0.3-preview-3d44b965", "@metamask-previews/permission-log-controller": "3.0.1-preview-3d44b965", "@metamask-previews/phishing-controller": "12.3.0-preview-3d44b965", "@metamask-previews/polling-controller": "12.0.1-preview-3d44b965", "@metamask-previews/preferences-controller": "14.0.0-preview-3d44b965", "@metamask-previews/profile-sync-controller": "1.0.0-preview-3d44b965", "@metamask-previews/queued-request-controller": "7.0.1-preview-3d44b965", "@metamask-previews/rate-limit-controller": "6.0.1-preview-3d44b965", "@metamask-previews/selected-network-controller": "19.0.0-preview-3d44b965", "@metamask-previews/signature-controller": "22.0.0-preview-3d44b965", "@metamask-previews/transaction-controller": "39.0.0-preview-3d44b965", "@metamask-previews/user-operation-controller": "18.0.0-preview-3d44b965" } ```

MajorLift commented 3 days ago

@metamaskbot publish-preview

github-actions[bot] commented 3 days ago

Preview builds have been published. See these instructions for more information about preview builds.

Expand for full list of packages and versions.

``` { "@metamask-previews/accounts-controller": "19.0.0-preview-335c4644", "@metamask-previews/address-book-controller": "6.0.1-preview-335c4644", "@metamask-previews/announcement-controller": "7.0.1-preview-335c4644", "@metamask-previews/approval-controller": "7.1.1-preview-335c4644", "@metamask-previews/assets-controllers": "44.0.0-preview-335c4644", "@metamask-previews/base-controller": "7.0.2-preview-335c4644", "@metamask-previews/build-utils": "3.0.1-preview-335c4644", "@metamask-previews/chain-controller": "0.1.3-preview-335c4644", "@metamask-previews/composable-controller": "9.0.1-preview-335c4644", "@metamask-previews/controller-utils": "11.4.3-preview-335c4644", "@metamask-previews/ens-controller": "15.0.0-preview-335c4644", "@metamask-previews/eth-json-rpc-provider": "4.1.6-preview-335c4644", "@metamask-previews/gas-fee-controller": "22.0.1-preview-335c4644", "@metamask-previews/json-rpc-engine": "10.0.1-preview-335c4644", "@metamask-previews/json-rpc-middleware-stream": "8.0.5-preview-335c4644", "@metamask-previews/keyring-controller": "18.0.0-preview-335c4644", "@metamask-previews/logging-controller": "6.0.2-preview-335c4644", "@metamask-previews/message-manager": "11.0.1-preview-335c4644", "@metamask-previews/multichain": "0.0.0-preview-335c4644", "@metamask-previews/name-controller": "8.0.1-preview-335c4644", "@metamask-previews/network-controller": "22.0.2-preview-335c4644", "@metamask-previews/notification-controller": "7.0.0-preview-335c4644", "@metamask-previews/notification-services-controller": "0.13.0-preview-335c4644", "@metamask-previews/permission-controller": "11.0.3-preview-335c4644", "@metamask-previews/permission-log-controller": "3.0.1-preview-335c4644", "@metamask-previews/phishing-controller": "12.3.0-preview-335c4644", "@metamask-previews/polling-controller": "12.0.1-preview-335c4644", "@metamask-previews/preferences-controller": "14.0.0-preview-335c4644", "@metamask-previews/profile-sync-controller": "1.0.0-preview-335c4644", "@metamask-previews/queued-request-controller": "7.0.1-preview-335c4644", "@metamask-previews/rate-limit-controller": "6.0.1-preview-335c4644", "@metamask-previews/selected-network-controller": "19.0.0-preview-335c4644", "@metamask-previews/signature-controller": "22.0.0-preview-335c4644", "@metamask-previews/transaction-controller": "39.0.0-preview-335c4644", "@metamask-previews/user-operation-controller": "18.0.0-preview-335c4644" } ```

Gudahtt commented 2 days ago

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?

MajorLift commented 2 days ago

@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 controllers array that includes non-controllers to instantiate both the datamodel and context fields.

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 reduce call by directly creating the Engine context property as an object keyed with controller names (this is also the approach taken by MetamaskController on extension).

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.

MajorLift commented 18 hours ago

Closing as wont-fix. Downstream consumers will need to ensure that only stateful controllers are present in the input array they pass into the ComposableController constructor.

Concerns over imposing performance penalty to mobile resolved here: https://github.com/MetaMask/metamask-mobile/pull/12374

MetaMask / core