Adding Key Update Before Every New GroupIntent

xmtp_mls/src/groups/mod.rs

@@ -306,8 +306,12 @@
         validate_initial_group_membership(client, provider.conn_ref(), &mls_group).await?;
 
         let stored_group = provider.conn().insert_or_ignore_group(to_store)?;
-        let xmtp_group = Self::new(context, stored_group.id, stored_group.created_at_ns);
-        Ok(xmtp_group)
+
+        Ok(Self::new(
+            client.context.clone(),
+            stored_group.id,
+            stored_group.created_at_ns,
+        ))
     }
 
     // Decrypt a welcome message using HPKE and then create and save a group from the stored message
@@ -477,36 +481,41 @@
             return Err(GroupError::UserLimitExceeded);
         }
 
-        let conn = self.context.store.conn()?;
-        let intent_data: Vec<u8> =
-            AddMembersIntentData::new(account_addresses.into()).try_into()?;
-
-        self.pre_intent_hook(client).await?;
-
-        let intent = conn.insert_group_intent(NewGroupIntent::new(
-            IntentKind::AddMembers,
-            self.group_id.clone(),
-            intent_data,
-        ))?;
+        if inbox_id_map.len() != account_addresses.len() {
+            let found_addresses: HashSet<&String> = inbox_id_map.keys().collect();
+            let to_add_hashset = HashSet::from_iter(account_addresses.iter());
+            self.pre_intent_hook(client).await?;
+            let missing_addresses = found_addresses.difference(&to_add_hashset);
+            return Err(GroupError::AddressNotFound(
+                missing_addresses.into_iter().cloned().cloned().collect(),
+            ));
+        }
 
         self.add_members_by_inbox_id(client, inbox_id_map.into_values().collect())
             .await
     }
 
     // Before calling this function, please verify pre_intent_hook has been called.
