-
-
Notifications
You must be signed in to change notification settings - Fork 1
feat: Store metadata when revoking a permission #228
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
feat: Store metadata when revoking a permission #228
Conversation
… feat/store-metadata-when-revoking-permission
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.
A couple of general comments, and, I think a more architectural concern:
I think we should remove isRevoked from the stored permission, and use the existence of revocationMetadata to determine whether the permission is revoked to help improve data integrity.
packages/gator-permissions-snap/src/clients/blockchainMetadataClient.ts
Outdated
Show resolved
Hide resolved
packages/gator-permissions-snap/src/clients/blockchainMetadataClient.ts
Outdated
Show resolved
Hide resolved
packages/gator-permissions-snap/src/clients/blockchainMetadataClient.ts
Outdated
Show resolved
Hide resolved
packages/gator-permissions-snap/src/clients/blockchainMetadataClient.ts
Outdated
Show resolved
Hide resolved
packages/gator-permissions-snap/src/clients/blockchainMetadataClient.ts
Outdated
Show resolved
Hide resolved
packages/gator-permissions-snap/src/clients/blockchainMetadataClient.ts
Outdated
Show resolved
Hide resolved
… make txHash property optional on RevocationMetadata type
…ationMetadata to handle legacy data that was created before revocationMetadata was introduced
…egation disable and transaction receipt functions checks to BlockchainMetadataClient class
packages/gator-permissions-snap/src/clients/blockchainMetadataClient.ts
Outdated
Show resolved
Hide resolved
…etadata optional. Infer TransactionReceipt type from zTransactionReceipt
- remove isRevoked from stored permission schema, rely on the existence of revocation metadata - parse legacy stored permission, inferring metadata - add recordedAt timestamp to revocation metadata
acdfdfd to
9e481b1
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.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
9e481b1 to
c42eb7b
Compare
- Use current timestamp for revocationMetadata recordedAt fields for permissions marked with legacy isRevoked - removed an errant isRevoked: false that was included on a new permission object
91442fd to
4d2e396
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.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
packages/gator-permissions-snap/src/clients/blockchainClient.ts
Outdated
Show resolved
Hide resolved
…provider snap (#7503) ## Explanation This PR extends the `GatorPermissionsController` to pass the hash of the transaction that revoked a permission (if available). ## References Requires(gator snap): [feat: Store metadata when revoking a permission](MetaMask/snap-7715-permissions#228) Required by(MM client): [chore: Bump @metamask/gator-permissions-controller to 0.9.0](MetaMask/metamask-extension#39076) ## Checklist - [x] I've updated the test suite for new or updated code as appropriate - [x] I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate - [x] I've communicated my changes to consumers by [updating changelogs for packages I've changed](https://github.com/MetaMask/core/tree/main/docs/contributing.md#updating-changelogs) - [x] I've introduced [breaking changes](https://github.com/MetaMask/core/tree/main/docs/breaking-changes.md) in this PR and have prepared draft pull requests for clients and consumer packages to resolve them <!-- CURSOR_SUMMARY --> --- > [!NOTE] > Introduces tx-aware revocations and related type/state updates. > > - **BREAKING:** `submitRevocation` now sends `txHash` via `RevocationParams`; stored permissions may include `revocationMetadata` (exported types updated) in `types.ts` and `index.ts` > - GatorPermissionsController: listens for `TransactionStatus`, skips submit on non-confirmed/failed, passes confirmed `hash` to `permissionsProvider_submitRevocation`, always refreshes permissions after terminal states > - Tests expanded to cover confirmed/failed paths and metadata propagation; minor typing/utility refinements in `decodePermission/utils` (explicit return types, generics) > - Changelog updated; bumps `@metamask/transaction-controller` > > <sup>Written by [Cursor Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit 7183271. This will update automatically on new commits. Configure [here](https://cursor.com/dashboard?tab=bugbot).</sup> <!-- /CURSOR_SUMMARY --> --------- Co-authored-by: Jeff Smale <[email protected]>
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.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
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.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
b48ffe5 to
ca86387
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.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
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!
V00D00-child
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.
LGTM. I can't approve since I opened the PR
Description
Previously, during permission revocation submission, we store a
booleanrevoked flag to track the permission revocation status in profile sync.This PR replaces this flag with
revocationMetadata, including the txHash of the transaction that set the delegation as revoked (if available), and the timestamp (in seconds) at which the permission was marked as revoked in the database (which may be different from the time that it was revoked onchain).References
Required by(Core): Attach metadata when submitting a revocation to the permission provider snap
Pre-merge author checklist
Pre-merge reviewer checklist
Note
Introduces structured revocation tracking and on-chain verification, and decouples blockchain queries into a dedicated client.
isRevokedwithrevocationMetadata(txHash?,recordedAt) in profile sync; adds legacy parsing for oldisRevoked; updates filters to infer revoked via presence ofrevocationMetadata; renamesupdatePermissionRevocationStatustomarkPermissionRevoked.BlockchainClient: Centralizes on-chain checks, includingdisabledDelegations(bytes32)andeth_getTransactionReceiptwith retry logic; moves delegation check out ofBlockchainTokenMetadataClient.submitRevocationnow accepts optionaltxHash, verifies delegation is disabled and (if provided) receiptstatusis success before persistingrevocationMetadata.TransactionReceiptzod schema and validators; extends revocation params to include optionaltxHash.BlockchainClientintoindex.tsandrpcHandler; adds/updates comprehensive unit tests for new flows and error handling.Written by Cursor Bugbot for commit ca86387. This will update automatically on new commits. Configure here.