Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions packages/snap-account-service/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
- Add `getLegacySnapKeyring` ([#8757](https://github.com/MetaMask/core/pull/8757))
- This is a concurrent-safe variant of the existing `getSnapKeyring` function that exist on clients.
- The service messenger now requires the `KeyringController:withController` action.
- Add `handleKeyringSnapMessage` ([#8758](https://github.com/MetaMask/core/pull/8758))
- This will be the new entry point for consumer that needs to forward keyring events to a account management Snap (instead of using the legacy Snap keyring instance directly).

### Changed

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,10 +43,23 @@ export type SnapAccountServiceGetLegacySnapKeyringAction = {
handler: SnapAccountService['getLegacySnapKeyring'];
};

/**
* Handle a message from a Snap.
*
* @param snapId - ID of the Snap.
* @param message - Message sent by the Snap.
* @returns The execution result.
*/
export type SnapAccountServiceHandleKeyringSnapMessageAction = {
type: `SnapAccountService:handleKeyringSnapMessage`;
handler: SnapAccountService['handleKeyringSnapMessage'];
};
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New action type not exported from package index

Medium Severity

SnapAccountServiceHandleKeyringSnapMessageAction is defined and added to the SnapAccountServiceActions union (which is exported), but the individual type itself is not re-exported from index.ts. The existing SnapAccountServiceEnsureReadyAction and SnapAccountServiceGetSnapsAction types are both exported individually, so this is inconsistent and prevents consumers from importing the new action type by name.

Fix in Cursor Fix in Web

Triggered by project rule: Guidance for Bugbot

Reviewed by Cursor Bugbot for commit 92068a5. Configure here.


/**
* Union of all SnapAccountService action types.
*/
export type SnapAccountServiceMethodActions =
| SnapAccountServiceGetSnapsAction
| SnapAccountServiceEnsureReadyAction
| SnapAccountServiceGetLegacySnapKeyringAction;
| SnapAccountServiceGetLegacySnapKeyringAction
| SnapAccountServiceHandleKeyringSnapMessageAction;
99 changes: 98 additions & 1 deletion packages/snap-account-service/src/SnapAccountService.test.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import type { SnapKeyring } from '@metamask/eth-snap-keyring';
import type { SnapKeyring, SnapMessage } from '@metamask/eth-snap-keyring';
import {
KeyringControllerState,
KeyringTypes,
Expand Down Expand Up @@ -192,6 +192,44 @@ function mockWithController(
return { addNewKeyring };
}

/**
* Configures `mocks.KeyringController.withController` to expose a single
* legacy Snap keyring with the provided mocked methods.
*
* @param mocks - The mocks object from {@link setup}.
* @param keyring - The mocked Snap keyring methods.
* @param keyring.handleKeyringSnapMessage - The mocked implementation.
*/
function mockLegacySnapKeyring(
mocks: Mocks,
{
handleKeyringSnapMessage,
}: {
handleKeyringSnapMessage: jest.MockedFunction<
SnapKeyring['handleKeyringSnapMessage']
>;
},
): void {
const snapKeyring = {
type: KeyringTypes.snap,
handleKeyringSnapMessage,
};
mocks.KeyringController.withController.mockImplementation(async (operation) =>
operation({
get keyrings() {
return Object.freeze([
{
keyring: snapKeyring as unknown as KeyringEntry['keyring'],
metadata: { id: 'id-snap', name: KeyringTypes.snap },
},
]);
},
addNewKeyring: jest.fn(),
removeKeyring: jest.fn(),
}),
);
}

/**
* Constructs the service under test with sensible defaults.
*
Expand Down Expand Up @@ -452,4 +490,63 @@ describe('SnapAccountService', () => {
await expect(service.getLegacySnapKeyring()).rejects.toThrow('boom');
});
});

describe('handleKeyringSnapMessage', () => {
const MOCK_MESSAGE = {
method: 'keyring_listAccounts',
params: {},
} as unknown as SnapMessage;

it('forwards the call to the legacy Snap keyring and returns its result', async () => {
const { service, mocks } = setup();
const handleKeyringSnapMessage = jest
.fn()
.mockResolvedValue({ ok: true });
mockLegacySnapKeyring(mocks, { handleKeyringSnapMessage });

const result = await service.handleKeyringSnapMessage(
MOCK_SNAP_ID,
MOCK_MESSAGE,
);

expect(handleKeyringSnapMessage).toHaveBeenCalledWith(
MOCK_SNAP_ID,
MOCK_MESSAGE,
);
expect(result).toStrictEqual({ ok: true });
});

it('propagates errors thrown by the Snap keyring', async () => {
const { service, mocks } = setup();
const error = new Error('snap boom');
const handleKeyringSnapMessage = jest.fn().mockRejectedValue(error);
mockLegacySnapKeyring(mocks, { handleKeyringSnapMessage });

await expect(
service.handleKeyringSnapMessage(MOCK_SNAP_ID, MOCK_MESSAGE),
).rejects.toThrow(error);
});

it('is exposed as a messenger action', async () => {
const { service, mocks, messenger } = setup();
const handleKeyringSnapMessage = jest.fn().mockResolvedValue('pong');
mockLegacySnapKeyring(mocks, { handleKeyringSnapMessage });

// Reference `service` so it isn't flagged as unused; constructing it
// registers the messenger action under test.
expect(service).toBeDefined();

const result = await messenger.call(
'SnapAccountService:handleKeyringSnapMessage',
MOCK_SNAP_ID,
MOCK_MESSAGE,
);

expect(handleKeyringSnapMessage).toHaveBeenCalledWith(
MOCK_SNAP_ID,
MOCK_MESSAGE,
);
expect(result).toBe('pong');
});
});
});
26 changes: 24 additions & 2 deletions packages/snap-account-service/src/SnapAccountService.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,7 @@
import type { SnapKeyring as LegacySnapKeyring } from '@metamask/eth-snap-keyring';
import type {
SnapKeyring as LegacySnapKeyring,
SnapMessage,
} from '@metamask/eth-snap-keyring';
import type {
KeyringControllerGetStateAction,
KeyringControllerStateChangeEvent,
Expand All @@ -21,12 +24,14 @@ import type {
SnapControllerStateChangeEvent,
} from '@metamask/snaps-controllers';
import { SnapId } from '@metamask/snaps-sdk';
import type { Json } from '@metamask/utils';

import { projectLogger as log } from './logger';
import type {
SnapAccountServiceEnsureReadyAction,
SnapAccountServiceGetLegacySnapKeyringAction,
SnapAccountServiceGetSnapsAction,
SnapAccountServiceHandleKeyringSnapMessageAction,
} from './SnapAccountService-method-action-types';
import { SnapPlatformWatcher } from './SnapPlatformWatcher';
import type { SnapPlatformWatcherConfig } from './SnapPlatformWatcher';
Expand All @@ -46,6 +51,7 @@ const MESSENGER_EXPOSED_METHODS = [
'ensureReady',
'getSnaps',
'getLegacySnapKeyring',
'handleKeyringSnapMessage',
] as const;

/**
Expand All @@ -54,7 +60,8 @@ const MESSENGER_EXPOSED_METHODS = [
export type SnapAccountServiceActions =
| SnapAccountServiceEnsureReadyAction
| SnapAccountServiceGetSnapsAction
| SnapAccountServiceGetLegacySnapKeyringAction;
| SnapAccountServiceGetLegacySnapKeyringAction
| SnapAccountServiceHandleKeyringSnapMessageAction;

/**
* Actions from other messengers that {@link SnapAccountService} calls.
Expand Down Expand Up @@ -246,4 +253,19 @@ export class SnapAccountService {

return (result as Result).snapKeyring;
}

/**
* Handle a message from a Snap.
*
* @param snapId - ID of the Snap.
* @param message - Message sent by the Snap.
* @returns The execution result.
*/
async handleKeyringSnapMessage(
snapId: SnapId,
message: SnapMessage,
): Promise<Json> {
const snapKeyring = await this.getLegacySnapKeyring();
return snapKeyring.handleKeyringSnapMessage(snapId, message);
}
}
Loading