-
-
Notifications
You must be signed in to change notification settings - Fork 262
feat: Attach metadata when submitting a revocation to the permission provider snap #7503
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?
feat: Attach metadata when submitting a revocation to the permission provider snap #7503
Conversation
jeffsmale90
left a comment
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.
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
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
| const revocationMetadata: RevocationMetadata = {}; | ||
| const { hash } = transactionMeta; | ||
| if (hash === undefined) { | ||
| controllerLog( | ||
| 'Failed to attach transaction hash after revocation transaction confirmed', | ||
| { | ||
| txId, | ||
| permissionContext, | ||
| error: new Error( | ||
| 'Confirmed transaction is missing transaction hash', | ||
| ), | ||
| }, | ||
| ); | ||
| } else { | ||
| revocationMetadata.txHash = hash as Hex; | ||
| } |
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.
nit: you could simplify this - moving the assignment out of the else block, and creating the revocationMetadata in a single statement:
| const revocationMetadata: RevocationMetadata = {}; | |
| const { hash } = transactionMeta; | |
| if (hash === undefined) { | |
| controllerLog( | |
| 'Failed to attach transaction hash after revocation transaction confirmed', | |
| { | |
| txId, | |
| permissionContext, | |
| error: new Error( | |
| 'Confirmed transaction is missing transaction hash', | |
| ), | |
| }, | |
| ); | |
| } else { | |
| revocationMetadata.txHash = hash as Hex; | |
| } | |
| const { hash } = transactionMeta; | |
| const revocationMetadata: RevocationMetadata = { | |
| txHash: hash as Hex | undefined, | |
| }; | |
| if (hash === undefined) { | |
| controllerLog( | |
| 'Failed to attach transaction hash after revocation transaction confirmed', | |
| { | |
| txId, | |
| permissionContext, | |
| error: new Error( | |
| 'Confirmed transaction is missing transaction hash', | |
| ), | |
| }, | |
| ); | |
| } |
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
Introduces metadata propagation when submitting revocations and updates types accordingly.
RevocationParamsnow requiresrevocationMetadata;submitRevocationandsubmitDirectRevocationforward it to the snaphashand attach it torevocationMetadata.txHash(logs if missing)RevocationMetadatatype; updates imports/exports inindex.tsand controllerrevocationMetadata, verify params sent topermissionsProvider_submitRevocation, and confirm permissions refresh after revocationWritten by Cursor Bugbot for commit 50338af. This will update automatically on new commits. Configure here.