feat: Implement wallet initialization library#8838
Conversation
|
@metamaskbot publish-previews |
|
Preview builds have been published. Learn how to use preview builds in other projects. Expand for full list of packages and versions. |
|
@metamaskbot publish-previews |
|
Preview builds have been published. Learn how to use preview builds in other projects. Expand for full list of packages and versions. |
Hmm. I wonder if "instances" sounds a bit too vague. It's true they are instances of classes, but then so are lots of other things. For context, we discussed a unified term for controllers and services in this PR: https://github.com/MetaMask/decisions/pull/41#discussion_r1809429486. I had some ideas such as "messaging actor" (or just "actor"), but I think "messenger client" was the least worst option (and the one that Mark also agreed upon). "Messageable" was also a contender. Any of these options appeal to you? |
mcmire
left a comment
mcmire
left a comment
There was a problem hiding this comment.
Going in a good direction. Here are some thoughts and comments.
There was a problem hiding this comment.
We have been advising teams not to add wildcard ("barrel") files. My understanding has been that it can increase the potential workflow of tools like ESLint (because it increases the number of routes these tools can take to find a file). There is an article I found (amazingly, from Atlassian) that goes into more detail here: https://www.atlassian.com/blog/atlassian-engineering/faster-builds-when-removing-barrel-files
What are your thoughts on this, is it possible to import these files directly where they are needed?
There was a problem hiding this comment.
For the default configurations, we somewhat need a wildcard import:
Unless we want to have an ever-growing list of imports here.
Though it isn't strictly required anywhere else, if that is the recommendation, I'm happy to follow.
| export type InitializationConfiguration<Instance, InstanceMessenger> = { | ||
| name: InstanceName<Instance>; | ||
| init(args: InitFunctionArguments<Instance, InstanceMessenger>): Instance; | ||
| messenger(parent: RootMessenger): InstanceMessenger; |
There was a problem hiding this comment.
What do you think about calling this method getMessenger instead of just messenger?
| messenger(parent: RootMessenger): InstanceMessenger; | |
| getMessenger(parent: RootMessenger): InstanceMessenger; |
There was a problem hiding this comment.
Yeah, that could make sense!
| ); | ||
| } | ||
|
|
||
| get messenger(): Readonly<RootMessenger<DefaultActions, DefaultEvents>> { |
There was a problem hiding this comment.
I am uneasy about using getters because I feel like if something is a method then it should be obvious when calling it, rather than hiding it behind property syntax. Are we using a getter, however, for compatibility with controllers/services?
There was a problem hiding this comment.
I was thinking we would do something similar to BaseController where we create setters that throw so that the messenger, state etc aren't assigned to.
| AllowedActions extends ActionConstraint = ActionConstraint, | ||
| AllowedEvents extends EventConstraint = EventConstraint, |
There was a problem hiding this comment.
Does this need to take type parameters? It would be nice if RootMessenger could never be unconstrained. Would it make sense to default these type parameters to DefaultActions and DefaultEvents? Or maybe we don't need to have defaults at all?
| AllowedActions extends ActionConstraint = ActionConstraint, | |
| AllowedEvents extends EventConstraint = EventConstraint, | |
| AllowedActions extends ActionConstraint, | |
| AllowedEvents extends EventConstraint, |
There was a problem hiding this comment.
Eventually it'll be useful to expand the types beyond the defaults, if possible. I removed the defaults for now, but they could probably be DefaultActions and DefaultEvents.
| : unknown; | ||
|
|
||
| export type InitFunctionArguments<Instance, InstanceMessenger> = { | ||
| state: InstanceState<Instance>; |
There was a problem hiding this comment.
Hmm. Services don't have state. So does this make sense as an argument to all init functions?
(Edit: I guess init functions for services don't need to care about this, is that the thought?)
There was a problem hiding this comment.
I guess init functions for services don't need to care about this, is that the thought
Essentially yes. Though we may be able to tighten the typing a bit by making InstanceState be null or something for services.
| initializationConfigurations?: InitializationConfiguration< | ||
| unknown, | ||
| unknown | ||
| >[]; | ||
| instanceOptions?: InstanceSpecificOptions; |
There was a problem hiding this comment.
I'm a bit concerned that engineers will get confused on the difference between initializationConfigurations and instanceOptions.
Is it true that:
initializationConfigurationsconfigures an array of instances which are added on the default set of instances?instanceOptionsconfigures constructors for default instances?
If so should we rename these options to something like this?
| initializationConfigurations?: InitializationConfiguration< | |
| unknown, | |
| unknown | |
| >[]; | |
| instanceOptions?: InstanceSpecificOptions; | |
| additionalInitializationConfigurations?: InitializationConfiguration< | |
| unknown, | |
| unknown | |
| >[]; | |
| optionsForDefaultInstances?: InstanceSpecificOptions; |
There was a problem hiding this comment.
The idea was that you would use initializationConfigurations for either overriding any default configuration or adding a client-specific configuration. E.g. for controllers that only live in one of the clients.
The instanceOptions are for any options that cannot be standardized across clients, for example keyringBuilders, encryptor, infuraProjectId etc.
optionsForDefaultInstances isn't technically true since you could use it to pass options to a configuration specified in initializationConfigurations. Though I guess that is a bit pointless 😅
additionalInitializationConfigurations seems like confusing naming given the override behaviour mentioned above.
|
@metamaskbot publish-previews |
@mcmire I haven't spent much time thinking about this, when the prototyping started I don't believe we had adopted "messenger client" on the MetaMask clients yet. If that already is our decided preferred naming, I can make changes, but "messenger client" seems similarly vague and maybe even an overloaded term considering API clients, the MetaMask clients themselves (extension/mobile) etc. |
|
Preview builds have been published. Learn how to use preview builds in other projects. Expand for full list of packages and versions. |
2 participants
Explanation
This PR implements a narrowly-scoped (as compared to the original feature branch) version of the wallet initialization library that only includes initializing the
KeyringController. This can eventually be used to demonstrate the integration of the library into the clients and serves as the base for future work.Overall it works in a similarly to the initialization pattern used in extension and mobile today, with some differences:
InitializationConfiguration. This object contains both a function to setup the messenger and the instance.InitializationConfiguration.messengeris expected to have access to actions/events necessary to initialize and operate the instance.There is no way to access the instances directly.The
Walletinstance provides access to the instances within using themessengerwhile also exposing a limited set of useful properties likestateandcontrollerMetadata.References
https://consensyssoftware.atlassian.net/browse/WPC-999
Checklist