V3 only dms #307
V3 only dms #307
Conversation
…/xmtp/xmtp-android into np/smart-contract-wallet-support
nplasterer
changed the base branch from
main
to
np/smart-contract-wallet-support
| @@ -615,6 +617,33 @@ class Client() { | |||
| } | |||
| } | |||
|
|
|||
| fun findConversation(conversationId: String): Conversation? { | |||
| val client = v3Client ?: throw XMTPException("Error no V3 client initialized") | |||
There was a problem hiding this comment.
Just ramping up here, in this version of the SDK are we always automatically creating a v3 client, or do we require implementers to enable it? Is the expectation that this method would only be called if a v3 client exists, otherwise findGroup() would be used?
There was a problem hiding this comment.
Correct in the follow up PR where I implement dual sending and clients that potentially have V2 clients associated I'll add in the deprecation warnings for the findGroups method.
| is V1 -> throw XMTPException("Only supported for V3") | ||
| is V2 -> throw XMTPException("Only supported for V3") | ||
| is Group -> group.name | ||
| is Dm -> dm.name |
There was a problem hiding this comment.
Are we planning to support changeable names for DM's? Same with imageURL/description (in general any group metadata)
There was a problem hiding this comment.
This is a great question. I just kept it because we didn't disable it in the backend but I don't have to expose it if people feel strongly about it.
| is V1 -> throw XMTPException("Only supported for V3") | ||
| is V2 -> throw XMTPException("Only supported for V3") | ||
| is Group -> group.id | ||
| is Dm -> dm.id |
There was a problem hiding this comment.
Do implementers need to switch between querying conversations by ID and by topic when they migrate from v2 to v3?
There was a problem hiding this comment.
There are some places where querying by topic is still necessary. For example push notifications still expose only the topic. Since the topic just has the id inside it I have a method that parses the topic into the id to then search for it. In V2 mostly people were looking up conversations by address not topic.
| is V1 -> throw XMTPException("Only supported for V3") | ||
| is V2 -> throw XMTPException("Only supported for V3") | ||
| is Group -> group.updateConsentState(state) | ||
| is Dm -> dm.updateConsentState(state) |
There was a problem hiding this comment.
Just ramping up here, how is consent state updated in V2?
There was a problem hiding this comment.
It's implemented on the client in a method on contacts. When it was originally written it was just consenting on addresses so that made sense. We could probably update the state here for V1 and V2 but feels unnecessary as we don't want people building new V2 functionality and instead moving to V3. https://github.com/xmtp/xmtp-android/blob/main/library/src/main/java/org/xmtp/android/library/Contacts.kt
| } | ||
| } | ||
|
|
||
| suspend fun send(prepared: PreparedMessage): String { | ||
| suspend fun <T> prepareMessageV3(content: T, options: SendOptions? = null): String { |
There was a problem hiding this comment.
Do implementers need to call different code when preparing messages when they migrate from v2 to v3? Just curious why we couldn't abstract it?
There was a problem hiding this comment.
Ya it's a bit unfortunate. I tried several different hacks to try and make it work for both V3 and V2 but since V2 depends so heavily on the envelope it would be a break change. I'm cool with not exposing this in the interface and just cutting the old one over eventually. Integrators will likely call these methods directly on the group or dm anyways.
| fun decryptV3( | ||
| message: MessageV3, | ||
| ): DecryptedMessage { | ||
| return when (this) { | ||
| is V1 -> conversationV1.decrypt(envelope) | ||
| is V2 -> conversationV2.decrypt(envelope) | ||
| is Group -> { | ||
| message?.decrypt() ?: throw XMTPException("Groups require message be passed") | ||
| } | ||
| is V1 -> throw XMTPException("Only supported for V3") | ||
| is V2 -> throw XMTPException("Only supported for V3") | ||
| is Group -> message.decrypt() | ||
| is Dm -> message.decrypt() | ||
| } | ||
| } | ||
|
|
||
| fun decodeV3(message: MessageV3): DecodedMessage { | ||
| return when (this) { | ||
| is V1 -> throw XMTPException("Only supported for V3") | ||
| is V2 -> throw XMTPException("Only supported for V3") | ||
| is Group -> message.decode() | ||
| is Dm -> message.decode() |
There was a problem hiding this comment.
Is there any reason we can't merge into the same decrypt() and decode() methods, so implementers don't need to change their code when moving from v2 to v3?
There was a problem hiding this comment.
Ya that would be ideal. Unfortunately the decrypt method in V2 takes an envelope and the decode takes a V3 message. I could make it take a envelope or a message which I think is how it worked before. But it just creates extra null checking for things that will be null half the time in either case. I'm open though if you think it's valuable.
| suspend fun findOrCreateDm(peerAddress: String): Dm { | ||
| if (client.hasV2Client) throw XMTPException("Only supported for V3 only clients.") | ||
| if (peerAddress.lowercase() == client.address.lowercase()) { | ||
| throw XMTPException("Recipient is sender") | ||
| } | ||
| val contacts = client.contacts | ||
| contacts.refreshConsentList() | ||
| if (contacts.consentList.state(peerAddress) == ConsentState.UNKNOWN) { | ||
| contacts.allow(listOf(peerAddress)) | ||
| val falseAddresses = | ||
| client.canMessageV3(listOf(peerAddress)).filter { !it.value }.map { it.key } | ||
| if (falseAddresses.isNotEmpty()) { | ||
| throw XMTPException("${falseAddresses.joinToString()} not on network") | ||
| } | ||
| var dm = client.findDm(peerAddress) | ||
| if (dm == null) { | ||
| val dmConversation = libXMTPConversations?.createDm(peerAddress.lowercase()) | ||
| ?: throw XMTPException("Client does not support V3 Dms") | ||
| dm = Dm(client, dmConversation) | ||
| client.contacts.allowGroups(groupIds = listOf(dm.id)) |
There was a problem hiding this comment.
I'm wondering if all of this should happen inside libxmtp? Then we can enforce the same logic across all SDK's, we have fewer calls across the FFI boundaries, and we can perhaps optimize things within libxmtp
There was a problem hiding this comment.
Thats a great idea!
| val signature = consentProof.signature | ||
| val timestamp = consentProof.timestamp | ||
|
|
||
| if (!KeyUtil.validateConsentSignature(signature, client.address, peerAddress, timestamp)) { |
There was a problem hiding this comment.
Do we need to log an error/warning, and possibly return it here?
There was a problem hiding this comment.
I don't think this code changed at all in this PR. Something to visit whenever we touch consent proofs for V3
| ConversationOrder.LAST_MESSAGE -> { | ||
| conversations.map { conversation -> | ||
| val message = | ||
| conversation.findMessages(FfiListMessagesOptions(null, null, null, null)) |
There was a problem hiding this comment.
I want to limit and order these descending
This is the support for just the v3 only dms and no dual sending. This is how someone would get a full working v3 only client.
Puts checks in place to catch if a V2 client is trying to access V3 dms.
Tests to make sure V3 dms are not being exposed anywhere when a V3 only client is not present.