-    pub async fn add_members_by_installation_id<ApiClient>(
+    pub async fn add_members_by_inbox_id<ApiClient: XmtpApi>(
         &self,
         client: &Client<ApiClient>,
-    ) -> Result<(), GroupError>
-    where
-        ApiClient: XmtpApi,
-    {
-        validate_ed25519_keys(&installation_ids)?;
-        let conn = self.context.store.conn()?;
-        let intent_data: Vec<u8> = AddMembersIntentData::new(installation_ids.into()).try_into()?;
+        inbox_ids: Vec<String>,
+    ) -> Result<(), GroupError> {
+        let conn = client.store().conn()?;
+        let provider = client.mls_provider(conn);
+        let intent_data = self
+            .get_membership_update_intent(client, &provider, inbox_ids, vec![])
+            .await?;
 
-        let intent = conn.insert_group_intent(NewGroupIntent::new(
-            IntentKind::AddMembers,
+        // TODO:nm this isn't the best test for whether the request is valid
+        // 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);
+        }
+
+        let intent = provider.conn().insert_group_intent(NewGroupIntent::new(
+            IntentKind::UpdateGroupMembership,
             self.group_id.clone(),
             intent_data.into(),
         ))?;
@@ -521,16 +530,7 @@
         account_addresses_to_remove: Vec<InboxId>,
     ) -> Result<(), GroupError> {
         let account_addresses = sanitize_evm_addresses(account_addresses_to_remove)?;
-        let conn = self.context.store.conn()?;
-        let intent_data: Vec<u8> = RemoveMembersIntentData::new(account_addresses.into()).into();
-
-        self.pre_intent_hook(client).await?;
-
-        let intent = conn.insert_group_intent(NewGroupIntent::new(
-            IntentKind::RemoveMembers,
-            self.group_id.clone(),
-            intent_data,
-        ))?;
+        let inbox_id_map = client.api_client.get_inbox_ids(account_addresses).await?;
 
         self.remove_members_by_inbox_id(client, inbox_id_map.into_values().collect())
             .await
@@ -626,10 +626,15 @@
         ApiClient: XmtpApi,
     {
         let conn = self.context.store.conn()?;
-        let intent_data: Vec<u8> = RemoveMembersIntentData::new(installation_ids.into()).into();
-
+        let intent_action_type = match action_type {
+            UpdateAdminListType::Add => AdminListActionType::Add,
+            UpdateAdminListType::Remove => AdminListActionType::Remove,
+            UpdateAdminListType::AddSuper => AdminListActionType::AddSuper,
+            UpdateAdminListType::RemoveSuper => AdminListActionType::RemoveSuper,
+        };
+        let intent_data: Vec<u8> =
+            UpdateAdminListIntentData::new(intent_action_type, inbox_id).into();
         self.pre_intent_hook(client).await?;
-
         let intent = conn.insert_group_intent(NewGroupIntent::new(
             IntentKind::UpdateAdminList,
             self.group_id.clone(),
@@ -936,14 +941,13 @@
 
 #[cfg(test)]
 mod tests {
+    use openmls::prelude::{tls_codec::Serialize, Member, MlsGroup as OpenMlsGroup};
     use openmls::prelude::tls_codec::Deserialize;
-    use openmls::prelude::Member;
     use openmls::prelude::MlsMessageIn;
     use openmls::prelude::MlsMessageBodyIn;
     use openmls::prelude::Proposal;
     use openmls::prelude::ProcessedMessageContent;
     use crate::groups::GroupMessageVersion;
-    use crate::groups::OpenMlsGroup;
     use prost::Message;
     use tracing_test::traced_test;
     use xmtp_cryptography::utils::generate_local_wallet;
@@ -1055,8 +1059,7 @@
             .query_group_messages(group.group_id, None)
             .await
             .expect("read topic");
-
-        assert_eq!(messages.len(), 2)
+        assert_eq!(messages.len(), 2);
     }
 
     #[tokio::test(flavor = "multi_thread", worker_threads = 1)]
@@ -1393,7 +1396,7 @@
 
         // client A makes a group with client B.
         let group = client_a.create_group(None).expect("create group");
         group.add_members(vec![client_b.account_address()], &client_a).await.unwrap();
 
         // client B creates it from welcome.
         let client_b_group = receive_group_invite(&client_b).await;
@@ -1442,7 +1445,7 @@
         };
     }
 
-    #[tokio::test]
+    #[tokio::test(flavor = "multi_thread", worker_threads = 1)]
     async fn test_post_commit() {
         let client = ClientBuilder::new_test_client(&generate_local_wallet()).await;
         let client_2 = ClientBuilder::new_test_client(&generate_local_wallet()).await;

xmtp_mls/src/storage/encrypted_store/group.rs

@@ -35,8 +35,9 @@ pub struct StoredGroup {
     pub installations_last_checked: i64,
     /// Enum, [`Purpose`] signifies the group purpose which extends to who can access it.
     pub purpose: Purpose,
-    /// The wallet address of who added the user to a group.
-    pub added_by_address: String,
+
+    /// The inbox_id of who added the user to a group.
+    pub added_by_inbox_id: String,
     pub rotated_at_ns: i64,
 }
 
@@ -57,7 +58,7 @@ impl StoredGroup {
             membership_state,
             installations_last_checked: 0,
             purpose: Purpose::Conversation,
-            added_by_address,
+            added_by_inbox_id,
             rotated_at_ns: 0,
         }
     }
@@ -75,7 +76,7 @@ impl StoredGroup {
             membership_state,
             installations_last_checked: 0,
             purpose: Purpose::Sync,
-            added_by_address: "".into(),
+            added_by_inbox_id: "".into(),
             rotated_at_ns: 0,
         }
     }

xmtp_mls/src/storage/encrypted_store/schema.rs

@@ -41,7 +41,7 @@ diesel::table! {
         membership_state -> Integer,
         installations_last_checked -> BigInt,
         purpose -> Integer,
-        added_by_address -> Text,
+        added_by_inbox_id -> Text,
         rotated_at_ns -> BigInt,
     }
 }