Various little cleanups (#567)

* Add some non-integration tests for pre-propose-base hooks.

* Remove duplicated proposal submission auth check logic.

* Rename `SetItem` `addr` field to `value`.

The `addr` naming was a legacy of when items could only store
validated addresses.

* Add tests for dao-core Info and ActiveProposalModules queries.

ci/integration-tests/src/tests/cw_core_test.rs

@@ -141,7 +141,7 @@ fn execute_items(chain: &mut Chain) {
                     contract_addr: dao.addr.clone(),
                     msg: to_binary(&dao_core::msg::ExecuteMsg::SetItem {
                         key: "meme".to_string(),
-                        addr: "foobar".to_string(),
+                        value: "foobar".to_string(),
                     })
                     .unwrap(),
                     funds: vec![],

contracts/dao-core/schema/dao-core.json

@@ -321,14 +321,14 @@
           "set_item": {
             "type": "object",
             "required": [
-              "addr",
-              "key"
+              "key",
+              "value"
             ],
             "properties": {
-              "addr": {
+              "key": {
                 "type": "string"
               },
-              "key": {
+              "value": {
                 "type": "string"
               }
             },

contracts/dao-core/src/contract.rs

@@ -112,7 +112,7 @@ pub fn execute(
         ExecuteMsg::Receive(_) => execute_receive_cw20(deps, info.sender),
         ExecuteMsg::ReceiveNft(_) => execute_receive_cw721(deps, info.sender),
         ExecuteMsg::RemoveItem { key } => execute_remove_item(deps, env, info.sender, key),
-        ExecuteMsg::SetItem { key, addr } => execute_set_item(deps, env, info.sender, key, addr),
+        ExecuteMsg::SetItem { key, value } => execute_set_item(deps, env, info.sender, key, value),
         ExecuteMsg::UpdateConfig { config } => {
             execute_update_config(deps, env, info.sender, config)
         }

contracts/dao-core/src/msg.rs

@@ -76,7 +76,7 @@ pub enum ExecuteMsg {
     /// Adds an item to the governance contract's item map. If the
     /// item already exists the existing value is overriden. If the
     /// item does not exist a new item is added.
-    SetItem { key: String, addr: String },
+    SetItem { key: String, value: String },
     /// Callable by the admin of the contract. If ADMIN is None the
     /// admin is set as the contract itself so that it may be updated
     /// later by vote. If ADMIN is Some a new admin is proposed and

contracts/dao-core/src/tests.rs

@@ -4,10 +4,14 @@ use cosmwasm_std::{
     testing::{mock_dependencies, mock_env},
     to_binary, Addr, CosmosMsg, Empty, Storage, Uint128, WasmMsg,
 };
+use cw2::ContractVersion;
 use cw_multi_test::{App, Contract, ContractWrapper, Executor};
 use cw_storage_plus::{Item, Map};
 use cw_utils::{Duration, Expiration};
-use dao_interface::{voting::VotingPowerAtHeightResponse, Admin, ModuleInstantiateInfo};
+use dao_interface::{
+    voting::{InfoResponse, VotingPowerAtHeightResponse},
+    Admin, ModuleInstantiateInfo,
+};
 
 use crate::{
     contract::{derive_proposal_module_prefix, migrate, CONTRACT_NAME, CONTRACT_VERSION},
@@ -601,6 +605,20 @@ fn test_removed_modules_can_not_execute() {
         }
     ));
 
+    // Check that the enabled query works.
+    let enabled_modules: Vec<ProposalModule> = app
+        .wrap()
+        .query_wasm_smart(
+            &gov_addr,
+            &QueryMsg::ActiveProposalModules {
+                start_after: None,
+                limit: None,
+            },
+        )
+        .unwrap();
+
+    assert_eq!(enabled_modules, vec![new_proposal_module.clone()]);
+
     // The new proposal module should be able to perform actions.
     app.execute_contract(
         Addr::unchecked(CREATOR_ADDR),
@@ -1423,11 +1441,11 @@ fn test_passthrough_voting_queries() {
     );
 }
 
-fn set_item(app: &mut App, gov_addr: Addr, key: String, addr: String) {
+fn set_item(app: &mut App, gov_addr: Addr, key: String, value: String) {
     app.execute_contract(
         gov_addr.clone(),
         gov_addr,
-        &ExecuteMsg::SetItem { key, addr },
+        &ExecuteMsg::SetItem { key, value },
         &[],
     )
     .unwrap();
@@ -1466,6 +1484,40 @@ fn list_items(
         .unwrap()
 }
 
+#[test]
+fn test_item_permissions() {
+    let (gov_addr, mut app) = do_standard_instantiate(true, None);
+
+    let err: ContractError = app
+        .execute_contract(
+            Addr::unchecked("ekez"),
+            gov_addr.clone(),
+            &ExecuteMsg::SetItem {
+                key: "k".to_string(),
+                value: "v".to_string(),
+            },
+            &[],
+        )
+        .unwrap_err()
+        .downcast()
+        .unwrap();
+    assert_eq!(err, ContractError::Unauthorized {});
+
+    let err: ContractError = app
+        .execute_contract(
+            Addr::unchecked("ekez"),
+            gov_addr,
+            &ExecuteMsg::RemoveItem {
+                key: "k".to_string(),
+            },
+            &[],
+        )
+        .unwrap_err()
+        .downcast()
+        .unwrap();
+    assert_eq!(err, ContractError::Unauthorized {});
+}
+
 #[test]
 fn test_add_remove_get() {
     let (gov_addr, mut app) = do_standard_instantiate(true, None);
@@ -1815,6 +1867,22 @@ fn test_cw20_receive_auto_add() {
         .unwrap();
     assert!(matches!(err, ContractError::Std(_)));
 
+    // Test that non-DAO can not update the list.
+    let err: ContractError = app
+        .execute_contract(
+            Addr::unchecked("ekez"),
+            gov_addr.clone(),
+            &ExecuteMsg::UpdateCw20List {
+                to_add: vec![],
+                to_remove: vec![gov_token.to_string()],
+            },
+            &[],
+        )
+        .unwrap_err()
+        .downcast()
+        .unwrap();
+    assert!(matches!(err, ContractError::Unauthorized {}));
+
     app.execute_contract(
         Addr::unchecked(gov_addr.clone()),
         gov_addr.clone(),
@@ -2015,6 +2083,22 @@ fn test_cw721_receive() {
         .unwrap();
     assert!(matches!(err, ContractError::Std(_)));
 
+    // Test that non-DAO can not update the list.
+    let err: ContractError = app
+        .execute_contract(
+            Addr::unchecked("ekez"),
+            gov_addr.clone(),
+            &ExecuteMsg::UpdateCw721List {
+                to_add: vec![],
+                to_remove: vec![cw721_addr.to_string()],
+            },
+            &[],
+        )
+        .unwrap_err()
+        .downcast()
+        .unwrap();
+    assert!(matches!(err, ContractError::Unauthorized {}));
+
     // Add a real cw721.
     app.execute_contract(
         Addr::unchecked(gov_addr.clone()),
@@ -2901,3 +2985,21 @@ pub fn test_migrate_update_version() {
     assert_eq!(version.version, CONTRACT_VERSION);
     assert_eq!(version.contract, CONTRACT_NAME);
 }
+
+#[test]
+fn test_query_info() {
+    let (core_addr, app) = do_standard_instantiate(true, None);
+    let res: InfoResponse = app
+        .wrap()
+        .query_wasm_smart(core_addr, &QueryMsg::Info {})
+        .unwrap();
+    assert_eq!(
+        res,
+        InfoResponse {
+            info: ContractVersion {
+                contract: CONTRACT_NAME.to_string(),
+                version: CONTRACT_VERSION.to_string()
+            }
+        }
+    )
+}

contracts/pre-propose/dao-pre-propose-approval-single/src/contract.rs

@@ -48,11 +48,9 @@ pub fn execute(
     match msg {
         ExecuteMsg::Propose { msg } => execute_propose(deps, env, info, msg),
 
-        // TODO(zeke): why not use behavior of base?
         ExecuteMsg::AddProposalSubmittedHook { address } => {
             execute_add_approver_hook(deps, info, address)
         }
-        // TODO(zeke): why not use behavior of base?
         ExecuteMsg::RemoveProposalSubmittedHook { address } => {
             execute_remove_approver_hook(deps, info, address)
         }

contracts/pre-propose/dao-pre-propose-single/Cargo.toml

@@ -36,4 +36,5 @@ cw-denom = { workspace = true }
 dao-interface = { workspace = true }
 dao-testing = { workspace = true }
 dao-proposal-hooks = { workspace = true }
-dao-proposal-single = { workspace = true }
+dao-proposal-single = { workspace = true }
+cw-hooks = { workspace = true }

contracts/pre-propose/dao-pre-propose-single/src/tests.rs

@@ -237,6 +237,30 @@ fn increase_allowance(app: &mut App, sender: &str, receiver: &Addr, cw20: Addr,
     .unwrap();
 }
 
+fn add_hook(app: &mut App, sender: &str, module: &Addr, hook: &str) {
+    app.execute_contract(
+        Addr::unchecked(sender),
+        module.clone(),
+        &ExecuteMsg::AddProposalSubmittedHook {
+            address: hook.to_string(),
+        },
+        &[],
+    )
+    .unwrap();
+}
+
+fn remove_hook(app: &mut App, sender: &str, module: &Addr, hook: &str) {
+    app.execute_contract(
+        Addr::unchecked(sender),
+        module.clone(),
+        &ExecuteMsg::RemoveProposalSubmittedHook {
+            address: hook.to_string(),
+        },
+        &[],
+    )
+    .unwrap();
+}
+
 fn get_balance_cw20<T: Into<String>, U: Into<String>>(
     app: &App,
     contract_addr: T,
@@ -287,6 +311,12 @@ fn get_dao(app: &App, module: Addr) -> Addr {
         .unwrap()
 }
 
+fn query_hooks(app: &App, module: Addr) -> cw_hooks::HooksResponse {
+    app.wrap()
+        .query_wasm_smart(module, &QueryMsg::ProposalSubmittedHooks {})
+        .unwrap()
+}
+
 fn get_proposal_module(app: &App, module: Addr) -> Addr {
     app.wrap()
         .query_wasm_smart(module, &QueryMsg::ProposalModule {})
@@ -1305,3 +1335,21 @@ fn test_withdraw() {
     let balance = get_balance_native(&app, core_addr.as_str(), "ujuno");
     assert_eq!(balance, Uint128::new(30));
 }
+
+#[test]
+fn test_hook_management() {
+    let app = &mut App::default();
+    let DefaultTestSetup {
+        core_addr,
+        proposal_single: _,
+        pre_propose,
+    } = setup_default_test(app, None, true);
+
+    add_hook(app, core_addr.as_str(), &pre_propose, "one");
+    add_hook(app, core_addr.as_str(), &pre_propose, "two");
+
+    remove_hook(app, core_addr.as_str(), &pre_propose, "one");
+
+    let hooks = query_hooks(app, pre_propose).hooks;
+    assert_eq!(hooks, vec!["two".to_string()])
+}

packages/dao-pre-propose-base/src/execute.rs

@@ -111,21 +111,9 @@ where
         info: MessageInfo,
         msg: ProposalMessage,
     ) -> Result<Response, PreProposeError> {
-        let config = self.config.load(deps.storage)?;
+        self.check_can_submit(deps.as_ref(), info.sender.clone())?;
 
-        if !config.open_proposal_submission {
-            let dao = self.dao.load(deps.storage)?;
-            let voting_power: VotingPowerAtHeightResponse = deps.querier.query_wasm_smart(
-                dao.into_string(),
-                &CwCoreQuery::VotingPowerAtHeight {
-                    address: info.sender.to_string(),
-                    height: None,
-                },
-            )?;
-            if voting_power.power.is_zero() {
-                return Err(PreProposeError::NotMember {});
-            }
-        }
+        let config = self.config.load(deps.storage)?;
 
         let deposit_messages = if let Some(ref deposit_info) = config.deposit_info {
             deposit_info.check_native_deposit_paid(&info)?;

packages/dao-pre-propose-base/src/lib.rs

@@ -4,3 +4,6 @@ pub mod error;
 pub mod execute;
 pub mod msg;
 pub mod state;
+
+#[cfg(test)]
+mod tests;