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

Teach example/CLI how to talk to V4/Replication Node #1214

Merged
merged 28 commits into from
Nov 20, 2024
Merged

Conversation

mkysel
Copy link
Contributor

@mkysel mkysel commented Nov 4, 2024

This is the first (large) step in the right direction. By no means is the migration to decentralization complete.

This teaches the CLI to do basic operations via xtmpd such as:

  • registering
  • creating groups
  • adding members to groups
  • messaging

One has to simply provide the --testnet flag to the CLI and everything will work against the new API.

The easiest way to spin up the new environment is to wait for #1281.

This refactors the Error class into its own file and adds an Internal error.

Also adds a new command to the CLI for debugging.

xmtp_proto/src/convert.rs Outdated Show resolved Hide resolved
@mkysel mkysel force-pushed the mkysel/cli-register branch from ebde379 to 5ffd6c7 Compare November 7, 2024 18:56
xmtp_proto/src/convert.rs Outdated Show resolved Hide resolved
xmtp_proto/src/convert.rs Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
@mkysel mkysel changed the title Implement register function Teach example/CLI how to talk to V4/Replication Node Nov 18, 2024
@mkysel mkysel marked this pull request as ready for review November 18, 2024 18:30
@mkysel mkysel requested a review from a team as a code owner November 18, 2024 18:30
@mkysel mkysel requested a review from neekolas November 19, 2024 13:44
Copy link
Contributor

@richardhuaaa richardhuaaa left a comment

Choose a reason for hiding this comment

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

Well done! The structure of the helper methods is very clean.

We'll also need to add validation of the payloads coming from the server eventually (e.g. signed correctly and from the expected originator), but we can document this properly in the XIP first.

query: Some(EnvelopesQuery {
topics: vec![build_group_message_topic(req.group_id.as_slice())],
originator_node_ids: vec![],
last_seen: None,
Copy link
Contributor

Choose a reason for hiding this comment

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

As a note - the PagingInfo of the QueryGroupMessagesRequest has an id_cursor that we could potentially convert into vector clock format here as a short-term hack. Unsure if we get subtle bugs without this, as the client expects to query from where it left off in the queue. In the long-term we need to do a bigger refactor to properly support vector clocks though, just making a note in case you run into bugs in the meantime

originator_node_ids: vec![],
last_seen: None,
}),
limit: 100,
Copy link
Contributor

Choose a reason for hiding this comment

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

There is a limit on the PagingInfo we should honor here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

where do I find it?

xmtp_api_grpc/src/replication_client.rs Outdated Show resolved Hide resolved

GroupMessage {
version: Some(group_message::Version::V1(group_message::V1 {
id: unsigned_originator_envelope.originator_sequence_id,
Copy link
Contributor

Choose a reason for hiding this comment

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

Just making a note, eventually we'll need to refactor the client to store both the originator node id and sequence ID in order to set the vector clock properly, but this is great for now

originator_node_ids: vec![],
last_seen: None,
}),
limit: 100,
Copy link
Contributor

Choose a reason for hiding this comment

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

To avoid bugs, I think we should take in the last_seen and limit from the caller of this method, and honor whatever semantics were previously used

xmtp_mls/src/hpke.rs Outdated Show resolved Hide resolved
@@ -0,0 +1,138 @@
use openmls::prelude::tls_codec::Error as TlsCodecError;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this error.rs file generated? Maybe I missed something, why do we need a new error type different from before?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just moved it to its own file as a drive-by refactor.

I added the InternalError and all of its subtypes to allow for error conversions.

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.

3 participants