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 Groups to Conversations #160

Merged
merged 92 commits into from
Feb 1, 2024
Merged

Add Groups to Conversations #160

merged 92 commits into from
Feb 1, 2024

Conversation

nplasterer
Copy link
Contributor

Part of #157

This adds a new class called Group that behaves similar to a Conversation and allows listing and sending of messages.

@nplasterer nplasterer self-assigned this Jan 23, 2024
@nplasterer nplasterer changed the base branch from main to np/group-spike January 23, 2024 20:18
@@ -99,13 +124,16 @@ sealed class Conversation {
.setKeyMaterial(conversationV2.keyMaterial.toByteString()),
),
).build()

is Group -> throw XMTPException("Groups do not support topics")
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is TopicData and keyMaterial exposed in v2? What will integrators do with it?

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 believe TopicData and keyMaterial is used for push notifications. That way they can export from react native or javascript and import into iOS or android and not need to reload the entire costly conversation.list() method to decrypt a push notification.

@@ -552,6 +559,13 @@ class Client() {
return runBlocking { query(Topic.contact(peerAddress)).envelopesList.size > 0 }
}

fun canMessage(addresses: List<String>): Boolean {
return runBlocking {
libXMTPClient != null && !libXMTPClient!!.canMessage(addresses.map { it.lowercase() })
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@richardhuaaa my tests failed after I implemented this. I think this is another scenario where lowercase should be handled on the rust side.

Copy link
Contributor

Choose a reason for hiding this comment

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

@@ -127,6 +155,8 @@ sealed class Conversation {
is V2 -> {
conversationV2.prepareMessage(content = content, options = options)
}

is Group -> throw XMTPException("Groups do not support prepared messages") // We return a encoded content not a preparedmessage which requires a envelope
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not a kotlin expert and am not familiar with sealed classes. Are these methods being exposed directly to implementors, and are they required to know which methods they can/can't call on groups?

If so, can we find a way to use strong typing (for example, separate types for group and 1:1) where only the methods they can call are exposed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ya I definitely have thought about this and maybe it can be spike/refactor. Since groups aren't being created users shouldn't experience this for now. But agree it almost feels like we need to return a type of either Conversation or Group instead of making Groups conform to conversations but it would be a little extra work. My hope is when we get rid of V2 this won't be a problem because most all the same functions should exist for both groups and 1to1 in that world.

@nplasterer nplasterer merged commit e6876ca into main Feb 1, 2024
5 of 6 checks passed
@nplasterer nplasterer deleted the np/group-conversations branch February 1, 2024 05:50
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