Repository: DA0-DA0/dao-contracts
Pull request #653 (2023-02-15 12:41:08)

Fix migrator
Fixed the issue, and added a specific test to the error we were getting


Patch:
diff --git a/contracts/dao-core/schema/dao-core.json b/contracts/dao-core/schema/dao-core.json
index 97ccd889..96faa71f 100644
--- a/contracts/dao-core/schema/dao-core.json
+++ b/contracts/dao-core/schema/dao-core.json
@@ -2075,7 +2075,17 @@
           "proposal_params": {
             "type": "array",
             "items": {
-              "$ref": "#/definitions/ProposalParams"
+              "type": "array",
+              "items": [
+                {
+                  "type": "string"
+                },
+                {
+                  "$ref": "#/definitions/ProposalParams"
+                }
+              ],
+              "maxItems": 2,
+              "minItems": 2
             }
           }
         },
diff --git a/contracts/dao-core/src/migrate_msg.rs b/contracts/dao-core/src/migrate_msg.rs
index 64798298..955a1ff2 100644
--- a/contracts/dao-core/src/migrate_msg.rs
+++ b/contracts/dao-core/src/migrate_msg.rs
@@ -54,7 +54,7 @@ pub struct MigrationModuleParams {
     /// migration will be aborted.
     pub migrate_stake_cw20_manager: Option<bool>,
     // dao_proposal_single
-    pub proposal_params: Vec<ProposalParams>,
+    pub proposal_params: Vec<(String, ProposalParams)>,
 }
 
 #[cw_serde]
diff --git a/contracts/external/dao-migrator/src/contract.rs b/contracts/external/dao-migrator/src/contract.rs
index b57aa441..7229a841 100644
--- a/contracts/external/dao-migrator/src/contract.rs
+++ b/contracts/external/dao-migrator/src/contract.rs
@@ -1,4 +1,4 @@
-use std::env;
+use std::{collections::HashSet, env};
 
 #[cfg(not(feature = "library"))]
 use cosmwasm_std::entry_point;
@@ -87,19 +87,34 @@ fn execute_migration_v1_v2(
         return Err(ContractError::Unauthorized {});
     }
 
+    //Check if params doesn't have duplicates
+    let mut uniq = HashSet::new();
+    if !migration_params
+        .proposal_params
+        .iter()
+        .all(|(addr, _)| uniq.insert(addr))
+    {
+        return Err(ContractError::DuplicateProposalParams);
+    }
+
     // List of code ids pairs we got and the migration msg of each one of them.
-    let proposal_pairs: Vec<CodeIdPair> = migration_params
+    let proposal_pairs: Vec<(String, CodeIdPair)> = migration_params
         .proposal_params
         .iter()
