Skip to content

Commit e462522

Browse files
hanzel98MoMannn
andauthored
feat: Handle revocation of already-disabled delegations (#38321)
## **Description** This PR adds support for revoking delegations that are already disabled on-chain, without requiring an on-chain transaction. **Problem:** When users attempt to revoke a delegation that has already been disabled on-chain (e.g., by another app or directly via contract), the revoke button would not properly disable and the UI would not refresh after the operation completed, causing a flash where the button briefly showed 'Revoke' before the permission disappeared. **Solution:** - Added `submitDirectRevocation` method to the GatorPermissionsController API - Updated the revocation flow to check if a delegation is already disabled on-chain - For already-disabled delegations, use the direct revocation flow (no transaction) - Controller now handles permission list refresh before clearing pending state to prevent button flashing - Simplified client-side logic by removing UI-level refresh mechanisms **Dependencies:** ⚠️ This PR requires an updated version of `@metamask/gator-permissions-controller` with: - `submitDirectRevocation` method that: 1. Adds permission to pending state (disables button) 2. Submits revocation to snap 3. Refreshes permission list (removes from UI) 4. Clears pending state (button already gone, no flash) ## **Changelog** CHANGELOG entry: null ## **Related issues** Fixes: #463 ## **Manual testing steps** ### Test Already-Disabled Delegation: 1. Enable Gator Permissions feature 2. Create a delegation via a dapp 3. Manually disable the delegation on-chain (via another app or contract) 4. In MetaMask, navigate to Gator Permissions → Token Transfer 5. Click 'Revoke' on the already-disabled delegation 6. Verify: Button shows 'Revocation Pending' and stays that way (no flash to 'Revoke') 7. Verify: Permission disappears from list smoothly 8. Verify: No errors in console ### Test Normal Delegation (On-Chain Transaction): 1. Create a new delegation via a dapp 2. In MetaMask, navigate to Gator Permissions → Token Transfer 3. Click 'Revoke' on an active delegation 4. Verify: Transaction confirmation popup appears 5. Approve the transaction 6. Verify: Button shows 'Revocation Pending' 7. Wait for transaction confirmation 8. Verify: Permission disappears from list after confirmation 9. Verify: No button flash, smooth UI update ## **Screenshots/Recordings** ### **Before** - Button would flash from 'Revocation Pending' → 'Revoke' → disappear - Permission list would not refresh automatically ### **After** - Button shows 'Revocation Pending' until permission disappears - No flash - Smooth UI update ## **Pre-merge author checklist** - [x] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Extension Coding Standards](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/CODING_GUIDELINES.md). - [x] I've completed the PR template to the best of my ability - [ ] I've included tests if applicable - [x] I've documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [ ] I've applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. ## **Pre-merge reviewer checklist** - [ ] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [ ] I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots. https://github.com/user-attachments/assets/220f0641-728e-4b99-b757-726c2b8c8167 <!-- CURSOR_SUMMARY --> --- > [!NOTE] > Adds a no-transaction revocation path and updates hooks/controllers to detect already-disabled delegations. > > - Replace `submitRevocation` with `submitDirectRevocation` in controller init, background (`metamask-controller.js`), and UI actions/tests > - Update revoke hooks (`useRevokeGatorPermissions*`) to: > - Compute `delegationHash` via `getDelegationHashOffchain` > - Query `checkDelegationDisabled`; if true, call `submitDirectRevocation` and skip creating a transaction > - Otherwise proceed with `encodeDisableDelegation` and create the transaction, tracking with `addPendingRevocation` > - Add `submitDirectRevocation` action and JSDoc; retain `addPendingRevocation` > - Extend tests to cover the direct revocation path and disabled-delegation behavior > > <sup>Written by [Cursor Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit 1aec919. This will update automatically on new commits. Configure [here](https://cursor.com/dashboard?tab=bugbot).</sup> <!-- /CURSOR_SUMMARY --> --------- Co-authored-by: Tadej Vengust <[email protected]>
1 parent 6c36850 commit e462522

File tree

8 files changed

+123
-18
lines changed

8 files changed

+123
-18
lines changed

app/scripts/controller-init/gator-permissions/gator-permissions-controller-init.test.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -98,7 +98,7 @@ describe('GatorPermissionsControllerInit', () => {
9898
expect(result.api).toEqual({
9999
fetchAndUpdateGatorPermissions: expect.any(Function),
100100
addPendingRevocation: expect.any(Function),
101-
submitRevocation: expect.any(Function),
101+
submitDirectRevocation: expect.any(Function),
102102
});
103103
});
104104

app/scripts/controller-init/gator-permissions/gator-permissions-controller-init.ts

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -61,7 +61,8 @@ export const GatorPermissionsControllerInit: ControllerInitFunction<
6161
fetchAndUpdateGatorPermissions:
6262
controller.fetchAndUpdateGatorPermissions.bind(controller),
6363
addPendingRevocation: controller.addPendingRevocation.bind(controller),
64-
submitRevocation: controller.submitRevocation.bind(controller),
64+
submitDirectRevocation:
65+
controller.submitDirectRevocation.bind(controller),
6566
},
6667
};
6768
};

