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

Add PlaintextEnvelope #150

Merged
merged 3 commits into from
Feb 21, 2024
Merged

Add PlaintextEnvelope #150

merged 3 commits into from
Feb 21, 2024

Conversation

neekolas
Copy link
Collaborator

@neekolas neekolas commented Feb 21, 2024

Summary

Adds a new PlainTextEnvelope that wraps both the EncodedContent and the idempotency_key fields.

It's important to clearly differentiate where this envelope is used, and where we want to break things apart.

Data going over the wire to the API: A serialized MlsEnvelope is the result of decrypting the data field of a V1 GroupMessage
Data stored in the DB: content and idempotency_key stored separately
Messages returned to callers of list or stream methods: content is only returned

Generally I don't think the idempotency key should ever make its way over the FFI boundary to integrators. WDYT about that?

Other names considered

  • Envelope. We already have a V1 Envelope, which has unencrypted fields. Seems confusing
  • EncryptedEnvelope. Feels unclear whether the contents are encrypted or whether this message is meant to be encrypted
  • InnerEnvelope. I don't hate this, although it's probably more accurate to say that it's in the middle. EncodedContent lives inside of it.

Issue

xmtp/libxmtp#529

Copy link
Contributor

@richardhuaaa richardhuaaa left a comment

Choose a reason for hiding this comment

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

Data stored in the DB: content and idempotency_key stored separately

Do you mean the libxmtp DB here? Could we just use the key to derive and store the message_id, and then throw the key away?

Generally I don't think the idempotency key should ever make its way over the FFI boundary to integrators. WDYT about that?

That makes a lot of sense!

proto/mls/message_contents/content.proto Show resolved Hide resolved
@richardhuaaa
Copy link
Contributor

There's 7k lines removed here - do you need to add the code-genned stuff back?

@neekolas
Copy link
Collaborator Author

neekolas commented Feb 21, 2024

Do you mean the libxmtp DB here? Could we just use the key to derive and store the message_id, and then throw the key away?

I'd be fine with that. Just need to make sure it works for all retry scenarios (I think it should). I'll leave that up to @tuddman

@tuddman
Copy link
Contributor

tuddman commented Feb 21, 2024

Screenshot 2024-02-21 at 19 45 44

Normally I love a PR that reduces LoC + increases functionality, but: Is this intentional & correct?

@richardhuaaa
Copy link
Contributor

Other names considered

Maybe DecryptedMessageEnvelope, or DecryptedEnvelope or PlaintextEnvelope? MlsEnvelope is fine too

@neekolas
Copy link
Collaborator Author

Something must have gone haywire in the go generator. Re-ran it and it added the code back

@neekolas
Copy link
Collaborator Author

@richardhuaaa I don't mind PlaintextEnvelope

@tuddman
Copy link
Contributor

tuddman commented Feb 21, 2024

@richardhuaaa I don't mind PlaintextEnvelope

👍 That's the clearest by my reading in describing what this is.

@neekolas neekolas merged commit 5d1c8db into main Feb 21, 2024
8 checks passed
@neekolas neekolas deleted the nm/add-idempotency-key branch February 21, 2024 19:10
Copy link

🎉 This PR is included in version 3.44.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@tuddman tuddman changed the title Add MlsEnvelope Add PlaintextEnvelope Feb 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants