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

feat: Build grpc API with swift-bridge and package with Swift #50

Merged
merged 31 commits into from
Apr 17, 2023

Conversation

michaelx11
Copy link
Contributor

@michaelx11 michaelx11 commented Apr 14, 2023

Overview

Top-level issue link

This PR goes hand-in-hand with an update to our Swift package repo to hide all of this nonsense behind a single API defined in ApiService.swift in that repo.

Why is this PR so big?

Things to ignore: 1) generated Rust proto code 2) generated Rust-Swift binding code 3) I also deleted corecrypto from the crate cause we didn't need it

What changed in the Rust<>Swift flow?

xmtp-ios should just be consuming a run-of-the-mill Swift package with a super simple Swift-only API.

How we did this was:

  1. Use swift-bridge to generate nice C headers and Swift files from our code
  2. Push the C headers into our include directory to get bundled into the XMTPRustSwift.xcframework
  3. Behind the scenes, push the Swift code into xmtp-rust-swift after adding an "import XMTPRustSwift" line to each file
  4. Finally within xmtp-rust-swift (our Swift package), we use the XMTPRustSwift.xcframework, wrap it with ApiService and deliver a nice clean Swift package interface

Right now the shim strategy is to convert protos to JSON and throw them across the Rust<>Swift boundary. We can optimize this as necessary but number one priority is to get it working.

Next Steps

Need to implement "subscribe" and make sure our Rust gRPC can target TLS.

Test Plan

Bring up a local xmtp-go-node with gRPC port mapped to 15555 (or change port in the test) and run unit tests.

@neekolas
Copy link
Contributor

Copying all our protos into this package seems less than ideal. Maybe a project for me next week is to avoid the need for that

@michaelx11
Copy link
Contributor Author

michaelx11 commented Apr 15, 2023

Copying all our protos into this package seems less than ideal. Maybe a project for me next week is to avoid the need for that

Oh yeah I should make them a submodule now actually. Let me fix that. Edit: actually I have googleapis as a submodule in the protos directory :/. I'll probably just have a pre-build step that clones it all down.

@neekolas
Copy link
Contributor

neekolas commented Apr 15, 2023

I'm hoping to get around the nightmare that is our proto repo build process by using Buf Schema Registry. Just pushed up the first package.

Submodule is fine for now, but I'm happy to take on buf-ifying this thing on Monday as a test before I buf-ify the mobile SDKs.

@michaelx11
Copy link
Contributor Author

@neekolas this commit addresses the base64 issue in a very targeted and brittle way. As long as we only serialize the MessageApi protos to JSON this should be fine, but gotta keep an eye on this for sure.

263a4d2

tokio-rs/prost#75

@neekolas
Copy link
Contributor

@neekolas this commit addresses the base64 issue in a very targeted and brittle way. As long as we only serialize the MessageApi protos to JSON this should be fine, but gotta keep an eye on this for sure.

263a4d2

tokio-rs/prost#75

For a prototype, that all seems fine. I suspect we're going to want to pass these back and forth as structs to optimize perf at some point.

Michael Xu and others added 2 commits April 14, 2023 21:10
* Initial commit

* Include lib

* Add features

* Correct import

* Add feature

* Add pbjson

* Almost passing

* Add build.rs

* Add .gitignore

* Rename folder

* Remove generated code

* Gitignore generated

* Automatically install required dependency
@michaelx11
Copy link
Contributor Author

Gonna add one quick fix to bindings/xmtp_rust_swift, since the bindings crate is in the Cargo.toml all artifacts go into a top-level target directory. I'll change the target-dir in the Makefile.

topic: String,
paging_info: Option<v1::PagingInfo>,
) -> Result<v1::QueryResponse, tonic::Status> {
let mut client = v1::message_api_client::MessageApiClient::connect(host)
Copy link
Contributor

Choose a reason for hiding this comment

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

At some point we are going to want to optimize this to re-use a long-lived GRPC connection, given how nice and performant it is that GRPC can multiplex a single HTTP/2 connection.

But that's a problem for another day.

.gitmodules Outdated Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
@michaelx11
Copy link
Contributor Author

FYI: I removed bindings/xmtp_rust_crate from the Cargo workspace so its release profile in its own Cargo.toml would be respected. This was why we were getting: warning: profiles for the non root package will be ignored, specify profiles at the workspace root: warnings

@michaelx11 michaelx11 merged commit f8a55e6 into main Apr 17, 2023
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.

2 participants