refactor: modular controller init #28948
Conversation
Builds ready [a41b2b1]
Page Load Metrics (1809 ± 62 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Builds ready [57a0608]
Page Load Metrics (1867 ± 58 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Builds ready [dede03f]
Page Load Metrics (2194 ± 69 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Builds ready [3ccc1a8]
Page Load Metrics (1789 ± 34 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Builds ready [69479a8]
Page Load Metrics (1870 ± 67 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Builds ready [b1ffbd5]
Page Load Metrics (1939 ± 102 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Add legacy init class. Add JSDoc.
Builds ready [c94d6e0]
Page Load Metrics (1990 ± 83 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Builds ready [55da18e]
Page Load Metrics (2072 ± 113 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Builds ready [dd59867]
Page Load Metrics (2036 ± 102 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Builds ready [fc39319]
Page Load Metrics (2036 ± 99 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Builds ready [8d2801c]
Page Load Metrics (1980 ± 74 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
app/scripts/controller-init/messengers/transaction-controller-messenger.ts
Show resolved
Hide resolved
|
Hey Matt, love this idea—breaking up metamask-controller.js and adding types to the controller initialization would be a huge win. Thanks for suggesting it! |
Builds ready [cf22570]
Page Load Metrics (1817 ± 233 ms)
|
Builds ready [584c15b]
Page Load Metrics (1721 ± 128 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
|
@davidmurdoch I just did a circular dependencies analysis on this PR. These circular deps were removed:
These circular deps were added:
Though I'm a little confused how this happened at all, when none of these files were changed 🤷 |
There was a problem hiding this comment.
I like the overall structure we're using here, and a lot of the ideas for how to organize the initialization code. I left a lot of comments about the "TypeScript migration" parts of this, but my last comment on what each of these abstractions are for is probably the most relevant one for the overall plan here.
Have you considered writing an ADR about this? An ADR might be a good place to explain the overall layout/structure independently of the controller-specific details. This may help not only in explaining the pattern, but in helping others replicate it for other controllers (and especially for mobile). There are so many details in these examples, it's a bit hard to see the big picture.
Though it's great that we have such a complex example to work from, surely this hits most complexities we'll encounter in other controllers.
| storageBackend: new IndexedDBPPOMStorage('PPOMDB', 1), | ||
| provider: getProvider(), | ||
| ppomProvider: { | ||
| // @ts-expect-error Mismatched types |
There was a problem hiding this comment.
Any idea why there is a type mismatch here?
| PPOM: PPOMModule.PPOM, | ||
| ppomInit: () => PPOMModule.default(process.env.PPOM_URI), | ||
| }, | ||
| state: persistedState.PPOMController as PPOMController['state'], |
There was a problem hiding this comment.
Nit: Hmm, this isn't ideal. How can we ensure the persisted state is correctly typed?
It would be nice to avoid casting, as this is contrary to our TypeScript best practices. At the very least we should explain in a comment why we're casting here when we normally wouldn't.
Unfortunately it would be an overly large amount of work to avoid type safety problems altogether, as we don't validate it at any point. That would be beyond the scope of this PR. But a great deal of the handling of this state is still in JavaScript, so it should be easily avoided. At this point in the initialization process, it's fair to say that we expect the type to be { [ControllerName]: Partial<ControllerState> }, so maybe we can type it as that for now, and figure out how to maintain type safety before here as a later problem.
| chainId: getGlobalChainId(), | ||
| securityAlertsEnabled: | ||
| preferencesController().state.securityAlertsEnabled, | ||
| // @ts-expect-error Mismatched types |
There was a problem hiding this comment.
Nit: Here is a bit more detail on the type issue here:
| // @ts-expect-error Mismatched types | |
| // @ts-expect-error `onPreferencesChange` type signature is incorrect in `PPOMController` | |
| // TODO: Use messenger directly in `PPOMController` rather than callback |
| initMessenger, | ||
| 'PreferencesController:stateChange', | ||
| ), | ||
| cdnBaseUrl: process.env.BLOCKAID_FILE_CDN as string, |
There was a problem hiding this comment.
Nit: Here is more detail on why we're casting here:
| cdnBaseUrl: process.env.BLOCKAID_FILE_CDN as string, | |
| // Both of these are given default values in `builds.yml`, so they should always be set to something | |
| cdnBaseUrl: process.env.BLOCKAID_FILE_CDN as string, |
There was a problem hiding this comment.
Hmm. Not sure what to expect from tests at this layer. I see that every constructor parameter isn't tested for example, and for the messenger callbacks we're not testing what capabilities they have. But maybe that's OK.
What do you see as the goals of these tests?
| (initObject as InitInstance).init | ||
| ? (initObject as InitInstance) | ||
| : new LegacyControllerInit(initObject as InitFunction), |
There was a problem hiding this comment.
Rather than using unsafe type assertions here, we can use a type guard:
| (initObject as InitInstance).init | |
| ? (initObject as InitInstance) | |
| : new LegacyControllerInit(initObject as InitFunction), | |
| isInitInstance(initObject) | |
| ? initObject | |
| : new LegacyControllerInit(initObject), |
Using a type guard like this:
/**
* Type guard to distinguish the two types of `InitObject`.
*
* @param initObject - The object to check
* @returns Returns `true` if it's an `InitInstance`, false otherwise.
*/
function isInitInstance(initObject: InitObject): initObject is InitInstance {
return 'init' in initObject;
}
| /** | ||
| * Adapter class to handle legacy controllers that are returned from a function. | ||
| */ | ||
| class LegacyControllerInit extends ControllerInit< |
There was a problem hiding this comment.
Perhaps there's a better term for these 🤔. We've been talking about "legacy controllers" for a while, but in this case it's a "legacy pattern for constructing controllers" I guess.
e.g. maybe we could call this ControllerFunctionInit - init based on a controller function.
| @@ -5704,6 +5562,7 @@ export default class MetamaskController extends EventEmitter { | |||
| handleUpdate(); | |||
| }, | |||
| getStatePatches: () => patchStore.flushPendingPatches(), | |||
| ...this.controllerApi, | |||
There was a problem hiding this comment.
Nit: Perhaps we could move this to before the patch methods, so they can't be accidentally overwritten? Not that that seems especially likely, but it's certainly not something we ever want to be possible.
| * Includes reducer properties. | ||
| * For example: `{ metamask: { transactions: [] } }`. | ||
| */ | ||
| getStateUI: () => unknown & { metamask: unknown }; |
There was a problem hiding this comment.
This is misleading because we're not really getting the UI state here. We're getting the flattened state nested in an object under the metamask key.
Maybe we could build this on-the-fly using getFlatState, and drop this from here? Just to simplify the initial request object.
| * Request to initialize and return a controller instance. | ||
| * Includes standard data and methods not coupled to any specific controller. | ||
| */ | ||
| export type ControllerInitRequest< |
There was a problem hiding this comment.
This proposal is abstraction-heavy; we're introducing a bunch of new constructs to facilitate this pattern of splitting up controller initialization. I'm wondering if we could cut any of them out and make this process simpler to understand.
If I'm understanding correctly, the current process is roughly:
- Controllers are initialized by
#initControllersinmetamask-controller.js #initControllerscalls theinitControllersfunction, passing in relevant state that onlymetamask-controller.jshas (persisted state, functions, etc.)initControllerswill, for each controller:- Create an "Init instance"
- This instance doesn't have access to any global capabilities. It receives them when it's initialized, and uses them to complete controller-specific initialization
- Create an "init request", and use it to initialize each "init object", which in turn initializes the controller.
- This request passes in a custom "init messenger" and "messenger" for each controller, but otherwise is responsible for passing in the same global functions to all controllers.
- Create a "API request" with the controller intance and an even more limited set of global functions (just one right now), and use this to get the "controller API" functions for each controller
- Use the return value of
getApiand various other bits of information from the "init object" to construct a collection of all controller API methods, memstate, persisted state, and a map of controllers by name.
- Create an "Init instance"
- Then finally,
initControllersreturns these four collections tometamask-controller.jsto finalize initialization.
The first two steps I understand, those seem necessary (#initControllers for handling state, initControllers for performing initialization with state). But after that point I'm not sure.
I can see the benefits of having a custom "init request" per controller, in that it enables this "init messenger" pattern. We could do without an "init messenger", but it would require us to pass around controller instances to every initializer, which is not ideal for auditability (it'd make refactors of controllers much more painful). In theory we shouldn't need any special capabilities to initialize a controller, controllers can request capabilities from the messenger directly, but in practice we're a long way off from every controller behaving that way. The "init messenger" seems like a good attempt at improving the situation in the short-term, even if it does come at some cost in requiring a custom "init request" per controller.
The benefits of "init instance" and "API request" are less clear to me. Could we have an "initialize" function per controller instead, which returns the controller alongside a collection of API methods, memstate, and persisted state? What benefit does a separate "init instance" and "API request" have? The "API request" part seems especially strange given that the functions it's passed (the controller instance and the getFlatState function) are already accessible from the "init instance", which is where getApi is defined.
If it works just as well as a function, then this would be a good opportunity to reduce cognitive load by reducing the number of abstractions we're using here. But perhaps there is some benefit I am not seeing.
Description
Architecture to support modular controller initialisation to avoid the current monolithic
metamask-controller.js.Includes:
PPOMControlleras a simple example.TransactionControlleras perhaps the most complex example.Overview
Uses isolated class files (in
app/scripts/controller-init/) extending aControllerInitbase class, each of which defines:initmethod to return the controller instance given aControllerInitRequest.getControllerMessengerCallbackoverride to specify callback to generate controller messenger.getInitMessengerCallbackoverride to specify callback to generate initialisation controller messenger.getApioverride to return object of background API functions.getPersistedStateKeyto override persisted state key, or disable.getMemStateKeyto override mem store state key, or disable.Dependent controllers can be retrieved via
getControllerfunction inControllerInitRequestto provide standard error message if controller is not yet initialised.Messengers are intentionally defined in separate isolated files to allow alternate code ownership, and not over-power the controller logic and introduce bugs.
Initialisation logic is encapsulated in
app/scripts/controller-init/utils.ts, which iterates over theControllerInitinstances to construct the controller instances, plus generate the API and store properties.Advantages
Related issues
Manual testing steps
Screenshots/Recordings
Before
After
Pre-merge author checklist
Pre-merge reviewer checklist