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.

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

Conversation

digiwand
Copy link
Contributor

@digiwand digiwand commented Aug 22, 2024

Explanation

Adding the request id to reference metric event fragments created from the createRPCMethodTrackingMiddleware in the client. See use-case here: MetaMask/metamask-extension#26597

References

Changelog

@metamask/message-manager

  • feat: Add request id to messageParams

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

@digiwand digiwand requested a review from a team as a code owner August 22, 2024 19:48
@@ -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

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

@digiwand
Copy link
Contributor Author

digiwand commented Aug 27, 2024

edit: Fixed coverage by adding a test for the setMessageStatus expected thrown error

before:
CleanShot 2024-08-27 at 11 00 16@2x

CleanShot 2024-08-27 at 23 24 06@2x

@MajorLift
Copy link
Contributor

MajorLift commented Aug 28, 2024

Hi, I wanted to give a quick heads-up that I'm prepping a release candidate that includes @metamask/message-manager: #4643. I'd be happy to wait for this PR to merge before proceeding with the release if you want this to be included.

Edit: 193.0.0 had to be merged urgently, but I created a new release candidate for this PR here: #4646.

@digiwand
Copy link
Contributor Author

Hi @MajorLift, thank you very much! That would be great. I can update you when this is merged

Copy link
Contributor

@MajorLift MajorLift left a comment

Choose a reason for hiding this comment

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

How would you feel about making addRequestToMessageParams and createUnapprovedMessage generic? This lets us narrow the return types of these methods, allowing us to get rid of the type assertions in TypedMessageManager.

This works because the type of messageParams is now inferred from the argument that it receives. Also, because of the generic constraint on MessageParams, an error will be raised if messageParams is not a subtype of AbstractMessageParams.

packages/message-manager/src/AbstractMessageManager.ts Outdated Show resolved Hide resolved
packages/message-manager/src/AbstractMessageManager.ts Outdated Show resolved Hide resolved
packages/message-manager/src/TypedMessageManager.ts Outdated Show resolved Hide resolved
Co-authored-by: Jongsun Suh <jongsun.suh@icloud.com>
@digiwand
Copy link
Contributor Author

Awesome suggestion @MajorLift! Thank you. Applied the extended return types to the methods and used satisfies to ensure the specified types are matched

@digiwand digiwand merged commit 478d62c into main Sep 3, 2024
116 checks passed
@digiwand digiwand deleted the feat-add-request-id-to-messageParams branch September 3, 2024 17:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants