Introduce gRPC replication client api #1062
Conversation
Your org has enabled the Graphite merge queue for merging into mainAdd the label “queue” to the PR and Graphite will automatically add it to the merge queue when it’s ready to merge. You must have a Graphite account and log in to Graphite in order to use the merge queue. Sign up using this link. |
mkysel
force-pushed
the
mkysel/replication-grpc
branch
from
cb9771b
to
9d486ca
Compare
|
@insipx @richardhuaaa could I get your eyes on this? It does not do everything it should yet, so I will keep it in draft status. But I would appreciate any rust feedback I can get! |
| @@ -335,12 +351,22 @@ impl MutableApiSubscription for GrpcMutableSubscription { | |||
| impl XmtpMlsClient for Client { | |||
| #[tracing::instrument(level = "trace", skip_all)] | |||
| async fn upload_key_package(&self, req: UploadKeyPackageRequest) -> Result<(), Error> { | |||
| let client = &mut self.mls_client.clone(); | |||
| if self.use_replication_v4 { | |||
There was a problem hiding this comment.
This is OK, we could also do something like this:
1.) define a feature in api_grpc, replication
2.) Create a new impl XmtpMlsClient definition exactly like the non-v4 one but for v4 (no flag on the struct)
3.) use #[cfg(feature = "replication")] and #[cfg(not(feature = "replication"))] on the impl XmtpMlsClient for Client
This would feature-gate replication, and result in easy access to the client through a flag during testing/compiling, i.e cargo test --feature replication. Mixing replication and v3 backend also isn't terribly useful in itself, so I would argue for using compile-time features here (i'm also just a fan of compile time features)
It also makes the eventual migration just a little bit easier, too
There was a problem hiding this comment.
I was considering that option as well. I decided against it, because I want the ability to point the client (or any SDK implementation) against any backend using run-time configuration. Otherwise you have to compile different binaries or recompile every time you switch the backend. This makes shipping the binaries a bit annoying.
Maybe a good idea would be to hide the new implementation behind a feature gate. Basically hide the new experimental code behind a feature thats off by default. And allow developers to compile it in, if they so desire.
We might have to mix v3 and v4 in some cases unfortunately.
There was a problem hiding this comment.
In that case, I'm wondering if it would be cleaner to define the replication client as a separate DecentralizedClient or V4Client, rather than mixing it into the existing Client implementation? If we want to borrow any of the old logic for now, we could just copy paste it.
The benefit of the XmtpMlsClient and XmtpIdentityClient traits are that you can have multiple implementations and easily swap between them. Then the caller can just instantiate the correct client as desired, rather than passing a boolean flag into one big combined client.
As an example of a benefit - it's conceivable that the decentralized client will need additional member variables and helper methods going forward that are not needed in the old client, for example storing the list of nodes and selecting the node to communicate with.
There was a problem hiding this comment.
Looks good so far, just left a bunch of nits!
Depending on how far you want to go, we could also consider modifying the setup for existing unit tests to test both API clients, as a shortcut for writing your own tests. Not super critical at this stage either way though
| #[allow(async_fn_in_trait)] | ||
| #[cfg_attr(not(target_arch = "wasm32"), trait_variant::make(XmtpReplicationClient: Send))] | ||
| #[cfg_attr(target_arch = "wasm32", trait_variant::make(XmtpReplicationClient: Wasm))] | ||
| pub trait LocalXmtpReplicationClient { |
There was a problem hiding this comment.
I'm guessing we don't need this trait anymore
xmtp_proto/src/convert.rs
Outdated
| @@ -16,3 +23,51 @@ mod inbox_id { | |||
| } | |||
| } | |||
| } | |||
|
|
|||
| impl From<SendWelcomeMessagesRequest> for ClientEnvelope { | |||
There was a problem hiding this comment.
Totally fine for now, but I think at some point we might need to move away from these traits, seeing as the aad will need to be specified in libxmtp rather than derived inside this trait
| @@ -335,12 +351,22 @@ impl MutableApiSubscription for GrpcMutableSubscription { | |||
| impl XmtpMlsClient for Client { | |||
| #[tracing::instrument(level = "trace", skip_all)] | |||
| async fn upload_key_package(&self, req: UploadKeyPackageRequest) -> Result<(), Error> { | |||
| let client = &mut self.mls_client.clone(); | |||
| if self.use_replication_v4 { | |||
There was a problem hiding this comment.
In that case, I'm wondering if it would be cleaner to define the replication client as a separate DecentralizedClient or V4Client, rather than mixing it into the existing Client implementation? If we want to borrow any of the old logic for now, we could just copy paste it.
The benefit of the XmtpMlsClient and XmtpIdentityClient traits are that you can have multiple implementations and easily swap between them. Then the caller can just instantiate the correct client as desired, rather than passing a boolean flag into one big combined client.
As an example of a benefit - it's conceivable that the decentralized client will need additional member variables and helper methods going forward that are not needed in the old client, for example storing the list of nodes and selecting the node to communicate with.
| } | ||
| } | ||
|
|
||
| impl XmtpReplicationClient for Client { |
There was a problem hiding this comment.
Do we need this impl? I'm guessing libxmtp doesn't need to call any of these methods directly?
| use prost::Message; | ||
| use xmtp_proto::xmtp::xmtpv4::{ClientEnvelope, PayerEnvelope, PublishEnvelopeRequest}; | ||
|
|
||
| pub fn wrap_client_envelope(req: ClientEnvelope) -> PublishEnvelopeRequest { |
There was a problem hiding this comment.
Nit: create_payer_envelope?
No description provided.