Skip to content

Conversation

pcarranzav
Copy link
Member

@pcarranzav pcarranzav commented Jan 22, 2025

Fixes #90

Copy link
Collaborator

@cmwhited cmwhited left a comment

Choose a reason for hiding this comment

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

good find. and who doesn't love one less dep

@nikgraf
Copy link
Collaborator

nikgraf commented Jan 23, 2025

Code looks good from my perspective. One thing I would do to be 100% certain is

  1. add @xmtp/xmtp-js as devDependency
  2. add a test to encrypt with our function and decrypt with xmtp
  3. add a test to encrypt with xmtp and decrypt with our function

This way we can be very certain that this is correct. Do you think it's worth it?

@pcarranzav
Copy link
Member Author

The problem is xmtp-js was breaking our tests because of the weird issue on webcrypto.subtle. Their serialization/deserialization after encryption is also different than ours so it would take a bit of wrangling. So I think it would be a good idea but considering the tradeoffs I would prefer not to add that test for now. Once we're closer to a release version we can work with the Foundation to set up an external security review. Does that sound reasonable?

@nikgraf
Copy link
Collaborator

nikgraf commented Jan 23, 2025

ah right, didn't think about the test not working 🤦 In this case my suggestion makes no sense.

@nikgraf nikgraf self-requested a review January 23, 2025 13:43
@pcarranzav pcarranzav merged commit efbb30b into main Jan 23, 2025
3 checks passed
@pcarranzav pcarranzav deleted the pcv/replace-xmtp-encryption branch January 23, 2025 15:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Investigate xmtp encrypt error in tests

3 participants