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

Refactor message history sync, add to CLI #930

Merged
merged 65 commits into from
Aug 28, 2024

Conversation

tuddman
Copy link
Contributor

@tuddman tuddman commented Jul 31, 2024

Summary

Refactored message history sync

  • Exposed message_history module
  • Exported official message history sync URLs
  • Changed storage of history message content from delimited strings to serialized structs
  • Removed use of bundle_hash and sign_key when managing history bundles
  • Removed automatic reply and bundle processing during sync
  • Always use the client's history_sync_url for syncing
  • Added more message history errors
  • Updated all message history functions to use MessageHistoryError when appropriate
  • Added get_sync_group function to get sync group and its ID
  • Refactored allow_history_sync to use existing sync group or create a new one
  • Refactored send_history_request
    • Added guard to only send a history request if there isn't one already pending
    • Updated return value to include both request_id and pin_code
  • Refactored send_history_reply
    • Added guard to only send a history reply when there's a pending request
  • Added get_pending_history_request function that returns the request_id and pin_code of a pending history request
  • Added reply_to_history_request function that replies to a pending history request
  • Added get_latest_history_reply function that returns a history reply if it's the last message in a sync group
  • Added process_history_reply function that downloads a history bundle using the latest history reply message

Notes

  • The guards added in send_history_request and send_history_reply are currently very basic and don't account for spamming (no time based checks). Since all installations use the same sync group, the sync group will always contain a request followed by a reply. There cannot be multiple requests or replies in a row.
  • The verify_pin function is not currently being used. I'm not sure what the use case is.

Future enhancements

  • Allow for history bundles with constraints (e.g. startTime) to limit size and processing time

TODO

  • Add tests for get_pending_history_request
  • Add tests for get_latest_history_reply
  • Add tests for reply_to_history_request

Enable message history sync-ing from the examples/cli mini-app.

Note: This will only work if you provide the same --seed-phrase when creating both client instances:

## tab 1
./xli.sh --db user1_a.db3 register --seed-phrase "celery home axis version spike fiction vehicle actual hair absurd diesel body"

## tab 2
./xli.sh --db user1_b.db3 register --seed-phrase "celery home axis version spike fiction vehicle actual hair absurd diesel body"

Request history sync

## in tab 1, for consistency
./xli.sh --db user1_a.db3 request-history-sync

Send history sync reply

## in tab 2
./xli.sh --db user1_b.db3 reply-to-history-sync-request

Process history sync reply

This will download and process a history bundle.

## in tab 1, for consistency
./xli.sh --db user1_a.db3 process-history-sync-reply

List history sync messages

This will display all messages in the history sync group.

./xli.sh --db user1_a.db3 list-history-sync-messages

Inspect

(after request-sync-history has been sent...)

## load sqlite3
sqlite3 user1_b.db3
> select hex(id) from groups;
B940E26CBD7FB35BD9041D06B46D1F0E

## in another tab
sqlite3 user1_a.db3
> select hex(id) from groups;
B940E26CBD7FB35BD9041D06B46D1F0E  <- user1_a should be joined into the sync group

Closes #877

@tuddman
Copy link
Contributor Author

tuddman commented Aug 2, 2024

@neekolas @richardhuaaa @insipx

There's a db lock issue when the responder is attempting to gather the groups + messages (from the local db) to send to the requester:

xmtp_mls::groups::subscriptions retrying function that failed with error=receive error: generic:could not send history reply: storage error: Diesel result error: database is locked

I'm confident any one of you can solve it in short order.

There's also the matter of standardizing on the message-history "api contract" i.e. make the endpoints strictly REST-ful to send to e.g. POST /bundles and for the other side to fetch the bundle from e.g. GET /bundles/<uuid>

Fix those and this whole feature-set should just work™

Best of luck

@rygine rygine force-pushed the st/877-end-to-end-app-test-of-message-history branch from a65da36 to ddd5848 Compare August 16, 2024 19:26
@rygine rygine force-pushed the st/877-end-to-end-app-test-of-message-history branch from 4519c26 to c492b99 Compare August 20, 2024 20:41
@rygine rygine force-pushed the st/877-end-to-end-app-test-of-message-history branch from 14c3e83 to 49edac1 Compare August 21, 2024 14:17
) -> Result<MessageHistoryReply, MessageHistoryError> {
let pending_request = self.get_pending_history_request().await?;

if let Some((request_id, _)) = pending_request {
Copy link
Contributor

Choose a reason for hiding this comment

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

How do i know which request I'm replying to? What happens if a recent request just snuck in

Copy link
Collaborator

Choose a reason for hiding this comment

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

this will reply the latest request message in the sync group, which is sync'd prior to checking. does that make sense?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that's fine for now. Just want to walk through a scenario.

  1. AppB receives a message history request from AppA and shows it to the user. They look at it and click OK.
  2. App receives a second message history request from App C
  3. libxmtp replies to the request from App C because it's the newest request at the time of reply

We can solve that issue later, but it's something we should write a ticket for

None,
)?;

let last_message = match messages.last() {
Copy link
Contributor

@insipx insipx Aug 27, 2024

Choose a reason for hiding this comment

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

Nesting match statements can get messy & make it difficult to intuit what's happening

We can compose the Result<> and Option<> in order to retain one level of indentation in the match

But, we also need the result of the deserialization from_slice operation. so we can make use of ok on the result and flatten on the what is now Option<Option> to get just one Option<Result>

let last_message = messages.last().map(|m| serde_json::from_slice(msg.decrypted_message_bytes).ok()).flatten();
match last_message {
    Some(Ok(MessageHistoryContent::Request(request))) => {
        /* ... */
    }
    Some(Ok(MessageHistoryContent::Reply(_))) => {
        /* */
    },
    None => return Err(MessageHistoryError::NoPendingRequest)
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

after implementing your suggestion, i realized that i need access to the value of msg inside of the match. any tips on how the accomplish that without a nested match?

@rygine rygine force-pushed the st/877-end-to-end-app-test-of-message-history branch from a176bb6 to eb8d370 Compare August 28, 2024 17:53
@rygine rygine merged commit 6acf381 into main Aug 28, 2024
9 checks passed
@rygine rygine deleted the st/877-end-to-end-app-test-of-message-history branch August 28, 2024 22:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

End to End App test of Message History
5 participants