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

feat: add request id to messageParams in @metamask/message-managers #4636

Merged
merged 14 commits into from
Sep 3, 2024
Merged

feat: add request id to messageParams in @metamask/message-managers #4636

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
9b12571
feat: add request id to messageParams
digiwand Aug 22, 2024
41878c9
refactor: message-manager abstract addRequestToMessageParams and crea…
digiwand Aug 26, 2024
c94b420
test: AbstractMessageManager protected methods
digiwand Aug 27, 2024
2e99d27
test:refactor: update mockMessageParams
digiwand Aug 27, 2024
395cb8e
fix: rm redundant MockExtendedAbstractTestManager
digiwand Aug 27, 2024
d108c99
test: add messageParams origin check
digiwand Aug 28, 2024
da3413f
test: fix 218 unchecked lines and hit 100% coverage by adding setMess…
digiwand Aug 28, 2024
b3202ef
fix: rm type MessageParams
digiwand Aug 28, 2024
ca78ab0
fix: rm unused MessageParams import
digiwand Aug 28, 2024
3845d90
fix: test AbstractMessageParams
digiwand Aug 28, 2024
6386aea
fix: revert message-manager updatedMessageParams removal
digiwand Aug 28, 2024
784bb66
refactor: TS message-manager add return time & use satisfies
digiwand Aug 29, 2024
70058af
fix: TS AbstractMessageManager test
digiwand Aug 29, 2024
f7db429
Merge branch 'main' into feat-add-request-id-to-messageParams
digiwand Sep 3, 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
3 changes: 3 additions & 0 deletions packages/message-manager/src/AbstractMessageManager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import { EventEmitter } from 'events';
// Convert to a `type` in a future major version.
// eslint-disable-next-line @typescript-eslint/consistent-type-definitions
export interface OriginalRequest {
id?: number;
origin?: string;
securityAlertResponse?: Record<string, Json>;
}
Expand Down Expand Up @@ -49,6 +50,7 @@ export interface AbstractMessage {
* Represents the parameters to pass to the signing method once the signature request is approved.
* @property from - Address from which the message is processed
* @property origin? - Added for request origin identification
* @property requestId? - Original request id
* @property deferSetAsSigned? - Whether to defer setting the message as signed immediately after the keyring is told to sign it
*/
// This interface was created before this ESLint rule was added.
Expand All @@ -57,6 +59,7 @@ export interface AbstractMessage {
export interface AbstractMessageParams {
from: string;
origin?: string;
requestId?: number;
deferSetAsSigned?: boolean;
}

Expand Down
3 changes: 2 additions & 1 deletion packages/message-manager/src/DecryptMessageManager.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,7 @@ describe('DecryptMessageManager', () => {
const messageStatus = 'unapproved';
const messageType = 'eth_decrypt';
const messageParams = { from: fromMock, data: dataMock };
const originalRequest = { origin: 'origin' };
const originalRequest = { id: 111, origin: 'origin' };
const messageId = await controller.addUnapprovedMessage(
messageParams,
originalRequest,
Expand All @@ -151,6 +151,7 @@ describe('DecryptMessageManager', () => {
throw new Error('"message" is falsy');
}
expect(message.messageParams.from).toBe(messageParams.from);
expect(message.messageParams.requestId).toBe(originalRequest.id);
Copy link
Contributor Author

@digiwand digiwand Aug 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

the requestId is not needed on decrypt or encrypt, but added for consistency

decrypt and encrypt are being deprecated

expect(message.time).toBeDefined();
expect(message.status).toBe(messageStatus);
expect(message.type).toBe(messageType);
Expand Down
1 change: 1 addition & 0 deletions packages/message-manager/src/DecryptMessageManager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,7 @@ export class DecryptMessageManager extends AbstractMessageManager<
req?: OriginalRequest,
) {
if (req) {
messageParams.requestId = req.id;
matthewwalsh0 marked this conversation as resolved.
Show resolved Hide resolved
messageParams.origin = req.origin;
}
messageParams.data = normalizeMessageData(messageParams.data);
Expand Down
3 changes: 2 additions & 1 deletion packages/message-manager/src/EncryptionPublicKeyManager.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@ describe('EncryptionPublicKeyManager', () => {
const messageParams = {
from: fromMock,
};
const originalRequest = { origin: 'origin' };
const originalRequest = { id: 111, origin: 'origin' };
const messageId = await controller.addUnapprovedMessage(
messageParams,
originalRequest,
Expand All @@ -131,6 +131,7 @@ describe('EncryptionPublicKeyManager', () => {
throw new Error('"message" is falsy');
}
expect(message.messageParams.from).toBe(messageParams.from);
expect(message.messageParams.requestId).toBe(originalRequest.id);
expect(message.time).toBeDefined();
expect(message.status).toBe(messageStatus);
expect(message.type).toBe(messageType);
Expand Down
1 change: 1 addition & 0 deletions packages/message-manager/src/EncryptionPublicKeyManager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,7 @@ export class EncryptionPublicKeyManager extends AbstractMessageManager<
req?: OriginalRequest,
): Promise<string> {
if (req) {
messageParams.requestId = req.id;
messageParams.origin = req.origin;
}
const messageId = random();
Expand Down
2 changes: 2 additions & 0 deletions packages/message-manager/src/PersonalMessageManager.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@ describe('PersonalMessageManager', () => {
from: fromMock,
};
const originalRequest = {
id: 111,
origin: 'origin',
// TODO: Either fix this lint violation or explain why it's necessary to ignore.
// eslint-disable-next-line @typescript-eslint/naming-convention
Expand All @@ -95,6 +96,7 @@ describe('PersonalMessageManager', () => {
}
expect(message.messageParams.from).toBe(messageParams.from);
expect(message.messageParams.data).toBe(messageParams.data);
expect(message.messageParams.requestId).toBe(originalRequest.id);
expect(message.time).toBeDefined();
expect(message.status).toBe(messageStatus);
expect(message.type).toBe(messageType);
Expand Down
1 change: 1 addition & 0 deletions packages/message-manager/src/PersonalMessageManager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,7 @@ export class PersonalMessageManager extends AbstractMessageManager<
): Promise<string> {
validateSignMessageData(messageParams);
if (req) {
messageParams.requestId = req.id;
messageParams.origin = req.origin;
}
messageParams.data = normalizeMessageData(messageParams.data);
Expand Down
10 changes: 7 additions & 3 deletions packages/message-manager/src/TypedMessageManager.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ describe('TypedMessageManager', () => {
data: messageData,
from: fromMock,
};
const originalRequest = { origin: 'origin' };
const originalRequest = { id: 111, origin: 'origin' };

await expect(
controller.addUnapprovedMessage(messageParams, originalRequest, version),
Expand All @@ -126,6 +126,7 @@ describe('TypedMessageManager', () => {
from: fromMock,
};
const originalRequest = {
id: 111,
origin: 'origin',
// TODO: Either fix this lint violation or explain why it's necessary to ignore.
// eslint-disable-next-line @typescript-eslint/naming-convention
Expand All @@ -143,6 +144,7 @@ describe('TypedMessageManager', () => {
}
expect(message.messageParams.from).toBe(messageParams.from);
expect(message.messageParams.data).toBe(messageParams.data);
expect(message.messageParams.requestId).toBe(originalRequest.id);
expect(message.time).toBeDefined();
expect(message.status).toBe(messageStatus);
expect(message.type).toBe(messageType);
Expand All @@ -160,7 +162,7 @@ describe('TypedMessageManager', () => {
data: messageData,
from: fromMock,
};
const originalRequest = { origin: 'origin' };
const originalRequest = { id: 111, origin: 'origin' };
const messageId = await controller.addUnapprovedMessage(
messageParams,
originalRequest,
Expand All @@ -173,6 +175,7 @@ describe('TypedMessageManager', () => {
}
expect(message.messageParams.from).toBe(messageParams.from);
expect(message.messageParams.data).toBe(messageParams.data);
expect(message.messageParams.requestId).toBe(originalRequest.id);
expect(message.time).toBeDefined();
expect(message.status).toBe(messageStatus);
expect(message.type).toBe(messageType);
Expand All @@ -188,7 +191,7 @@ describe('TypedMessageManager', () => {
data: messageData,
from: fromMock,
};
const originalRequest = { origin: 'origin' };
const originalRequest = { id: 111, origin: 'origin' };
const messageId = await controller.addUnapprovedMessage(
messageParams,
originalRequest,
Expand All @@ -201,6 +204,7 @@ describe('TypedMessageManager', () => {
}
expect(message.messageParams.from).toBe(messageParams.from);
expect(message.messageParams.data).toBe(messageParams.data);
expect(message.messageParams.requestId).toBe(originalRequest.id);
expect(message.time).toBeDefined();
expect(message.status).toBe(messageStatus);
expect(message.type).toBe(messageType);
Expand Down
1 change: 1 addition & 0 deletions packages/message-manager/src/TypedMessageManager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,7 @@ export class TypedMessageManager extends AbstractMessageManager<
version,
};
if (req) {
messageParams.requestId = req.id;
messageParams.origin = req.origin;
}
const messageData: TypedMessage = {
Expand Down
Loading