From f312da854652050e1935e61419ca7d7e2a5292cf Mon Sep 17 00:00:00 2001 From: Andrew Plaza Date: Wed, 21 Aug 2024 18:21:22 -0400 Subject: [PATCH 1/9] remove lots of unwraps --- xmtp_mls/src/api/identity.rs | 7 +++- xmtp_mls/src/builder.rs | 6 ++-- xmtp_mls/src/client.rs | 14 +++++--- xmtp_mls/src/groups/message_history.rs | 4 ++- xmtp_mls/src/groups/mod.rs | 8 ++++- xmtp_mls/src/groups/subscriptions.rs | 34 ++++++++++--------- xmtp_mls/src/groups/sync.rs | 5 ++- xmtp_mls/src/groups/validated_commit.rs | 11 +++--- xmtp_mls/src/identity.rs | 10 +++--- xmtp_mls/src/identity_updates.rs | 2 +- xmtp_mls/src/lib.rs | 4 +++ .../encrypted_store/association_state.rs | 2 +- xmtp_mls/src/storage/encrypted_store/group.rs | 2 +- .../src/storage/encrypted_store/identity.rs | 30 +++++++++------- xmtp_mls/src/subscriptions.rs | 2 +- 15 files changed, 85 insertions(+), 56 deletions(-) diff --git a/xmtp_mls/src/api/identity.rs b/xmtp_mls/src/api/identity.rs index dbf3a1e62..5a7a5a245 100644 --- a/xmtp_mls/src/api/identity.rs +++ b/xmtp_mls/src/api/identity.rs @@ -140,7 +140,12 @@ where .responses .into_iter() .filter(|inbox_id| inbox_id.inbox_id.is_some()) - .map(|inbox_id| (inbox_id.address, inbox_id.inbox_id.unwrap())) + .map(|inbox_id| { + ( + inbox_id.address, + inbox_id.inbox_id.expect("Checked for some"), + ) + }) .collect()) } } diff --git a/xmtp_mls/src/builder.rs b/xmtp_mls/src/builder.rs index 321c399c8..bd568a423 100644 --- a/xmtp_mls/src/builder.rs +++ b/xmtp_mls/src/builder.rs @@ -480,7 +480,8 @@ mod tests { credential: Credential::new(CredentialType::Basic, rand_vec()), signature_request: None, }) - .into(); + .try_into() + .unwrap(); stored.store(&store.conn().unwrap()).unwrap(); let wrapper = ApiClientWrapper::new(mock_api, Retry::default()); @@ -506,7 +507,8 @@ mod tests { credential: Credential::new(CredentialType::Basic, rand_vec()), signature_request: None, }) - .into(); + .try_into() + .unwrap(); stored.store(&store.conn().unwrap()).unwrap(); diff --git a/xmtp_mls/src/client.rs b/xmtp_mls/src/client.rs index 225d33c2f..acc3018b2 100644 --- a/xmtp_mls/src/client.rs +++ b/xmtp_mls/src/client.rs @@ -95,11 +95,17 @@ pub enum ClientError { SignatureRequest(#[from] SignatureRequestError), // the box is to prevent infinite cycle between client and group errors #[error(transparent)] - Group(#[from] Box), + Group(Box), #[error("generic:{0}")] Generic(String), } +impl From for ClientError { + fn from(err: GroupError) -> ClientError { + ClientError::Group(Box::new(err)) + } +} + impl crate::retry::RetryableError for ClientError { fn is_retryable(&self) -> bool { match self { @@ -353,8 +359,7 @@ where GroupMembershipState::Allowed, permissions_policy_set.unwrap_or_default(), opts, - ) - .map_err(Box::new)?; + )?; // notify any streams of the new group let _ = self.local_events.send(LocalEvents::NewGroup(group.clone())); @@ -364,8 +369,7 @@ where pub(crate) fn create_sync_group(&self) -> Result { log::info!("creating sync group"); - let sync_group = - MlsGroup::create_and_insert_sync_group(self.context.clone()).map_err(Box::new)?; + let sync_group = MlsGroup::create_and_insert_sync_group(self.context.clone())?; Ok(sync_group) } diff --git a/xmtp_mls/src/groups/message_history.rs b/xmtp_mls/src/groups/message_history.rs index 3a5450db8..e2a84f3e5 100644 --- a/xmtp_mls/src/groups/message_history.rs +++ b/xmtp_mls/src/groups/message_history.rs @@ -444,7 +444,9 @@ pub(crate) async fn download_history_bundle( response ); Err(MessageHistoryError::Reqwest( - response.error_for_status().unwrap_err(), + response + .error_for_status() + .expect_err("Checked for success"), )) } } diff --git a/xmtp_mls/src/groups/mod.rs b/xmtp_mls/src/groups/mod.rs index 1645769b2..213fe4733 100644 --- a/xmtp_mls/src/groups/mod.rs +++ b/xmtp_mls/src/groups/mod.rs @@ -178,6 +178,8 @@ pub enum GroupError { PublishCancelled, #[error("the publish failed to complete due to panic")] PublishPanicked, + #[error("Missing metadata field {name}")] + MissingMetadataField { name: String }, } impl RetryableError for GroupError { @@ -1069,7 +1071,11 @@ pub fn build_extensions_for_permissions_update( PermissionUpdateType::UpdateMetadata => { let mut metadata_policy = existing_policy_set.update_metadata_policy.clone(); metadata_policy.insert( - update_permissions_intent.metadata_field_name.unwrap(), + update_permissions_intent.metadata_field_name.ok_or( + GroupError::MissingMetadataField { + name: "metadata_field_name".into(), + }, + )?, update_permissions_intent.policy_option.into(), ); PolicySet::new( diff --git a/xmtp_mls/src/groups/subscriptions.rs b/xmtp_mls/src/groups/subscriptions.rs index e62eca3c8..18f9bbc40 100644 --- a/xmtp_mls/src/groups/subscriptions.rs +++ b/xmtp_mls/src/groups/subscriptions.rs @@ -9,7 +9,6 @@ use crate::storage::group_message::StoredGroupMessage; use crate::subscriptions::{MessagesStreamInfo, StreamHandle}; use crate::XmtpApi; use crate::{retry::Retry, retry_async, Client}; -use prost::Message; use xmtp_proto::xmtp::mls::api::v1::GroupMessage; impl MlsGroup { @@ -82,21 +81,6 @@ impl MlsGroup { Ok(new_message) } - pub async fn process_streamed_group_message( - &self, - envelope_bytes: Vec, - client: Arc>, - ) -> Result - where - ApiClient: XmtpApi, - { - let envelope = GroupMessage::decode(envelope_bytes.as_slice()) - .map_err(|e| GroupError::Generic(e.to_string()))?; - - let message = self.process_stream_entry(envelope, client).await?; - Ok(message.unwrap()) - } - pub async fn stream( &self, client: Arc>, @@ -140,6 +124,7 @@ impl MlsGroup { #[cfg(test)] mod tests { + use super::*; use prost::Message; use std::{sync::Arc, time::Duration}; use tokio_stream::wrappers::UnboundedReceiverStream; @@ -151,6 +136,23 @@ mod tests { }; use futures::StreamExt; + impl MlsGroup { + pub async fn process_streamed_group_message( + &self, + envelope_bytes: Vec, + client: Arc>, + ) -> Result + where + ApiClient: XmtpApi, + { + let envelope = GroupMessage::decode(envelope_bytes.as_slice()) + .map_err(|e| GroupError::Generic(e.to_string()))?; + + let message = self.process_stream_entry(envelope, client).await?; + Ok(message.unwrap()) + } + } + #[tokio::test(flavor = "multi_thread", worker_threads = 1)] async fn test_decode_group_message_bytes() { let amal = ClientBuilder::new_test_client(&generate_local_wallet()).await; diff --git a/xmtp_mls/src/groups/sync.rs b/xmtp_mls/src/groups/sync.rs index 0723de47c..07b98020e 100644 --- a/xmtp_mls/src/groups/sync.rs +++ b/xmtp_mls/src/groups/sync.rs @@ -89,7 +89,7 @@ impl MlsGroup { log::info!( "current epoch for [{}] in sync() is Epoch: [{}]", client.inbox_id(), - self.load_mls_group(mls_provider).unwrap().epoch() + self.load_mls_group(mls_provider)?.epoch() ); self.maybe_update_installations(conn.clone(), None, client) .await?; @@ -984,8 +984,7 @@ impl MlsGroup { )?; for intent in intents { - if intent.post_commit_data.is_some() { - let post_commit_data = intent.post_commit_data.unwrap(); + if let Some(post_commit_data) = intent.post_commit_data { let post_commit_action = PostCommitAction::from_bytes(post_commit_data.as_slice())?; match post_commit_action { PostCommitAction::SendWelcomes(action) => { diff --git a/xmtp_mls/src/groups/validated_commit.rs b/xmtp_mls/src/groups/validated_commit.rs index 6c4f8271a..f8d6e671e 100644 --- a/xmtp_mls/src/groups/validated_commit.rs +++ b/xmtp_mls/src/groups/validated_commit.rs @@ -782,18 +782,19 @@ fn extract_actor( )?; // If there is both a path update and there are proposals we need to make sure that they are from the same actor - if path_update_leaf_node.is_some() && proposal_author_leaf_index.is_some() { - let proposal_author = openmls_group - .member_at(*proposal_author_leaf_index.unwrap()) + if let (Some(path_update_leaf_node), Some(proposal_author_leaf_index)) = + (path_update_leaf_node, proposal_author_leaf_index) + { + let proposal_author_leaf_index = openmls_group + .member_at(*proposal_author_leaf_index) .ok_or(CommitValidationError::ActorCouldNotBeFound)?; // Verify that the signature keys are the same if path_update_leaf_node - .unwrap() .signature_key() .as_slice() .to_vec() - .ne(&proposal_author.signature_key) + .ne(&proposal_author_leaf_index.signature_key) { return Err(CommitValidationError::MultipleActors); } diff --git a/xmtp_mls/src/identity.rs b/xmtp_mls/src/identity.rs index bb40eee84..f0da90ab9 100644 --- a/xmtp_mls/src/identity.rs +++ b/xmtp_mls/src/identity.rs @@ -68,7 +68,9 @@ impl IdentityStrategy { let stored_identity: Option = provider .conn() .fetch(&())? - .map(|i: StoredIdentity| i.into()); + .map(|i: StoredIdentity| i.try_into()) + .transpose()?; + debug!("identity in store: {:?}", stored_identity); match self { IdentityStrategy::CachedOnly => { @@ -428,7 +430,7 @@ impl Identity { let kp_bytes = kp.tls_serialize_detached()?; api_client.upload_key_package(kp_bytes, true).await?; - Ok(StoredIdentity::from(self).store(provider.conn_ref())?) + Ok(StoredIdentity::try_from(self)?.store(provider.conn_ref())?) } } @@ -440,9 +442,7 @@ async fn sign_with_installation_key( let verifying_key = signing_key.verifying_key(); let mut prehashed: Sha512 = Sha512::new(); prehashed.update(signature_text.clone()); - let sig = signing_key - .sign_prehashed(prehashed, Some(INSTALLATION_KEY_SIGNATURE_CONTEXT)) - .unwrap(); + let sig = signing_key.sign_prehashed(prehashed, Some(INSTALLATION_KEY_SIGNATURE_CONTEXT))?; let installation_key_sig = InstallationKeySignature::new( signature_text.clone(), diff --git a/xmtp_mls/src/identity_updates.rs b/xmtp_mls/src/identity_updates.rs index 817f40143..eed546f03 100644 --- a/xmtp_mls/src/identity_updates.rs +++ b/xmtp_mls/src/identity_updates.rs @@ -183,7 +183,7 @@ where StoredAssociationState::write_to_cache( conn, inbox_id.as_ref().to_string(), - last_sequence_id.unwrap(), + last_sequence_id.expect("checked for some"), final_state.clone(), )?; } diff --git a/xmtp_mls/src/lib.rs b/xmtp_mls/src/lib.rs index a4bb7e98b..43c892541 100644 --- a/xmtp_mls/src/lib.rs +++ b/xmtp_mls/src/lib.rs @@ -1,4 +1,8 @@ #![recursion_limit = "256"] +#![cfg_attr( + not(any(test, feature = "test-utils", feature = "bench")), + warn(clippy::unwrap_used) +)] pub mod api; pub mod builder; pub mod client; diff --git a/xmtp_mls/src/storage/encrypted_store/association_state.rs b/xmtp_mls/src/storage/encrypted_store/association_state.rs index 210009853..079c6ec47 100644 --- a/xmtp_mls/src/storage/encrypted_store/association_state.rs +++ b/xmtp_mls/src/storage/encrypted_store/association_state.rs @@ -78,7 +78,7 @@ impl StoredAssociationState { }) .transpose(); - if result.is_ok() && result.as_ref().unwrap().is_some() { + if let Ok(Some(_)) = result { log::debug!( "Loaded association state from cache: {} {}", inbox_id, diff --git a/xmtp_mls/src/storage/encrypted_store/group.rs b/xmtp_mls/src/storage/encrypted_store/group.rs index 272fa7456..d5516e7ec 100644 --- a/xmtp_mls/src/storage/encrypted_store/group.rs +++ b/xmtp_mls/src/storage/encrypted_store/group.rs @@ -220,7 +220,7 @@ impl DbConnection { .optional()?; if maybe_inserted_group.is_none() { - let existing_group: StoredGroup = dsl::groups.find(group.id).first(conn).unwrap(); + let existing_group: StoredGroup = dsl::groups.find(group.id).first(conn)?; if existing_group.welcome_id == group.welcome_id { // Error so OpenMLS db transaction are rolled back on duplicate welcomes return Err(diesel::result::Error::DatabaseError( diff --git a/xmtp_mls/src/storage/encrypted_store/identity.rs b/xmtp_mls/src/storage/encrypted_store/identity.rs index 68e36b598..ad54cdd65 100644 --- a/xmtp_mls/src/storage/encrypted_store/identity.rs +++ b/xmtp_mls/src/storage/encrypted_store/identity.rs @@ -1,4 +1,4 @@ -use crate::storage::encrypted_store::schema::identity; +use crate::storage::{encrypted_store::schema::identity, StorageError}; use diesel::prelude::*; use xmtp_id::InboxId; @@ -33,25 +33,29 @@ impl StoredIdentity { } } -impl From<&Identity> for StoredIdentity { - fn from(identity: &Identity) -> Self { - StoredIdentity { +impl TryFrom<&Identity> for StoredIdentity { + type Error = StorageError; + + fn try_from(identity: &Identity) -> Result { + Ok(StoredIdentity { inbox_id: identity.inbox_id.clone(), - installation_keys: db_serialize(&identity.installation_keys).unwrap(), - credential_bytes: db_serialize(&identity.credential()).unwrap(), + installation_keys: db_serialize(&identity.installation_keys)?, + credential_bytes: db_serialize(&identity.credential())?, rowid: None, - } + }) } } -impl From for Identity { - fn from(identity: StoredIdentity) -> Self { - Identity { +impl TryFrom for Identity { + type Error = StorageError; + + fn try_from(identity: StoredIdentity) -> Result { + Ok(Identity { inbox_id: identity.inbox_id.clone(), - installation_keys: db_deserialize(&identity.installation_keys).unwrap(), - credential: db_deserialize(&identity.credential_bytes).unwrap(), + installation_keys: db_deserialize(&identity.installation_keys)?, + credential: db_deserialize(&identity.credential_bytes)?, signature_request: None, - } + }) } } diff --git a/xmtp_mls/src/subscriptions.rs b/xmtp_mls/src/subscriptions.rs index 5e0b1c4d3..6c3fd03e1 100644 --- a/xmtp_mls/src/subscriptions.rs +++ b/xmtp_mls/src/subscriptions.rs @@ -114,7 +114,7 @@ where } } - Ok(creation_result.unwrap()) + Ok(creation_result?) } pub async fn process_streamed_welcome_message( From d4f22f37cd0a095bd3318392cf0ff4eb7c220bfb Mon Sep 17 00:00:00 2001 From: Andrew Plaza Date: Wed, 21 Aug 2024 18:30:20 -0400 Subject: [PATCH 2/9] revert name change typos --- xmtp_mls/src/groups/validated_commit.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/xmtp_mls/src/groups/validated_commit.rs b/xmtp_mls/src/groups/validated_commit.rs index 5a5089671..03f0b3b98 100644 --- a/xmtp_mls/src/groups/validated_commit.rs +++ b/xmtp_mls/src/groups/validated_commit.rs @@ -785,7 +785,7 @@ fn extract_actor( if let (Some(path_update_leaf_node), Some(proposal_author_leaf_index)) = (path_update_leaf_node, proposal_author_leaf_index) { - let proposal_author_leaf_index = openmls_group + let proposal_author = openmls_group .member_at(*proposal_author_leaf_index) .ok_or(CommitValidationError::ActorCouldNotBeFound)?; @@ -794,7 +794,7 @@ fn extract_actor( .signature_key() .as_slice() .to_vec() - .ne(&proposal_author_leaf_index.signature_key) + .ne(&proposal_author.signature_key) { return Err(CommitValidationError::MultipleActors); } From 66e3f87507972fc39b517ad3c212964db632af7a Mon Sep 17 00:00:00 2001 From: Andrew Plaza Date: Wed, 21 Aug 2024 18:32:40 -0400 Subject: [PATCH 3/9] prefer if let some --- xmtp_mls/src/identity_updates.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/xmtp_mls/src/identity_updates.rs b/xmtp_mls/src/identity_updates.rs index eed546f03..20e656f9f 100644 --- a/xmtp_mls/src/identity_updates.rs +++ b/xmtp_mls/src/identity_updates.rs @@ -179,11 +179,11 @@ where } log::debug!("Final state at {:?}: {:?}", last_sequence_id, final_state); - if last_sequence_id.is_some() { + if let Some(last_sequence_id) = last_sequence_id { StoredAssociationState::write_to_cache( conn, inbox_id.as_ref().to_string(), - last_sequence_id.expect("checked for some"), + last_sequence_id, final_state.clone(), )?; } From 96365df0d4f195e9bbe91c1c648dd5dc9f1e4e9e Mon Sep 17 00:00:00 2001 From: Andrew Plaza Date: Wed, 21 Aug 2024 18:51:20 -0400 Subject: [PATCH 4/9] fix bindings --- bindings_ffi/src/lib.rs | 1 + bindings_ffi/src/mls.rs | 1 - bindings_node/src/lib.rs | 2 ++ xmtp_mls/src/groups/mod.rs | 2 ++ xmtp_mls/src/groups/subscriptions.rs | 34 ++++++++++++---------------- 5 files changed, 20 insertions(+), 20 deletions(-) diff --git a/bindings_ffi/src/lib.rs b/bindings_ffi/src/lib.rs index 5df8d2918..ed28ded83 100755 --- a/bindings_ffi/src/lib.rs +++ b/bindings_ffi/src/lib.rs @@ -1,4 +1,5 @@ #![recursion_limit = "256"] +#![cfg_attr(not(test), warn(clippy::unwrap_used))] pub mod inbox_owner; pub mod logger; pub mod mls; diff --git a/bindings_ffi/src/mls.rs b/bindings_ffi/src/mls.rs index 4deb0fc8d..ef96a83df 100644 --- a/bindings_ffi/src/mls.rs +++ b/bindings_ffi/src/mls.rs @@ -69,7 +69,6 @@ pub type RustXmtpClient = MlsClient; /// xmtp.create_client(account_address, nonce, inbox_id, Option) /// ``` #[allow(clippy::too_many_arguments)] -#[allow(unused)] #[uniffi::export(async_runtime = "tokio")] pub async fn create_client( logger: Box, diff --git a/bindings_node/src/lib.rs b/bindings_node/src/lib.rs index 6f9b0ba3b..62b189960 100755 --- a/bindings_node/src/lib.rs +++ b/bindings_node/src/lib.rs @@ -1,4 +1,6 @@ #![recursion_limit = "256"] +#![cfg_attr(not(test), warn(clippy::unwrap_used))] + mod conversations; pub mod encoded_content; mod groups; diff --git a/xmtp_mls/src/groups/mod.rs b/xmtp_mls/src/groups/mod.rs index 4aa89fa38..e77044e53 100644 --- a/xmtp_mls/src/groups/mod.rs +++ b/xmtp_mls/src/groups/mod.rs @@ -180,6 +180,8 @@ pub enum GroupError { PublishPanicked, #[error("Missing metadata field {name}")] MissingMetadataField { name: String }, + #[error("Message was processed but is missing")] + MissingMessage, } impl RetryableError for GroupError { diff --git a/xmtp_mls/src/groups/subscriptions.rs b/xmtp_mls/src/groups/subscriptions.rs index 18f9bbc40..c469437fc 100644 --- a/xmtp_mls/src/groups/subscriptions.rs +++ b/xmtp_mls/src/groups/subscriptions.rs @@ -9,6 +9,7 @@ use crate::storage::group_message::StoredGroupMessage; use crate::subscriptions::{MessagesStreamInfo, StreamHandle}; use crate::XmtpApi; use crate::{retry::Retry, retry_async, Client}; +use prost::Message; use xmtp_proto::xmtp::mls::api::v1::GroupMessage; impl MlsGroup { @@ -120,12 +121,24 @@ impl MlsGroup { callback, ) } + pub async fn process_streamed_group_message( + &self, + envelope_bytes: Vec, + client: Arc>, + ) -> Result + where + ApiClient: XmtpApi, + { + let envelope = GroupMessage::decode(envelope_bytes.as_slice()) + .map_err(|e| GroupError::Generic(e.to_string()))?; + + let message = self.process_stream_entry(envelope, client).await?; + message.ok_or(GroupError::MissingMessage) + } } #[cfg(test)] mod tests { - use super::*; - use prost::Message; use std::{sync::Arc, time::Duration}; use tokio_stream::wrappers::UnboundedReceiverStream; use xmtp_cryptography::utils::generate_local_wallet; @@ -136,23 +149,6 @@ mod tests { }; use futures::StreamExt; - impl MlsGroup { - pub async fn process_streamed_group_message( - &self, - envelope_bytes: Vec, - client: Arc>, - ) -> Result - where - ApiClient: XmtpApi, - { - let envelope = GroupMessage::decode(envelope_bytes.as_slice()) - .map_err(|e| GroupError::Generic(e.to_string()))?; - - let message = self.process_stream_entry(envelope, client).await?; - Ok(message.unwrap()) - } - } - #[tokio::test(flavor = "multi_thread", worker_threads = 1)] async fn test_decode_group_message_bytes() { let amal = ClientBuilder::new_test_client(&generate_local_wallet()).await; From d9eb666e2d68ae0c1885a07a14eef1bafa5732ef Mon Sep 17 00:00:00 2001 From: Andrew Plaza Date: Wed, 21 Aug 2024 19:13:07 -0400 Subject: [PATCH 5/9] add clippy configuration, remove even more unwraps --- .clippy.toml | 1 + bindings_ffi/src/lib.rs | 2 +- bindings_node/src/lib.rs | 2 +- xmtp_mls/src/groups/group_permissions.rs | 36 +++++++++++++++--------- xmtp_mls/src/groups/intents.rs | 20 +++++-------- xmtp_mls/src/groups/subscriptions.rs | 3 +- xmtp_mls/src/groups/sync.rs | 6 ++-- xmtp_mls/src/lib.rs | 3 ++ xmtp_mls/src/utils/bench.rs | 2 ++ xmtp_mls/src/utils/test.rs | 1 + 10 files changed, 43 insertions(+), 33 deletions(-) create mode 100644 .clippy.toml mode change 100644 => 100755 xmtp_mls/src/utils/test.rs diff --git a/.clippy.toml b/.clippy.toml new file mode 100644 index 000000000..154626ef4 --- /dev/null +++ b/.clippy.toml @@ -0,0 +1 @@ +allow-unwrap-in-tests = true diff --git a/bindings_ffi/src/lib.rs b/bindings_ffi/src/lib.rs index ed28ded83..2faef1ae2 100755 --- a/bindings_ffi/src/lib.rs +++ b/bindings_ffi/src/lib.rs @@ -1,5 +1,5 @@ #![recursion_limit = "256"] -#![cfg_attr(not(test), warn(clippy::unwrap_used))] +#![warn(clippy::unwrap_used)] pub mod inbox_owner; pub mod logger; pub mod mls; diff --git a/bindings_node/src/lib.rs b/bindings_node/src/lib.rs index 62b189960..0dea07153 100755 --- a/bindings_node/src/lib.rs +++ b/bindings_node/src/lib.rs @@ -1,5 +1,5 @@ #![recursion_limit = "256"] -#![cfg_attr(not(test), warn(clippy::unwrap_used))] +#![warn(clippy::unwrap_used)] mod conversations; pub mod encoded_content; diff --git a/xmtp_mls/src/groups/group_permissions.rs b/xmtp_mls/src/groups/group_permissions.rs index 443ef9bc2..05d6f691c 100644 --- a/xmtp_mls/src/groups/group_permissions.rs +++ b/xmtp_mls/src/groups/group_permissions.rs @@ -72,7 +72,7 @@ impl GroupMutablePermissions { if proto.policies.is_none() { return Err(GroupMutablePermissionsError::MissingPolicies); } - let policies = proto.policies.unwrap(); + let policies = proto.policies.expect("checked for none"); Ok(Self::new(PolicySet::from_proto(policies)?)) } @@ -574,6 +574,8 @@ pub enum PolicyError { Serialization(#[from] prost::EncodeError), #[error("deserialization {0}")] Deserialization(#[from] prost::DecodeError), + #[error("Missing metadata policy field: {name}")] + MissingMetadataPolicyField { name: String }, #[error("invalid policy")] InvalidPolicy, #[error("unexpected preset policy")] @@ -1035,38 +1037,46 @@ impl PolicySet { // since the group was created, the number of metadata policies might not match // the default All Members Policy Set. As long as all metadata policies are allow, we will // match against All Members Preconfigured Policy -pub fn is_policy_all_members(policy: &PolicySet) -> bool { +pub fn is_policy_all_members(policy: &PolicySet) -> Result { let mut metadata_policies_equal = true; for field_name in policy.update_metadata_policy.keys() { - let metadata_policy = policy.update_metadata_policy.get(field_name).unwrap(); + let metadata_policy = policy.update_metadata_policy.get(field_name).ok_or( + PolicyError::MissingMetadataPolicyField { + name: field_name.to_string(), + }, + )?; metadata_policies_equal = metadata_policies_equal && metadata_policy.eq(&MetadataPolicies::allow()); } - metadata_policies_equal + Ok(metadata_policies_equal && policy.add_member_policy == MembershipPolicies::allow() && policy.remove_member_policy == MembershipPolicies::allow_if_actor_admin() && policy.add_admin_policy == PermissionsPolicies::allow_if_actor_super_admin() && policy.remove_admin_policy == PermissionsPolicies::allow_if_actor_super_admin() - && policy.update_permissions_policy == PermissionsPolicies::allow_if_actor_super_admin() + && policy.update_permissions_policy == PermissionsPolicies::allow_if_actor_super_admin()) } // Depending on if the client is on a newer or older version of libxmtp // since the group was created, the number of metadata policies might not match // the default Admin Only Policy Set. As long as all metadata policies are admin only, we will // match against Admin Only Preconfigured Policy -pub fn is_policy_admin_only(policy: &PolicySet) -> bool { +pub fn is_policy_admin_only(policy: &PolicySet) -> Result { let mut metadata_policies_equal = true; for field_name in policy.update_metadata_policy.keys() { - let metadata_policy = policy.update_metadata_policy.get(field_name).unwrap(); + let metadata_policy = policy.update_metadata_policy.get(field_name).ok_or( + PolicyError::MissingMetadataPolicyField { + name: field_name.to_string(), + }, + )?; metadata_policies_equal = metadata_policies_equal && metadata_policy.eq(&MetadataPolicies::allow_if_actor_admin()); } - metadata_policies_equal + Ok(metadata_policies_equal && policy.add_member_policy == MembershipPolicies::allow_if_actor_admin() && policy.remove_member_policy == MembershipPolicies::allow_if_actor_admin() && policy.add_admin_policy == PermissionsPolicies::allow_if_actor_super_admin() && policy.remove_admin_policy == PermissionsPolicies::allow_if_actor_super_admin() - && policy.update_permissions_policy == PermissionsPolicies::allow_if_actor_super_admin() + && policy.update_permissions_policy == PermissionsPolicies::allow_if_actor_super_admin()) } /// A policy where any member can add or remove any other member @@ -1122,9 +1132,9 @@ impl PreconfiguredPolicies { } pub fn from_policy_set(policy_set: &PolicySet) -> Result { - if is_policy_all_members(policy_set) { + if is_policy_all_members(policy_set)? { Ok(PreconfiguredPolicies::AllMembers) - } else if is_policy_admin_only(policy_set) { + } else if is_policy_admin_only(policy_set)? { Ok(PreconfiguredPolicies::AdminsOnly) } else { Err(PolicyError::InvalidPresetPolicy) @@ -1442,7 +1452,7 @@ mod tests { update_permissions_policy: PermissionsPolicies::allow_if_actor_super_admin(), }; - assert!(is_policy_all_members(&policy_set_new_metadata_permission)); + assert!(is_policy_all_members(&policy_set_new_metadata_permission).unwrap()); let mut metadata_policies_map = MetadataPolicies::default_map(MetadataPolicies::allow_if_actor_admin()); @@ -1459,7 +1469,7 @@ mod tests { update_permissions_policy: PermissionsPolicies::allow_if_actor_super_admin(), }; - assert!(is_policy_admin_only(&policy_set_new_metadata_permission)); + assert!(is_policy_admin_only(&policy_set_new_metadata_permission).unwrap()); } #[test] diff --git a/xmtp_mls/src/groups/intents.rs b/xmtp_mls/src/groups/intents.rs index aea0aa1d5..588c832d9 100644 --- a/xmtp_mls/src/groups/intents.rs +++ b/xmtp_mls/src/groups/intents.rs @@ -59,16 +59,12 @@ impl SendMessageIntentData { } pub(crate) fn to_bytes(&self) -> Vec { - let mut buf = Vec::new(); SendMessageData { version: Some(SendMessageVersion::V1(SendMessageV1 { payload_bytes: self.message.clone(), })), } - .encode(&mut buf) - .unwrap(); - - buf + .encode_to_vec() } pub(crate) fn from_bytes(data: &[u8]) -> Result { @@ -613,7 +609,6 @@ impl SendWelcomesAction { } pub(crate) fn to_bytes(&self) -> Vec { - let mut buf = Vec::new(); PostCommitActionProto { kind: Some(PostCommitActionKind::SendWelcomes(SendWelcomesProto { installations: self @@ -625,10 +620,7 @@ impl SendWelcomesAction { welcome_message: self.welcome_message.clone(), })), } - .encode(&mut buf) - .unwrap(); - - buf + .encode_to_vec() } } @@ -667,9 +659,11 @@ impl PostCommitAction { } } -impl From> for PostCommitAction { - fn from(data: Vec) -> Self { - PostCommitAction::from_bytes(data.as_slice()).unwrap() +impl TryFrom> for PostCommitAction { + type Error = IntentError; + + fn try_from(data: Vec) -> Result { + PostCommitAction::from_bytes(data.as_slice()) } } diff --git a/xmtp_mls/src/groups/subscriptions.rs b/xmtp_mls/src/groups/subscriptions.rs index c469437fc..8e77a65b1 100644 --- a/xmtp_mls/src/groups/subscriptions.rs +++ b/xmtp_mls/src/groups/subscriptions.rs @@ -139,7 +139,8 @@ impl MlsGroup { #[cfg(test)] mod tests { - use std::{sync::Arc, time::Duration}; + use super::*; + use std::time::Duration; use tokio_stream::wrappers::UnboundedReceiverStream; use xmtp_cryptography::utils::generate_local_wallet; diff --git a/xmtp_mls/src/groups/sync.rs b/xmtp_mls/src/groups/sync.rs index c3edb2116..cadff01b4 100644 --- a/xmtp_mls/src/groups/sync.rs +++ b/xmtp_mls/src/groups/sync.rs @@ -321,7 +321,7 @@ impl MlsGroup { return Ok(()); } - let validated_commit = maybe_validated_commit.unwrap(); + let validated_commit = maybe_validated_commit.expect("Checked for error"); log::info!( "[{}] merging pending commit for intent {}", @@ -1021,9 +1021,7 @@ impl MlsGroup { .get_installations_time_checked(self.group_id.clone())?; let elapsed = now - last; if elapsed > interval { - self.add_missing_installations(provider, client) - .await - .unwrap(); + self.add_missing_installations(provider, client).await?; provider .conn_ref() .update_installations_time_checked(self.group_id.clone())?; diff --git a/xmtp_mls/src/lib.rs b/xmtp_mls/src/lib.rs index 43c892541..5226a25e3 100644 --- a/xmtp_mls/src/lib.rs +++ b/xmtp_mls/src/lib.rs @@ -1,8 +1,11 @@ #![recursion_limit = "256"] +#![warn(clippy::unwrap_used)] +/* #![cfg_attr( not(any(test, feature = "test-utils", feature = "bench")), warn(clippy::unwrap_used) )] +*/ pub mod api; pub mod builder; pub mod client; diff --git a/xmtp_mls/src/utils/bench.rs b/xmtp_mls/src/utils/bench.rs index a880b1bf9..61ac2d590 100644 --- a/xmtp_mls/src/utils/bench.rs +++ b/xmtp_mls/src/utils/bench.rs @@ -1,6 +1,8 @@ //! Utilities for xmtp_mls benchmarks //! Utilities mostly include pre-generating identities in order to save time when writing/testing //! benchmarks. +#![allow(clippy::unwrap_used)] + use crate::{builder::ClientBuilder, Client}; use ethers::signers::{LocalWallet, Signer}; use indicatif::{ProgressBar, ProgressStyle}; diff --git a/xmtp_mls/src/utils/test.rs b/xmtp_mls/src/utils/test.rs old mode 100644 new mode 100755 index 9c7e8bd18..5fc724d5e --- a/xmtp_mls/src/utils/test.rs +++ b/xmtp_mls/src/utils/test.rs @@ -1,3 +1,4 @@ +#![allow(clippy::unwrap_used)] use std::env; use rand::{ From cde8e049ae88b749b16819bf23ec8f7c4bde7f1f Mon Sep 17 00:00:00 2001 From: Andrew Plaza Date: Wed, 21 Aug 2024 19:14:44 -0400 Subject: [PATCH 6/9] no longer require cfg_attr --- xmtp_mls/src/lib.rs | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/xmtp_mls/src/lib.rs b/xmtp_mls/src/lib.rs index 5226a25e3..c94f307ef 100644 --- a/xmtp_mls/src/lib.rs +++ b/xmtp_mls/src/lib.rs @@ -1,11 +1,6 @@ #![recursion_limit = "256"] #![warn(clippy::unwrap_used)] -/* -#![cfg_attr( - not(any(test, feature = "test-utils", feature = "bench")), - warn(clippy::unwrap_used) -)] -*/ + pub mod api; pub mod builder; pub mod client; From 803dd7a04937a25b53f8d9d634d466e64413d8fa Mon Sep 17 00:00:00 2001 From: Andrew Plaza Date: Wed, 21 Aug 2024 19:17:12 -0400 Subject: [PATCH 7/9] put process_streamed_group_message back --- xmtp_mls/src/groups/subscriptions.rs | 29 ++++++++++++++-------------- 1 file changed, 15 insertions(+), 14 deletions(-) diff --git a/xmtp_mls/src/groups/subscriptions.rs b/xmtp_mls/src/groups/subscriptions.rs index 8e77a65b1..23c97e50f 100644 --- a/xmtp_mls/src/groups/subscriptions.rs +++ b/xmtp_mls/src/groups/subscriptions.rs @@ -100,6 +100,21 @@ impl MlsGroup { .await?) } + pub async fn process_streamed_group_message( + &self, + envelope_bytes: Vec, + client: Arc>, + ) -> Result + where + ApiClient: XmtpApi, + { + let envelope = GroupMessage::decode(envelope_bytes.as_slice()) + .map_err(|e| GroupError::Generic(e.to_string()))?; + + let message = self.process_stream_entry(envelope, client).await?; + message.ok_or(GroupError::MissingMessage) + } + pub fn stream_with_callback( client: Arc>, group_id: Vec, @@ -121,20 +136,6 @@ impl MlsGroup { callback, ) } - pub async fn process_streamed_group_message( - &self, - envelope_bytes: Vec, - client: Arc>, - ) -> Result - where - ApiClient: XmtpApi, - { - let envelope = GroupMessage::decode(envelope_bytes.as_slice()) - .map_err(|e| GroupError::Generic(e.to_string()))?; - - let message = self.process_stream_entry(envelope, client).await?; - message.ok_or(GroupError::MissingMessage) - } } #[cfg(test)] From 8311051c9441305ba816227b1b9ebcc1e0095137 Mon Sep 17 00:00:00 2001 From: Andrew Plaza Date: Wed, 21 Aug 2024 19:18:25 -0400 Subject: [PATCH 8/9] fix --- xmtp_mls/src/groups/subscriptions.rs | 30 ++++++++++++++-------------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/xmtp_mls/src/groups/subscriptions.rs b/xmtp_mls/src/groups/subscriptions.rs index 23c97e50f..bff42214e 100644 --- a/xmtp_mls/src/groups/subscriptions.rs +++ b/xmtp_mls/src/groups/subscriptions.rs @@ -82,6 +82,21 @@ impl MlsGroup { Ok(new_message) } + pub async fn process_streamed_group_message( + &self, + envelope_bytes: Vec, + client: Arc>, + ) -> Result + where + ApiClient: XmtpApi, + { + let envelope = GroupMessage::decode(envelope_bytes.as_slice()) + .map_err(|e| GroupError::Generic(e.to_string()))?; + + let message = self.process_stream_entry(envelope, client).await?; + message.ok_or(GroupError::MissingMessage) + } + pub async fn stream( &self, client: Arc>, @@ -100,21 +115,6 @@ impl MlsGroup { .await?) } - pub async fn process_streamed_group_message( - &self, - envelope_bytes: Vec, - client: Arc>, - ) -> Result - where - ApiClient: XmtpApi, - { - let envelope = GroupMessage::decode(envelope_bytes.as_slice()) - .map_err(|e| GroupError::Generic(e.to_string()))?; - - let message = self.process_stream_entry(envelope, client).await?; - message.ok_or(GroupError::MissingMessage) - } - pub fn stream_with_callback( client: Arc>, group_id: Vec, From 18cc41ba6d7425e35a1de61e92a3a4bed98444a0 Mon Sep 17 00:00:00 2001 From: Andrew Plaza Date: Wed, 21 Aug 2024 19:25:41 -0400 Subject: [PATCH 9/9] last lint --- bindings_ffi/src/v2.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bindings_ffi/src/v2.rs b/bindings_ffi/src/v2.rs index afee41745..090306e97 100644 --- a/bindings_ffi/src/v2.rs +++ b/bindings_ffi/src/v2.rs @@ -317,7 +317,7 @@ impl FfiV2Subscription { return Err(GenericError::Generic { err: format!( "subscription event loop join error {}", - join_result.unwrap_err() + join_result.expect_err("checked for err") ), }); }