feat: Attach metadata when submitting a revocation to the permission provider snap #7503
Conversation
V00D00-child
changed the title
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
jeffsmale90
left a comment
jeffsmale90
left a comment
There was a problem hiding this comment.
Should we also add revocationMetadata to StoredGatorPermission (and as per my comments on the related PR, perhaps remove the isRevoked flag?
This would probably require some changes to how we load the permission from storage also, but IMO worthwhile,
packages/gator-permissions-controller/src/GatorPermissionsController.ts
Outdated
Show resolved
Hide resolved
packages/gator-permissions-controller/src/GatorPermissionsController.ts
Outdated
Show resolved
Hide resolved
jeffsmale90
left a comment
jeffsmale90
left a comment
There was a problem hiding this comment.
Looks good to me!
One nit that I'm not especially concerned about, but might be a nice tidy up.
I am keen to understand the behaviour regarding failed transactions - we definitely don't want to be marking a permission as revoked if the transaction has failed.
| // Handle confirmed transaction - submit revocation | ||
| handlers.confirmed = (transactionMeta) => { | ||
| if (transactionMeta.id === txId) { | ||
| controllerLog('Transaction confirmed, submitting revocation', { |
There was a problem hiding this comment.
not strictly related to this PR - but do we need to check transactionMeta.status here?
When a transaction is confirmed, do we explicitly guard against failed transactions?
packages/gator-permissions-controller/src/GatorPermissionsController.ts
Outdated
Show resolved
Hide resolved
| const { hash } = transactionMeta; | ||
| const revocationMetadata: RevocationMetadata = { | ||
| txHash: hash as Hex | undefined, | ||
| }; |
There was a problem hiding this comment.
Metadata object contains undefined key instead of being empty
Medium Severity
When transactionMeta.hash is undefined, the revocationMetadata object is created as { txHash: undefined } rather than {}. The test at line 1223 expects revocationMetadata: {} when no hash is provided, but the implementation always includes the txHash key even when undefined. These two object shapes are not equal in Jest's deep equality check (Object.keys({ txHash: undefined }) yields ['txHash'], while Object.keys({}) yields []). The snap receiving this data may also treat a present-but-undefined property differently from an absent property.
…no hash is provided on submitRevocation
Explanation
This PR extends the
GatorPermissionsControllerto allow attaching metadata when submitting a revocation to the permission provider snap. Metadata includes:The block timestampReferences
Requires(gator snap): feat: Store metadata when revoking a permission
Required by(MM client): chore: Bump @metamask/gator-permissions-controller to 0.9.0
Checklist
Note
BREAKING: Enables attaching metadata to permission revocations and requires it in API.
GatorPermissionsControllernow submitsrevocationMetadata(includestxHashwhen available) withpermissionsProvider_submitRevocation; captured ontransactionConfirmedinaddPendingRevocationRevocationParamsupdated to requirerevocationMetadata; newRevocationMetadatatype added and exported; index exports updatedtxHashorundefined) and that permissions refresh still occurs post-revocation@metamask/transaction-controllerpatch versionWritten by Cursor Bugbot for commit 2ea2304. This will update automatically on new commits. Configure here.