Consent Sync #1152
Consent Sync #1152
Conversation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (7)
xmtp_mls/src/groups/device_sync/consent_sync.rs (3)
15-31: Add documentation for the consent sync reply flow.The method implements a critical part of the consent synchronization process but lacks documentation explaining the flow and design decisions.
+/// Replies to a consent sync request by sending local consent records. +/// +/// This method: +/// 1. Retrieves the pending sync request +/// 2. Collects local consent records +/// 3. Sends the records as a sync reply +/// +/// Note: PIN code verification is handled at a lower level in the sync process. +/// +/// # Returns +/// * `Ok(DeviceSyncReplyProto)` - The sync reply containing consent records +/// * `Err(DeviceSyncError)` - If no pending request exists or sync fails pub async fn reply_to_consent_sync_request( &self, ) -> Result<DeviceSyncReplyProto, DeviceSyncError> {
50-51: Use more descriptive constant names.The constants could be more descriptive to better indicate their purpose.
- const HISTORY_SERVER_HOST: &str = "0.0.0.0"; - const HISTORY_SERVER_PORT: u16 = 5558; + const MOCK_SYNC_SERVER_HOST: &str = "0.0.0.0"; + const MOCK_SYNC_SERVER_BASE_PORT: u16 = 5558;
63-157: Add comprehensive test coverage for error scenarios.The current test only covers the happy path. Consider adding tests for error scenarios to ensure robust error handling.
Add the following test cases:
#[tokio::test(flavor = "multi_thread", worker_threads = 1)] async fn test_consent_sync_no_pending_request() { let wallet = generate_local_wallet(); let client = ClientBuilder::new_test_client(&wallet).await; // Attempt to reply without a pending request let result = client.reply_to_consent_sync_request().await; assert!(matches!(result, Err(DeviceSyncError::NoPendingRequest))); } #[tokio::test(flavor = "multi_thread", worker_threads = 1)] async fn test_process_consent_sync_reply_no_reply() { let wallet = generate_local_wallet(); let client = ClientBuilder::new_test_client(&wallet).await; // Attempt to process reply without sending a request let result = client.process_consent_sync_reply().await; assert!(matches!(result, Err(DeviceSyncError::NoReplyToProcess))); }Also, mark the main test as ignored and document why:
+ // This test is ignored because it requires a running mock server + // and specific port availability #[tokio::test(flavor = "multi_thread", worker_threads = 1)] + #[ignore] async fn test_consent_sync() {xmtp_mls/src/groups/device_sync/message_sync.rs (3)
129-133: Add test assertions for message content.The test verifies the number of messages but doesn't verify the actual content of the messages. Consider adding assertions to ensure the message content is correctly synchronized.
Add assertions to verify message content:
let syncable_messages = amal_a.syncable_messages().unwrap(); assert_eq!(syncable_messages.len(), 2); // welcome message, and message that was just sent +// Verify message content +if let Syncable::GroupMessage(msg) = &syncable_messages[1] { + assert_eq!(msg.content(), &[1, 2, 3]); +}
13-16: Add documentation for the return values.The comment "returns (request_id, pin_code)" should be formatted as proper documentation.
Apply this diff to improve documentation:
- // returns (request_id, pin_code) + /// Sends a history sync request and returns a tuple containing: + /// - `request_id`: Unique identifier for the sync request + /// - `pin_code`: PIN code required for verification pub async fn send_history_sync_request(&self) -> Result<(String, String), DeviceSyncError> {
39-42: Add error documentation.The
process_history_sync_replymethod should document the possible error cases.Add error documentation:
+ /// Process a history sync reply. + /// + /// # Errors + /// Returns `DeviceSyncError` if: + /// - No reply is found to process + /// - Storage operations fail + /// - Decryption of the sync data fails pub async fn process_history_sync_reply(&self) -> Result<(), DeviceSyncError> {xmtp_mls/src/groups/device_sync.rs (1)
234-238: Optimize Time Discrepancy Check UsingDurationWhen checking for time discrepancies in
process_sync_reply, comparing nanoseconds directly can be error-prone and less readable.Consider using
std::time::Durationfor clearer semantics:+ use std::time::Duration; - let time_diff = reply.timestamp_ns.abs_diff(now_ns() as u64); - if time_diff > NS_IN_HOUR as u64 { + let time_diff = Duration::from_nanos(reply.timestamp_ns.abs_diff(now_ns() as u64)); + if time_diff > Duration::from_secs(3600) { // time discrepancy is too much return Err(DeviceSyncError::SyncReplyTimestamp); }This change improves code readability and leverages the
Durationtype for time comparisons.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- xmtp_mls/src/groups/device_sync.rs (1 hunks)
- xmtp_mls/src/groups/device_sync/consent_sync.rs (1 hunks)
- xmtp_mls/src/groups/device_sync/message_sync.rs (1 hunks)
- xmtp_mls/src/storage/encrypted_store/consent_record.rs (4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- xmtp_mls/src/storage/encrypted_store/consent_record.rs
🧰 Additional context used
📓 Learnings (1)
xmtp_mls/src/groups/device_sync/consent_sync.rs (1)
Learnt from: codabrink PR: xmtp/libxmtp#1152 File: xmtp_mls/src/groups/device_sync/consent_sync.rs:15-31 Timestamp: 2024-10-28T20:23:03.635Z Learning: In `xmtp_mls/src/groups/device_sync/consent_sync.rs`, the `reply_to_consent_sync_request` method intentionally does not require a `pin_code` parameter.
🔇 Additional comments (2)
xmtp_mls/src/groups/device_sync/consent_sync.rs (1)
10-13: 🛠️ Refactor suggestionConsider using a named struct for better code clarity.
The tuple return type
(String, String)makes it unclear what each string represents. A named struct would improve code readability and maintainability.+#[derive(Debug)] +pub struct ConsentSyncRequest { + pub group_id: String, + pub pin_code: String, +} -pub async fn send_consent_sync_request(&self) -> Result<(String, String), DeviceSyncError> { +pub async fn send_consent_sync_request(&self) -> Result<ConsentSyncRequest, DeviceSyncError> { let request = DeviceSyncRequest::new(DeviceSyncKind::Consent); - self.send_sync_request(request).await + let (group_id, pin_code) = self.send_sync_request(request).await?; + Ok(ConsentSyncRequest { group_id, pin_code }) }Likely invalid or redundant comment.
xmtp_mls/src/groups/device_sync/message_sync.rs (1)
18-37:⚠️ Potential issueAdd PIN code parameter for security.
The
reply_to_history_sync_requestmethod should require a PIN code parameter to verify the request before accessing and sending sensitive data.Apply this diff to add PIN verification:
pub async fn reply_to_history_sync_request( &self, + pin_code: &str, ) -> Result<DeviceSyncReplyProto, DeviceSyncError> { let (_msg, request) = self .pending_sync_request(DeviceSyncKind::MessageHistory) .await?; + + self.verify_pin(&request.request_id, pin_code)?; let groups = self.syncable_groups()?; let messages = self.syncable_messages()?;Likely invalid or redundant comment.
|
Would it make sense to do a code walkthrough? I always like to suggest that for large or complex PR's, if it's too late to split into smaller pieces |
codabrink
force-pushed
the
coda/sync-consent
branch
from
ee9567c to
a59284a
Compare
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (1)
xmtp_mls/src/groups/device_sync/consent_sync.rs (1)
22-29: Consider adding retry mechanism for sync reply sending.
The sync reply sending operation could fail due to network issues. Consider implementing a retry mechanism with exponential backoff.
Example retry implementation:
let reply = self.create_sync_reply(
&request.request_id,
&[consent_records],
DeviceSyncKind::Consent,
).await?;
let max_retries = 3;
let mut retry_count = 0;
while retry_count < max_retries {
match self.send_sync_reply(reply.clone()).await {
Ok(_) => break,
Err(e) if retry_count < max_retries - 1 => {
let delay = std::time::Duration::from_secs(2u64.pow(retry_count as u32));
tokio::time::sleep(delay).await;
retry_count += 1;
continue;
}
Err(e) => return Err(e),
}
}📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- xmtp_mls/src/groups/device_sync.rs (1 hunks)
- xmtp_mls/src/groups/device_sync/consent_sync.rs (1 hunks)
- xmtp_mls/src/groups/device_sync/message_sync.rs (1 hunks)
- xmtp_mls/src/storage/encrypted_store/schema.rs (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- xmtp_mls/src/storage/encrypted_store/schema.rs
🔇 Additional comments (4)
xmtp_mls/src/groups/device_sync/consent_sync.rs (1)
1-9: LGTM! Clean and well-structured imports with appropriate trait bounds.
The imports and trait bounds are well-organized and properly constrained for the required functionality.
xmtp_mls/src/groups/device_sync.rs (3)
1-49: LGTM! Well-organized imports and appropriate cryptographic constants.
The imports are logically grouped, and the cryptographic constants are correctly defined for AES-256-GCM encryption (256-bit key and 96-bit nonce).
513-517:
Improve PIN generation security.
The current PIN generation has several issues:
- The range 0-10000 allows 5-digit PINs, but the format string
{:04}assumes 4 digits - Leading zeros in PINs could cause confusion
pub(super) fn new_pin() -> String {
let mut rng = crypto_utils::rng();
- let pin: u32 = rng.gen_range(0..10000);
- format!("{:04}", pin)
+ let pin: u32 = rng.gen_range(1000..10000); // Ensure 4 digits without leading zeros
+ pin.to_string()
}Likely invalid or redundant comment.
578-596:
Clear sensitive data after encryption.
The encryption functions should clear sensitive data from memory after use to prevent potential memory dumps from exposing secrets.
fn encrypt_syncables_with_key(
syncables: &[Vec<Syncable>],
enc_key: DeviceSyncKeyType,
) -> Result<(Vec<u8>, DeviceSyncKeyType), DeviceSyncError> {
let syncables: Vec<&Syncable> = syncables.iter().flat_map(|s| s.iter()).collect();
let payload = serde_json::to_vec(&syncables)?;
let enc_key_bytes = enc_key.as_bytes();
let mut result = generate_nonce().to_vec();
let cipher = Aes256Gcm::new(GenericArray::from_slice(enc_key_bytes));
let nonce_array = GenericArray::from_slice(&result);
- result.append(&mut cipher.encrypt(nonce_array, &*payload)?);
+ let mut encrypted = cipher.encrypt(nonce_array, &*payload)?;
+ result.append(&mut encrypted);
+
+ // Clear sensitive data from memory
+ encrypted.fill(0);
+ payload.fill(0);
Ok((result, enc_key))
}Likely invalid or redundant comment.
| @@ -13,8 +13,8 @@ use super::{ | |||
| validated_commit::extract_group_membership, | |||
There was a problem hiding this comment.
how about we name this mls_sync or intent_sync, node sync could be conflated/confused with some kind of blockchain/consensus sync
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Outside diff range and nitpick comments (1)
xmtp_mls/src/groups/device_sync/consent_sync.rs (1)
1-4: Document the consent record reconciliation strategy.Based on the implementation and PR discussion, it's crucial to document how consent records are reconciled between devices, especially in scenarios like:
- Concurrent updates from multiple devices
- Conflicting consent states (e.g., one device blocks while another allows)
- Race conditions during sync
Consider adding a module-level documentation explaining the reconciliation rules and their implications.
+//! # Consent Synchronization +//! +//! This module implements device synchronization for consent records. +//! +//! ## Reconciliation Strategy +//! +//! When multiple devices update consent records: +//! 1. Latest sync operation takes precedence (last-write-wins) +//! 2. No conflict detection or resolution between devices +//! 3. No timestamp tracking for modifications +//! +//! ### Known Limitations +//! +//! - Race conditions possible during concurrent updates +//! - No way to detect or resolve conflicts between devices +//! - Consider implementing vector clocks or similar mechanism for conflict detection +//! use super::*; use crate::{Client, XmtpApi}; use xmtp_id::scw_verifier::SmartContractSignatureVerifier;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
examples/cli/cli-client.rs(8 hunks)xmtp_mls/src/groups/device_sync.rs(1 hunks)xmtp_mls/src/groups/device_sync/consent_sync.rs(1 hunks)xmtp_mls/src/groups/device_sync/message_sync.rs(1 hunks)xmtp_mls/src/storage/encrypted_store/schema.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- xmtp_mls/src/storage/encrypted_store/schema.rs
🔇 Additional comments (11)
xmtp_mls/src/groups/device_sync/consent_sync.rs (3)
35-40: 🛠️ Refactor suggestion
Add validation for received consent records.
The method should validate the received consent records before processing them to ensure data integrity and prevent conflicts.
pub async fn process_consent_sync_reply(
&self,
conn: &DbConnection,
) -> Result<(), DeviceSyncError> {
- self.process_sync_reply(conn, DeviceSyncKind::Consent).await
+ let reply = self.process_sync_reply(conn, DeviceSyncKind::Consent).await?;
+
+ // Validate received consent records
+ if let Some(records) = reply.records {
+ for record in records {
+ if let Syncable::ConsentRecord(record) = record {
+ if !record.is_valid() {
+ return Err(DeviceSyncError::InvalidPayload);
+ }
+ }
+ }
+ }
+
+ Ok(())
}55-56: 🛠️ Refactor suggestion
Use dynamic port allocation for tests.
Hardcoded port numbers can cause conflicts in CI environments or when running tests in parallel.
-const HISTORY_SERVER_HOST: &str = "0.0.0.0";
-const HISTORY_SERVER_PORT: u16 = 5558;
+use std::net::TcpListener;
+
+fn find_available_port() -> u16 {
+ TcpListener::bind("127.0.0.1:0")
+ .expect("Failed to bind to address")
+ .local_addr()
+ .expect("Failed to get local address")
+ .port()
+}
+
+const HISTORY_SERVER_HOST: &str = "127.0.0.1";Likely invalid or redundant comment.
15-33: 🛠️ Refactor suggestion
Add validation for consent records before sending.
The method should validate the consent records before sending them to ensure data integrity and prevent synchronization of invalid records.
pub async fn reply_to_consent_sync_request(
&self,
conn: &DbConnection,
) -> Result<DeviceSyncReplyProto, DeviceSyncError> {
let (_msg, request) = self.pending_sync_request(DeviceSyncKind::Consent).await?;
let consent_records = self.syncable_consent_records()?;
+ // Validate consent records before sending
+ if consent_records.is_empty() {
+ log::info!("No consent records to sync");
+ }
+ for record in &consent_records {
+ if let Syncable::ConsentRecord(record) = record {
+ if !record.is_valid() {
+ return Err(DeviceSyncError::InvalidPayload);
+ }
+ }
+ }
let reply = self
.create_sync_reply(
&request.request_id,
&[consent_records],
DeviceSyncKind::Consent,
)
.await?;
self.send_sync_reply(conn, reply.clone()).await?;
Ok(reply)
}xmtp_mls/src/groups/device_sync/message_sync.rs (1)
74-74: Add WASM integration tests.
As noted in the past review comments, WASM integration tests are missing. Consider adding them when the message-history/consent sync server is added to docker-compose (tracked in issue #1201).
✅ Verification successful
Let me check if there are WASM tests specifically for the message sync functionality.
Let me check the actual test implementations in this file to see if there are WASM-specific tests or if they're only configured but not implemented.
Let me check if there are any integration tests in this file, as the review specifically mentions WASM integration tests.
WASM integration tests are indeed missing for message sync functionality
The file has WASM test configuration set up (wasm_bindgen_test_configure!), but it explicitly excludes WASM architecture for the test module (#[cfg(all(not(target_arch = "wasm32"), test))]). The integration tests are currently implemented only for non-WASM environments using tokio::test. The review comment is correct in requesting WASM integration tests to be added when the message-history/consent sync server is added to docker-compose.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there are any WASM-specific tests
rg -l "wasm_bindgen_test" .
Length of output: 1721
Script:
#!/bin/bash
# Check for WASM tests in message_sync.rs
rg "wasm_bindgen_test" ./xmtp_mls/src/groups/device_sync/message_sync.rs -B2 -A5
Length of output: 328
Script:
#!/bin/bash
# Check the test implementations in message_sync.rs
rg "#\[test\]|#\[wasm_bindgen_test\]" ./xmtp_mls/src/groups/device_sync/message_sync.rs -B1 -A10
Length of output: 1043
Script:
#!/bin/bash
# Look for integration test configurations and implementations
rg "mod tests|integration|#\[tokio::test\]" ./xmtp_mls/src/groups/device_sync/message_sync.rs -B1 -A10
Length of output: 1088
examples/cli/cli-client.rs (1)
408-415: 🛠️ Refactor suggestion
Enhance error handling and logging in message processing
The current implementation could be improved to handle deserialization errors explicitly and provide more informative logging.
Apply this diff to enhance the implementation:
- let message_history_content =
- serde_json::from_slice::<DeviceSyncContent>(&message.decrypted_message_bytes);
+ let message_history_content = match serde_json::from_slice::<DeviceSyncContent>(
+ &message.decrypted_message_bytes,
+ ) {
+ Ok(content) => content,
+ Err(e) => {
+ error!(
+ "Failed to deserialize message content: {:?}, bytes: {:?}",
+ e,
+ hex::encode(&message.decrypted_message_bytes)
+ );
+ continue;
+ }
+ };
- match message_history_content {
- Ok(DeviceSyncContent::Request(ref request)) => {
- info!("Request: {:?}", request);
- }
- Ok(DeviceSyncContent::Reply(ref reply)) => {
- info!("Reply: {:?}", reply);
- }
- _ => {
- info!("Unknown message type: {:?}", message);
- }
- }
+ match message_history_content {
+ DeviceSyncContent::Request(ref request) => {
+ info!("Request: {:?}", request);
+ }
+ DeviceSyncContent::Reply(ref reply) => {
+ info!("Reply: {:?}", reply);
+ }
+ other => {
+ info!(
+ "Unexpected DeviceSyncContent variant: {:?}, message: {:?}",
+ other, message
+ );
+ }
+ }Likely invalid or redundant comment.
xmtp_mls/src/groups/device_sync.rs (6)
51-57: Consider using tagged enum for more robust serialization.
The Syncable enum is currently using #[serde(untagged)], which could lead to ambiguous deserialization if the variants have similar fields. Consider using #[serde(tag = "type")] for more explicit and robust serialization.
334-337: Avoid logging sensitive HTTP response data.
Logging the entire HTTP response using {:?} may expose sensitive information in the headers or body.
523-527: Improve PIN generation security.
The current PIN generation has several issues:
- The range 0-10000 allows 5-digit PINs, but the format string
{:04}assumes 4 digits - Leading zeros in PINs could cause confusion
529-538: Potential panic due to unchecked payload length.
The call to payload.split_at(NONCE_SIZE) can panic if payload is shorter than NONCE_SIZE. This might occur if the data is corrupted or incomplete.
592-604: Clear sensitive data after encryption.
Sensitive data like encryption keys and plaintext payloads should be cleared from memory after use to prevent potential leakage.
206-209: Improve error propagation in publish_messages.
The error from publish_messages() is logged but not propagated, which could lead to silent failures.
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (6)
xmtp_mls/src/groups/device_sync/consent_sync.rs (4)
1-1: Consider using explicit imports instead ofsuper::*.Using wildcard imports can make dependencies unclear and harder to track. Consider explicitly importing the required types and traits.
-use super::*; +use super::{DeviceSyncError, DeviceSyncKind, DeviceSyncRequest, DeviceSyncReplyProto, XmtpOpenMlsProvider};
15-36: Add documentation for the consent sync reply flow.This method implements a critical part of the consent synchronization process. Consider adding documentation that explains:
- The purpose and flow of the consent sync reply
- The expected behavior when no consent records exist
- Error cases and their handling
+/// Replies to a consent sync request by sending local consent records. +/// +/// # Arguments +/// * `provider` - The MLS provider instance +/// +/// # Returns +/// * `Ok(DeviceSyncReplyProto)` - The sync reply containing encrypted consent records +/// * `Err(DeviceSyncError)` - If no pending request exists or if sending reply fails pub async fn reply_to_consent_sync_request( &self, provider: &XmtpOpenMlsProvider, ) -> Result<DeviceSyncReplyProto, DeviceSyncError> {
38-44: Add documentation for process_consent_sync_reply.Document the purpose and error cases of this method.
+/// Processes a received consent sync reply by applying the received consent records. +/// +/// # Arguments +/// * `provider` - The MLS provider instance +/// +/// # Returns +/// * `Ok(())` - If the reply was processed successfully +/// * `Err(DeviceSyncError)` - If no reply exists or if processing fails pub async fn process_consent_sync_reply(
74-178: Add test cases for error scenarios.The current test only covers the happy path. Consider adding tests for:
- Invalid or missing consent records
- Network failures during sync
- Missing history sync URL
- No pending request/reply scenarios
Example test case:
#[tokio::test(flavor = "multi_thread", worker_threads = 1)] async fn test_consent_sync_no_history_url() { let wallet = generate_local_wallet(); let client = ClientBuilder::new_test_client(&wallet).await; // Don't set history_sync_url let result = client.send_consent_sync_request().await; assert!(matches!(result, Err(DeviceSyncError::MissingHistorySyncUrl))); }examples/cli/cli-client.rs (2)
Line range hint
288-310: Improve error handling in group member management.The group member management functions use
expectwhich could cause panics in production. Consider implementing proper error handling.Apply this diff to improve error handling:
- .add_members(account_addresses) - .await - .expect("failed to add member"); + .add_members(account_addresses) + .await + .map_err(|e| { + error!("Failed to add members: {:?}", e); + e + })?; - .remove_members(account_addresses) - .await - .expect("failed to add member"); + .remove_members(account_addresses) + .await + .map_err(|e| { + error!("Failed to remove members: {:?}", e); + e + })?;
Line range hint
1-564: Consider implementing command result handling.The CLI could benefit from a consistent approach to handling command results and displaying errors to users.
Consider implementing a common result handling pattern for all commands. This would involve:
- Creating a
CommandResulttype that wraps the success/failure outcomes- Implementing a common error display format
- Using this pattern consistently across all commands
Example implementation:
#[derive(Debug)] enum CommandResult { Success(String), Error(String), } impl CommandResult { fn display(&self) { match self { CommandResult::Success(msg) => info!("Success: {}", msg), CommandResult::Error(msg) => error!("Error: {}", msg), } } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
examples/cli/cli-client.rs(8 hunks)xmtp_mls/src/groups/device_sync.rs(1 hunks)xmtp_mls/src/groups/device_sync/consent_sync.rs(1 hunks)xmtp_mls/src/groups/device_sync/message_sync.rs(1 hunks)
🔇 Additional comments (10)
xmtp_mls/src/groups/device_sync/consent_sync.rs (2)
61-62: 🛠️ Refactor suggestion
Use dynamic port allocation for tests.
Hardcoded port numbers could cause conflicts in CI environments or when running tests in parallel.
-const HISTORY_SERVER_HOST: &str = "0.0.0.0";
-const HISTORY_SERVER_PORT: u16 = 5558;
+use std::net::TcpListener;
+
+fn find_available_port() -> u16 {
+ TcpListener::bind("127.0.0.1:0")
+ .expect("Failed to bind to address")
+ .local_addr()
+ .expect("Failed to get local address")
+ .port()
+}
+
+const HISTORY_SERVER_HOST: &str = "127.0.0.1";
+lazy_static! {
+ static ref HISTORY_SERVER_PORT: u16 = find_available_port();
+}Likely invalid or redundant comment.
10-13: 🛠️ Refactor suggestion
Consider using a named struct for the return type.
The tuple return type (String, String) makes it unclear what each string represents. A named struct would improve code clarity and maintainability.
+#[derive(Debug)]
+pub struct ConsentSyncRequest {
+ pub group_id: String,
+ pub pin_code: String,
+}
-pub async fn send_consent_sync_request(&self) -> Result<(String, String), DeviceSyncError> {
+pub async fn send_consent_sync_request(&self) -> Result<ConsentSyncRequest, DeviceSyncError> {
let request = DeviceSyncRequest::new(DeviceSyncKind::Consent);
- self.send_sync_request(&self.mls_provider()?, request).await
+ let (group_id, pin_code) = self.send_sync_request(&self.mls_provider()?, request).await?;
+ Ok(ConsentSyncRequest { group_id, pin_code })
}Likely invalid or redundant comment.
xmtp_mls/src/groups/device_sync/message_sync.rs (4)
13-17: LGTM!
The method is well-implemented with clear return types and error handling.
43-49: LGTM!
The method provides a clean wrapper around the generic sync reply processing.
51-58: LGTM!
The method efficiently uses iterators to transform groups into syncable format.
60-72: 🛠️ Refactor suggestion
Optimize message collection using iterators.
The current implementation uses nested loops. Consider using iterators with flat_map for better readability and performance.
fn syncable_messages(&self, conn: &DbConnection) -> Result<Vec<Syncable>, DeviceSyncError> {
- let groups = conn.find_groups(None, None, None, None, Some(ConversationType::Group))?;
-
- let mut all_messages = vec![];
- for StoredGroup { id, .. } in groups.into_iter() {
- let messages = conn.get_group_messages(&id, &MsgQueryArgs::default())?;
- for msg in messages {
- all_messages.push(Syncable::GroupMessage(msg));
- }
- }
-
- Ok(all_messages)
+ Ok(conn
+ .find_groups(None, None, None, None, Some(ConversationType::Group))?
+ .into_iter()
+ .flat_map(|StoredGroup { id, .. }| {
+ conn.get_group_messages(&id, &MsgQueryArgs::default())
+ .map(|msgs| msgs.into_iter().map(Syncable::GroupMessage))
+ .unwrap_or_else(|_| vec![].into_iter())
+ })
+ .collect())
}Likely invalid or redundant comment.
examples/cli/cli-client.rs (3)
23-24: LGTM: Clean import transition from message history to device sync.
The replacement of message history imports with device sync imports aligns with the codebase's transition to the new synchronization approach.
387-397:
Implement proper error handling in ProcessConsentSyncReply.
The new consent sync processing follows the same pattern of using unwrap which could lead to runtime panics.
The same error handling improvements suggested in the previous comments should be applied here.
413-420:
Improve error handling in message content deserialization.
The deserialization of message content could fail silently in some cases.
Apply this diff to improve error handling:
- let message_history_content =
- serde_json::from_slice::<DeviceSyncContent>(&message.decrypted_message_bytes);
+ let message_history_content = match serde_json::from_slice::<DeviceSyncContent>(
+ &message.decrypted_message_bytes
+ ) {
+ Ok(content) => content,
+ Err(e) => {
+ error!(
+ "Failed to deserialize message content: {:?}",
+ e
+ );
+ continue;
+ }
+ };
- match message_history_content {
- Ok(DeviceSyncContent::Request(ref request)) => {
+ match message_history_content {
+ DeviceSyncContent::Request(ref request) => {
info!("Request: {:?}", request);
}
- Ok(DeviceSyncContent::Reply(ref reply)) => {
+ DeviceSyncContent::Reply(ref reply) => {
info!("Reply: {:?}", reply);
}
_ => {
info!("Unknown message type: {:?}", message);
}
}Likely invalid or redundant comment.
xmtp_mls/src/groups/device_sync.rs (1)
394-409:
Improve HTTP request error handling
The error handling in download_history_payload could be more robust by adding retries for transient failures and providing more specific error types.
pub(crate) async fn download_history_payload(url: &str) -> Result<Vec<u8>, DeviceSyncError> {
tracing::info!("downloading history bundle from {:?}", url);
- let response = reqwest::Client::new().get(url).send().await?;
+ let client = reqwest::Client::new();
+ let mut attempts = 0;
+ const MAX_ATTEMPTS: u32 = 3;
+ const INITIAL_DELAY_MS: u64 = 1000;
+
+ while attempts < MAX_ATTEMPTS {
+ match client.get(url).send().await {
+ Ok(response) if response.status().is_success() => {
+ return Ok(response.bytes().await?.to_vec());
+ }
+ Ok(response) if response.status().is_server_error() => {
+ attempts += 1;
+ if attempts < MAX_ATTEMPTS {
+ tokio::time::sleep(std::time::Duration::from_millis(
+ INITIAL_DELAY_MS * 2u64.pow(attempts - 1)
+ )).await;
+ continue;
+ }
+ return Err(DeviceSyncError::Generic(format!(
+ "Server error after {} attempts: {}",
+ MAX_ATTEMPTS,
+ response.status()
+ )));
+ }
+ Ok(response) => {
+ return Err(DeviceSyncError::Generic(format!(
+ "Unexpected status code: {}",
+ response.status()
+ )));
+ }
+ Err(e) if e.is_timeout() || e.is_connect() => {
+ attempts += 1;
+ if attempts < MAX_ATTEMPTS {
+ tokio::time::sleep(std::time::Duration::from_millis(
+ INITIAL_DELAY_MS * 2u64.pow(attempts - 1)
+ )).await;
+ continue;
+ }
+ return Err(DeviceSyncError::Reqwest(e));
+ }
+ Err(e) => return Err(DeviceSyncError::Reqwest(e)),
+ }
+ }
- if !response.status().is_success() {
- tracing::error!(
- "Failed to download file. Status code: {} Response: {:?}",
- response.status(),
- response
- );
- response.error_for_status()?;
- unreachable!("Checked for error");
- }
-
- Ok(response.bytes().await?.to_vec())
+ Err(DeviceSyncError::Generic("Max retry attempts reached".to_string()))
}Likely invalid or redundant comment.
Issue: #1113
Proto PR: xmtp/proto#222
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Documentation
Chores