-
-
Notifications
You must be signed in to change notification settings - Fork 183
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
Migrate PermissionController and SubjectMetadataController #692
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work! I can see why this took longer than expected. Just a handful of things to clear up.
src/util.ts
Outdated
req: JsonRpcRequest<U>, | ||
res: PendingJsonRpcResponse<V>, | ||
next: JsonRpcEngineNextCallback, | ||
end: JsonRpcEngineEndCallback, | ||
hooks: T, | ||
) => void | Promise<void>; | ||
|
||
type BaseHandlerExport = { | ||
methodNames: string[]; | ||
}; | ||
|
||
/** | ||
* We use a mapped object type in order to create a type that requires the | ||
* presence of the names of all hooks for the given handler. | ||
* This can then be used to select only the necessary hooks whenever a method | ||
* is called for purposes of POLA. | ||
*/ | ||
export type HookNames<T> = { | ||
[Property in keyof T]: true; | ||
}; | ||
|
||
export type PermittedHandlerExport<T, U, V> = { | ||
implementation: HandlerMiddlewareFunction<T, U, V>; | ||
hookNames: HookNames<T>; | ||
} & BaseHandlerExport; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's import this from @metamask/rpc-methods
instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that might be problematic, as we might want to reference @metamask/controllers
types in the RPC methods at some point. For example, when connecting the Snap RPC method to the new notification controller 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It'll be a circular dependency then, right? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We already have a circular dependency between @metamask/rpc-methods
and the permission controller in @metamask/snap-controllers
, so this wouldn't be a new problem at least. But it doesn't seem ideal. Maybe better to move this type from @metamask/rpc-methods
to the @metamask/types
package, and reference it in both places?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR should be good to go once we import those types @metamask/types
.
41a8c2c
to
8d64247
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
* Initial copy-paste * Fix a bunch of imports etc * Fix some typing issues * Fix another type issue * Fix linting issues + add missing doc strings * Run Prettier on README * Fix some test typing issues + ignore others * Revert a recent change * Remove unused function * Remove remains of bold formatting * Fix usage of messengers to improve test typing * Fix some issues with JSDocs * Add util function tests * Use @metamask/types for a bunch of shared types * Fix a few import issues * Remove unnecessary ts-expect-error comments
* Initial copy-paste * Fix a bunch of imports etc * Fix some typing issues * Fix another type issue * Fix linting issues + add missing doc strings * Run Prettier on README * Fix some test typing issues + ignore others * Revert a recent change * Remove unused function * Remove remains of bold formatting * Fix usage of messengers to improve test typing * Fix some issues with JSDocs * Add util function tests * Use @metamask/types for a bunch of shared types * Fix a few import issues * Remove unnecessary ts-expect-error comments
Migrated
PermissionsController
andSubjectMetadataController
fromsnaps-skunkworks
.Description
PermissionsController
andSubjectMetadataController
Would probably recommend looking through some of the history when reviewing this. The very first commit is a 1:1 copy paste of the files from
snaps-skunkworks
. The following commits are the changes made to satisfy linting, making it build and have tests run successfully.Checklist
Issue
Resolves: MetaMask/snaps#197