Skip to content

Commit

Permalink
need to fix one more test
Browse files Browse the repository at this point in the history
  • Loading branch information
@insipx
insipx committed Dec 6, 2023
1 parent 9a8fcba commit a8f5d0c
Show file tree
Hide file tree
Showing 6 changed files with 198 additions and 246 deletions.
2 changes: 1 addition & 1 deletion xmtp_mls/src/client.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -404,7 +404,7 @@ mod tests {

let alice_bob_group = alice.create_group().unwrap();
alice_bob_group
.add_members_by_installation_id(vec![bob.installation_public_key()])
.add_members(vec![bob.account_address()])
.await
.unwrap();

Expand Down
49 changes: 18 additions & 31 deletions xmtp_mls/src/groups/intents.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,7 @@ use xmtp_proto::xmtp::mls::database::{
AddMembersData, PostCommitAction as PostCommitActionProto, RemoveMembersData, SendMessageData,
};

use crate::{
types::Address, verified_key_package::KeyPackageVerificationError,
xmtp_openmls_provider::XmtpOpenMlsProvider,
};
use crate::{types::Address, verified_key_package::KeyPackageVerificationError};

#[derive(Debug, Error)]
pub enum IntentError {
Expand All @@ -25,8 +22,6 @@ pub enum IntentError {
TlsCodec(#[from] tls_codec::Error),
#[error("generic: {0}")]
Generic(String),
#[error(transparent)]
FromHex(#[from] hex::FromHexError),
}

#[derive(Debug, Clone)]
Expand Down Expand Up @@ -71,24 +66,20 @@ impl From<SendMessageIntentData> for Vec<u8> {

#[derive(Debug, Clone)]
pub struct AddMembersIntentData {
pub wallet_addresses: Vec<Address>,
pub account_addresses: Vec<Address>,
}

impl AddMembersIntentData {
pub fn new(wallet_addresses: Vec<Address>) -> Self {
Self { wallet_addresses }
pub fn new(account_addresses: Vec<Address>) -> Self {
Self { account_addresses }
}

pub(crate) fn to_bytes(&self) -> Result<Vec<u8>, IntentError> {
pub(crate) fn to_bytes(self) -> Result<Vec<u8>, IntentError> {

Check warning on line 77 in xmtp_mls/src/groups/intents.rs

View workflow job for this annotation

GitHub Actions / workspace

methods with the following characteristics: (`to_*` and `self` type is not `Copy`) usually take `self` by reference 

warning: methods with the following characteristics: (`to_*` and `self` type is not `Copy`) usually take `self` by reference
  --> xmtp_mls/src/groups/intents.rs:77:28
   |
77 |     pub(crate) fn to_bytes(self) -> Result<Vec<u8>, IntentError> {
   |                            ^^^^
   |
   = help: consider choosing a less ambiguous name
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#wrong_self_convention
   = note: `#[warn(clippy::wrong_self_convention)]` on by default
let mut buf = Vec::new();
let wallet_addresses = self
.wallet_addresses
.iter()
.map(|addr| hex::decode(&addr[2..]))
.collect::<Result<Vec<_>, _>>()?;

AddMembersData {
version: Some(AddMembersVersion::V1(AddMembersV1 { wallet_addresses })),
version: Some(AddMembersVersion::V1(AddMembersV1 {
account_addresses: self.account_addresses,
})),
}
.encode(&mut buf)
.expect("encode error");
Expand All @@ -98,14 +89,10 @@ impl AddMembersIntentData {

pub(crate) fn from_bytes(data: &[u8]) -> Result<Self, IntentError> {
let msg = AddMembersData::decode(data)?;
let address_bytes = match msg.version {
Some(AddMembersVersion::V1(v1)) => v1.wallet_addresses,
let addresses = match msg.version {
Some(AddMembersVersion::V1(v1)) => v1.account_addresses,
None => return Err(IntentError::Generic("missing payload".to_string())),
};
let addresses = address_bytes
.iter()
.map(|addr| format!("0x{}", hex::encode(addr)))
.collect();

Ok(Self::new(addresses))
}
Expand All @@ -121,20 +108,20 @@ impl TryFrom<AddMembersIntentData> for Vec<u8> {

#[derive(Debug, Clone)]
pub struct RemoveMembersIntentData {
pub installation_ids: Vec<Vec<u8>>,
pub account_addresses: Vec<String>,
}

impl RemoveMembersIntentData {
pub fn new(installation_ids: Vec<Vec<u8>>) -> Self {
Self { installation_ids }
pub fn new(account_addresses: Vec<String>) -> Self {
Self { account_addresses }
}

pub(crate) fn to_bytes(&self) -> Vec<u8> {
pub(crate) fn to_bytes(self) -> Vec<u8> {

Check warning on line 119 in xmtp_mls/src/groups/intents.rs

View workflow job for this annotation

GitHub Actions / workspace

methods with the following characteristics: (`to_*` and `self` type is not `Copy`) usually take `self` by reference 

warning: methods with the following characteristics: (`to_*` and `self` type is not `Copy`) usually take `self` by reference
   --> xmtp_mls/src/groups/intents.rs:119:28
    |
119 |     pub(crate) fn to_bytes(self) -> Vec<u8> {
    |                            ^^^^
    |
    = help: consider choosing a less ambiguous name
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#wrong_self_convention
let mut buf = Vec::new();

RemoveMembersData {
version: Some(RemoveMembersVersion::V1(RemoveMembersV1 {
installation_ids: self.installation_ids.clone(),
account_addresses: self.account_addresses,
})),
}
.encode(&mut buf)
Expand All @@ -145,12 +132,12 @@ impl RemoveMembersIntentData {

pub(crate) fn from_bytes(data: &[u8]) -> Result<Self, IntentError> {
let msg = RemoveMembersData::decode(data)?;
let installation_ids = match msg.version {
Some(RemoveMembersVersion::V1(v1)) => v1.installation_ids,
let account_addresses = match msg.version {
Some(RemoveMembersVersion::V1(v1)) => v1.account_addresses,
None => return Err(IntentError::Generic("missing payload".to_string())),
};

Ok(Self::new(installation_ids))
Ok(Self::new(account_addresses))
}
}

Expand Down
5 changes: 1 addition & 4 deletions xmtp_mls/src/groups/members.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -67,10 +67,7 @@ mod tests {
let group = amal.create_group().unwrap();
// Add both of Bola's installations to the group
group
.add_members_by_installation_id(vec![
bola_a.installation_public_key(),
bola_b.installation_public_key(),
])
.add_members(vec![bola_a.account_address(), bola_b.account_address()])
.await
.unwrap();

Expand Down
80 changes: 27 additions & 53 deletions xmtp_mls/src/groups/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -434,20 +434,9 @@ where
self.sync_with_conn(conn).await
}

pub async fn add_members_by_installation_id(
&self,
_installation_ids: Vec<Vec<u8>>,
) -> Result<(), GroupError> {
// remove
unimplemented!()
}

pub(crate) async fn remove_members_by_installation_id(
&self,
installation_ids: Vec<Vec<u8>>,
) -> Result<(), GroupError> {
pub async fn remove_members(&self, wallet_addresses: Vec<String>) -> Result<(), GroupError> {
let conn = &mut self.client.store.conn()?;
let intent_data: Vec<u8> = RemoveMembersIntentData::new(installation_ids).into();
let intent_data: Vec<u8> = RemoveMembersIntentData::new(wallet_addresses).into();
let intent = NewGroupIntent::new(
IntentKind::RemoveMembers,
self.group_id.clone(),
Expand All @@ -458,20 +447,6 @@ where
self.sync_with_conn(conn).await
}

pub async fn remove_members(&self, wallet_addresses: Vec<String>) -> Result<(), GroupError> {
let installation_ids = self
.members()?
.into_iter()
.filter(|member| wallet_addresses.contains(&member.wallet_address))
.fold(vec![], |mut acc, member| {
acc.extend(member.installation_ids);
acc
});

self.remove_members_by_installation_id(installation_ids)
.await
}

pub async fn key_update(&self) -> Result<(), GroupError> {
let conn = &mut self.client.store.conn()?;
let intent = NewGroupIntent::new(IntentKind::KeyUpdate, self.group_id.clone(), vec![]);
Expand Down Expand Up @@ -566,15 +541,11 @@ where
IntentKind::AddMembers => {
let intent_data = AddMembersIntentData::from_bytes(intent.data.as_slice())?;

log::debug!("INTENT_DATA: {:?}", intent_data);

let key_packages = self
.client
.get_key_packages_for_wallet_addresses(intent_data.wallet_addresses)
.get_key_packages_for_wallet_addresses(intent_data.account_addresses)
.await?;

log::debug!("KEY PACKAGES: {:?}", key_packages);

let mls_key_packages: Vec<KeyPackage> =
key_packages.iter().map(|kp| kp.inner.clone()).collect();

Expand All @@ -598,18 +569,32 @@ where
}
IntentKind::RemoveMembers => {
let intent_data = RemoveMembersIntentData::from_bytes(intent.data.as_slice())?;

let installation_ids = self
.members()?
.into_iter()
.filter(|member| {
intent_data
.account_addresses
.contains(&member.wallet_address)
})
.fold(vec![], |mut acc, member| {
acc.extend(member.installation_ids);
acc
});

let leaf_nodes: Vec<LeafNodeIndex> = openmls_group
.members()
.filter(|member| intent_data.installation_ids.contains(&member.signature_key))
.filter(|member| installation_ids.contains(&member.signature_key))
.map(|member| member.index)
.collect();

let num_leaf_nodes = leaf_nodes.len();

if num_leaf_nodes != intent_data.installation_ids.len() {
if num_leaf_nodes != installation_ids.len() {
return Err(GroupError::Generic(format!(
"expected {} leaf nodes, found {}",
intent_data.installation_ids.len(),
installation_ids.len(),
num_leaf_nodes
)));
}
Expand Down Expand Up @@ -741,7 +726,7 @@ mod tests {
let amal_group = amal.create_group().unwrap();
// Add bola
amal_group
.add_members_by_installation_id(vec![bola.installation_public_key()])
.add_members(vec![bola.account_address()])
.await
.unwrap();

Expand All @@ -751,11 +736,11 @@ mod tests {

// Have amal and bola both invite charlie.
amal_group
.add_members_by_installation_id(vec![charlie.installation_public_key()])
.add_members(vec![charlie.account_address()])
.await
.expect("failed to add charlie");
bola_group
.add_members_by_installation_id(vec![charlie.installation_public_key()])
.add_members(vec![charlie.account_address()])
.await
.unwrap();

Expand Down Expand Up @@ -828,9 +813,7 @@ mod tests {
let client = ClientBuilder::new_test_client(generate_local_wallet().into()).await;
let group = client.create_group().expect("create group");

let result = group
.add_members_by_installation_id(vec![b"1234".to_vec()])
.await;
let result = group.add_members(vec!["1234".to_string()]).await;

assert!(result.is_err());
}
Expand All @@ -844,19 +827,13 @@ mod tests {

let group = client_1.create_group().expect("create group");
group
.add_members_by_installation_id(vec![client_2
.identity
.installation_keys
.to_public_vec()])
.add_members(vec![client_2.account_address()])
.await
.expect("group create failure");

// Try and add another member without merging the pending commit
group
.remove_members_by_installation_id(vec![client_2
.identity
.installation_keys
.to_public_vec()])
.remove_members(vec![client_2.account_address()])
.await
.expect("group create failure");

Expand Down Expand Up @@ -901,10 +878,7 @@ mod tests {
let group = client.create_group().expect("create group");

group
.add_members_by_installation_id(vec![client_2
.identity
.installation_keys
.to_public_vec()])
.add_members(vec![client_2.account_address()])
.await
.unwrap();

Expand Down
Loading

0 comments on commit a8f5d0c

Please sign in to comment.