-        .map(|proposal_params| {
-            CodeIdPair::new(
-                v1_code_ids.proposal_single,
-                v2_code_ids.proposal_single,
-                MigrationMsgs::DaoProposalSingle(dao_proposal_single::msg::MigrateMsg::FromV1 {
-                    close_proposal_on_execution_failure: proposal_params
-                        .close_proposal_on_execution_failure,
-                    pre_propose_info: proposal_params.pre_propose_info.clone(),
-                }),
+        .map(|(addr, proposal_params)| {
+            (
+                addr.clone(),
+                CodeIdPair::new(
+                    v1_code_ids.proposal_single,
+                    v2_code_ids.proposal_single,
+                    MigrationMsgs::DaoProposalSingle(
+                        dao_proposal_single::msg::MigrateMsg::FromV1 {
+                            close_proposal_on_execution_failure: proposal_params
+                                .close_proposal_on_execution_failure,
+                            pre_propose_info: proposal_params.pre_propose_info.clone(),
+                        },
+                    ),
+                ),
             )
         })
         .collect(); // cw-proposal-single -> dao_proposal_single
@@ -215,20 +230,30 @@ fn execute_migration_v1_v2(
         },
     )?;
 
-    // We add 1 because migration module is added last, and we skip it.
-    if proposal_modules.len() != (proposal_pairs.len() + 1) {
+    // We remove 1 because migration module is a proposal module, and we skip it.
+    if proposal_modules.len() - 1 != (proposal_pairs.len()) {
         return Err(ContractError::MigrationParamsNotEqualProposalModulesLength);
     }
 
     // Loop over proposals and verify that they are valid DAO DAO modules
     // and set them to be migrated.
-    proposal_modules.into_iter().enumerate().try_for_each(
-        |(proposal_index, module)| -> Result<(), ContractError> {
+    proposal_modules
+        .iter()
+        .try_for_each(|module| -> Result<(), ContractError> {
             // Instead of doing 2 loops, just ignore our module, we don't care about the vec after this.
             if module.address == env.contract.address {
                 return Ok(());
             }
 
+            let proposal_pair = proposal_pairs
+                .iter()
+                .find(|(addr, _)| addr == module.address.as_str())
+                .ok_or(ContractError::ProposalModuleNotFoundInParams {
+                    addr: module.address.clone().into(),
+                })?
+                .1
+                .clone();
+
             // Get the code id of the module
             let proposal_code_id = if let Ok(contract_info) = deps
                 .querier
@@ -243,16 +268,16 @@ fn execute_migration_v1_v2(
             }?;
 
             // check if Code id is valid DAO DAO code id
-            if proposal_code_id == proposal_pairs[proposal_index].v1_code_id {
+            if proposal_code_id == proposal_pair.v1_code_id {
                 msgs.push(
                     WasmMsg::Migrate {
                         contract_addr: module.address.to_string(),
-                        new_code_id: proposal_pairs[proposal_index].v2_code_id,
-                        msg: to_binary(&proposal_pairs[proposal_index].migrate_msg).unwrap(),
+                        new_code_id: proposal_pair.v2_code_id,
+                        msg: to_binary(&proposal_pair.migrate_msg).unwrap(),
                     }
                     .into(),
                 );
-                modules_addrs.proposals.push(module.address);
+                modules_addrs.proposals.push(module.address.clone());
                 Ok(())
             } else {
                 // Return false because we couldn't find the code id on our list.
@@ -262,8 +287,7 @@ fn execute_migration_v1_v2(
             }?;
 
             Ok(())
-        },
-    )?;
+        })?;
 
     // We successfully verified all modules of the DAO, we can send migration msgs.
 
diff --git a/contracts/external/dao-migrator/src/error.rs b/contracts/external/dao-migrator/src/error.rs
index 3f113101..c99a5cc9 100644
--- a/contracts/external/dao-migrator/src/error.rs
+++ b/contracts/external/dao-migrator/src/error.rs
@@ -34,9 +34,15 @@ pub enum ContractError {
     #[error("Failed to verify any DAO proposal single module address")]
     DaoProposalSingleNotFound,
 
+    #[error("We couldn't find the proposal modules in provided migration params: {addr}")]
+    ProposalModuleNotFoundInParams { addr: String },
+
     #[error("Failed to verify proposal in {module_addr}")]
     NoProposalsOnModule { module_addr: String },
 
+    #[error("Duplicate params found for the same module")]
+    DuplicateProposalParams,
+
     #[error("Proposal migration params length is not equal to proposal modules length")]
     MigrationParamsNotEqualProposalModulesLength,
 }
diff --git a/contracts/external/dao-migrator/src/types.rs b/contracts/external/dao-migrator/src/types.rs
index 960431de..cdb0adb9 100644
--- a/contracts/external/dao-migrator/src/types.rs
+++ b/contracts/external/dao-migrator/src/types.rs
@@ -57,7 +57,7 @@ pub struct MigrationParams {
     /// migration will be aborted.
     pub migrate_stake_cw20_manager: Option<bool>,
     // dao_proposal_single
-    pub proposal_params: Vec<ProposalParams>,
+    pub proposal_params: Vec<(String, ProposalParams)>,
 }
 
 /// Wrapper enum that helps us to hold different types of migration msgs
@@ -71,6 +71,7 @@ pub enum MigrationMsgs {
 }
 
 /// Module data we need for migrations and tests.
+#[derive(Clone)]
 pub struct CodeIdPair {
     /// The code id used in V1 module
     pub v1_code_id: u64,
diff --git a/typescript/contracts/dao-core/DaoCore.types.ts b/typescript/contracts/dao-core/DaoCore.types.ts
index 309a81ca..80856e91 100644
--- a/typescript/contracts/dao-core/DaoCore.types.ts
+++ b/typescript/contracts/dao-core/DaoCore.types.ts
@@ -366,7 +366,7 @@ export interface MigrateV1ToV2 {
 }
 export interface MigrationModuleParams {
   migrate_stake_cw20_manager?: boolean | null;
-  proposal_params: ProposalParams[];
+  proposal_params: [string, ProposalParams][];
 }
 export interface ProposalParams {
   close_proposal_on_execution_failure: boolean;


Test patch:
diff --git a/contracts/external/dao-migrator/src/testing/helpers.rs b/contracts/external/dao-migrator/src/testing/helpers.rs
index 20d81900..9e05af99 100644
--- a/contracts/external/dao-migrator/src/testing/helpers.rs
+++ b/contracts/external/dao-migrator/src/testing/helpers.rs
@@ -1,4 +1,7 @@
-use cosmwasm_std::{to_binary, Addr, Empty, Uint128, WasmMsg};
+use cosmwasm_std::{
+    to_binary, Addr, Binary, Deps, DepsMut, Empty, Env, MessageInfo, Response, StdResult, Uint128,
+    WasmMsg,
+};
 use cw_multi_test::{next_block, App, Contract, ContractWrapper, Executor};
 use dao_core::query::SubDao;
 use dao_testing::contracts::{
@@ -6,7 +9,10 @@ use dao_testing::contracts::{
     dao_core_contract, proposal_single_contract, v1_dao_core_contract, v1_proposal_single_contract,
 };
 
-use crate::types::{V1CodeIds, V2CodeIds};
+use crate::{
+    types::{V1CodeIds, V2CodeIds},
+    ContractError,
+};
 
 pub(crate) const SENDER_ADDR: &str = "creator";
 
@@ -26,7 +32,7 @@ pub struct ExecuteParams {
     pub migrate_cw20: Option<bool>,
 }
 
-#[derive(Clone)]
+#[derive(Clone, Debug)]
 pub struct ModuleAddrs {
     pub core: Addr,
     pub proposals: Vec<Addr>,
@@ -320,3 +326,27 @@ pub fn dao_voting_cw4_contract() -> Box<dyn Contract<Empty>> {
     .with_migrate(dao_voting_cw4::contract::migrate);
     Box::new(contract)
 }
+
+fn some_init(
+    _deps: DepsMut,
+    _env: Env,
+    _info: MessageInfo,
+    _msg: (),
+) -> Result<Response, ContractError> {
+    Ok(Response::default())
+}
+fn some_execute(
+    _deps: DepsMut,
+    _env: Env,
+    _info: MessageInfo,
+    _msg: (),
+) -> Result<Response, ContractError> {
+    Ok(Response::default())
+}
+fn some_query(_deps: Deps, _env: Env, _msg: ()) -> StdResult<Binary> {
+    Ok(Binary::default())
+}
+
+pub fn demo_contract() -> Box<dyn Contract<Empty>> {
+    Box::new(ContractWrapper::new(some_execute, some_init, some_query))
+}
diff --git a/contracts/external/dao-migrator/src/testing/setup.rs b/contracts/external/dao-migrator/src/testing/setup.rs
index 6f372b32..f0d0bcec 100644
--- a/contracts/external/dao-migrator/src/testing/setup.rs
+++ b/contracts/external/dao-migrator/src/testing/setup.rs
@@ -228,7 +228,7 @@ pub fn execute_migration(
     module_addrs: &ModuleAddrs,
     v1_code_ids: V1CodeIds,
     params: Option<ExecuteParams>,
-    multiple_proposals: bool,
+    custom_proposal_params: Option<Vec<(String, ProposalParams)>>,
 ) -> Result<AppResponse, anyhow::Error> {
     let sender = Addr::unchecked(SENDER_ADDR);
     let migrator_code_id = app.store_code(migrator_contract());
@@ -237,17 +237,25 @@ pub fn execute_migration(
         sub_daos: Some(vec![]),
         migrate_cw20: Some(true),
     });
-    let mut proposal_params = vec![ProposalParams {
-        close_proposal_on_execution_failure: true,
-        pre_propose_info: dao_voting::pre_propose::PreProposeInfo::AnyoneMayPropose {},
-    }];
-
-    if multiple_proposals {
-        proposal_params.push(ProposalParams {
-            close_proposal_on_execution_failure: true,
-            pre_propose_info: dao_voting::pre_propose::PreProposeInfo::AnyoneMayPropose {},
-        })
-    }
+
+    let proposal_params = if let Some(params) = custom_proposal_params {
+        params
+    } else {
+        module_addrs
+            .proposals
+            .iter()
+            .map(|addr| {
+                (
+                    addr.clone().into(),
+                    ProposalParams {
+                        close_proposal_on_execution_failure: true,
+                        pre_propose_info:
+                            dao_voting::pre_propose::PreProposeInfo::AnyoneMayPropose {},
+                    },
+                )
+            })
+            .collect::<Vec<(String, ProposalParams)>>()
+    };
 
     app.execute_contract(
         sender.clone(),
@@ -332,7 +340,6 @@ pub fn execute_migration_from_core(
     module_addrs: &ModuleAddrs,
     v1_code_ids: V1CodeIds,
     params: Option<ExecuteParams>,
-    multiple_proposals: bool,
 ) -> Result<AppResponse, anyhow::Error> {
     let sender = Addr::unchecked(SENDER_ADDR);
     let migrator_code_id = app.store_code(migrator_contract());
@@ -341,17 +348,20 @@ pub fn execute_migration_from_core(
         sub_daos: Some(vec![]),
         migrate_cw20: Some(true),
     });
-    let mut proposal_params = vec![dao_core::migrate_msg::ProposalParams {
-        close_proposal_on_execution_failure: true,
-        pre_propose_info: dao_core::migrate_msg::PreProposeInfo::AnyoneMayPropose {},
-    }];
-
-    if multiple_proposals {
-        proposal_params.push(dao_core::migrate_msg::ProposalParams {
-            close_proposal_on_execution_failure: true,
-            pre_propose_info: dao_core::migrate_msg::PreProposeInfo::AnyoneMayPropose {},
+
+    let proposal_params = module_addrs
+        .proposals
+        .iter()
+        .map(|addr| {
+            (
+                addr.clone().into(),
+                dao_core::migrate_msg::ProposalParams {
+                    close_proposal_on_execution_failure: true,
+                    pre_propose_info: dao_core::migrate_msg::PreProposeInfo::AnyoneMayPropose {},
+                },
+            )
         })
-    }
+        .collect::<Vec<(String, dao_core::migrate_msg::ProposalParams)>>();
 
     app.execute_contract(
         sender.clone(),
diff --git a/contracts/external/dao-migrator/src/testing/test_migration.rs b/contracts/external/dao-migrator/src/testing/test_migration.rs
index 20346f6e..228602bc 100644
--- a/contracts/external/dao-migrator/src/testing/test_migration.rs
+++ b/contracts/external/dao-migrator/src/testing/test_migration.rs
@@ -1,5 +1,7 @@
 use std::borrow::BorrowMut;
 
+use cosmwasm_std::Addr;
+use cw_multi_test::Executor;
 use dao_core::{query::SubDao, state::ProposalModuleStatus};
 
 use crate::{
@@ -11,10 +13,11 @@ use crate::{
             query_state_v1_cw20, query_state_v1_cw4, query_state_v2_cw20, query_state_v2_cw4,
         },
     },
+    types::ProposalParams,
     ContractError,
 };
 
-use super::setup::setup_dao_v1_multiple_proposals;
+use super::{helpers::demo_contract, setup::setup_dao_v1_multiple_proposals};
 
 pub fn basic_test(voting_type: VotingType, from_core: bool) {
     let (mut app, module_addrs, v1_code_ids) = setup_dao_v1(voting_type.clone());
@@ -36,11 +39,10 @@ pub fn basic_test(voting_type: VotingType, from_core: bool) {
 
     match from_core {
         true => {
-            execute_migration_from_core(app.borrow_mut(), &module_addrs, v1_code_ids, None, false)
-                .unwrap()
+            execute_migration_from_core(app.borrow_mut(), &module_addrs, v1_code_ids, None).unwrap()
         }
         false => {
-            execute_migration(app.borrow_mut(), &module_addrs, v1_code_ids, None, false).unwrap()
+            execute_migration(app.borrow_mut(), &module_addrs, v1_code_ids, None, None).unwrap()
         }
     };
 
@@ -85,6 +87,62 @@ fn test_execute_migration() {
     basic_test(VotingType::Cw4, true);
 }
 
+#[test]
+fn test_migrator_address_is_first() {
+    let (mut app, module_addrs, v1_code_ids) = setup_dao_v1(VotingType::Cw20);
+
+    // We init some demo contracts so we can bump the contract addr to "contract1X"
+    // That way, when we do a migration, the newely created migrator contract address
+    // will be "contract11", because the proposal module address is "contract4"
+    // when we query the dao for "ProposalModules", the migrator address
+    // will appear first in the list ("contract11" < "contract4")
+    let demo_code_id = app.store_code(demo_contract());
+    for _ in 0..6 {
+        app.instantiate_contract(
+            demo_code_id,
+            Addr::unchecked("some"),
+            &(),
+            &[],
+            "demo",
+            None,
+        )
+        .unwrap();
+    }
+
+    let mut test_state_v1 = query_state_v1_cw20(
+        &mut app,
+        module_addrs.proposals[0].clone(),
+        module_addrs.voting.clone(),
+    );
+    //NOTE: We add 1 to count because we create a new proposal in execute_migration
+    test_state_v1.proposal_count += 1;
+
+    execute_migration(app.borrow_mut(), &module_addrs, v1_code_ids, None, None).unwrap();
+
+    let test_state_v2 = query_state_v2_cw20(
+        &mut app,
+        module_addrs.proposals[0].clone(),
+        module_addrs.voting,
+    );
+
+    assert_eq!(test_state_v1, test_state_v2);
+
+    let modules: Vec<dao_core::state::ProposalModule> = app
+        .wrap()
+        .query_wasm_smart(
+            module_addrs.core,
+            &dao_core::msg::QueryMsg::ProposalModules {
+                start_after: None,
+                limit: None,
+            },
+        )
+        .unwrap();
+
+    assert_eq!(modules.len(), 2);
+    assert_eq!(modules[1].address, module_addrs.proposals[0]); // proposal module
+    assert_eq!(modules[0].status, ProposalModuleStatus::Disabled); // migrator module
+}
+
 #[test]
 fn test_multiple_proposal_modules() {
     let (mut app, module_addrs, v1_code_ids) = setup_dao_v1_multiple_proposals();
@@ -97,7 +155,7 @@ fn test_multiple_proposal_modules() {
     //NOTE: We add 1 to count because we create a new proposal in execute_migration
     test_state_v1.proposal_count += 1;
 
-    execute_migration(app.borrow_mut(), &module_addrs, v1_code_ids, None, true).unwrap();
+    execute_migration(app.borrow_mut(), &module_addrs, v1_code_ids, None, None).unwrap();
 
     let test_state_v2 = query_state_v2_cw20(
         &mut app,
@@ -108,12 +166,52 @@ fn test_multiple_proposal_modules() {
     assert_eq!(test_state_v1, test_state_v2);
 }
 
+#[test]
+fn test_duplicate_proposal_params() {
+    let (mut app, module_addrs, v1_code_ids) = setup_dao_v1_multiple_proposals();
+
+    // 2 pararms with the same addr
+    let custom_params = vec![
+        (
+            module_addrs.proposals[0].to_string(),
+            ProposalParams {
+                close_proposal_on_execution_failure: true,
+                pre_propose_info: dao_voting::pre_propose::PreProposeInfo::AnyoneMayPropose {},
+            },
+        ),
+        (
+            module_addrs.proposals[0].to_string(),
+            ProposalParams {
+                close_proposal_on_execution_failure: true,
+                pre_propose_info: dao_voting::pre_propose::PreProposeInfo::AnyoneMayPropose {},
+            },
+        ),
+    ];
+
+    let err = execute_migration(
+        app.borrow_mut(),
+        &module_addrs,
+        v1_code_ids,
+        None,
+        Some(custom_params),
+    )
+    .unwrap_err()
+    .downcast::<ContractError>()
+    .unwrap();
+
+    assert_eq!(err, ContractError::DuplicateProposalParams)
+}
+
 #[test]
 fn test_multiple_proposal_modules_failing() {
     // Test single proposal with multiple proposal params.
-    let (mut app, module_addrs, v1_code_ids) = setup_dao_v1(VotingType::Cw20);
+    let (mut app, mut module_addrs, v1_code_ids) = setup_dao_v1(VotingType::Cw20);
 
-    let err = execute_migration(app.borrow_mut(), &module_addrs, v1_code_ids, None, true)
+    // `module_addrs.proposals` is only used to set migration params based on how many proposals we have here
+    // Its safe to add/remove 2nd proposal in this tests because the actual proposals are taken within the contract
+    // and are not provided externally
+    module_addrs.proposals.push(Addr::unchecked("proposal2"));
+    let err = execute_migration(app.borrow_mut(), &module_addrs, v1_code_ids, None, None)
         .unwrap_err()
         .downcast::<ContractError>()
         .unwrap();
@@ -123,9 +221,10 @@ fn test_multiple_proposal_modules_failing() {
     );
 
     // Test multiple proposals with single proposal params.
-    let (mut app, module_addrs, v1_code_ids) = setup_dao_v1_multiple_proposals();
+    let (mut app, mut module_addrs, v1_code_ids) = setup_dao_v1_multiple_proposals();
 
-    let err = execute_migration(app.borrow_mut(), &module_addrs, v1_code_ids, None, false)
+    module_addrs.proposals.remove(1);
+    let err = execute_migration(app.borrow_mut(), &module_addrs, v1_code_ids, None, None)
         .unwrap_err()
         .downcast::<ContractError>()
         .unwrap();
@@ -140,7 +239,7 @@ fn test_wrong_code_id() {
     let (mut app, module_addrs, mut v1_code_ids) = setup_dao_v1(VotingType::Cw20);
     let old_v1_code_ids = v1_code_ids.clone();
     v1_code_ids.proposal_single = 555;
-    let err = execute_migration(app.borrow_mut(), &module_addrs, v1_code_ids, None, false)
+    let err = execute_migration(app.borrow_mut(), &module_addrs, v1_code_ids, None, None)
         .unwrap_err()
         .downcast::<ContractError>()
         .unwrap();
@@ -154,7 +253,7 @@ fn test_wrong_code_id() {
     let (mut app, module_addrs, mut v1_code_ids) = setup_dao_v1(VotingType::Cw20);
     let old_v1_code_ids = v1_code_ids.clone();
     v1_code_ids.cw20_stake = 555;
-    let err = execute_migration(app.borrow_mut(), &module_addrs, v1_code_ids, None, false)
+    let err = execute_migration(app.borrow_mut(), &module_addrs, v1_code_ids, None, None)
         .unwrap_err()
         .downcast::<ContractError>()
         .unwrap();
@@ -167,7 +266,7 @@ fn test_wrong_code_id() {
 
     let (mut app, module_addrs, mut v1_code_ids) = setup_dao_v1(VotingType::Cw20);
     v1_code_ids.cw20_staked_balances_voting = 555;
-    let err = execute_migration(app.borrow_mut(), &module_addrs, v1_code_ids, None, false)
+    let err = execute_migration(app.borrow_mut(), &module_addrs, v1_code_ids, None, None)
         .unwrap_err()
         .downcast::<ContractError>()
         .unwrap();
@@ -175,7 +274,7 @@ fn test_wrong_code_id() {
 
     let (mut app, module_addrs, mut v1_code_ids) = setup_dao_v1(VotingType::Cw4);
     v1_code_ids.cw4_voting = 555;
-    let err = execute_migration(app.borrow_mut(), &module_addrs, v1_code_ids, None, false)
+    let err = execute_migration(app.borrow_mut(), &module_addrs, v1_code_ids, None, None)
         .unwrap_err()
         .downcast::<ContractError>()
         .unwrap();
@@ -194,7 +293,7 @@ fn test_dont_migrate_cw20() {
             sub_daos: Some(vec![]),
             migrate_cw20: None,
         }),
-        false,
+        None,
     )
     .unwrap_err()
     .downcast::<ContractError>()
@@ -209,7 +308,7 @@ fn test_dont_migrate_cw20() {
             sub_daos: Some(vec![]),
             migrate_cw20: Some(false),
         }),
-        false,
+        None,
     )
     .unwrap_err()
     .downcast::<ContractError>()
@@ -233,7 +332,7 @@ fn test_sub_daos() {
             sub_daos: Some(vec![sub_dao.clone()]),
             migrate_cw20: Some(true),
         }),
-        false,
+        None,
     )
     .unwrap();