diff --git a/packages/gator-permissions-controller/CHANGELOG.md b/packages/gator-permissions-controller/CHANGELOG.md index e35d5a4855e..dbe19e7d6b0 100644 --- a/packages/gator-permissions-controller/CHANGELOG.md +++ b/packages/gator-permissions-controller/CHANGELOG.md @@ -7,6 +7,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Fixed + +- Does not add a pending revocation if user cancels the transaction ([#7157](https://github.com/MetaMask/core/pull/7157)) +- **BREAKING** The GatorPermissionsController messenger must allow `TransactionController:transactionApproved` and `TransactionController:transactionRejected` events ([#7157](https://github.com/MetaMask/core/pull/7157)) + ## [0.4.0] ### Added diff --git a/packages/gator-permissions-controller/src/GatorPermissionsController.test.ts b/packages/gator-permissions-controller/src/GatorPermissionsController.test.ts index 8ce4944721b..7fd4b0e9e84 100644 --- a/packages/gator-permissions-controller/src/GatorPermissionsController.test.ts +++ b/packages/gator-permissions-controller/src/GatorPermissionsController.test.ts @@ -845,6 +845,11 @@ describe('GatorPermissionsController', () => { await controller.addPendingRevocation({ txId, permissionContext }); + // Emit transaction approved event (user confirms) + rootMessenger.publish('TransactionController:transactionApproved', { + transactionMeta: { id: txId } as TransactionMeta, + }); + // Emit transaction confirmed event rootMessenger.publish('TransactionController:transactionConfirmed', { id: txId, @@ -865,6 +870,44 @@ describe('GatorPermissionsController', () => { }); }); + it('should cleanup without adding to state when transaction is rejected by user', async () => { + const mockHandleRequestHandler = jest.fn().mockResolvedValue(undefined); + const rootMessenger = getRootMessenger({ + snapControllerHandleRequestActionHandler: mockHandleRequestHandler, + }); + const messenger = getMessenger(rootMessenger); + + const controller = new GatorPermissionsController({ + messenger, + state: { + isGatorPermissionsEnabled: true, + gatorPermissionsProviderSnapId: + MOCK_GATOR_PERMISSIONS_PROVIDER_SNAP_ID, + }, + }); + + const txId = 'test-tx-id'; + const permissionContext = '0x1234567890abcdef1234567890abcdef12345678'; + + await controller.addPendingRevocation({ txId, permissionContext }); + + // Verify pending revocation is not in state yet + expect(controller.pendingRevocations).toStrictEqual([]); + + // Emit transaction rejected event (user cancels) + rootMessenger.publish('TransactionController:transactionRejected', { + transactionMeta: { id: txId } as TransactionMeta, + }); + + // Wait for async operations + await Promise.resolve(); + + // Should not call submitRevocation + expect(mockHandleRequestHandler).not.toHaveBeenCalled(); + // Should not be in pending revocations + expect(controller.pendingRevocations).toStrictEqual([]); + }); + it('should cleanup without submitting revocation when transaction fails', async () => { const mockHandleRequestHandler = jest.fn().mockResolvedValue(undefined); const rootMessenger = getRootMessenger({ @@ -963,6 +1006,41 @@ describe('GatorPermissionsController', () => { expect(mockHandleRequestHandler).not.toHaveBeenCalled(); }); + it('should add to pending revocations state only after user approval', async () => { + const mockHandleRequestHandler = jest.fn().mockResolvedValue(undefined); + const rootMessenger = getRootMessenger({ + snapControllerHandleRequestActionHandler: mockHandleRequestHandler, + }); + const messenger = getMessenger(rootMessenger); + + const controller = new GatorPermissionsController({ + messenger, + state: { + isGatorPermissionsEnabled: true, + gatorPermissionsProviderSnapId: + MOCK_GATOR_PERMISSIONS_PROVIDER_SNAP_ID, + }, + }); + + const txId = 'test-tx-id'; + const permissionContext = '0x1234567890abcdef1234567890abcdef12345678'; + + await controller.addPendingRevocation({ txId, permissionContext }); + + // Before approval, pending revocation should not be in state + expect(controller.pendingRevocations).toStrictEqual([]); + + // Emit transaction approved event (user confirms) + rootMessenger.publish('TransactionController:transactionApproved', { + transactionMeta: { id: txId } as TransactionMeta, + }); + + // After approval, pending revocation should be in state + expect(controller.pendingRevocations).toStrictEqual([ + { txId, permissionContext }, + ]); + }); + it('should not submit revocation for different transaction IDs', async () => { const mockHandleRequestHandler = jest.fn().mockResolvedValue(undefined); const rootMessenger = getRootMessenger({ @@ -984,6 +1062,11 @@ describe('GatorPermissionsController', () => { await controller.addPendingRevocation({ txId, permissionContext }); + // Emit transaction approved event for our transaction + rootMessenger.publish('TransactionController:transactionApproved', { + transactionMeta: { id: txId } as TransactionMeta, + }); + // Emit transaction confirmed event for different transaction rootMessenger.publish('TransactionController:transactionConfirmed', { id: 'different-tx-id', @@ -1019,6 +1102,11 @@ describe('GatorPermissionsController', () => { await controller.addPendingRevocation({ txId, permissionContext }); + // Emit transaction approved event (user confirms) + rootMessenger.publish('TransactionController:transactionApproved', { + transactionMeta: { id: txId } as TransactionMeta, + }); + // Emit transaction confirmed event rootMessenger.publish('TransactionController:transactionConfirmed', { id: txId, @@ -1159,6 +1247,8 @@ function getGatorPermissionsControllerMessenger( messenger: gatorPermissionsControllerMessenger, actions: ['SnapController:handleRequest', 'SnapController:has'], events: [ + 'TransactionController:transactionApproved', + 'TransactionController:transactionRejected', 'TransactionController:transactionConfirmed', 'TransactionController:transactionFailed', 'TransactionController:transactionDropped', diff --git a/packages/gator-permissions-controller/src/GatorPermissionsController.ts b/packages/gator-permissions-controller/src/GatorPermissionsController.ts index 1b079ff29c3..4c5124f0b22 100644 --- a/packages/gator-permissions-controller/src/GatorPermissionsController.ts +++ b/packages/gator-permissions-controller/src/GatorPermissionsController.ts @@ -11,9 +11,11 @@ import type { HandleSnapRequest, HasSnap } from '@metamask/snaps-controllers'; import type { SnapId } from '@metamask/snaps-sdk'; import { HandlerType } from '@metamask/snaps-utils'; import type { + TransactionControllerTransactionApprovedEvent, TransactionControllerTransactionConfirmedEvent, TransactionControllerTransactionDroppedEvent, TransactionControllerTransactionFailedEvent, + TransactionControllerTransactionRejectedEvent, } from '@metamask/transaction-controller'; import type { Hex, Json } from '@metamask/utils'; @@ -267,6 +269,8 @@ export type GatorPermissionsControllerEvents = */ type AllowedEvents = | GatorPermissionsControllerStateChangeEvent + | TransactionControllerTransactionApprovedEvent + | TransactionControllerTransactionRejectedEvent | TransactionControllerTransactionConfirmedEvent | TransactionControllerTransactionFailedEvent | TransactionControllerTransactionDroppedEvent; @@ -764,8 +768,15 @@ export default class GatorPermissionsController extends BaseController< /** * Adds a pending revocation that will be submitted once the transaction is confirmed. * - * This method sets up listeners for terminal transaction states (confirmed, failed, dropped) - * and includes a timeout safety net to prevent memory leaks if the transaction never + * This method sets up listeners for the user's approval/rejection decision and + * terminal transaction states (confirmed, failed, dropped). The flow is: + * 1. Wait for user to approve or reject the transaction + * 2. If approved, add to pending revocations state + * 3. If rejected, cleanup without adding to state + * 4. If confirmed, submit the revocation + * 5. If failed or dropped, cleanup + * + * Includes a timeout safety net to prevent memory leaks if the transaction never * reaches a terminal state. * * @param params - The pending revocation parameters. @@ -782,9 +793,14 @@ export default class GatorPermissionsController extends BaseController< }); this.#assertGatorPermissionsEnabled(); - this.#addPendingRevocationToState(txId, permissionContext); type PendingRevocationHandlers = { + approved?: ( + ...args: TransactionControllerTransactionApprovedEvent['payload'] + ) => void; + rejected?: ( + ...args: TransactionControllerTransactionRejectedEvent['payload'] + ) => void; confirmed?: ( ...args: TransactionControllerTransactionConfirmedEvent['payload'] ) => void; @@ -799,14 +815,35 @@ export default class GatorPermissionsController extends BaseController< // Track handlers and timeout for cleanup const handlers: PendingRevocationHandlers = { + approved: undefined, + rejected: undefined, confirmed: undefined, failed: undefined, dropped: undefined, timeoutId: undefined, }; + // Helper to unsubscribe from approval/rejection events after decision is made + const cleanupApprovalHandlers = () => { + if (handlers.approved) { + this.messenger.unsubscribe( + 'TransactionController:transactionApproved', + handlers.approved, + ); + handlers.approved = undefined; + } + if (handlers.rejected) { + this.messenger.unsubscribe( + 'TransactionController:transactionRejected', + handlers.rejected, + ); + handlers.rejected = undefined; + } + }; + // Cleanup function to unsubscribe from all events and clear timeout - const cleanup = (txIdToRemove: string) => { + const cleanup = (txIdToRemove: string, removeFromState = true) => { + cleanupApprovalHandlers(); if (handlers.confirmed) { this.messenger.unsubscribe( 'TransactionController:transactionConfirmed', @@ -829,8 +866,41 @@ export default class GatorPermissionsController extends BaseController< clearTimeout(handlers.timeoutId); } - // Remove the pending revocation from the state - this.#removePendingRevocationFromStateByTxId(txIdToRemove); + // Remove the pending revocation from the state (only if it was added) + if (removeFromState) { + this.#removePendingRevocationFromStateByTxId(txIdToRemove); + } + }; + + // Handle approved transaction - add to pending revocations state + handlers.approved = (payload) => { + if (payload.transactionMeta.id === txId) { + controllerLog( + 'Transaction approved by user, adding to pending revocations', + { + txId, + permissionContext, + }, + ); + + this.#addPendingRevocationToState(txId, permissionContext); + + // Unsubscribe from approval/rejection events since decision is made + cleanupApprovalHandlers(); + } + }; + + // Handle rejected transaction - cleanup without adding to state + handlers.rejected = (payload) => { + if (payload.transactionMeta.id === txId) { + controllerLog('Transaction rejected by user, cleaning up listeners', { + txId, + permissionContext, + }); + + // Don't remove from state since it was never added + cleanup(payload.transactionMeta.id, false); + } }; // Handle confirmed transaction - submit revocation @@ -881,6 +951,16 @@ export default class GatorPermissionsController extends BaseController< } }; + // Subscribe to user approval/rejection events + this.messenger.subscribe( + 'TransactionController:transactionApproved', + handlers.approved, + ); + this.messenger.subscribe( + 'TransactionController:transactionRejected', + handlers.rejected, + ); + // Subscribe to terminal transaction events this.messenger.subscribe( 'TransactionController:transactionConfirmed',