app/scripts/metamask-controller.js

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3320,9 +3320,10 @@ export default class MetamaskController extends EventEmitter {
33203320
gatorPermissionsController.addPendingRevocation.bind(
33213321
gatorPermissionsController,
33223322
),
3323-
submitRevocation: gatorPermissionsController.submitRevocation.bind(
3324-
gatorPermissionsController,
3325-
),
3323+
submitDirectRevocation:
3324+
gatorPermissionsController.submitDirectRevocation.bind(
3325+
gatorPermissionsController,
3326+
),
33263327
checkDelegationDisabled: this.checkDelegationDisabled.bind(this),
33273328

33283329
// KeyringController

ui/hooks/gator-permissions/useRevokeGatorPermissions.test.tsx

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,6 @@ import {
3131
} from '../../selectors';
3232
import {
3333
addPendingRevocation,
34-
submitRevocation,
3534
checkDelegationDisabled,
3635
} from '../../store/controller-actions/gator-permissions-controller';
3736
import { useRevokeGatorPermissions } from './useRevokeGatorPermissions';
@@ -51,7 +50,6 @@ jest.mock(
5150
'../../store/controller-actions/gator-permissions-controller',
5251
() => ({
5352
addPendingRevocation: jest.fn(),
54-
submitRevocation: jest.fn(),
5553
checkDelegationDisabled: jest.fn(),
5654
}),
5755
);
@@ -110,9 +108,6 @@ const mockGetDelegationHashOffchain =
110108
const mockAddPendingRevocation = addPendingRevocation as jest.MockedFunction<
111109
typeof addPendingRevocation
112110
>;
113-
const mockSubmitRevocation = submitRevocation as jest.MockedFunction<
114-
typeof submitRevocation
115-
>;
116111
const mockCheckDelegationDisabled =
117112
checkDelegationDisabled as jest.MockedFunction<
118113
typeof checkDelegationDisabled
@@ -273,7 +268,6 @@ describe('useRevokeGatorPermissions', () => {
273268
'0xfd165b374563126931d2be865bbec75623dca111840d148cf88492c0bb997f96' as `0x${string}`,
274269
);
275270
mockCheckDelegationDisabled.mockResolvedValue(false);
276-
mockSubmitRevocation.mockResolvedValue(undefined);
277271
mockAddPendingRevocation.mockResolvedValue(undefined);
278272
mockAddTransaction.mockResolvedValue(mockTransactionMeta as never);
279273

ui/hooks/gator-permissions/useRevokeGatorPermissions.ts

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,7 @@ import {
2121
} from '../../../shared/lib/delegation/delegation';
2222
import {
2323
addPendingRevocation,
24-
submitRevocation,
24+
submitDirectRevocation,
2525
checkDelegationDisabled,
2626
} from '../../store/controller-actions/gator-permissions-controller';
2727
import { useGatorPermissionRedirect } from './useGatorPermissionRedirect';
@@ -172,9 +172,10 @@ export function useRevokeGatorPermissions({
172172
delegationHash,
173173
networkClientId,
174174
);
175+
175176
if (isDisabled) {
176-
await submitRevocation({ permissionContext });
177177
// Return null since no actual transaction is needed when already disabled
178+
await submitDirectRevocation({ permissionContext });
178179
return null;
179180
}
180181

ui/hooks/gator-permissions/useRevokeGatorPermissionsMultiChain.test.tsx

Lines changed: 62 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,8 +20,15 @@ import {
2020
addTransaction,
2121
findNetworkClientIdByChainId,
2222
} from '../../store/actions';
23-
import { encodeDisableDelegation } from '../../../shared/lib/delegation/delegation';
23+
import {
24+
encodeDisableDelegation,
25+
getDelegationHashOffchain,
26+
} from '../../../shared/lib/delegation/delegation';
2427
import { getMemoizedInternalAccountByAddress } from '../../selectors/accounts';
28+
import {
29+
checkDelegationDisabled,
30+
submitDirectRevocation,
31+
} from '../../store/controller-actions/gator-permissions-controller';
2532
import {
2633
useRevokeGatorPermissionsMultiChain,
2734
RevokeGatorPermissionsMultiChainResults,
@@ -40,6 +47,7 @@ jest.mock('@metamask/delegation-core', () => ({
4047

4148
jest.mock('../../../shared/lib/delegation/delegation', () => ({
4249
encodeDisableDelegation: jest.fn(),
50+
getDelegationHashOffchain: jest.fn(),
4351
}));
4452

4553
jest.mock('../../../shared/lib/delegation', () => ({
@@ -54,6 +62,15 @@ jest.mock('../../selectors/accounts', () => ({
5462
getMemoizedInternalAccountByAddress: jest.fn(),
5563
}));
5664

65+
jest.mock(
66+
'../../store/controller-actions/gator-permissions-controller',
67+
() => ({
68+
addPendingRevocation: jest.fn(),
69+
submitDirectRevocation: jest.fn(),
70+
checkDelegationDisabled: jest.fn(),
71+
}),
72+
);
73+
5774
// Mock useConfirmationNavigation hook
5875
const mockNavigateToId = jest.fn();
5976
const mockConfirmations: Partial<ApprovalRequest<Record<string, Json>>>[] = [];
@@ -79,10 +96,20 @@ const mockEncodeDisableDelegation =
7996
encodeDisableDelegation as jest.MockedFunction<
8097
typeof encodeDisableDelegation
8198
>;
99+
const mockGetDelegationHashOffchain =
100+
getDelegationHashOffchain as jest.MockedFunction<
101+
typeof getDelegationHashOffchain
102+
>;
82103
const mockGetMemoizedInternalAccountByAddress =
83104
getMemoizedInternalAccountByAddress as jest.MockedFunction<
84105
typeof getMemoizedInternalAccountByAddress
85106
>;
107+
const mockCheckDelegationDisabled =
108+
checkDelegationDisabled as jest.MockedFunction<
109+
typeof checkDelegationDisabled
110+
>;
111+
const mockSubmitDirectRevocation =
112+
submitDirectRevocation as jest.MockedFunction<typeof submitDirectRevocation>;
86113

87114
const mockStore = configureStore();
88115

@@ -208,9 +235,12 @@ describe('useRevokeGatorPermissionsMultiChain', () => {
208235

209236
mockDecodeDelegations.mockReturnValue([mockDelegation]);
210237
mockEncodeDisableDelegation.mockReturnValue('0xencodedCallData' as Hex);
238+
mockGetDelegationHashOffchain.mockReturnValue('0xdelegationhash' as Hex);
211239
mockFindNetworkClientIdByChainId.mockResolvedValue(
212240
mockNetworkClientId1 as never,
213241
);
242+
// Default to false so transactions are created (delegation is not disabled)
243+
mockCheckDelegationDisabled.mockResolvedValue(false);
214244
});
215245

216246
const wrapper = ({ children }: { children: React.ReactNode }) => (
@@ -358,6 +388,37 @@ describe('useRevokeGatorPermissionsMultiChain', () => {
358388
expect(results[mockChainId1].revoked).toHaveLength(0);
359389
});
360390

391+
it('should call submitDirectRevocation without creating transaction when delegation is already disabled', async () => {
392+
mockCheckDelegationDisabled.mockResolvedValue(true);
393+
394+
const { result } = renderHook(
395+
() => useRevokeGatorPermissionsMultiChain(),
396+
{ wrapper },
397+
);
398+
399+
const permissionsByChain = {
400+
[mockChainId1]: [mockPermission1],
401+
};
402+
403+
let results: RevokeGatorPermissionsMultiChainResults | undefined;
404+
await act(async () => {
405+
results =
406+
await result.current.revokeGatorPermissionsBatchMultiChain(
407+
permissionsByChain,
408+
);
409+
});
410+
411+
if (!results) {
412+
throw new Error('Results should be defined');
413+
}
414+
expect(mockSubmitDirectRevocation).toHaveBeenCalledWith({
415+
permissionContext: mockPermissionContext1,
416+
});
417+
expect(mockAddTransaction).not.toHaveBeenCalled();
418+
expect(results[mockChainId1].revoked).toHaveLength(0);
419+
expect(results[mockChainId1].errors).toHaveLength(0);
420+
});
421+
361422
it('should handle empty permissionsByChain object', async () => {
362423
const { result } = renderHook(
363424
() => useRevokeGatorPermissionsMultiChain(),

ui/hooks/gator-permissions/useRevokeGatorPermissionsMultiChain.ts

Lines changed: 28 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -15,8 +15,15 @@ import {
1515
findNetworkClientIdByChainId,
1616
} from '../../store/actions';
1717
import { getMemoizedInternalAccountByAddress } from '../../selectors/accounts';
18-
import { encodeDisableDelegation } from '../../../shared/lib/delegation/delegation';
19-
import { addPendingRevocation } from '../../store/controller-actions/gator-permissions-controller';
18+
import {
19+
encodeDisableDelegation,
20+
getDelegationHashOffchain,
21+
} from '../../../shared/lib/delegation/delegation';
22+
import {
23+
addPendingRevocation,
24+
submitDirectRevocation,
25+
checkDelegationDisabled,
26+
} from '../../store/controller-actions/gator-permissions-controller';
2027
import { extractDelegationFromGatorPermissionContext } from './utils';
2128
import { useGatorPermissionRedirect } from './useGatorPermissionRedirect';
2229

@@ -114,6 +121,25 @@ export function useRevokeGatorPermissionsMultiChain({
114121
permissionResponse.context,
115122
);
116123

124+
const delegationHash = getDelegationHashOffchain(delegation);
125+
126+
// Check if delegation is already disabled on-chain
127+
const isDisabled = await checkDelegationDisabled(
128+
permissionResponse.signerMeta.delegationManager,
129+
delegationHash,
130+
networkClientId,
131+
);
132+
133+
if (isDisabled) {
134+
// Delegation is already disabled, submit direct revocation without transaction
135+
await submitDirectRevocation({
136+
permissionContext: permissionResponse.context,
137+
});
138+
results[currentChainId as Hex].skipped.push(permission);
139+
// Continue to next permission - no transaction needed
140+
continue;
141+
}
142+
117143
// Encode disable delegation call
118144
const encodedCallData = encodeDisableDelegation({ delegation });
119145

ui/store/controller-actions/gator-permissions-controller.ts

Lines changed: 23 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,15 @@ export const fetchAndUpdateGatorPermissions = async (
1515
);
1616
};
1717

18+
/**
19+
* Adds a pending revocation for a delegation that requires an on-chain transaction.
20+
* This method is used for revocations that come with a transaction that has to be
21+
* confirmed before marking the permission as revoked.
22+
*
23+
* @param params - The parameters for pending revocation.
24+
* @param params.txId - The transaction ID associated with the revocation.
25+
* @param params.permissionContext - The permission context to revoke.
26+
*/
1827
export const addPendingRevocation = async ({
1928
txId,
2029
permissionContext,
@@ -30,12 +39,24 @@ export const addPendingRevocation = async ({
3039
]);
3140
};
3241

33-
export const submitRevocation = async ({
42+
/**
43+
* Submits a revocation for a delegation that is already disabled on-chain.
44+
* This method is used when a delegation has already been disabled and does not
45+
* require an on-chain transaction to revoke the permission.
46+
*
47+
* @param params - The parameters for direct revocation.
48+
* @param params.permissionContext - The permission context to revoke.
49+
*/
50+
export const submitDirectRevocation = async ({
3451
permissionContext,
3552
}: {
3653
permissionContext: Hex;
3754
}): Promise<void> => {
38-
await submitRequestToBackground('submitRevocation', [{ permissionContext }]);
55+
await submitRequestToBackground('submitDirectRevocation', [
56+
{
57+
permissionContext,
58+
},
59+
]);
3960
};
4061

4162
/**

0 commit comments

Comments
 (0)