-
-
Notifications
You must be signed in to change notification settings - Fork 242
feat: add gator permissions controller #6033
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
base: main
Are you sure you want to change the base?
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.
Had a high level glance through and this looks good 👍
Just one question regarding the long-term scope of this controller.
...es/gator-permissions-controller/src/GatorPermissionsController/GatorPermissionsController.ts
Outdated
Show resolved
Hide resolved
cbf1238
to
22ab42d
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.
Owned files LGTM! Nice usage of our controllers, I'm very happy to see this.
…schema enforcement has been deprecated
…t directly to gator-snap
const defaultGatorPermissionsList: GatorPermissionsList = { | ||
'native-token-stream': {}, | ||
'native-token-periodic': {}, | ||
'erc20-token-stream': {}, |
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.
is erc20-token-periodic missing here?
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.
Yeah, need to add that type. Started building this controller before type was supported in gator-snap
.
Erc20TokenStreamPermission | ||
>, | ||
); | ||
break; |
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.
erc20-token-periodic missing here as well and at other instances where needed
break; | ||
default: | ||
throw new Error( | ||
`Unsupported permission type: ${permissionType as string}`, |
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.
Will we always have to update the controller for a new permission type? I believe the plan is also to support developer creating custom permissions which will have type 'custom' and will then need to provide a name
. I think it still makes sense to save those permissions and display to the user."
Tagging @jeffsmale90 who should have more info on this.
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.
Permission types defined by a third party would likely not be shown here.
It does feel like a reasonably large amount of overhead for surfacing permission types here for revocation.
A couple thoughts:
- can we generalise this logic so that we're not having to duplicate it for every type?
- do we want to have a case for permission types that don't fall into any of the "known" categories? It feels like there's a possibility that a user could grant a permission that the extension doesn't know how to format - we probably don't want to fail, or hide that permission. Maybe group them into "Other" permission types?
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 will likely need to update this controller for any new permission type that we support in gator-snap
to keep this controller in sync.
can we generalise this logic so that we're not having to duplicate it for every type?
Yeah, I can take a look at that.
do we want to have a case for permission types that don't fall into any of the "known" categories? It feels like there's a possibility that a user could grant a permission that the extension doesn't know how to format - we probably don't want to fail, or hide that permission. Maybe group them into "Other" permission types?
I intentionally left that out, but I can update the default statement to serve as a catch-all for all custom types. The key in GatorPermissionsList
for the see custom permissions type can be "other" or "custom and will have a known
type.
/** | ||
* Boolean value that allows DApp to define whether the permission can be attenuated–adjusted to meet the user’s terms. | ||
*/ | ||
isAdjustmentAllowed: boolean; |
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.
possibly a parameter we don't need to save
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 stored the entire permission response, so if we want to remove this data, we will need to make the change on gator-snap
first before making that change in this 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.
Bug: ERC20 Token Missing Periodic Permission Type
The Erc20TokenPeriodicPermission
type definition is missing and not included in the PermissionTypes
union. Consequently, the GatorPermissionsList
type also lacks the 'erc20-token-periodic'
key. This creates an inconsistency where native tokens support both stream and periodic permissions, but ERC20 tokens only support stream.
packages/gator-permissions-controller/src/types.ts#L56-L61
core/packages/gator-permissions-controller/src/types.ts
Lines 56 to 61 in 746a079
*/ | |
export type PermissionTypes = | |
| NativeTokenStreamPermission | |
| NativeTokenPeriodicPermission | |
| Erc20TokenStreamPermission; | |
packages/gator-permissions-controller/src/types.ts#L178-L198
core/packages/gator-permissions-controller/src/types.ts
Lines 178 to 198 in 746a079
*/ | |
export type GatorPermissionsList = { | |
'native-token-stream': { | |
[chainId: Hex]: StoredGatorPermission< | |
SignerParam, | |
NativeTokenStreamPermission | |
>[]; | |
}; | |
'native-token-periodic': { | |
[chainId: Hex]: StoredGatorPermission< | |
SignerParam, | |
NativeTokenPeriodicPermission | |
>[]; | |
}; | |
'erc20-token-stream': { | |
[chainId: Hex]: StoredGatorPermission< | |
SignerParam, | |
Erc20TokenStreamPermission | |
>[]; | |
}; | |
}; |
Bug: Missing Permission Type Handling
The #categorizePermissionsDataByTypeAndChainId
method in GatorPermissionsController.ts
is missing a case
for the 'erc20-token-periodic'
permission type in its switch
statement. This causes permissions of this type to fall into the default
case, resulting in an "Unsupported permission type" error.
packages/gator-permissions-controller/src/GatorPermissionsController.ts#L334-L373
core/packages/gator-permissions-controller/src/GatorPermissionsController.ts
Lines 334 to 373 in 746a079
switch (permissionType) { | |
case 'native-token-stream': | |
if (!gatorPermissionsList['native-token-stream'][chainId]) { | |
gatorPermissionsList['native-token-stream'][chainId] = []; | |
} | |
gatorPermissionsList['native-token-stream'][chainId].push( | |
storedGatorPermission as StoredGatorPermission< | |
SignerParam, | |
NativeTokenStreamPermission | |
>, | |
); | |
break; | |
case 'native-token-periodic': | |
if (!gatorPermissionsList['native-token-periodic'][chainId]) { | |
gatorPermissionsList['native-token-periodic'][chainId] = []; | |
} | |
gatorPermissionsList['native-token-periodic'][chainId].push( | |
storedGatorPermission as StoredGatorPermission< | |
SignerParam, | |
NativeTokenPeriodicPermission | |
>, | |
); | |
break; | |
case 'erc20-token-stream': | |
if (!gatorPermissionsList['erc20-token-stream'][chainId]) { | |
gatorPermissionsList['erc20-token-stream'][chainId] = []; | |
} | |
gatorPermissionsList['erc20-token-stream'][chainId].push( | |
storedGatorPermission as StoredGatorPermission< | |
SignerParam, | |
Erc20TokenStreamPermission | |
>, | |
); | |
break; | |
default: | |
throw new Error( | |
`Unsupported permission type: ${permissionType as string}`, | |
); | |
} |
Was this report helpful? Give feedback by reacting with 👍 or 👎
}; | ||
|
||
/** | ||
* Represents a list of gator permissions filtered by permission type. |
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.
* Represents a list of gator permissions filtered by permission type. | |
* Represents a list of gator permissions grouped by permission type. |
break; | ||
default: | ||
throw new Error( | ||
`Unsupported permission type: ${permissionType as string}`, |
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.
Permission types defined by a third party would likely not be shown here.
It does feel like a reasonably large amount of overhead for surfacing permission types here for revocation.
A couple thoughts:
- can we generalise this logic so that we're not having to duplicate it for every type?
- do we want to have a case for permission types that don't fall into any of the "known" categories? It feels like there's a possibility that a user could grant a permission that the extension doesn't know how to format - we probably don't want to fail, or hide that permission. Maybe group them into "Other" permission types?
Explanation
@metamask/gator-permissions-controller
Current State and Why Change is Needed
MetaMask clients currently lack a dedicated system for managing gator permissions that are stored to profile sync via the
@metamask/gator-permissions-snap
.Solution and How It Works
This change introduces a new
@metamask/gator-permissions-controller
package that provides a comprehensive solution for managing gator permissions in MetaMask clients with gator-snap integration.Changes That Might Not Be Obvious
state
uses JSON serialization for storing permission data fetched from@metamask/gator-permissions-snap
, which allows for efficient storage and retrieval while maintaining data integrity. The deserialize permission data is represented as a list of gator permissions filtered by permission type and chainId.Package Dependencies and Integration
The new package depends on
@metamask/snaps-controllers
as a peer dependency, ensuring it can leverage sending RPC requests to an installed Metamask Snap. This integration allows theGatorPermissionsController
to forward requests to@metamask/gator-permissions-snap
to fetch users' Gator permissions that have been stored in the MetaMask Profile Sync service.The
@metamask/gator-permissions-snap
will take on the responsibility of authenticating with MetaMask Profile Sync service using anSRP
identifier via integration with@metamask/message-signing-snap
.No Dependency Upgrades Required
This is a new package that introduces new functionality without requiring changes to existing dependencies. The package uses the current stable versions of
@metamask/base-controller
,@metamask/utils
,@metamask/snaps-sdk
, and@metamask/snaps-utils
following the established patterns in the MetaMask codebase.References
Related to(MM snap-7715-permissions): Persisting Granted Permissions with MM Profile Sync
Requires(MM snap-7715-permissions):Add new permissionsProvider_getGrantedPermissions RPC
Required by(MM Extension): MetaMask/metamask-extension#33996
Gator Permissions Data Flow
Checklist