From 1eaaee416534e4e259cf8ff04fe41c48ca754954 Mon Sep 17 00:00:00 2001 From: hanzel98 Date: Tue, 25 Nov 2025 21:42:09 -0600 Subject: [PATCH 1/5] feat(gator-permissions-controller): add submitDirectRevocation for already-disabled delegations - Add submitDirectRevocation method that combines adding to pending state and submitting revocation - Add isPendingRevocation helper method to check if a permission is pending - Update submitRevocation to use finally block for clearing pending revocations - Export new action types in index.ts --- .../gator-permissions-controller/CHANGELOG.md | 7 ++ .../src/GatorPermissionsController.ts | 85 ++++++++++++++++++- .../gator-permissions-controller/src/index.ts | 2 + 3 files changed, 90 insertions(+), 4 deletions(-) diff --git a/packages/gator-permissions-controller/CHANGELOG.md b/packages/gator-permissions-controller/CHANGELOG.md index d720b08aee8..378472aeee9 100644 --- a/packages/gator-permissions-controller/CHANGELOG.md +++ b/packages/gator-permissions-controller/CHANGELOG.md @@ -7,6 +7,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Added + +- Add `submitDirectRevocation` method for already-disabled delegations that don't require an on-chain transaction ([#TBD](https://github.com/MetaMask/core/pull/TBD)) + - This method adds a pending revocation to state and immediately submits the revocation + - Includes `isPendingRevocation` helper method to check if a permission context is pending revocation + - Pending revocations are automatically cleared in a `finally` block to prevent stuck states + ### Changed - Move peer dependencies for controller and service packages to direct dependencies ([#7209](https://github.com/MetaMask/core/pull/7209), [#7220](https://github.com/MetaMask/core/pull/7220), [#7236](https://github.com/MetaMask/core/pull/7236)) diff --git a/packages/gator-permissions-controller/src/GatorPermissionsController.ts b/packages/gator-permissions-controller/src/GatorPermissionsController.ts index 4c5124f0b22..2675aa41d99 100644 --- a/packages/gator-permissions-controller/src/GatorPermissionsController.ts +++ b/packages/gator-permissions-controller/src/GatorPermissionsController.ts @@ -227,6 +227,23 @@ export type GatorPermissionsControllerAddPendingRevocationAction = { handler: GatorPermissionsController['addPendingRevocation']; }; +/** + * The action which can be used to submit a revocation directly without requiring + * an on-chain transaction (for already-disabled delegations). + */ +export type GatorPermissionsControllerSubmitDirectRevocationAction = { + type: `${typeof controllerName}:submitDirectRevocation`; + handler: GatorPermissionsController['submitDirectRevocation']; +}; + +/** + * The action which can be used to check if a permission context is pending revocation. + */ +export type GatorPermissionsControllerIsPendingRevocationAction = { + type: `${typeof controllerName}:isPendingRevocation`; + handler: GatorPermissionsController['isPendingRevocation']; +}; + /** * All actions that {@link GatorPermissionsController} registers, to be called * externally. @@ -238,7 +255,9 @@ export type GatorPermissionsControllerActions = | GatorPermissionsControllerDisableGatorPermissionsAction | GatorPermissionsControllerDecodePermissionFromPermissionContextForOriginAction | GatorPermissionsControllerSubmitRevocationAction - | GatorPermissionsControllerAddPendingRevocationAction; + | GatorPermissionsControllerAddPendingRevocationAction + | GatorPermissionsControllerSubmitDirectRevocationAction + | GatorPermissionsControllerIsPendingRevocationAction; /** * All actions that {@link GatorPermissionsController} calls internally. @@ -390,6 +409,16 @@ export default class GatorPermissionsController extends BaseController< `${controllerName}:addPendingRevocation`, this.addPendingRevocation.bind(this), ); + + this.messenger.registerActionHandler( + `${controllerName}:submitDirectRevocation`, + this.submitDirectRevocation.bind(this), + ); + + this.messenger.registerActionHandler( + `${controllerName}:isPendingRevocation`, + this.isPendingRevocation.bind(this), + ); } /** @@ -743,9 +772,8 @@ export default class GatorPermissionsController extends BaseController< snapRequest, ); - this.#removePendingRevocationFromStateByPermissionContext( - revocationParams.permissionContext, - ); + // Refresh list first (permission removed from list) + await this.fetchAndUpdateGatorPermissions({ isRevoked: false }); controllerLog('Successfully submitted revocation', { permissionContext: revocationParams.permissionContext, @@ -762,6 +790,10 @@ export default class GatorPermissionsController extends BaseController< GatorPermissionsSnapRpcMethod.PermissionProviderSubmitRevocation, cause: error as Error, }); + } finally { + this.#removePendingRevocationFromStateByPermissionContext( + revocationParams.permissionContext, + ); } } @@ -984,4 +1016,49 @@ export default class GatorPermissionsController extends BaseController< cleanup(txId); }, PENDING_REVOCATION_TIMEOUT); } + + /** + * Submits a revocation directly without requiring an on-chain transaction. + * Used for already-disabled delegations that don't require an on-chain transaction. + * + * This method: + * 1. Adds the permission context to pending revocations state (disables UI button) + * 2. Immediately calls submitRevocation to remove from snap storage + * 3. On success, removes from pending revocations state (re-enables UI button) + * 4. On failure, keeps in pending revocations so UI can show error/retry state + * + * @param params - The revocation parameters containing the permission context. + * @returns A promise that resolves when the revocation is submitted successfully. + * @throws {GatorPermissionsNotEnabledError} If the gator permissions are not enabled. + * @throws {GatorPermissionsProviderError} If the snap request fails. + */ + public async submitDirectRevocation(params: RevocationParams): Promise { + this.#assertGatorPermissionsEnabled(); + + // Use a placeholder txId that doesn't conflict with real transaction IDs + const placeholderTxId = `no-tx-${params.permissionContext}`; + + // Add to pending revocations state first (disables UI button immediately) + this.#addPendingRevocationToState( + placeholderTxId, + params.permissionContext, + ); + + // Immediately submit the revocation (will remove from pending on success) + await this.submitRevocation(params); + } + + /** + * Checks if a permission context is in the pending revocations list. + * + * @param permissionContext - The permission context to check. + * @returns `true` if the permission context is pending revocation, `false` otherwise. + */ + public isPendingRevocation(permissionContext: Hex): boolean { + return this.state.pendingRevocations.some( + (pendingRevocation) => + pendingRevocation.permissionContext.toLowerCase() === + permissionContext.toLowerCase(), + ); + } } diff --git a/packages/gator-permissions-controller/src/index.ts b/packages/gator-permissions-controller/src/index.ts index 6fb66704515..ed643ea4802 100644 --- a/packages/gator-permissions-controller/src/index.ts +++ b/packages/gator-permissions-controller/src/index.ts @@ -13,6 +13,8 @@ export type { GatorPermissionsControllerDisableGatorPermissionsAction, GatorPermissionsControllerSubmitRevocationAction, GatorPermissionsControllerAddPendingRevocationAction, + GatorPermissionsControllerSubmitDirectRevocationAction, + GatorPermissionsControllerIsPendingRevocationAction, GatorPermissionsControllerActions, GatorPermissionsControllerEvents, GatorPermissionsControllerStateChangeEvent, From 71cf3f47a725fb6c4c55b4ce209ebf84cc9cb599 Mon Sep 17 00:00:00 2001 From: hanzel98 Date: Wed, 26 Nov 2025 09:43:05 -0600 Subject: [PATCH 2/5] test: adding tests --- .../gator-permissions-controller/CHANGELOG.md | 3 - .../src/GatorPermissionsController.test.ts | 242 ++++++++++++++++++ 2 files changed, 242 insertions(+), 3 deletions(-) diff --git a/packages/gator-permissions-controller/CHANGELOG.md b/packages/gator-permissions-controller/CHANGELOG.md index 378472aeee9..44014090f84 100644 --- a/packages/gator-permissions-controller/CHANGELOG.md +++ b/packages/gator-permissions-controller/CHANGELOG.md @@ -10,9 +10,6 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added - Add `submitDirectRevocation` method for already-disabled delegations that don't require an on-chain transaction ([#TBD](https://github.com/MetaMask/core/pull/TBD)) - - This method adds a pending revocation to state and immediately submits the revocation - - Includes `isPendingRevocation` helper method to check if a permission context is pending revocation - - Pending revocations are automatically cleared in a `finally` block to prevent stuck states ### Changed diff --git a/packages/gator-permissions-controller/src/GatorPermissionsController.test.ts b/packages/gator-permissions-controller/src/GatorPermissionsController.test.ts index 7fd4b0e9e84..68d2d835877 100644 --- a/packages/gator-permissions-controller/src/GatorPermissionsController.test.ts +++ b/packages/gator-permissions-controller/src/GatorPermissionsController.test.ts @@ -813,6 +813,248 @@ describe('GatorPermissionsController', () => { 'Failed to handle snap request to gator permissions provider for method permissionsProvider_submitRevocation', ); }); + + it('should clear pending revocation in finally block even if refresh fails', async () => { + const mockHandleRequestHandler = jest.fn().mockResolvedValue(undefined); + const messenger = getMessenger( + getRootMessenger({ + snapControllerHandleRequestActionHandler: mockHandleRequestHandler, + }), + ); + + const controller = new GatorPermissionsController({ + messenger, + state: { + isGatorPermissionsEnabled: true, + gatorPermissionsProviderSnapId: + MOCK_GATOR_PERMISSIONS_PROVIDER_SNAP_ID, + pendingRevocations: [ + { + txId: 'test-tx-id', + permissionContext: '0x1234567890abcdef1234567890abcdef12345678', + }, + ], + }, + }); + + // Mock fetchAndUpdateGatorPermissions to fail + const fetchError = new Error('Refresh failed'); + jest + .spyOn(controller, 'fetchAndUpdateGatorPermissions') + .mockRejectedValue(fetchError); + + const revocationParams: RevocationParams = { + permissionContext: '0x1234567890abcdef1234567890abcdef12345678', + }; + + await expect( + controller.submitRevocation(revocationParams), + ).rejects.toThrow(); + + // Pending revocation should still be cleared despite refresh failure + expect(controller.pendingRevocations).toStrictEqual([]); + }); + }); + + describe('submitDirectRevocation', () => { + it('should add to pending revocations and immediately submit revocation', async () => { + const mockHandleRequestHandler = jest.fn().mockResolvedValue(undefined); + const messenger = getMessenger( + getRootMessenger({ + snapControllerHandleRequestActionHandler: mockHandleRequestHandler, + }), + ); + + const controller = new GatorPermissionsController({ + messenger, + state: { + isGatorPermissionsEnabled: true, + gatorPermissionsProviderSnapId: + MOCK_GATOR_PERMISSIONS_PROVIDER_SNAP_ID, + }, + }); + + const revocationParams: RevocationParams = { + permissionContext: '0x1234567890abcdef1234567890abcdef12345678', + }; + + await controller.submitDirectRevocation(revocationParams); + + // Should have called submitRevocation + expect(mockHandleRequestHandler).toHaveBeenCalledWith({ + snapId: MOCK_GATOR_PERMISSIONS_PROVIDER_SNAP_ID, + origin: 'metamask', + handler: 'onRpcRequest', + request: { + jsonrpc: '2.0', + method: 'permissionsProvider_submitRevocation', + params: revocationParams, + }, + }); + + // Pending revocation should be cleared after successful submission + expect(controller.pendingRevocations).toStrictEqual([]); + }); + + it('should add pending revocation with placeholder txId', async () => { + const mockHandleRequestHandler = jest.fn().mockResolvedValue(undefined); + const messenger = getMessenger( + getRootMessenger({ + snapControllerHandleRequestActionHandler: mockHandleRequestHandler, + }), + ); + + const controller = new GatorPermissionsController({ + messenger, + state: { + isGatorPermissionsEnabled: true, + gatorPermissionsProviderSnapId: + MOCK_GATOR_PERMISSIONS_PROVIDER_SNAP_ID, + }, + }); + + const permissionContext = + '0x1234567890abcdef1234567890abcdef12345678' as Hex; + const revocationParams: RevocationParams = { + permissionContext, + }; + + // Spy on submitRevocation to check pending state before it's called + const submitRevocationSpy = jest.spyOn(controller, 'submitRevocation'); + + await controller.submitDirectRevocation(revocationParams); + + // Verify that pending revocation was added (before submitRevocation clears it) + // We check by verifying submitRevocation was called, which clears pending + expect(submitRevocationSpy).toHaveBeenCalledWith(revocationParams); + expect(controller.pendingRevocations).toStrictEqual([]); + }); + + it('should throw GatorPermissionsNotEnabledError when gator permissions are disabled', async () => { + const messenger = getMessenger(); + const controller = new GatorPermissionsController({ + messenger, + state: { + isGatorPermissionsEnabled: false, + }, + }); + + const revocationParams: RevocationParams = { + permissionContext: '0x1234567890abcdef1234567890abcdef12345678', + }; + + await expect( + controller.submitDirectRevocation(revocationParams), + ).rejects.toThrow('Gator permissions are not enabled'); + }); + + it('should clear pending revocation even if submitRevocation fails (finally block)', async () => { + const mockHandleRequestHandler = jest + .fn() + .mockRejectedValue(new Error('Snap request failed')); + const messenger = getMessenger( + getRootMessenger({ + snapControllerHandleRequestActionHandler: mockHandleRequestHandler, + }), + ); + + const controller = new GatorPermissionsController({ + messenger, + state: { + isGatorPermissionsEnabled: true, + gatorPermissionsProviderSnapId: + MOCK_GATOR_PERMISSIONS_PROVIDER_SNAP_ID, + }, + }); + + const permissionContext = + '0x1234567890abcdef1234567890abcdef12345678' as Hex; + const revocationParams: RevocationParams = { + permissionContext, + }; + + await expect( + controller.submitDirectRevocation(revocationParams), + ).rejects.toThrow( + 'Failed to handle snap request to gator permissions provider for method permissionsProvider_submitRevocation', + ); + + // Pending revocation is cleared in finally block even if submission failed + // This prevents stuck state, though the error is still thrown for caller handling + expect(controller.pendingRevocations).toStrictEqual([]); + }); + }); + + describe('isPendingRevocation', () => { + it('should return true when permission context is in pending revocations', () => { + const messenger = getMessenger(); + const permissionContext = + '0x1234567890abcdef1234567890abcdef12345678' as Hex; + const controller = new GatorPermissionsController({ + messenger, + state: { + isGatorPermissionsEnabled: true, + gatorPermissionsProviderSnapId: + MOCK_GATOR_PERMISSIONS_PROVIDER_SNAP_ID, + pendingRevocations: [ + { + txId: 'test-tx-id', + permissionContext, + }, + ], + }, + }); + + expect(controller.isPendingRevocation(permissionContext)).toBe(true); + }); + + it('should return false when permission context is not in pending revocations', () => { + const messenger = getMessenger(); + const controller = new GatorPermissionsController({ + messenger, + state: { + isGatorPermissionsEnabled: true, + gatorPermissionsProviderSnapId: + MOCK_GATOR_PERMISSIONS_PROVIDER_SNAP_ID, + pendingRevocations: [ + { + txId: 'test-tx-id', + permissionContext: '0x1234567890abcdef1234567890abcdef12345678', + }, + ], + }, + }); + + expect( + controller.isPendingRevocation( + '0xabcdefabcdefabcdefabcdefabcdefabcdefabcdef' as Hex, + ), + ).toBe(false); + }); + + it('should be case-insensitive when checking permission context', () => { + const messenger = getMessenger(); + const permissionContext = + '0x1234567890abcdef1234567890abcdef12345678' as Hex; + const controller = new GatorPermissionsController({ + messenger, + state: { + isGatorPermissionsEnabled: true, + gatorPermissionsProviderSnapId: + MOCK_GATOR_PERMISSIONS_PROVIDER_SNAP_ID, + pendingRevocations: [ + { + txId: 'test-tx-id', + permissionContext: permissionContext.toLowerCase() as Hex, + }, + ], + }, + }); + + expect( + controller.isPendingRevocation(permissionContext.toUpperCase() as Hex), + ).toBe(true); + }); }); describe('addPendingRevocation', () => { From 94279f7610ff569c6f812a706bbb0c876d3b0ee3 Mon Sep 17 00:00:00 2001 From: hanzel98 Date: Wed, 26 Nov 2025 09:45:35 -0600 Subject: [PATCH 3/5] chore: update changelog with PR number --- packages/gator-permissions-controller/CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/gator-permissions-controller/CHANGELOG.md b/packages/gator-permissions-controller/CHANGELOG.md index 44014090f84..ccf7d99ff36 100644 --- a/packages/gator-permissions-controller/CHANGELOG.md +++ b/packages/gator-permissions-controller/CHANGELOG.md @@ -9,7 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added -- Add `submitDirectRevocation` method for already-disabled delegations that don't require an on-chain transaction ([#TBD](https://github.com/MetaMask/core/pull/TBD)) +- Add `submitDirectRevocation` method for already-disabled delegations that don't require an on-chain transaction ([#7244](https://github.com/MetaMask/core/pull/7244)) ### Changed From 475354b39a61f13bdd4f6395a525c7567348fbf6 Mon Sep 17 00:00:00 2001 From: hanzel98 Date: Wed, 26 Nov 2025 10:16:52 -0600 Subject: [PATCH 4/5] chore: better error indication --- .../src/GatorPermissionsController.test.ts | 20 +++++++-- .../src/GatorPermissionsController.ts | 42 +++++++++++++------ 2 files changed, 47 insertions(+), 15 deletions(-) diff --git a/packages/gator-permissions-controller/src/GatorPermissionsController.test.ts b/packages/gator-permissions-controller/src/GatorPermissionsController.test.ts index 68d2d835877..60883b7e565 100644 --- a/packages/gator-permissions-controller/src/GatorPermissionsController.test.ts +++ b/packages/gator-permissions-controller/src/GatorPermissionsController.test.ts @@ -21,6 +21,7 @@ import type { SnapId } from '@metamask/snaps-sdk'; import type { TransactionMeta } from '@metamask/transaction-controller'; import { hexToBigInt, numberToHex, type Hex } from '@metamask/utils'; +import { GatorPermissionsFetchError } from './errors'; import type { GatorPermissionsControllerMessenger } from './GatorPermissionsController'; import GatorPermissionsController, { DELEGATION_FRAMEWORK_VERSION, @@ -837,8 +838,12 @@ describe('GatorPermissionsController', () => { }, }); - // Mock fetchAndUpdateGatorPermissions to fail - const fetchError = new Error('Refresh failed'); + // Mock fetchAndUpdateGatorPermissions to fail with GatorPermissionsFetchError + // (which is what it actually throws in real scenarios) + const fetchError = new GatorPermissionsFetchError({ + message: 'Failed to fetch gator permissions', + cause: new Error('Refresh failed'), + }); jest .spyOn(controller, 'fetchAndUpdateGatorPermissions') .mockRejectedValue(fetchError); @@ -847,9 +852,18 @@ describe('GatorPermissionsController', () => { permissionContext: '0x1234567890abcdef1234567890abcdef12345678', }; + // Should throw GatorPermissionsFetchError (not GatorPermissionsProviderError) + // because revocation succeeded but refresh failed + await expect( + controller.submitRevocation(revocationParams), + ).rejects.toThrow(GatorPermissionsFetchError); + + // Verify the error message indicates refresh failure, not revocation failure await expect( controller.submitRevocation(revocationParams), - ).rejects.toThrow(); + ).rejects.toThrow( + 'Failed to refresh permissions list after successful revocation', + ); // Pending revocation should still be cleared despite refresh failure expect(controller.pendingRevocations).toStrictEqual([]); diff --git a/packages/gator-permissions-controller/src/GatorPermissionsController.ts b/packages/gator-permissions-controller/src/GatorPermissionsController.ts index 2675aa41d99..69154cd093b 100644 --- a/packages/gator-permissions-controller/src/GatorPermissionsController.ts +++ b/packages/gator-permissions-controller/src/GatorPermissionsController.ts @@ -754,19 +754,19 @@ export default class GatorPermissionsController extends BaseController< this.#assertGatorPermissionsEnabled(); - try { - const snapRequest = { - snapId: this.state.gatorPermissionsProviderSnapId, - origin: 'metamask', - handler: HandlerType.OnRpcRequest, - request: { - jsonrpc: '2.0', - method: - GatorPermissionsSnapRpcMethod.PermissionProviderSubmitRevocation, - params: revocationParams, - }, - }; + const snapRequest = { + snapId: this.state.gatorPermissionsProviderSnapId, + origin: 'metamask', + handler: HandlerType.OnRpcRequest, + request: { + jsonrpc: '2.0', + method: + GatorPermissionsSnapRpcMethod.PermissionProviderSubmitRevocation, + params: revocationParams, + }, + }; + try { const result = await this.messenger.call( 'SnapController:handleRequest', snapRequest, @@ -780,6 +780,24 @@ export default class GatorPermissionsController extends BaseController< result, }); } catch (error) { + // If it's a GatorPermissionsFetchError, revocation succeeded but refresh failed + if (error instanceof GatorPermissionsFetchError) { + controllerLog( + 'Revocation submitted successfully but failed to refresh permissions list', + { + error, + permissionContext: revocationParams.permissionContext, + }, + ); + // Wrap with a more specific message indicating revocation succeeded + throw new GatorPermissionsFetchError({ + message: + 'Failed to refresh permissions list after successful revocation', + cause: error as Error, + }); + } + + // Otherwise, revocation failed - wrap in provider error controllerLog('Failed to submit revocation', { error, permissionContext: revocationParams.permissionContext, From 4ea1f04efc7c6b4ed9aea4da173758e81abd9acd Mon Sep 17 00:00:00 2001 From: hanzel98 Date: Wed, 26 Nov 2025 12:34:51 -0600 Subject: [PATCH 5/5] chore: case sensitive comparison --- .../src/GatorPermissionsController.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/gator-permissions-controller/src/GatorPermissionsController.ts b/packages/gator-permissions-controller/src/GatorPermissionsController.ts index 69154cd093b..55255d8053d 100644 --- a/packages/gator-permissions-controller/src/GatorPermissionsController.ts +++ b/packages/gator-permissions-controller/src/GatorPermissionsController.ts @@ -372,7 +372,8 @@ export default class GatorPermissionsController extends BaseController< this.update((state) => { state.pendingRevocations = state.pendingRevocations.filter( (pendingRevocations) => - pendingRevocations.permissionContext !== permissionContext, + pendingRevocations.permissionContext.toLowerCase() !== + permissionContext.toLowerCase(), ); }); }