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

Group Chat Streaming #166

Merged
merged 81 commits into from
Feb 2, 2024
Merged

Group Chat Streaming #166

merged 81 commits into from
Feb 2, 2024

Conversation

nplasterer
Copy link
Contributor

This adds the streaming components of group chat

  • stream messages
  • stream groups
  • stream both groups and conversations together

@nplasterer nplasterer self-assigned this Jan 30, 2024
@nplasterer nplasterer changed the base branch from main to np/group-conversations January 30, 2024 20:03
Base automatically changed from np/group-conversations to main February 1, 2024 05:50
@nplasterer nplasterer marked this pull request as ready for review February 2, 2024 17:35
@nplasterer nplasterer requested a review from a team as a code owner February 2, 2024 17:35
fun streamDecryptedMessages(): Flow<DecryptedMessage> = callbackFlow {
val messageCallback = object : FfiMessageCallback {
override fun onMessage(message: FfiMessage) {
trySend(decrypt(Message(client, message)))
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we want to refer to what you're doing in here as decoding rather than decrypting. The messages all get decrypted in libxmtp.

Copy link
Contributor Author

@nplasterer nplasterer Feb 2, 2024

Choose a reason for hiding this comment

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

So we have two streaming methods one for decoding and one for decrypting. We need this stream decrypted messages for React Native custom content types. @nakajima could speak more to it.

Since Groups are conforming to the current Conversation model I have to use these methods. Once we do the refactor to Groups Dms and Conversation parent we might be able to get rid of some of these things.

But to break as little of the existing flow as possible. This is needed.

Copy link
Contributor

@richardhuaaa richardhuaaa Feb 2, 2024

Choose a reason for hiding this comment

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

What is the intended difference between a DecryptedMessage and a DecodedMessage? When I trace the method calls from both streaming methods, they seem to produce the same result (they both return something with a field called encodedContent that is set to EncodedContent.parseFrom(libXMTPMessage.content))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think there is going to be any difference in the V3 world but again I didn't follow as closely to the reasoning for custom contentTypes. I found this for context xmtp/xmtp-ios#196

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.

Have a question in the PR but accepting to unblock React Native, well done on the persistence and fast turnaround! Did you manage to get both streams working simultaneously?

fun streamDecryptedMessages(): Flow<DecryptedMessage> = callbackFlow {
val messageCallback = object : FfiMessageCallback {
override fun onMessage(message: FfiMessage) {
trySend(decrypt(Message(client, message)))
Copy link
Contributor

@richardhuaaa richardhuaaa Feb 2, 2024

Choose a reason for hiding this comment

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

What is the intended difference between a DecryptedMessage and a DecodedMessage? When I trace the method calls from both streaming methods, they seem to produce the same result (they both return something with a field called encodedContent that is set to EncodedContent.parseFrom(libXMTPMessage.content))

@nplasterer nplasterer merged commit e041fa8 into main Feb 2, 2024
5 of 6 checks passed
@nplasterer nplasterer deleted the np/group-streaming branch February 2, 2024 21:23
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.

3 participants