diff --git a/bindings_ffi/Cargo.lock b/bindings_ffi/Cargo.lock index bb3293c11..1f37401ed 100644 --- a/bindings_ffi/Cargo.lock +++ b/bindings_ffi/Cargo.lock @@ -3051,9 +3051,9 @@ dependencies = [ [[package]] name = "parking_lot" -version = "0.12.1" +version = "0.12.3" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "3742b2c103b9f06bc9fff0a37ff4912935851bee6d36f3c02bcc755bcfec228f" +checksum = "f1bf18183cf54e8d6059647fc3063646a1801cf30896933ec2311622cc4b9a27" dependencies = [ "lock_api", "parking_lot_core", @@ -5981,6 +5981,7 @@ dependencies = [ "openmls_basic_credential", "openmls_rust_crypto", "openmls_traits", + "parking_lot", "prost 0.12.3", "rand", "reqwest 0.12.4", diff --git a/xmtp_mls/src/groups/mod.rs b/xmtp_mls/src/groups/mod.rs index 2ce711899..329035559 100644 --- a/xmtp_mls/src/groups/mod.rs +++ b/xmtp_mls/src/groups/mod.rs @@ -110,8 +110,8 @@ pub enum GroupError { CreateMessage(#[from] openmls::prelude::CreateMessageError), #[error("TLS Codec error: {0}")] TlsError(#[from] TlsCodecError), - #[error("No changes found in commit")] - NoChanges, + #[error("SequenceId not found in local db")] + MissingSequenceId, #[error("Addresses not found {0:?}")] AddressNotFound(Vec), #[error("add members: {0}")] @@ -592,7 +592,8 @@ impl MlsGroup { // If some existing group member has an update, this will return an intent with changes // when we really should return an error if intent_data.is_empty() { - return Err(GroupError::NoChanges); + log::warn!("Member already added"); + return Ok(()); } let intent = provider.conn().insert_group_intent(NewGroupIntent::new( @@ -1441,6 +1442,7 @@ mod tests { // Get bola's version of the same group let bola_groups = bola.sync_welcomes().await.unwrap(); let bola_group = bola_groups.first().unwrap(); + bola_group.sync(&bola).await.unwrap(); log::info!("Adding charlie from amal"); // Have amal and bola both invite charlie. @@ -1452,7 +1454,7 @@ mod tests { bola_group .add_members_by_inbox_id(&bola, vec![charlie.inbox_id()]) .await - .expect_err("expected err"); + .expect_err("expected error"); amal_group .receive(&amal.store().conn().unwrap(), &amal) @@ -1484,15 +1486,15 @@ mod tests { .unwrap(); assert_eq!(amal_uncommitted_intents.len(), 0); - let bola_uncommitted_intents = bola_db + let bola_failed_intents = bola_db .find_group_intents( bola_group.group_id.clone(), - Some(vec![IntentState::ToPublish, IntentState::Published]), + Some(vec![IntentState::Error]), None, ) .unwrap(); - // Bola should have one uncommitted intent for the failed attempt at adding Charlie, who is already in the group - assert_eq!(bola_uncommitted_intents.len(), 1); + // Bola should have one uncommitted intent in `Error::Failed` state for the failed attempt at adding Charlie, who is already in the group + assert_eq!(bola_failed_intents.len(), 1); } #[tokio::test(flavor = "multi_thread", worker_threads = 1)] diff --git a/xmtp_mls/src/groups/sync.rs b/xmtp_mls/src/groups/sync.rs index 6cf14caa0..27c1c339e 100644 --- a/xmtp_mls/src/groups/sync.rs +++ b/xmtp_mls/src/groups/sync.rs @@ -810,35 +810,42 @@ impl MlsGroup { return Err(err); } - let (payload, post_commit_data) = result.expect("already checked"); - let payload_slice = payload.as_slice(); + if let Some((payload, post_commit_data)) = result.expect("checked") { + let payload_slice = payload.as_slice(); - client - .api_client - .send_group_messages(vec![payload_slice]) - .await?; - log::info!( - "[{}] published intent [{}] of type [{}]", - client.inbox_id(), - intent.id, - intent.kind - ); - provider.conn().set_group_intent_published( - intent.id, - sha256(payload_slice), - post_commit_data, - )?; - log::debug!( - "client [{}] set stored intent [{}] to state `published`", - client.inbox_id(), - intent.id - ); + client + .api_client + .send_group_messages(vec![payload_slice]) + .await?; + log::info!( + "[{}] published intent [{}] of type [{}]", + client.inbox_id(), + intent.id, + intent.kind + ); + provider.conn().set_group_intent_published( + intent.id, + sha256(payload_slice), + post_commit_data, + )?; + log::debug!( + "client [{}] set stored intent [{}] to state `published`", + client.inbox_id(), + intent.id + ); + } else { + provider + .conn() + .set_group_intent_error_and_fail_msg(&intent)?; + } } Ok(()) } // Takes a StoredGroupIntent and returns the payload and post commit data as a tuple + // A return value of [`Option::None`] means this intent would not change the group. + #[allow(clippy::type_complexity)] #[tracing::instrument(level = "trace", skip_all)] async fn get_publish_intent_data( &self, @@ -846,7 +853,7 @@ impl MlsGroup { client: &Client, openmls_group: &mut OpenMlsGroup, intent: &StoredGroupIntent, - ) -> Result<(Vec, Option>), GroupError> + ) -> Result, Option>)>, GroupError> where ApiClient: XmtpApi, { @@ -854,19 +861,22 @@ impl MlsGroup { IntentKind::UpdateGroupMembership => { let intent_data = UpdateGroupMembershipIntentData::try_from(&intent.data)?; let signer = &self.context.identity.installation_keys; - let (commit, post_commit_action) = apply_update_group_membership_intent( + if let Some((commit, post_commit_action)) = apply_update_group_membership_intent( client, provider, openmls_group, intent_data, signer, ) - .await?; - - Ok(( - commit.tls_serialize_detached()?, - post_commit_action.map(|action| action.to_bytes()), - )) + .await? + { + Ok(Some(( + commit.tls_serialize_detached()?, + post_commit_action.map(|action| action.to_bytes()), + ))) + } else { + Ok(None) + } } IntentKind::SendMessage => { // We can safely assume all SendMessage intents have data @@ -879,13 +889,13 @@ impl MlsGroup { )?; let msg_bytes = msg.tls_serialize_detached()?; - Ok((msg_bytes, None)) + Ok(Some((msg_bytes, None))) } IntentKind::KeyUpdate => { let (commit, _, _) = openmls_group .self_update(&provider, &self.context.identity.installation_keys)?; - Ok((commit.tls_serialize_detached()?, None)) + Ok(Some((commit.tls_serialize_detached()?, None))) } IntentKind::MetadataUpdate => { let metadata_intent = UpdateMetadataIntentData::try_from(intent.data.clone())?; @@ -903,7 +913,7 @@ impl MlsGroup { let commit_bytes = commit.tls_serialize_detached()?; - Ok((commit_bytes, None)) + Ok(Some((commit_bytes, None))) } IntentKind::UpdateAdminList => { let admin_list_update_intent = @@ -919,7 +929,7 @@ impl MlsGroup { &self.context.identity.installation_keys, )?; let commit_bytes = commit.tls_serialize_detached()?; - Ok((commit_bytes, None)) + Ok(Some((commit_bytes, None))) } IntentKind::UpdatePermission => { let update_permissions_intent = @@ -934,7 +944,7 @@ impl MlsGroup { &self.context.identity.installation_keys, )?; let commit_bytes = commit.tls_serialize_detached()?; - Ok((commit_bytes, None)) + Ok(Some((commit_bytes, None))) } } } @@ -1088,7 +1098,7 @@ impl MlsGroup { "Could not find existing sequence ID for inbox {}", inbox_id ); - return Err(GroupError::NoChanges); + return Err(GroupError::MissingSequenceId); } } @@ -1189,7 +1199,7 @@ async fn apply_update_group_membership_intent( openmls_group: &mut OpenMlsGroup, intent_data: UpdateGroupMembershipIntentData, signer: &SignatureKeyPair, -) -> Result<(MlsMessageOut, Option), GroupError> { +) -> Result)>, GroupError> { let extensions: Extensions = openmls_group.extensions().clone(); let old_group_membership = extract_group_membership(&extensions)?; @@ -1239,7 +1249,7 @@ async fn apply_update_group_membership_intent( && new_key_packages.is_empty() && membership_diff.updated_inboxes.is_empty() { - return Err(GroupError::NoChanges); + return Ok(None); } // Update the extensions to have the new GroupMembership @@ -1263,7 +1273,7 @@ async fn apply_update_group_membership_intent( None => None, }; - Ok((commit, post_commit_action)) + Ok(Some((commit, post_commit_action))) } fn get_removed_leaf_nodes( @@ -1281,6 +1291,7 @@ fn get_removed_leaf_nodes( mod tests { use super::*; use crate::builder::ClientBuilder; + use futures::future; use std::sync::Arc; use xmtp_cryptography::utils::generate_local_wallet; @@ -1310,6 +1321,6 @@ mod tests { group.publish_intents(conn, &client).await.unwrap(); }); } - futures::future::join_all(futures).await; + future::join_all(futures).await; } } diff --git a/xmtp_mls/src/storage/encrypted_store/identity_update.rs b/xmtp_mls/src/storage/encrypted_store/identity_update.rs index d25b10cb7..a8a8bca8f 100644 --- a/xmtp_mls/src/storage/encrypted_store/identity_update.rs +++ b/xmtp_mls/src/storage/encrypted_store/identity_update.rs @@ -97,7 +97,7 @@ impl DbConnection { Ok(self.raw_query(|conn| query.first::(conn))?) } - /// Given a list of inbox_ids return a hashamp of each inbox ID -> highest known sequence ID + /// Given a list of inbox_ids return a HashMap of each inbox ID -> highest known sequence ID #[tracing::instrument(level = "trace", skip_all)] pub fn get_latest_sequence_id( &self,