Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: remove global network from signature controller #4797

Merged
merged 9 commits into from
Oct 24, 2024
+131 −59
Merged

feat: remove global network from signature controller #4797

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
6d497e3
Store chain ID and network client ID in metadata
matthewwalsh0 Oct 11, 2024
d7451fe
Merge branch 'main' into feat/signature-controller-no-global-chain-id
matthewwalsh0 Oct 23, 2024
1800a17
Merge branch 'main' into feat/signature-controller-no-global-chain-id
matthewwalsh0 Oct 23, 2024
9ae1d0f
Remove unused constructor properties
matthewwalsh0 Oct 24, 2024
c77aebd
Align network controller versions
matthewwalsh0 Oct 24, 2024
6d418e8
Merge branch 'main' into feat/signature-controller-no-global-chain-id
matthewwalsh0 Oct 24, 2024
be49ad0
Fix unit tests
matthewwalsh0 Oct 24, 2024
5350121
Fix network controller version
matthewwalsh0 Oct 24, 2024
bef7c99
Merge branch 'main' into feat/signature-controller-no-global-chain-id
matthewwalsh0 Oct 24, 2024
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
4 changes: 3 additions & 1 deletion packages/signature-controller/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@
"@metamask/auto-changelog": "^3.4.4",
"@metamask/keyring-controller": "^17.3.0",
"@metamask/logging-controller": "^6.0.1",
"@metamask/network-controller": "^22.0.0",
"@types/jest": "^27.4.1",
"deepmerge": "^4.2.2",
"jest": "^27.5.1",
Expand All @@ -71,7 +72,8 @@
"peerDependencies": {
"@metamask/approval-controller": "^7.0.0",
"@metamask/keyring-controller": "^17.0.0",
"@metamask/logging-controller": "^6.0.0"
"@metamask/logging-controller": "^6.0.0",
"@metamask/network-controller": "^22.0.0"
},
"engines": {
"node": "^18.18 || >=20"
Expand Down
97 changes: 69 additions & 28 deletions packages/signature-controller/src/SignatureController.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ jest.mock('@metamask/controller-utils', () => ({

const ID_MOCK = '123-456';
const CHAIN_ID_MOCK = '0x1';
const NETWORK_CLIENT_ID_MOCK = 'testNetworkClientId';
const FROM_MOCK = '0x456DEF';
const DATA_MOCK = '0xABC123';
const SIGNATURE_HASH_MOCK = '0x123ABC';
Expand All @@ -49,9 +50,15 @@ const PARAMS_MOCK = {
version: SignTypedDataVersion.V1,
};

const REQUEST_MOCK = {
networkClientId: NETWORK_CLIENT_ID_MOCK,
};

const SIGNATURE_REQUEST_MOCK: SignatureRequest = {
chainId: CHAIN_ID_MOCK,
id: ID_MOCK,
messageParams: PARAMS_MOCK,
networkClientId: NETWORK_CLIENT_ID_MOCK,
status: SignatureRequestStatus.Signed,
time: Date.now(),
type: SignatureRequestType.PersonalSign,
Expand All @@ -66,6 +73,7 @@ function createMessengerMock() {
const approvalControllerAddRequestMock = jest.fn();
const keyringControllerSignPersonalMessageMock = jest.fn();
const keyringControllerSignTypedMessageMock = jest.fn();
const networkControllerGetNetworkClientByIdMock = jest.fn();

// eslint-disable-next-line @typescript-eslint/no-explicit-any
const callMock = (method: string, ...args: any[]) => {
Expand All @@ -78,6 +86,8 @@ function createMessengerMock() {
return keyringControllerSignPersonalMessageMock(...args);
case 'KeyringController:signTypedMessage':
return keyringControllerSignTypedMessageMock(...args);
case 'NetworkController:getNetworkClientById':
return networkControllerGetNetworkClientByIdMock(...args);
default:
throw new Error(`Messenger method not recognised: ${method}`);
}
Expand All @@ -93,6 +103,12 @@ function createMessengerMock() {
approvalControllerAddRequestMock.mockResolvedValue({});
loggingControllerAddMock.mockResolvedValue({});

networkControllerGetNetworkClientByIdMock.mockReturnValue({
configuration: {
chainId: CHAIN_ID_MOCK,
},
});

return {
approvalControllerAddRequestMock,
keyringControllerSignPersonalMessageMock,
Expand All @@ -112,7 +128,6 @@ function createController(options?: Partial<SignatureControllerOptions>) {

const controller = new SignatureController({
messenger: messengerMocks.messenger,
getCurrentChainId: () => CHAIN_ID_MOCK,
...options,
});

Expand Down Expand Up @@ -338,7 +353,7 @@ describe('SignatureController', () => {
'newUnsignedPersonalMessage',
(
controller: SignatureController,
request?: OriginalRequest,
request: OriginalRequest,
params?: Partial<MessageParamsPersonal>,
) =>
controller.newUnsignedPersonalMessage(
Expand All @@ -351,7 +366,7 @@ describe('SignatureController', () => {
'newUnsignedTypedMessage',
(
controller: SignatureController,
request?: OriginalRequest,
request: OriginalRequest,
params?: Partial<MessageParamsTyped>,
) =>
controller.newUnsignedTypedMessage(
Expand All @@ -372,7 +387,7 @@ describe('SignatureController', () => {
approvalControllerAddRequestMock.mockRejectedValueOnce(error);

await expect(
controller.newUnsignedPersonalMessage({ ...PARAMS_MOCK }, {}),
controller.newUnsignedPersonalMessage({ ...PARAMS_MOCK }, REQUEST_MOCK),
).rejects.toMatchObject({
message: ERROR_MESSAGE_MOCK,
code: ERROR_CODE_MOCK,
Expand Down Expand Up @@ -403,7 +418,7 @@ describe('SignatureController', () => {
SIGNATURE_HASH_MOCK,
);

await fn(controller);
await fn(controller, REQUEST_MOCK);

expect(resultCallbackSuccessMock).toHaveBeenCalledTimes(1);
expect(resultCallbackSuccessMock).toHaveBeenCalledWith(
Expand All @@ -418,7 +433,7 @@ describe('SignatureController', () => {

controller.hub.on(`${ID_MOCK}:finished`, listener);

await fn(controller);
await fn(controller, REQUEST_MOCK);

const state = controller.state.signatureRequests[ID_MOCK];

Expand All @@ -438,7 +453,7 @@ describe('SignatureController', () => {

approvalControllerAddRequestMock.mockRejectedValueOnce(errorMock);

await fn(controller).catch(() => {
await fn(controller, REQUEST_MOCK).catch(() => {
// Ignore error
});

Expand All @@ -451,7 +466,7 @@ describe('SignatureController', () => {
it('adds logs to logging controller if approved', async () => {
const { controller, loggingControllerAddMock } = createController();

await fn(controller);
await fn(controller, REQUEST_MOCK);

expect(loggingControllerAddMock).toHaveBeenCalledTimes(2);

Expand Down Expand Up @@ -485,7 +500,7 @@ describe('SignatureController', () => {

approvalControllerAddRequestMock.mockRejectedValueOnce(errorMock);

await expect(fn(controller)).rejects.toThrow(errorMock);
await expect(fn(controller, REQUEST_MOCK)).rejects.toThrow(errorMock);

expect(loggingControllerAddMock).toHaveBeenCalledTimes(2);

Expand All @@ -511,7 +526,7 @@ describe('SignatureController', () => {
it('populates origin from request if present', async () => {
const { controller } = createController();

await fn(controller, { origin: 'test' });
await fn(controller, { ...REQUEST_MOCK, origin: 'test' });

expect(
controller.state.signatureRequests[ID_MOCK].messageParams.origin,
Expand All @@ -521,7 +536,7 @@ describe('SignatureController', () => {
it('populates origin from message params if no request', async () => {
const { controller } = createController();

await fn(controller);
await fn(controller, REQUEST_MOCK);

expect(
controller.state.signatureRequests[ID_MOCK].messageParams.origin,
Expand All @@ -531,7 +546,7 @@ describe('SignatureController', () => {
it('populates request ID from request', async () => {
const { controller } = createController();

await fn(controller, { id: 123 });
await fn(controller, { ...REQUEST_MOCK, id: 123 });

expect(
controller.state.signatureRequests[ID_MOCK].messageParams.requestId,
Expand All @@ -541,21 +556,29 @@ describe('SignatureController', () => {
it('populates metamask ID using ID', async () => {
const { controller } = createController();

await fn(controller);
await fn(controller, REQUEST_MOCK);

expect(
controller.state.signatureRequests[ID_MOCK].messageParams.metamaskId,
).toBe(ID_MOCK);
});

it('throws if no network client ID in request', async () => {
const { controller } = createController();

await expect(fn(controller, {} as OriginalRequest)).rejects.toThrow(
'Network client ID not found in request',
);
});

it('emits unapproved message event', async () => {
const listener = jest.fn();

const { controller } = createController();

controller.hub.on('unapprovedMessage', listener);

await fn(controller);
await fn(controller, REQUEST_MOCK);

expect(listener).toHaveBeenCalledTimes(1);
expect(listener).toHaveBeenCalledWith({
Expand Down Expand Up @@ -583,7 +606,7 @@ describe('SignatureController', () => {

controller.hub.on(`${type}:signed`, listener);

await fn(controller);
await fn(controller, REQUEST_MOCK);

expect(listener).toHaveBeenCalledTimes(1);
expect(listener).toHaveBeenCalledWith({
Expand All @@ -608,7 +631,7 @@ describe('SignatureController', () => {
keyringControllerSignPersonalMessageMock.mockRejectedValueOnce(errorMock);
keyringControllerSignTypedMessageMock.mockRejectedValueOnce(errorMock);

await fn(controller).catch(() => {
await fn(controller, REQUEST_MOCK).catch(() => {
// Ignore error
});

Expand Down Expand Up @@ -639,7 +662,9 @@ describe('SignatureController', () => {
keyringControllerSignPersonalMessageMock.mockRejectedValueOnce(errorMock);
keyringControllerSignTypedMessageMock.mockRejectedValueOnce(errorMock);

await expect(fn(controller)).rejects.toThrow(ERROR_MESSAGE_MOCK);
await expect(fn(controller, REQUEST_MOCK)).rejects.toThrow(
ERROR_MESSAGE_MOCK,
);

expect(resultCallbackErrorMock).toHaveBeenCalledTimes(1);
expect(resultCallbackErrorMock).toHaveBeenCalledWith(errorMock);
Expand All @@ -649,7 +674,7 @@ describe('SignatureController', () => {
const { controller, approvalControllerAddRequestMock } =
createController();

await fn(controller);
await fn(controller, REQUEST_MOCK);

expect(approvalControllerAddRequestMock).toHaveBeenCalledTimes(1);
expect(approvalControllerAddRequestMock).toHaveBeenCalledWith(
Expand All @@ -671,7 +696,7 @@ describe('SignatureController', () => {
const { controller, approvalControllerAddRequestMock } =
createController();

await fn(controller, undefined, { origin: undefined });
await fn(controller, REQUEST_MOCK, { origin: undefined });

expect(approvalControllerAddRequestMock).toHaveBeenCalledTimes(1);
expect(approvalControllerAddRequestMock).toHaveBeenCalledWith(
Expand All @@ -688,6 +713,19 @@ describe('SignatureController', () => {
true,
);
});

it('persists network client ID and chain ID in metadata', async () => {
const { controller } = createController();

await fn(controller, REQUEST_MOCK);

expect(controller.state.signatureRequests[ID_MOCK]).toStrictEqual(
expect.objectContaining({
chainId: CHAIN_ID_MOCK,
networkClientId: NETWORK_CLIENT_ID_MOCK,
}),
);
});
});

describe('newUnsignedPersonalMessage', () => {
Expand All @@ -701,7 +739,7 @@ describe('SignatureController', () => {

const result = await controller.newUnsignedPersonalMessage(
{ ...PARAMS_MOCK },
{},
REQUEST_MOCK,
);

expect(result).toBe(SIGNATURE_HASH_MOCK);
Expand All @@ -717,7 +755,10 @@ describe('SignatureController', () => {

detectSIWEMock.mockReturnValueOnce(siweMock);

await controller.newUnsignedPersonalMessage({ ...PARAMS_MOCK }, {});
await controller.newUnsignedPersonalMessage(
{ ...PARAMS_MOCK },
REQUEST_MOCK,
);

const messageParams = controller.state.signatureRequests[ID_MOCK]
.messageParams as MessageParamsPersonal;
Expand All @@ -737,7 +778,7 @@ describe('SignatureController', () => {

const result = await controller.newUnsignedTypedMessage(
PARAMS_MOCK,
{},
REQUEST_MOCK,
SignTypedDataVersion.V1,
{ parseJsonData: false },
);
Expand All @@ -760,7 +801,7 @@ describe('SignatureController', () => {
...PARAMS_MOCK,
data: JSON.stringify({ test: 123 }),
},
{},
REQUEST_MOCK,
version as SignTypedDataVersion,
{ parseJsonData: true },
);
Expand Down Expand Up @@ -791,7 +832,7 @@ describe('SignatureController', () => {
...PARAMS_MOCK,
data: JSON.stringify({ test: 123 }),
},
{},
REQUEST_MOCK,
SignTypedDataVersion.V1,
{ parseJsonData: true },
);
Expand All @@ -818,7 +859,7 @@ describe('SignatureController', () => {
await expect(
controller.newUnsignedTypedMessage(
PARAMS_MOCK,
{},
REQUEST_MOCK,
SignTypedDataVersion.V3,
{ parseJsonData: false },
),
Expand All @@ -837,7 +878,7 @@ describe('SignatureController', () => {

await controller.newUnsignedTypedMessage(
PARAMS_MOCK,
{},
REQUEST_MOCK,
SignTypedDataVersion.V3,
{ parseJsonData: false },
);
Expand Down Expand Up @@ -885,7 +926,7 @@ describe('SignatureController', () => {
...PARAMS_MOCK,
deferSetAsSigned: true,
},
{},
REQUEST_MOCK,
)
.then((result) => {
resolved = true;
Expand Down Expand Up @@ -963,7 +1004,7 @@ describe('SignatureController', () => {
...PARAMS_MOCK,
deferSetAsSigned: true,
},
{},
REQUEST_MOCK,
)
.catch((error) => {
rejectedError = error;
Expand Down
Loading
Loading