refactor: Use permission decoders from @metamask/7715-permission-types#9164
refactor: Use permission decoders from @metamask/7715-permission-types#9164jeffsmale90 wants to merge 8 commits into
@metamask/7715-permission-types#9164Conversation
7a4724f to
7eda56a
Compare
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
@metamask/gator-permissions-controller consumes Advanced Permission definitions from @metamask/7715-permission-types
cb23bf0 to
c3fbd8e
Compare
@metamask/gator-permissions-controller consumes Advanced Permission definitions from @metamask/7715-permission-types@metamask/7715-permission-types
2719213 to
c436c43
Compare
c436c43 to
174d64e
Compare
mj-kiwi
left a comment
There was a problem hiding this comment.
Solid refactor, just a few minor notes.
| }, | ||
| "dependencies": { | ||
| "@metamask/7715-permission-types": "^0.7.1", | ||
| "@metamask/7715-permission-types": "^0.8.0", |
There was a problem hiding this comment.
Should we pump the version to 0.9.0?
| ## [Unreleased] | ||
|
|
||
| ### Changed | ||
|
|
There was a problem hiding this comment.
Small suggestion — the Unreleased entries currently just say "Changed" for switching to package decoders / bumping the dependency, but don't mention that erc20-token-revocation support seems to have been dropped along the way (see the comment on types.ts). If that drop is intentional, might be worth calling it out explicitly (possibly with a BREAKING: prefix per our changelog convention), so consumers relying on that permission type aren't caught off guard after upgrading.
| @@ -26,7 +26,6 @@ const gatorPermissionsController = new GatorPermissionsController({ | |||
| 'native-token-periodic', | |||
There was a problem hiding this comment.
Minor doc nit — the example supportedPermissionTypes array dropped erc20-token-revocation but doesn't seem to have added token-approval-revocation in its place, even though that type still looks supported. Might just need one line added back so the example stays accurate.
There was a problem hiding this comment.
I don't think this example needs to include all implemented permission types - this is showing how the controller may be instantiated with a list of permission types that should be supported.
This list can be incomplete - it's actually highlighting the capability to restrict supported permissions, but also we probably don't want to update this every time we add permission types to the dependency.
| @@ -1,11 +1,9 @@ | |||
| import type { DeployedContractsByName } from '@metamask/7715-permission-types'; | |||
There was a problem hiding this comment.
Noticed DeployedContractsByName moved from the precise local alias (typeof DELEGATOR_CONTRACTS)[number][number] to the package's Record<string, Hex>. Not a big deal, but it does mean a typo'd contract name would only surface at runtime (via the Contract not found throw) rather than at compile time like before. Wanted to flag in case that tradeoff wasn't deliberate.
| @@ -9,12 +9,8 @@ import type { | |||
| ApprovalRevocationTerms, | |||
| Caveat, | |||
There was a problem hiding this comment.
While in here — it looks like Erc20TokenAllowancePermission, NativeTokenAllowancePermission, and TokenApprovalRevocationPermission (lines 23, 40, 53 below) are still declared locally with a "not yet defined in the package" comment, but @metamask/7715-permission-types@^0.8.0 seems to export all three now (worth double-checking, since the local TokenApprovalRevocationPermission shape looks slightly different from the package's version). If the package really does cover these now, importing them directly would avoid future drift.
There was a problem hiding this comment.
Good call - I've removed the 3 locally defined permission types, along with ExtendedPermissionTypes
| import { makeErc20TokenPeriodicDecoderConfig } from './erc20TokenPeriodic'; | ||
| import { makeErc20TokenRevocationDecoderConfig } from './erc20TokenRevocation'; | ||
| import { makeErc20TokenStreamDecoderConfig } from './erc20TokenStream'; | ||
| import { makePermissionDecoderConfigs } from '@metamask/7715-permission-types'; |
There was a problem hiding this comment.
Just flagging for awareness — with per-type validation logic now living in @metamask/7715-permission-types, this package's tests only exercise a synthetic specified-permission-type rather than the real permission types end-to-end. Might be worth a lightweight integration test here (or confirming coverage exists upstream) so a future bump of the dependency that loosens a validation rule wouldn't slip by unnoticed.
| @@ -1,45 +1,22 @@ | |||
| import type { Rule } from '@metamask/7715-permission-types'; | |||
| import type { | |||
There was a problem hiding this comment.
Small one — enforcer addresses look like they get checksummed once for matching, then re-checksummed again per candidate decoder inside validateAndDecodePermission on the ambiguous-type path. Probably not costly today, but could be worth reusing the already-checksummed set if this path gets exercised more.
| @@ -9,12 +9,8 @@ import type { | |||
| ApprovalRevocationTerms, | |||
There was a problem hiding this comment.
RuleDecoder looks like it's only referenced from a test file now, not from any production decoder — the real shape seems to live in the package's PermissionDecoderConfig. If that's right, might be simpler to move it into the test file (or drop it) so it doesn't read as something production code still relies on.
| import type { ChecksumEnforcersByChainId } from './types'; | ||
|
|
||
| /** | ||
| * The names of the enforcer contracts for each permission type. |
There was a problem hiding this comment.
Nit — ENFORCER_CONTRACT_NAMES maps each key to an identical string value, so getChecksumEnforcersByChainId ends up repeating the same 3-line pattern 12 times. Could probably just use the string literals directly and drop the map, but totally optional.
… narrow scope to just test this function
Also fix various linting issues
a4ef722 to
7bc7040
Compare
7bc7040 to
7cbbc65
Compare
Explanation
MetaMask/smart-accounts-kit#259 adds permission decoder definitions to @metamask/7715-permission-types released in 0.8.0.
This PR updates @metamask/gator-permissions-controller to consume these permission definitions instead of defining the permission decoders directly in the controller.
Note:
erc20-token-revocationpermission type is no longer supported.References
Checklist
Note
High Risk
Breaking removal of
erc20-token-revocationand a large shift of delegation decode logic to an external dependency affect how permissions are interpreted—a security-sensitive path for wallet users.Overview
Delegates permission decoding to
@metamask/7715-permission-types(^1.0.0) instead of maintaining per-type decoders and rule helpers insidegator-permissions-controller.createPermissionDecodersForContractsnow builds decoders frommakePermissionDecoderConfigs, and chain enforcer addresses are normalized via newenforcerAddressesmapping before decode runs inGatorPermissionsController.Breaking:
erc20-token-revocationis removed from supported types, docs, and tests. Local decoder/rule implementations and their exhaustive unit tests are deleted; controller-level integration tests cover decode for the remaining permission types.makePermissionDecoderis kept as a thin adapter over sharedPermissionDecoderConfig, with checksummed enforcer handling aligned to the external package.Reviewed by Cursor Bugbot for commit 7cbbc65. Bugbot is set up for automated code reviews on this repo. Configure here.