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

Introduce group chat to xmtp-ios #229

Merged
merged 43 commits into from
Feb 8, 2024
Merged

Introduce group chat to xmtp-ios #229

merged 43 commits into from
Feb 8, 2024

Conversation

nakajima
Copy link
Contributor

@nakajima nakajima commented Feb 1, 2024

Begins to introduce group chats to xmtp-ios. Right now, chats are loaded by calling client.conversations.groups(). The example app has been updated to call this and combine groups with conversations under the ConversationOrGroup enum. Once this PR lands, we'll embark on a broader change where Conversation is renamed to DirectMessage which will free up the Conversation name to be a union of DirectMessage and Group.

TODO:

  • The most recent version of libxmtp-swift bindings didn't build correctly for me so I switched back to building a dylib in my local libxmtp for now. It was missing the LegacyIdentitySource stuff needed for client creation. We'll need to work that out before merging. done
  • Message/Group Pagination done
  • Handle GroupMemberChanged messages in the example app done
  • Group Streaming (could happen in a follow up)

Here's some screenshots from the example app:

Creating a group Viewing a group Messages Group settings
Screenshot 2024-02-01 at 3 06 57 PM Screenshot 2024-02-01 at 3 06 57 PM Simulator Screenshot - iPhone 15 Plus - 2024-02-06 at 13 12 08 image

Sources/XMTPiOS/CodecRegistry.swift Outdated Show resolved Hide resolved
Sources/XMTPiOS/Conversations.swift Outdated Show resolved Hide resolved
Sources/XMTPiOS/Conversations.swift Show resolved Hide resolved
Sources/XMTPiOS/Group.swift Outdated Show resolved Hide resolved
Sources/XMTPiOS/Group.swift Outdated Show resolved Hide resolved
Sources/XMTPiOS/Group.swift Outdated Show resolved Hide resolved
Sources/XMTPiOS/XMTPLogger.swift Outdated Show resolved Hide resolved
Sources/XMTPiOS/Client.swift Outdated Show resolved Hide resolved
Sources/XMTPiOS/Client.swift Outdated Show resolved Hide resolved
Sources/XMTPiOS/Client.swift Outdated Show resolved Hide resolved
@cameronvoell
Copy link
Contributor

cameronvoell commented Feb 1, 2024

The most recent version of libxmtp-swift bindings didn't build correctly for me so I switched back to building a dylib in my local libxmtp for now. It was missing the LegacyIdentitySource stuff needed for client creation. We'll need to work that out before merging.

@nakajima If you let me know what commit in libxmtp you would like bindings from, I should be able to create static lib bindings that will work for you in Package.swift. Let me know and I can add it as a PR on to this one.

@nakajima
Copy link
Contributor Author

nakajima commented Feb 2, 2024

@cameronvoell I think whatever is on main should have everything I need? So xmtp/libxmtp@60ee755 should do it.

@nakajima nakajima marked this pull request as ready for review February 2, 2024 00:57
@nakajima nakajima requested a review from a team as a code owner February 2, 2024 00:57
@nakajima
Copy link
Contributor Author

nakajima commented Feb 2, 2024

@cameronvoell Things seem to work pointing at xmtp/libxmtp-swift#6. Thanks for the libxmtp fix!

Package.swift Outdated Show resolved Hide resolved
nakajima and others added 2 commits February 1, 2024 21:23
@cameronvoell
Copy link
Contributor

Tested on RN via PR here https://github.com/xmtp/xmtp-react-native/pull/234/files. Standard V2 features all look good. Looking forward to integrating ios groups on RN, nice work!

Copy link
Contributor

@nplasterer nplasterer left a comment

Choose a reason for hiding this comment

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

Might be nice to pull out the client creation from the rest of the group stuff so that it's easier to review. I just took another pass on that but not the rest of the code.

Sources/XMTPiOS/Client.swift Outdated Show resolved Hide resolved
Sources/XMTPiOS/Client.swift Outdated Show resolved Hide resolved
Sources/XMTPiOS/Client.swift Outdated Show resolved Hide resolved
Sources/XMTPiOS/Client.swift Outdated Show resolved Hide resolved
Base automatically changed from v3-client to main February 5, 2024 22:07
@nakajima nakajima mentioned this pull request Feb 6, 2024
Copy link
Contributor

@nplasterer nplasterer left a comment

Choose a reason for hiding this comment

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

Looks like this PR is missing the call to list both groups and conversations in the same call. https://github.com/xmtp/xmtp-android/pull/160/files#diff-479fe53ececf1836cd910bfba9a721b20fce9fd212b204329abef95ff79429f9R220-R255
But probably okay to do that in a follow up.
Don't forget to bump the pod spec.

Sources/XMTPTestHelpers/TestHelpers.swift Outdated Show resolved Hide resolved
Sources/XMTPiOS/Conversations.swift Show resolved Hide resolved
Comment on lines +64 to +73
let erroredAddresses = try await withThrowingTaskGroup(of: (String?).self) { group in
for address in addresses {
group.addTask {
if try await self.client.canMessageV3(address: address) {
return nil
} else {
return address
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I like how you handled this. I can improve this on android for sure.

Sources/XMTPiOS/Group.swift Outdated Show resolved Hide resolved
Sources/XMTPiOS/Group.swift Outdated Show resolved Hide resolved
XMTP.podspec Outdated Show resolved Hide resolved
@nakajima
Copy link
Contributor Author

nakajima commented Feb 8, 2024

Don't forget to bump the pod spec.

Do we think this should be a 0.9.0-beta?

Nevermind, I think we should wait until the Conversation -> DirectMessage rename.

* ui updates, fix lint

* Add streaming

* use convo id

* bump podspec
@nakajima nakajima merged commit 5b40ee9 into main Feb 8, 2024
1 check passed
@nakajima nakajima deleted the v3-groups branch February 8, 2024 17:13
@cameronvoell cameronvoell restored the v3-groups branch February 9, 2024 02:42
kele-leanes pushed a commit that referenced this pull request Feb 19, 2024
* Introduce group chat to xmtp-ios

* fix lint, add address to example app for groups

* paginate

* Handle membership changes in example app

* Make syncing explicit

* Pass legacySignedPrivateKeyProto

* add more validations

* Add ClientOptions.enableAlphaMLS

* Make group changes codec opt-in

* Point to libxmtp-swift package, not local filesystem

* Update Package.swift

Co-authored-by: Cameron Voell <[email protected]>

* bump podspec

* include env in db url

* extract method

* return members as strings not objects

* Error when trying to enable alpha MLS with no signer and no keys

* pass encryption key

* Move client test to clientests

* Pull v3 client into its own PR

* Update GroupMembershipChanged.swift

* Add tests

* rename mls alpha

* fix codec

* cleanup

* Fix example app (needed to add manual syncs)

* Update GroupMembershipChanged.swift

* fix nav

* cleanup

* add group settings view

* rename members to member addresses

* uncomment local node skip

* improve group error

* use group id for topic

* fix lint

* allow mls on dev, other cleanup

* V3 group streaming (#239)

* ui updates, fix lint

* Add streaming

* use convo id

* bump podspec

* bump podspec

---------

Co-authored-by: Cameron Voell <[email protected]>
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.

5 participants