Skip to content

Commit 210763d

Browse files
zone-liveccharly
andauthored
chore: adds safeguard for keyring being locked or unlocked (#5473)
## Explanation We are listening for the `KeyringController` `lock & unlock` events in order to safeguard the request to the snap. ## References Fixes: MetaMask/metamask-extension#30259 (comment) ## Changelog <!-- If you're making any consumer-facing changes, list those changes here as if you were updating a changelog, using the template below as a guide. (CATEGORY is one of BREAKING, ADDED, CHANGED, DEPRECATED, REMOVED, or FIXED. For security-related issues, follow the Security Advisory process.) Please take care to name the exact pieces of the API you've added or changed (e.g. types, interfaces, functions, or methods). If there are any breaking changes, make sure to offer a solution for consumers to follow once they upgrade to the changes. Finally, if you're only making changes to development scripts or tests, you may replace the template below with "None". --> ### `@metamask/multichain-transactions-controller` - **Adds**: Safeguard for `KeyringController` `lock & unlock` events ### `@metamask/assets-controllers` - **Adds**: Safeguard for `KeyringController` `lock & unlock` events in `MultichainBalancesController` ## Checklist - [ ] I've updated the test suite for new or updated code as appropriate - [ ] I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate - [ ] I've highlighted breaking changes using the "BREAKING" category above as appropriate - [ ] I've prepared draft pull requests for clients and consumer packages to resolve any breaking changes --------- Co-authored-by: Charly Chevalier <[email protected]>
1 parent 177ae0d commit 210763d

File tree

4 files changed

+82
-1
lines changed

4 files changed

+82
-1
lines changed

packages/assets-controllers/src/MultichainBalancesController/MultichainBalancesController.test.ts

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -135,6 +135,7 @@ function getRestrictedMessenger(
135135
'SnapController:handleRequest',
136136
'AccountsController:listMultichainAccounts',
137137
'MultichainAssetsController:getState',
138+
'KeyringController:getState',
138139
],
139140
allowedEvents: [
140141
'AccountsController:accountAdded',
@@ -184,6 +185,13 @@ const setupController = ({
184185
mockGetAssetsState,
185186
);
186187

188+
const mockGetKeyringState = jest.fn().mockReturnValue({
189+
isUnlocked: true,
190+
});
191+
messenger.registerActionHandler(
192+
'KeyringController:getState',
193+
mockGetKeyringState,
194+
);
187195
const controller = new MultichainBalancesController({
188196
messenger: multichainBalancesMessenger,
189197
state,
@@ -195,6 +203,7 @@ const setupController = ({
195203
mockSnapHandleRequest,
196204
mockListMultichainAccounts,
197205
mockGetAssetsState,
206+
mockGetKeyringState,
198207
};
199208
};
200209

@@ -226,6 +235,10 @@ describe('BalancesController', () => {
226235
'MultichainAssetsController:getState',
227236
jest.fn(),
228237
);
238+
messenger.registerActionHandler(
239+
'KeyringController:getState',
240+
jest.fn().mockReturnValue({ isUnlocked: true }),
241+
);
229242

230243
const controller = new MultichainBalancesController({
231244
messenger: multichainBalancesMessenger,
@@ -426,4 +439,20 @@ describe('BalancesController', () => {
426439
mockBalanceResult,
427440
);
428441
});
442+
443+
it('resumes updating balances after unlocking KeyringController', async () => {
444+
const { controller, mockGetKeyringState } = setupController();
445+
446+
mockGetKeyringState.mockReturnValue({ isUnlocked: false });
447+
448+
await controller.updateBalance(mockBtcAccount.id);
449+
expect(controller.state.balances[mockBtcAccount.id]).toBeUndefined();
450+
451+
mockGetKeyringState.mockReturnValue({ isUnlocked: true });
452+
453+
await controller.updateBalance(mockBtcAccount.id);
454+
expect(controller.state.balances[mockBtcAccount.id]).toStrictEqual(
455+
mockBalanceResult,
456+
);
457+
});
429458
});

packages/assets-controllers/src/MultichainBalancesController/MultichainBalancesController.ts

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ import type {
1616
CaipAssetType,
1717
AccountBalancesUpdatedEventPayload,
1818
} from '@metamask/keyring-api';
19+
import type { KeyringControllerGetStateAction } from '@metamask/keyring-controller';
1920
import type { InternalAccount } from '@metamask/keyring-internal-api';
2021
import { KeyringClient } from '@metamask/keyring-snap-client';
2122
import type { HandleSnapRequest } from '@metamask/snaps-controllers';
@@ -94,7 +95,8 @@ export type MultichainBalancesControllerEvents =
9495
type AllowedActions =
9596
| HandleSnapRequest
9697
| AccountsControllerListMultichainAccountsAction
97-
| MultichainAssetsControllerGetStateAction;
98+
| MultichainAssetsControllerGetStateAction
99+
| KeyringControllerGetStateAction;
98100

99101
/**
100102
* Events that this controller is allowed to subscribe.
@@ -199,6 +201,14 @@ export class MultichainBalancesController extends BaseController<
199201
accountId: string,
200202
assets: CaipAssetType[],
201203
): Promise<void> {
204+
const { isUnlocked } = this.messagingSystem.call(
205+
'KeyringController:getState',
206+
);
207+
208+
if (!isUnlocked) {
209+
return;
210+
}
211+
202212
try {
203213
const account = this.#getAccount(accountId);
204214

packages/multichain-transactions-controller/src/MultichainTransactionsController.test.ts

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -145,6 +145,7 @@ const setupController = ({
145145
allowedActions: [
146146
'SnapController:handleRequest',
147147
'AccountsController:listMultichainAccounts',
148+
'KeyringController:getState',
148149
],
149150
allowedEvents: [
150151
'AccountsController:accountAdded',
@@ -169,6 +170,14 @@ const setupController = ({
169170
),
170171
);
171172

173+
const mockGetKeyringState = jest.fn().mockReturnValue({
174+
isUnlocked: true,
175+
});
176+
messenger.registerActionHandler(
177+
'KeyringController:getState',
178+
mockGetKeyringState,
179+
);
180+
172181
const controller = new MultichainTransactionsController({
173182
messenger: multichainTransactionsControllerMessenger,
174183
state,
@@ -179,6 +188,7 @@ const setupController = ({
179188
messenger,
180189
mockSnapHandleRequest,
181190
mockListMultichainAccounts,
191+
mockGetKeyringState,
182192
};
183193
};
184194

@@ -661,4 +671,26 @@ describe('MultichainTransactionsController', () => {
661671
nullTimestampTx2,
662672
]);
663673
});
674+
675+
it('resumes updating transactions after unlocking KeyringController', async () => {
676+
const { controller, mockGetKeyringState } = setupController();
677+
678+
mockGetKeyringState.mockReturnValue({ isUnlocked: false });
679+
680+
await controller.updateTransactionsForAccount(mockBtcAccount.id);
681+
expect(
682+
controller.state.nonEvmTransactions[mockBtcAccount.id],
683+
).toBeUndefined();
684+
685+
mockGetKeyringState.mockReturnValue({ isUnlocked: true });
686+
687+
await controller.updateTransactionsForAccount(mockBtcAccount.id);
688+
expect(
689+
controller.state.nonEvmTransactions[mockBtcAccount.id],
690+
).toStrictEqual({
691+
transactions: mockTransactionResult.data,
692+
next: null,
693+
lastUpdated: expect.any(Number),
694+
});
695+
});
664696
});

packages/multichain-transactions-controller/src/MultichainTransactionsController.ts

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ import {
1515
type Transaction,
1616
type AccountTransactionsUpdatedEventPayload,
1717
} from '@metamask/keyring-api';
18+
import type { KeyringControllerGetStateAction } from '@metamask/keyring-controller';
1819
import type { InternalAccount } from '@metamask/keyring-internal-api';
1920
import { KeyringClient } from '@metamask/keyring-snap-client';
2021
import type { HandleSnapRequest } from '@metamask/snaps-controllers';
@@ -110,6 +111,7 @@ export type MultichainTransactionsControllerMessenger = RestrictedMessenger<
110111
*/
111112
export type AllowedActions =
112113
| HandleSnapRequest
114+
| KeyringControllerGetStateAction
113115
| AccountsControllerListMultichainAccountsAction;
114116

115117
/**
@@ -244,6 +246,14 @@ export class MultichainTransactionsController extends BaseController<
244246
* @param accountId - The ID of the account to get transactions for.
245247
*/
246248
async updateTransactionsForAccount(accountId: string) {
249+
const { isUnlocked } = this.messagingSystem.call(
250+
'KeyringController:getState',
251+
);
252+
253+
if (!isUnlocked) {
254+
return;
255+
}
256+
247257
try {
248258
const account = this.#listAccounts().find(
249259
(accountItem) => accountItem.id === accountId,

0 commit comments

Comments
 (0)