refactor plugin controller to use v2 #13
Conversation
- add tests around plugin controller + worker controller + rpc handler
shanejonas
force-pushed
the
refactor-plugin-controller-v2
branch
from
aa22f43
to
c89e588
Compare
There was a problem hiding this comment.
Looks pretty good so far! The biggest issue left is this memstore. I'm OK with doing this in two parts if you'd prefer, removing that in a follow-up PR. But it'll be in a weird hybrid state until then.
I'll need to read up a bit more on how this worker controller works - I don't understand that part very well yet.
Co-authored-by: Mark Stacey <[email protected]>
| terminateWorkerOf: TerminateWorkerOf; | ||
| command: Command; | ||
| terminateAll: TerminateAll; | ||
| createPluginWorker: CreatePluginWorker; | ||
| startPlugin: StartPlugin; |
There was a problem hiding this comment.
Is the idea that we'll eventually extract these into that generic execution environment service we discussed?
| const plugin = plugins[pluginName]; | ||
| const newPlugin = { ...plugin, [property]: value }; | ||
| const newPlugins = { ...plugins, [pluginName]: newPlugin }; | ||
| this.updateState({ | ||
| plugins: newPlugins, | ||
| this.update((state: any) => { |
There was a problem hiding this comment.
Nit: probably worth commenting here about why we needed to use any - to avoid the incompatibility between Immer's WritableState type and recursive types like Json.
There was a problem hiding this comment.
Same for the other cases as well - anytime there's an update I suppose 🤔
| const plugin = pluginsState[pluginName]; | ||
| const { initialPermissions } = plugin; | ||
|
|
||
| // Don't prompt if there are no permissions requested: | ||
| if (Object.keys(initialPermissions).length === 0) { | ||
| return []; | ||
| } | ||
| if (initialPermissions === null) { |
There was a problem hiding this comment.
I guess this is an otherwise unrelated bug fix?
| plugins: newPlugins, | ||
| pluginStates: newPluginStates, | ||
| }); | ||
| this._removeAllPermissionsFor(pluginNames); |
There was a problem hiding this comment.
I noticed that the order of this has changed now - it's after the state update rather than before. This seems fine though. I don't know of a reason this would matter.
| metadata: { | ||
| inlinePluginIsRunning: { | ||
| persist: false, | ||
| anonymous: false, |
There was a problem hiding this comment.
We could probably make this true here? This isn't exactly personally identifiable. Though I'd imagine this is not here long-term, since it's only for testing.
You can see an example of it implemented (and try it out) here:
https://github.com/MetaMask/metamask-snaps-beta/blob/develop-temp-05-20-2021/app/scripts/metamask-controller.js#L312-L348