-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Convert MMIController to a non-controller (without renaming it) #25926
Labels
Comments
29 tasks
mcmire
changed the title
Migrate MMIController to BaseController v2
Convert MMIController to a non-controller (without renaming it)
Jul 24, 2024
We should tag the MMI team to discuss further if this feels like the right path forward. |
7 tasks
The below part of the ticket is not covered at the moment because
|
As we don't have state, we do not need a MMIControllerActions and also MMIControllerEvents. These two will be removed from the Acceptance Criteria.
|
github-merge-queue bot
pushed a commit
that referenced
this issue
Nov 7, 2024
<!-- Please submit this PR as a draft initially. Do not mark it as "Ready for review" until the template has been completely filled out, and PR status checks have passed at least once. --> ## **Description** 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). <!-- Write a short description of the changes included in this pull request, also include relevant motivation and context. Have in mind the following questions: 1. What is the reason for the change? 2. What is the improvement/solution? --> [![Open in GitHub Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/27983?quickstart=1) ## **Related issues** Fixes: #25926 ## **Manual testing steps** N/A ## **Screenshots/Recordings** <!-- If applicable, add screenshots and/or recordings to visualize the before and after of your change. --> ### **Before** <!-- [screenshots/recordings] --> ### **After** <!-- [screenshots/recordings] --> ## **Pre-merge author checklist** - [x] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Extension Coding Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md). - [x] I've completed the PR template to the best of my ability - [x] I’ve included tests if applicable - [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [x] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. ## **Pre-merge reviewer checklist** - [ ] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [ ] I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.
metamaskbot
added
the
release-12.8.0
Issue or pull request that will be included in release 12.8.0
label
Nov 7, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
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 fromBaseController
(or anything, for that matter).Scenario
No response
Design
No response
Technical Details
No response
Threat Modeling Framework
No response
Acceptance Criteria
name
exists which holds the namespace for actions and events.MMIController
does not inherit from a superclass (it does not have state, so it is not a controller).super()
is removed from the constructor.MMIController
usesthis.messagingSystem
to refer to the messenger instead ofthis.messenger
for consistency with controllers.MMIController
no longer takesmetaMetricsController
as a constructor option.MetaMetricsController:getState
action through the messenger to do so instead.MMIController
no longer takesnetworkController
as a constructor option.NetworkController:getState
action through the messenger to do so instead.setActiveNetwork
directly on the controller, but calls theNetworkController:setActiveNetwork
messenger action instead.setProviderType
on the controller — this method is deprecated — but calls theNetworkController:setActiveNetwork
messenger action instead.MMIController
still takesmmiConfigurationController
,keyringController
,preferencesController
,appStateController
,transactionUpdateController
,custodyController
,permissionController
, andsignatureController
as 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.MMIControllerActions
andMMIControllerEvents
types does not exist.AllowedActions
andAllowedEvents
types exist.MMIControllerMessenger
type exists and expectsAccountsController:getAccountByAddress
,AccountsController:setAccountName
,AccountsController:listAccounts
,AccountsController:getSelectedAccount
, andAccountsController:setSelectedAccount
.messagingSystem
property is corrected fromany
.beforeEach
is not corrected, as this would take too long. We can solve this in another PR.MMIController
is 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
References
The text was updated successfully, but these errors were encountered: