-
Notifications
You must be signed in to change notification settings - Fork 24
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
Create validation service #278
Conversation
@@ -13,11 +13,16 @@ plugins: | |||
- plugin: buf.build/community/neoeinstein-tonic | |||
out: src/gen | |||
opt: | |||
- no_server=true | |||
- no_server=false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We now need to generate servers everywhere. We may want to add a second feature to the server attributes so that we can toggle them off as needed.
pub fn new( | ||
api_client: ApiClient, | ||
network: Network, | ||
identity: Identity, | ||
store: EncryptedMessageStore, | ||
) -> Self { | ||
Self { | ||
api_client, | ||
api_client: ApiClientWrapper::new(api_client), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wrap the ApiClient
into a prettier interface that abstracts away the Protobuf nonsense. Open to better names for it than ApiClientWrapper
.
@@ -5,7 +5,7 @@ if ! cargo install --list | grep "protoc-gen-prost-crate" > /dev/null; then | |||
exit 1 | |||
fi | |||
fi | |||
if ! buf generate https://github.com/xmtp/proto.git#branch=xmtpv3,subdir=proto; then | |||
if ! buf generate https://github.com/xmtp/proto.git#branch=nmolnar/mls-verification-service,subdir=proto; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will switch this back to main
once we merge xmtp/proto#107
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Summarizing what I understood:
-
The validation service is a rust server that is consumed by the golang gRPC server, NOT the libxmtp client. It is included in libxmtp for now because it shares the Rust validation logic used by libxmtp. In future when libxmtp is a published crate the validation server could be moved to a separate repo.
-
Separately this PR adds a client for the golang gRPC server entry point that libxmtp uses.
FYI @snormore
match validate_group_message(message.group_message_bytes_tls_serialized) { | ||
Ok(res) => ValidateGroupMessageValidationResponse { | ||
group_id: res.group_id, | ||
epoch: res.epoch, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would be used for proposals/commits too right? If so, is this the epoch going forward, or the epoch the message was expected to be sent on - don't we need both?
And is the golang server going to validate that the current epoch matches the expected epoch outside of this one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will get used for any group message (application, proposal, commit).
We don't do any validation on the epoch in the node. I probably should just remove the field from the response since the node ignores it. The field predates our convo with @Bren2010 around relying on total ordering rather than epoch validation.
@@ -57,8 +63,18 @@ pub enum InnerApiClient { | |||
), | |||
} | |||
|
|||
pub enum InnerMlsClient { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Confirming I understood here - the MlsClient is NOT the validation service, it is a client for the golang gRPC server?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's right.
xmtp_mls
talks to xmtp-node-go
which talks to mls_validation_service
client: InnerApiClient::Tls(tls_client), | ||
mls_client: InnerMlsClient::Tls(mls_client), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any consumer that would need both of these clients? Wondering if we should just return one or the other based on a flag? Or do we rely on endpoints from both of these clients?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
xmtp_mls
is going to need both. My intention is to rely on all of our existing Query
, BatchQuery
and Stream
methods for reading messages from topics, since we don't need to do anything special on the read side.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, in that case why not merge them, seeing as v2 is on a separate branch?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They are implemented as separate GRPC services on the node with separate protobuf services, so they need separate underlying clients (although the clients can share a single connection).
Summary
xmtp_mls