Skip to content

Commit

Permalink
Make NoChanges NoOp (#927)
Browse files Browse the repository at this point in the history 
* Make `NoChanges` a no-op
* Make `NoChanges` Error specific to Missing Sequence ID
* Move Intent to Error state if intent data is `None` (no-op)
  • Loading branch information
@insipx
insipx authored Jul 29, 2024
1 parent 0e12063 commit bdeaa75
Show file tree
Hide file tree
Showing 4 changed files with 65 additions and 51 deletions.
5 changes: 3 additions & 2 deletions bindings_ffi/Cargo.lock
View file

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

18 changes: 10 additions & 8 deletions xmtp_mls/src/groups/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -110,8 +110,8 @@ pub enum GroupError {
CreateMessage(#[from] openmls::prelude::CreateMessageError<sql_key_store::SqlKeyStoreError>),
#[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<String>),
#[error("add members: {0}")]
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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.
Expand All @@ -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)
Expand Down Expand Up @@ -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)]
Expand Down
91 changes: 51 additions & 40 deletions xmtp_mls/src/groups/sync.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -810,63 +810,73 @@ 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<ApiClient>(
&self,
provider: &XmtpOpenMlsProvider,
client: &Client<ApiClient>,
openmls_group: &mut OpenMlsGroup,
intent: &StoredGroupIntent,
) -> Result<(Vec<u8>, Option<Vec<u8>>), GroupError>
) -> Result<Option<(Vec<u8>, Option<Vec<u8>>)>, GroupError>
where
ApiClient: XmtpApi,
{
match intent.kind {
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
Expand All @@ -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())?;
Expand All @@ -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 =
Expand All @@ -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 =
Expand All @@ -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)))
}
}
}
Expand Down Expand Up @@ -1088,7 +1098,7 @@ impl MlsGroup {
"Could not find existing sequence ID for inbox {}",
inbox_id
);
return Err(GroupError::NoChanges);
return Err(GroupError::MissingSequenceId);
}
}

Expand Down Expand Up @@ -1189,7 +1199,7 @@ async fn apply_update_group_membership_intent<ApiClient: XmtpApi>(
openmls_group: &mut OpenMlsGroup,
intent_data: UpdateGroupMembershipIntentData,
signer: &SignatureKeyPair,
) -> Result<(MlsMessageOut, Option<PostCommitAction>), GroupError> {
) -> Result<Option<(MlsMessageOut, Option<PostCommitAction>)>, GroupError> {
let extensions: Extensions = openmls_group.extensions().clone();

let old_group_membership = extract_group_membership(&extensions)?;
Expand Down Expand Up @@ -1239,7 +1249,7 @@ async fn apply_update_group_membership_intent<ApiClient: XmtpApi>(
&& 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
Expand All @@ -1263,7 +1273,7 @@ async fn apply_update_group_membership_intent<ApiClient: XmtpApi>(
None => None,
};

Ok((commit, post_commit_action))
Ok(Some((commit, post_commit_action)))
}

fn get_removed_leaf_nodes(
Expand All @@ -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;

Expand Down Expand Up @@ -1310,6 +1321,6 @@ mod tests {
group.publish_intents(conn, &client).await.unwrap();
});
}
futures::future::join_all(futures).await;
future::join_all(futures).await;
}
}
2 changes: 1 addition & 1 deletion xmtp_mls/src/storage/encrypted_store/identity_update.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ impl DbConnection {
Ok(self.raw_query(|conn| query.first::<i64>(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,
Expand Down

0 comments on commit bdeaa75

Please sign in to comment.