diff --git a/contracts/cw4-group/src/contract.rs b/contracts/cw4-group/src/contract.rs index fe6270655..68bd654ae 100644 --- a/contracts/cw4-group/src/contract.rs +++ b/contracts/cw4-group/src/contract.rs @@ -13,6 +13,7 @@ use cw_storage_plus::Bound; use cw_utils::maybe_addr; use crate::error::ContractError; +use crate::helpers::validate_unique_members; use crate::msg::{ExecuteMsg, InstantiateMsg, QueryMsg}; use crate::state::{ADMIN, HOOKS, MEMBERS, TOTAL}; @@ -39,9 +40,12 @@ pub fn instantiate( pub fn create( mut deps: DepsMut, admin: Option, - members: Vec, + mut members: Vec, height: u64, ) -> Result<(), ContractError> { + validate_unique_members(&mut members)?; + let members = members; // let go of mutability + let admin_addr = admin .map(|admin| deps.api.addr_validate(&admin)) .transpose()?; @@ -116,9 +120,12 @@ pub fn update_members( deps: DepsMut, height: u64, sender: Addr, - to_add: Vec, + mut to_add: Vec, to_remove: Vec, ) -> Result { + validate_unique_members(&mut to_add)?; + let to_add = to_add; // let go of mutability + ADMIN.assert_admin(deps.as_ref(), &sender)?; let mut total = Uint64::from(TOTAL.load(deps.storage)?); diff --git a/contracts/cw4-group/src/error.rs b/contracts/cw4-group/src/error.rs index a5d2f57cf..bd5acb30f 100644 --- a/contracts/cw4-group/src/error.rs +++ b/contracts/cw4-group/src/error.rs @@ -19,4 +19,7 @@ pub enum ContractError { #[error("Unauthorized")] Unauthorized {}, + + #[error("Message contained duplicate member: {member}")] + DuplicateMember { member: String }, } diff --git a/contracts/cw4-group/src/helpers.rs b/contracts/cw4-group/src/helpers.rs index b7719d90b..9611ed9d6 100644 --- a/contracts/cw4-group/src/helpers.rs +++ b/contracts/cw4-group/src/helpers.rs @@ -4,7 +4,7 @@ use cosmwasm_schema::cw_serde; use cosmwasm_std::{to_binary, Addr, CosmosMsg, StdResult, WasmMsg}; use cw4::{Cw4Contract, Member}; -use crate::msg::ExecuteMsg; +use crate::{msg::ExecuteMsg, ContractError}; /// Cw4GroupContract is a wrapper around Cw4Contract that provides a lot of helpers /// for working with cw4-group contracts. @@ -40,3 +40,17 @@ impl Cw4GroupContract { self.encode_msg(msg) } } + +/// Sorts the slice and verifies all member addresses are unique. +pub fn validate_unique_members(members: &mut [Member]) -> Result<(), ContractError> { + members.sort_by(|a, b| a.addr.cmp(&b.addr)); + for (a, b) in members.iter().zip(members.iter().skip(1)) { + if a.addr == b.addr { + return Err(ContractError::DuplicateMember { + member: a.addr.clone(), + }); + } + } + + Ok(()) +} diff --git a/contracts/cw4-group/src/tests.rs b/contracts/cw4-group/src/tests.rs index ca542efa1..9ed220523 100644 --- a/contracts/cw4-group/src/tests.rs +++ b/contracts/cw4-group/src/tests.rs @@ -8,13 +8,14 @@ use crate::contract::{ }; use crate::msg::{ExecuteMsg, InstantiateMsg}; use crate::state::{ADMIN, HOOKS}; +use crate::ContractError; const INIT_ADMIN: &str = "juan"; const USER1: &str = "somebody"; const USER2: &str = "else"; const USER3: &str = "funny"; -fn do_instantiate(deps: DepsMut) { +fn set_up(deps: DepsMut) { let msg = InstantiateMsg { admin: Some(INIT_ADMIN.into()), members: vec![ @@ -35,7 +36,7 @@ fn do_instantiate(deps: DepsMut) { #[test] fn proper_instantiation() { let mut deps = mock_dependencies(); - do_instantiate(deps.as_mut()); + set_up(deps.as_mut()); // it worked, let's query the state let res = ADMIN.query_admin(deps.as_ref()).unwrap(); @@ -48,7 +49,7 @@ fn proper_instantiation() { #[test] fn try_member_queries() { let mut deps = mock_dependencies(); - do_instantiate(deps.as_mut()); + set_up(deps.as_mut()); let member1 = query_member(deps.as_ref(), USER1.into(), None).unwrap(); assert_eq!(member1.weight, Some(11)); @@ -64,6 +65,72 @@ fn try_member_queries() { // TODO: assert the set is proper } +#[test] +fn duplicate_members_instantiation() { + let mut deps = mock_dependencies(); + + let msg = InstantiateMsg { + admin: Some(INIT_ADMIN.into()), + members: vec![ + Member { + addr: USER1.into(), + weight: 5, + }, + Member { + addr: USER2.into(), + weight: 6, + }, + Member { + addr: USER1.into(), + weight: 6, + }, + ], + }; + let info = mock_info("creator", &[]); + let err = instantiate(deps.as_mut(), mock_env(), info, msg).unwrap_err(); + assert_eq!( + err, + ContractError::DuplicateMember { + member: USER1.to_string() + } + ); +} + +#[test] +fn duplicate_members_execution() { + let mut deps = mock_dependencies(); + + set_up(deps.as_mut()); + + let add = vec![ + Member { + addr: USER3.into(), + weight: 15, + }, + Member { + addr: USER3.into(), + weight: 11, + }, + ]; + + let height = mock_env().block.height; + let err = update_members( + deps.as_mut(), + height + 5, + Addr::unchecked(INIT_ADMIN), + add, + vec![], + ) + .unwrap_err(); + + assert_eq!( + err, + ContractError::DuplicateMember { + member: USER3.to_string() + } + ); +} + fn assert_users( deps: &OwnedDeps, user1_weight: Option, @@ -99,7 +166,7 @@ fn assert_users( #[test] fn add_new_remove_old_member() { let mut deps = mock_dependencies(); - do_instantiate(deps.as_mut()); + set_up(deps.as_mut()); // add a new one and remove existing one let add = vec![Member { @@ -148,7 +215,7 @@ fn add_new_remove_old_member() { fn add_old_remove_new_member() { // add will over-write and remove have no effect let mut deps = mock_dependencies(); - do_instantiate(deps.as_mut()); + set_up(deps.as_mut()); // add a new one and remove existing one let add = vec![Member { @@ -174,7 +241,7 @@ fn add_old_remove_new_member() { fn add_and_remove_same_member() { // add will over-write and remove have no effect let mut deps = mock_dependencies(); - do_instantiate(deps.as_mut()); + set_up(deps.as_mut()); // USER1 is updated and remove in the same call, we should remove this an add member3 let add = vec![ @@ -206,7 +273,7 @@ fn add_and_remove_same_member() { fn add_remove_hooks() { // add will over-write and remove have no effect let mut deps = mock_dependencies(); - do_instantiate(deps.as_mut()); + set_up(deps.as_mut()); let hooks = HOOKS.query_hooks(deps.as_ref()).unwrap(); assert!(hooks.hooks.is_empty()); @@ -274,7 +341,7 @@ fn add_remove_hooks() { #[test] fn hooks_fire() { let mut deps = mock_dependencies(); - do_instantiate(deps.as_mut()); + set_up(deps.as_mut()); let hooks = HOOKS.query_hooks(deps.as_ref()).unwrap(); assert!(hooks.hooks.is_empty()); @@ -317,14 +384,18 @@ fn hooks_fire() { // ensure 2 messages for the 2 hooks assert_eq!(res.messages.len(), 2); // same order as in the message (adds first, then remove) + // order of added users is not guaranteed to be preserved let diffs = vec![ - MemberDiff::new(USER1, Some(11), Some(20)), MemberDiff::new(USER3, None, Some(5)), + MemberDiff::new(USER1, Some(11), Some(20)), MemberDiff::new(USER2, Some(6), None), ]; let hook_msg = MemberChangedHookMsg { diffs }; let msg1 = SubMsg::new(hook_msg.clone().into_cosmos_msg(contract1).unwrap()); let msg2 = SubMsg::new(hook_msg.into_cosmos_msg(contract2).unwrap()); + dbg!(&res.messages); + dbg!(&msg1); + dbg!(&msg2); assert_eq!(res.messages, vec![msg1, msg2]); } @@ -332,7 +403,7 @@ fn hooks_fire() { fn raw_queries_work() { // add will over-write and remove have no effect let mut deps = mock_dependencies(); - do_instantiate(deps.as_mut()); + set_up(deps.as_mut()); // get total from raw key let total_raw = deps.storage.get(TOTAL_KEY.as_bytes()).unwrap(); @@ -352,7 +423,7 @@ fn raw_queries_work() { #[test] fn total_at_height() { let mut deps = mock_dependencies(); - do_instantiate(deps.as_mut()); + set_up(deps.as_mut()); let height = mock_env().block.height;