Remove lots of unwraps & add clippy lint (#983)

* remove lots of unwraps
* Add clippy warning when unwrap is used out side of tests/benchmarks

.clippy.toml

@@ -0,0 +1 @@
+allow-unwrap-in-tests = true

bindings_ffi/src/lib.rs

@@ -1,4 +1,5 @@
 #![recursion_limit = "256"]
+#![warn(clippy::unwrap_used)]
 pub mod inbox_owner;
 pub mod logger;
 pub mod mls;

bindings_ffi/src/mls.rs

@@ -69,7 +69,6 @@ pub type RustXmtpClient = MlsClient<TonicApiClient>;
 /// xmtp.create_client(account_address, nonce, inbox_id, Option<legacy_signed_private_key_proto>)
 /// ```
 #[allow(clippy::too_many_arguments)]
-#[allow(unused)]
 #[uniffi::export(async_runtime = "tokio")]
 pub async fn create_client(
     logger: Box<dyn FfiLogger>,

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")
                     ),
                 });
             }

bindings_node/src/lib.rs

@@ -1,4 +1,6 @@
 #![recursion_limit = "256"]
+#![warn(clippy::unwrap_used)]
+
 mod conversations;
 pub mod encoded_content;
 mod groups;

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())
     }
 }

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();
 

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<GroupError>),
+    Group(Box<GroupError>),
     #[error("generic:{0}")]
     Generic(String),
 }
 
+impl From<GroupError> 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 {
@@ -355,8 +361,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()));
@@ -366,8 +371,7 @@ where
 
     pub(crate) fn create_sync_group(&self) -> Result<MlsGroup, ClientError> {
         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)
     }

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<bool, PolicyError> {
     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<bool, PolicyError> {
     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<Self, PolicyError> {
-        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]

xmtp_mls/src/groups/intents.rs

@@ -59,16 +59,12 @@ impl SendMessageIntentData {
     }
 
     pub(crate) fn to_bytes(&self) -> Vec<u8> {
-        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<Self, IntentError> {
@@ -613,7 +609,6 @@ impl SendWelcomesAction {
     }
 
     pub(crate) fn to_bytes(&self) -> Vec<u8> {
-        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<Vec<u8>> for PostCommitAction {
-    fn from(data: Vec<u8>) -> Self {
-        PostCommitAction::from_bytes(data.as_slice()).unwrap()
+impl TryFrom<Vec<u8>> for PostCommitAction {
+    type Error = IntentError;
+
+    fn try_from(data: Vec<u8>) -> Result<Self, Self::Error> {
+        PostCommitAction::from_bytes(data.as_slice())
     }
 }
 

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"),
         ))
     }
 }

xmtp_mls/src/groups/mod.rs

@@ -178,6 +178,10 @@ pub enum GroupError {
     PublishCancelled,
     #[error("the publish failed to complete due to panic")]
     PublishPanicked,
+    #[error("Missing metadata field {name}")]
+    MissingMetadataField { name: String },
+    #[error("Message was processed but is missing")]
+    MissingMessage,
 }
 
 impl RetryableError for GroupError {
@@ -1074,7 +1078,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(

xmtp_mls/src/groups/subscriptions.rs

@@ -94,7 +94,7 @@ impl MlsGroup {
             .map_err(|e| GroupError::Generic(e.to_string()))?;
 
         let message = self.process_stream_entry(envelope, client).await?;
-        Ok(message.unwrap())
+        message.ok_or(GroupError::MissingMessage)
     }
 
     pub async fn stream<ApiClient>(
@@ -140,8 +140,8 @@ impl MlsGroup {
 
 #[cfg(test)]
 mod tests {
-    use prost::Message;
-    use std::{sync::Arc, time::Duration};
+    use super::*;
+    use std::time::Duration;
     use tokio_stream::wrappers::UnboundedReceiverStream;
     use xmtp_cryptography::utils::generate_local_wallet;
 

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(&mls_provider, None, client)
             .await?;
@@ -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 {}",
@@ -985,8 +985,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) => {
@@ -1022,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())?;

xmtp_mls/src/groups/validated_commit.rs

@@ -782,14 +782,15 @@ 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() {
+    if let (Some(path_update_leaf_node), Some(proposal_author_leaf_index)) =
+        (path_update_leaf_node, proposal_author_leaf_index)
+    {
         let proposal_author = openmls_group
-            .member_at(*proposal_author_leaf_index.unwrap())
+            .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()