From 9b8102ab490d63205b4856b2e38e16a3d8123b0c Mon Sep 17 00:00:00 2001 From: codehans <94654388+codehans@users.noreply.github.com> Date: Wed, 13 Apr 2022 15:34:57 +0100 Subject: [PATCH 1/6] Handle duplicate members in cw4-group create --- contracts/cw4-group/src/contract.rs | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/contracts/cw4-group/src/contract.rs b/contracts/cw4-group/src/contract.rs index 913d427f6..d7952d9c3 100644 --- a/contracts/cw4-group/src/contract.rs +++ b/contracts/cw4-group/src/contract.rs @@ -51,7 +51,12 @@ pub fn create( for member in members.into_iter() { total += member.weight; let member_addr = deps.api.addr_validate(&member.addr)?; - MEMBERS.save(deps.storage, &member_addr, &member.weight, height)?; + MEMBERS.update(deps.storage, &member_addr, height, |v| -> StdResult { + match v { + Some(weight) => Ok(weight + member.weight), + None => Ok(member.weight), + } + })?; } TOTAL.save(deps.storage, &total)?; @@ -229,7 +234,11 @@ mod tests { }, Member { addr: USER2.into(), - weight: 6, + weight: 2, + }, + Member { + addr: USER2.into(), + weight: 4, }, ], }; From 0049fa7e6fbb86bf7fee9c746e269314ac536efe Mon Sep 17 00:00:00 2001 From: Tomasz Kurcz Date: Wed, 12 Oct 2022 19:15:47 +0200 Subject: [PATCH 2/6] cw4-group: regression test for duplicate members --- contracts/cw4-group/src/tests.rs | 69 +++++++++++++++++++++++++++----- 1 file changed, 59 insertions(+), 10 deletions(-) diff --git a/contracts/cw4-group/src/tests.rs b/contracts/cw4-group/src/tests.rs index ca542efa1..212438458 100644 --- a/contracts/cw4-group/src/tests.rs +++ b/contracts/cw4-group/src/tests.rs @@ -14,7 +14,7 @@ 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![ @@ -32,10 +32,19 @@ fn do_instantiate(deps: DepsMut) { instantiate(deps, mock_env(), info, msg).unwrap(); } +fn set_up_with_members(deps: DepsMut, members: impl Into>) { + let msg = InstantiateMsg { + admin: Some(INIT_ADMIN.into()), + members: members.into(), + }; + let info = mock_info("creator", &[]); + instantiate(deps, mock_env(), info, msg).unwrap(); +} + #[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 +57,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 +73,46 @@ fn try_member_queries() { // TODO: assert the set is proper } +#[test] +fn duplicate_members() { + let mut deps = mock_dependencies(); + + set_up_with_members( + deps.as_mut(), + [ + Member { + addr: USER1.into(), + weight: 5, + }, + Member { + addr: USER2.into(), + weight: 6, + }, + Member { + addr: USER1.into(), + weight: 6, + }, + ], + ); + + let res = query_total_weight(deps.as_ref(), None).unwrap(); + assert_eq!(17, res.weight); + + assert_eq!( + query_member(deps.as_ref(), USER1.into(), None) + .unwrap() + .weight, + Some(11) + ); + + assert_eq!( + query_member(deps.as_ref(), USER2.into(), None) + .unwrap() + .weight, + Some(6) + ); +} + fn assert_users( deps: &OwnedDeps, user1_weight: Option, @@ -99,7 +148,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 +197,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 +223,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 +255,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 +323,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()); @@ -332,7 +381,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 +401,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; From 7b7be21d6d1c92bbe561d031ab8c61ee0b157676 Mon Sep 17 00:00:00 2001 From: Tomasz Kurcz Date: Wed, 12 Oct 2022 19:41:26 +0200 Subject: [PATCH 3/6] cw4-group: update duplicate member tests --- contracts/cw4-group/src/error.rs | 3 ++ contracts/cw4-group/src/tests.rs | 68 ++++++++++++++++++++------------ 2 files changed, 46 insertions(+), 25 deletions(-) diff --git a/contracts/cw4-group/src/error.rs b/contracts/cw4-group/src/error.rs index a5d2f57cf..83016d3ce 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}")] + DuplicateMembers { member: String }, } diff --git a/contracts/cw4-group/src/tests.rs b/contracts/cw4-group/src/tests.rs index 212438458..9bd483586 100644 --- a/contracts/cw4-group/src/tests.rs +++ b/contracts/cw4-group/src/tests.rs @@ -8,6 +8,7 @@ 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"; @@ -32,15 +33,6 @@ fn set_up(deps: DepsMut) { instantiate(deps, mock_env(), info, msg).unwrap(); } -fn set_up_with_members(deps: DepsMut, members: impl Into>) { - let msg = InstantiateMsg { - admin: Some(INIT_ADMIN.into()), - members: members.into(), - }; - let info = mock_info("creator", &[]); - instantiate(deps, mock_env(), info, msg).unwrap(); -} - #[test] fn proper_instantiation() { let mut deps = mock_dependencies(); @@ -74,12 +66,12 @@ fn try_member_queries() { } #[test] -fn duplicate_members() { +fn duplicate_members_instantiation() { let mut deps = mock_dependencies(); - set_up_with_members( - deps.as_mut(), - [ + let msg = InstantiateMsg { + admin: Some(INIT_ADMIN.into()), + members: vec![ Member { addr: USER1.into(), weight: 5, @@ -93,23 +85,49 @@ fn duplicate_members() { weight: 6, }, ], + }; + let info = mock_info("creator", &[]); + let err = instantiate(deps.as_mut(), mock_env(), info, msg).unwrap_err(); + assert_eq!( + err, + ContractError::DuplicateMembers { + member: USER1.to_string() + } ); +} - let res = query_total_weight(deps.as_ref(), None).unwrap(); - assert_eq!(17, res.weight); +#[test] +fn duplicate_members_execution() { + let mut deps = mock_dependencies(); - assert_eq!( - query_member(deps.as_ref(), USER1.into(), None) - .unwrap() - .weight, - Some(11) - ); + 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!( - query_member(deps.as_ref(), USER2.into(), None) - .unwrap() - .weight, - Some(6) + err, + ContractError::DuplicateMembers { + member: USER3.to_string() + } ); } From 11845328fcbb6a08a0e71b5912fd2b9a2af29147 Mon Sep 17 00:00:00 2001 From: Tomasz Kurcz Date: Wed, 12 Oct 2022 20:28:12 +0200 Subject: [PATCH 4/6] cw4-group: throw error on duplicate additions --- contracts/cw4-group/src/contract.rs | 11 +++++++++-- contracts/cw4-group/src/helpers.rs | 16 +++++++++++++++- contracts/cw4-group/src/tests.rs | 6 +++++- 3 files changed, 29 insertions(+), 4 deletions(-) diff --git a/contracts/cw4-group/src/contract.rs b/contracts/cw4-group/src/contract.rs index d2111a47e..711b6b655 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()?; @@ -122,9 +126,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/helpers.rs b/contracts/cw4-group/src/helpers.rs index b7719d90b..10e3edf99 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::DuplicateMembers { + member: a.addr.clone(), + }); + } + } + + Ok(()) +} diff --git a/contracts/cw4-group/src/tests.rs b/contracts/cw4-group/src/tests.rs index 9bd483586..d18189d9c 100644 --- a/contracts/cw4-group/src/tests.rs +++ b/contracts/cw4-group/src/tests.rs @@ -384,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]); } From 050f3536305b8c69d0e2508da31f7f66f4b5d628 Mon Sep 17 00:00:00 2001 From: Tomasz Kurcz Date: Wed, 12 Oct 2022 20:29:13 +0200 Subject: [PATCH 5/6] style --- contracts/cw4-group/src/error.rs | 2 +- contracts/cw4-group/src/helpers.rs | 2 +- contracts/cw4-group/src/tests.rs | 4 ++-- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/contracts/cw4-group/src/error.rs b/contracts/cw4-group/src/error.rs index 83016d3ce..bd5acb30f 100644 --- a/contracts/cw4-group/src/error.rs +++ b/contracts/cw4-group/src/error.rs @@ -21,5 +21,5 @@ pub enum ContractError { Unauthorized {}, #[error("Message contained duplicate member: {member}")] - DuplicateMembers { member: String }, + DuplicateMember { member: String }, } diff --git a/contracts/cw4-group/src/helpers.rs b/contracts/cw4-group/src/helpers.rs index 10e3edf99..9611ed9d6 100644 --- a/contracts/cw4-group/src/helpers.rs +++ b/contracts/cw4-group/src/helpers.rs @@ -46,7 +46,7 @@ pub fn validate_unique_members(members: &mut [Member]) -> Result<(), ContractErr 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::DuplicateMembers { + return Err(ContractError::DuplicateMember { member: a.addr.clone(), }); } diff --git a/contracts/cw4-group/src/tests.rs b/contracts/cw4-group/src/tests.rs index d18189d9c..9ed220523 100644 --- a/contracts/cw4-group/src/tests.rs +++ b/contracts/cw4-group/src/tests.rs @@ -90,7 +90,7 @@ fn duplicate_members_instantiation() { let err = instantiate(deps.as_mut(), mock_env(), info, msg).unwrap_err(); assert_eq!( err, - ContractError::DuplicateMembers { + ContractError::DuplicateMember { member: USER1.to_string() } ); @@ -125,7 +125,7 @@ fn duplicate_members_execution() { assert_eq!( err, - ContractError::DuplicateMembers { + ContractError::DuplicateMember { member: USER3.to_string() } ); From 9bb42edc06da4c88e9977657201be130a6c8e44d Mon Sep 17 00:00:00 2001 From: Tomasz Kurcz Date: Wed, 12 Oct 2022 20:35:16 +0200 Subject: [PATCH 6/6] cw4-group: remove unnecessary update --- contracts/cw4-group/src/contract.rs | 6 ------ 1 file changed, 6 deletions(-) diff --git a/contracts/cw4-group/src/contract.rs b/contracts/cw4-group/src/contract.rs index 711b6b655..68bd654ae 100644 --- a/contracts/cw4-group/src/contract.rs +++ b/contracts/cw4-group/src/contract.rs @@ -56,12 +56,6 @@ pub fn create( let member_weight = Uint64::from(member.weight); total = total.checked_add(member_weight)?; let member_addr = deps.api.addr_validate(&member.addr)?; - MEMBERS.update(deps.storage, &member_addr, height, |v| -> StdResult { - match v { - Some(weight) => Ok(weight + member_weight.u64()), - None => Ok(member_weight.u64()), - } - })?; MEMBERS.save(deps.storage, &member_addr, &member_weight.u64(), height)?; } TOTAL.save(deps.storage, &total.u64(), height)?;