Convert MMIController to a non-controller (without renaming it)
What is this about?
Following the Wallet Framework team's OKRs for Q3 2024, we want to bring MMIController up to date with our latest controller patterns.
After review, it turns out that MMIController does not have any state and therefore should not inherit from BaseController (or anything, for that matter).
Scenario
No response
Design
No response
Technical Details
No response
Threat Modeling Framework
No response
Acceptance Criteria
- A constant called
nameexists which holds the namespace for actions and events. -
MMIControllerdoes not inherit from a superclass (it does not have state, so it is not a controller).-
super()is removed from the constructor. -
MMIControllerusesthis.messagingSystemto refer to the messenger instead ofthis.messengerfor consistency with controllers. -
MMIControllerno longer takesmetaMetricsControlleras a constructor option.- It no longer reads the state directly from this controller, but uses the
MetaMetricsController:getStateaction through the messenger to do so instead.
- It no longer reads the state directly from this controller, but uses the
-
MMIControllerno longer takesnetworkControlleras a constructor option.- It no longer reads the state directly from this controller, but uses the
NetworkController:getStateaction through the messenger to do so instead. - It no longer calls
setActiveNetworkdirectly on the controller, but calls theNetworkController:setActiveNetworkmessenger action instead. - It no longer calls
setProviderTypeon the controller — this method is deprecated — but calls theNetworkController:setActiveNetworkmessenger action instead.
- It no longer reads the state directly from this controller, but uses the
- ❌
MMIControllerstill takesmmiConfigurationController,keyringController,preferencesController,appStateController,transactionUpdateController,custodyController,permissionController, andsignatureControlleras well asgetState,getPendingNonce,updateTransactionHash,setChannelId, andsetConnectionRequest. In theory we should use the messenger for these, but we'd need to add actions to the corresponding controllers to support this, and this would be too much work.
-
- Supporting types exist.
- The
MMIControllerActionsandMMIControllerEventstypes exist. - The
AllowedActionsandAllowedEventstypes exist. - The
MMIControllerMessengertype exists and expectsAccountsController:getAccountByAddress,AccountsController:setAccountName,AccountsController:listAccounts,AccountsController:getSelectedAccount, andAccountsController:setSelectedAccount. - The type of the
messagingSystemproperty is corrected fromany.
- The
- The tests are updated to follow suit.
- ❌ The use of
beforeEachis not corrected, as this would take too long. We can solve this in another PR.
- ❌ The use of
- ❌
MMIControlleris not renamed, as it would mess with the diff too much. We can do this in a different PR.
Stakeholder review needed before the work gets merged
- [X] 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)
- [ ] Security
- [ ] Legal
- [ ] Marketing
- [ ] Management (please specify)
- [ ] Other (please specify)
References
- https://github.com/MetaMask/core/issues/1509
We should tag the MMI team to discuss further if this feels like the right path forward.
The below part of the ticket is not covered at the moment because MetaMetricsController:getState doesn't exist yet!
MMIController no longer takes metaMetricsController as a constructor option.
It no longer reads the state directly from this controller, but uses the MetaMetricsController:getState action through the messenger to do so instead.
As we don't have state, we do not need a MMIControllerActions and also MMIControllerEvents. These two will be removed from the Acceptance Criteria.
The MMIControllerActions and MMIControllerEvents types exist.