[WIP] Improve framework coverage

.circleci/config.yml

@@ -4,6 +4,15 @@ orbs:
   codecov: codecov/[email protected]
   discord: antonioned/[email protected]
 
+commands:
+  rust_install_nightly:
+    steps:
+      - run:
+          name: "Install nightly toolchain"
+          command: |
+            rustup toolchain install nightly-x86_64-unknown-linux-gnu
+            rustup component add llvm-tools-preview
+
 parameters:
   GHA_Event:
     type: string
@@ -368,6 +377,7 @@ jobs:
       - image: cimg/rust:1.80.0
     resource_class: xlarge
     steps:
+      - rust_install_nightly
       - setup_remote_docker
       - checkout
       - run:

framework/contracts/account/src/config.rs

@@ -1,7 +1,7 @@
 use crate::{
     contract::{AccountResponse, AccountResult},
     error::AccountError,
-    modules::{_remove_whitelist_modules, _whitelist_modules, update_module_addresses},
+    modules::{_update_whitelisted_modules, update_module_addresses},
 };
 use abstract_sdk::cw_helpers::AbstractAttributes;
 use abstract_std::{
@@ -49,7 +49,7 @@ pub fn update_suspension_status(
 /// Allows the owner to manually update the internal configuration of the account.
 /// This can be used to unblock the account and its modules in case of a bug/lock on the account.
 pub fn update_internal_config(
-    mut deps: DepsMut,
+    deps: DepsMut,
     info: MessageInfo,
     action: InternalConfigAction,
 ) -> AccountResult {
@@ -71,19 +71,21 @@ pub fn update_internal_config(
 
             update_module_addresses(deps, add, to_remove)
         }
-        // TODO: Add tests for this action
         InternalConfigAction::UpdateWhitelist { to_add, to_remove } => {
-            let module_addresses_to_add: Result<Vec<Addr>, _> = to_add
+            let module_addresses_to_add = to_add
                 .into_iter()
                 .map(|str_addr| deps.api.addr_validate(&str_addr))
-                .collect();
-            let module_addresses_to_remove: Result<Vec<Addr>, _> = to_remove
+                .collect::<Result<Vec<Addr>, _>>()?;
+            let module_addresses_to_remove = to_remove
                 .into_iter()
                 .map(|str_addr| deps.api.addr_validate(&str_addr))
-                .collect();
+                .collect::<Result<Vec<Addr>, _>>()?;
 
-            _whitelist_modules(deps.branch(), module_addresses_to_add?)?;
-            _remove_whitelist_modules(deps, module_addresses_to_remove?)?;
+            _update_whitelisted_modules(
+                deps.storage,
+                module_addresses_to_add,
+                module_addresses_to_remove,
+            )?;
 
             Ok(AccountResponse::action("update_whitelist"))
         }
@@ -517,9 +519,11 @@ mod tests {
     }
 
     mod update_internal_config {
-        use abstract_std::account::InternalConfigAction::UpdateModuleAddresses;
+        use abstract_std::account::InternalConfigAction;
         use ownership::GovOwnershipError;
 
+        use crate::modules::WHITELIST_SIZE_LIMIT;
+
         use super::*;
 
         #[test]
@@ -530,10 +534,11 @@ mod tests {
 
             mock_init(&mut deps)?;
 
-            let msg = ExecuteMsg::UpdateInternalConfig(UpdateModuleAddresses {
-                to_add: vec![],
-                to_remove: vec![],
-            });
+            let msg =
+                ExecuteMsg::UpdateInternalConfig(InternalConfigAction::UpdateModuleAddresses {
+                    to_add: vec![],
+                    to_remove: vec![],
+                });
 
             let bad_sender = deps.api.addr_make("not_account_owner");
             let res = execute_as(&mut deps, &bad_sender, msg.clone());
@@ -550,6 +555,134 @@ mod tests {
 
             Ok(())
         }
+
+        #[test]
+        fn whitelist_size_limit() -> anyhow::Result<()> {
+            let mut deps = mock_dependencies();
+            let abstr = AbstractMockAddrs::new(deps.api);
+            let owner = abstr.owner;
+
+            mock_init(&mut deps)?;
+
+            // One too many
+            let mut to_add: Vec<String> = (0..WHITELIST_SIZE_LIMIT + 1)
+                .map(|i| deps.api.addr_make(&format!("white_list_{i}")).to_string())
+                .collect();
+            let too_many_msg =
+                ExecuteMsg::UpdateInternalConfig(InternalConfigAction::UpdateWhitelist {
+                    to_add: to_add.clone(),
+                    to_remove: vec![],
+                });
+            let too_many = execute_as(&mut deps, &owner, too_many_msg).unwrap_err();
+            assert_eq!(too_many, AccountError::ModuleLimitReached {});
+
+            // Exact amount
+            to_add.pop();
+            let exactly_limit_msg =
+                ExecuteMsg::UpdateInternalConfig(InternalConfigAction::UpdateWhitelist {
+                    to_add: to_add.clone(),
+                    to_remove: vec![],
+                });
+            let white_list_add = execute_as(&mut deps, &owner, exactly_limit_msg);
+            assert!(white_list_add.is_ok());
+
+            // Can't add after hitting limit
+            let to_add = vec![deps.api.addr_make("over_limit").to_string()];
+            let module_limit_reached = execute_as(
+                &mut deps,
+                &owner,
+                ExecuteMsg::UpdateInternalConfig(InternalConfigAction::UpdateWhitelist {
+                    to_add,
+                    to_remove: vec![],
+                }),
+            )
+            .unwrap_err();
+            assert_eq!(module_limit_reached, AccountError::ModuleLimitReached {});
+
+            Ok(())
+        }
+
+        #[test]
+        fn whitelist_duplicates() -> anyhow::Result<()> {
+            let mut deps = mock_dependencies();
+            let abstr = AbstractMockAddrs::new(deps.api);
+            let owner = abstr.owner;
+
+            mock_init(&mut deps)?;
+
+            // duplicate after add
+            let to_add: Vec<String> = vec![deps.api.addr_make("module").to_string()];
+            let msg = ExecuteMsg::UpdateInternalConfig(InternalConfigAction::UpdateWhitelist {
+                to_add: to_add.clone(),
+                to_remove: vec![],
+            });
+            execute_as(&mut deps, &owner, msg.clone()).unwrap();
+
+            let duplicate_err = execute_as(&mut deps, &owner, msg).unwrap_err();
+            assert_eq!(
+                duplicate_err,
+                AccountError::AlreadyWhitelisted(to_add[0].clone())
+            );
+
+            // duplicate inside add
+            let to_add: Vec<String> = vec![
+                deps.api.addr_make("module2").to_string(),
+                deps.api.addr_make("module2").to_string(),
+            ];
+            let msg = ExecuteMsg::UpdateInternalConfig(InternalConfigAction::UpdateWhitelist {
+                to_add: to_add.clone(),
+                to_remove: vec![],
+            });
+            let duplicate_err = execute_as(&mut deps, &owner, msg).unwrap_err();
+            assert_eq!(
+                duplicate_err,
+                AccountError::AlreadyWhitelisted(to_add[0].clone())
+            );
+
+            Ok(())
+        }
+
+        #[test]
+        fn whitelist_remove() -> anyhow::Result<()> {
+            let mut deps = mock_dependencies();
+            let abstr = AbstractMockAddrs::new(deps.api);
+            let owner = abstr.owner;
+
+            mock_init(&mut deps)?;
+
+            // Add and remove same
+            let to_add: Vec<String> = vec![deps.api.addr_make("module").to_string()];
+            let msg = ExecuteMsg::UpdateInternalConfig(InternalConfigAction::UpdateWhitelist {
+                to_add: to_add.clone(),
+                to_remove: to_add.clone(),
+            });
+            let no_changes = execute_as(&mut deps, &owner, msg.clone());
+            assert!(no_changes.is_ok());
+
+            // Remove not whitelisted
+            let to_remove: Vec<String> = vec![deps.api.addr_make("module").to_string()];
+            let msg = ExecuteMsg::UpdateInternalConfig(InternalConfigAction::UpdateWhitelist {
+                to_add: vec![],
+                to_remove,
+            });
+            let not_whitelisted = execute_as(&mut deps, &owner, msg.clone()).unwrap_err();
+            assert_eq!(not_whitelisted, AccountError::NotWhitelisted {});
+
+            // Remove same twice
+            let to_add: Vec<String> = vec![deps.api.addr_make("module").to_string()];
+            let to_remove: Vec<String> = vec![
+                deps.api.addr_make("module").to_string(),
+                deps.api.addr_make("module").to_string(),
+            ];
+            let msg = ExecuteMsg::UpdateInternalConfig(InternalConfigAction::UpdateWhitelist {
+                to_add: to_add.clone(),
+                to_remove: to_remove.clone(),
+            });
+            let not_whitelisted = execute_as(&mut deps, &owner, msg.clone()).unwrap_err();
+            assert_eq!(not_whitelisted, AccountError::NotWhitelisted {});
+
+            Ok(())
+        }
     }
 
     mod update_ownership {

framework/contracts/account/src/contract.rs

@@ -175,13 +175,8 @@ pub fn instantiate(
     };
 
     // Set owner
-    let cw_gov_owner = ownership::initialize_owner(
-        deps.branch(),
-        // TODO: support no owner here (ownership handled in SUDO)
-        // Or do we want to add a `Sudo` governance type?
-        owner.clone(),
-        registry.address.clone(),
-    )?;
+    let cw_gov_owner =
+        ownership::initialize_owner(deps.branch(), owner.clone(), registry.address.clone())?;
 
     SUSPENSION_STATUS.save(deps.storage, &false)?;
 

framework/contracts/account/src/error.rs

@@ -72,7 +72,7 @@ pub enum AccountError {
     DependencyNotMet(String, String),
 
     #[error("Max amount of modules registered")]
-    ModuleLimitReached,
+    ModuleLimitReached {},
 
     #[error("Module with address {0} is already whitelisted")]
     AlreadyWhitelisted(String),