diff --git a/contracts/pre-propose/dao-pre-propose-approval-multiple/src/contract.rs b/contracts/pre-propose/dao-pre-propose-approval-multiple/src/contract.rs index 9a7b8f731..eb5a08bb1 100644 --- a/contracts/pre-propose/dao-pre-propose-approval-multiple/src/contract.rs +++ b/contracts/pre-propose/dao-pre-propose-approval-multiple/src/contract.rs @@ -125,6 +125,8 @@ pub fn execute_propose( Ok(SubMsg::new(execute_msg)) })?; + let approver = APPROVER.load(deps.storage)?; + // Save the proposal and its information as pending. PENDING_PROPOSALS.save( deps.storage, @@ -132,6 +134,7 @@ pub fn execute_propose( &Proposal { status: ApprovalProposalStatus::Pending {}, approval_id, + approver: approver.clone(), proposer: info.sender, msg: propose_msg_internal, deposit: config.deposit_info, @@ -142,7 +145,8 @@ pub fn execute_propose( .add_messages(deposit_messages) .add_submessages(hooks_msgs) .add_attribute("method", "pre-propose") - .add_attribute("id", approval_id.to_string())) + .add_attribute("id", approval_id.to_string()) + .add_attribute("approver", approver.to_string())) } pub fn execute_approve( @@ -150,16 +154,15 @@ pub fn execute_approve( info: MessageInfo, id: u64, ) -> Result { - // Check sender is the approver - let approver = APPROVER.load(deps.storage)?; - if approver != info.sender { - return Err(PreProposeError::Unauthorized {}); - } - // Load proposal and send propose message to the proposal module let proposal = PENDING_PROPOSALS.may_load(deps.storage, id)?; match proposal { Some(proposal) => { + // Check sender is the approver + if proposal.approver != info.sender { + return Err(PreProposeError::Unauthorized {}); + } + let proposal_module = PrePropose::default().proposal_module.load(deps.storage)?; // Snapshot the deposit for the proposal that we're about @@ -188,6 +191,7 @@ pub fn execute_approve( created_proposal_id: proposal_id, }, approval_id: proposal.approval_id, + approver: proposal.approver, proposer: proposal.proposer, msg: proposal.msg, deposit: proposal.deposit, @@ -211,14 +215,9 @@ pub fn execute_reject( info: MessageInfo, id: u64, ) -> Result { - // Check sender is the approver - let approver = APPROVER.load(deps.storage)?; - if approver != info.sender { - return Err(PreProposeError::Unauthorized {}); - } - let Proposal { approval_id, + approver, proposer, msg, deposit, @@ -227,12 +226,18 @@ pub fn execute_reject( .may_load(deps.storage, id)? .ok_or(PreProposeError::ProposalNotFound {})?; + // Check sender is the approver + if approver != info.sender { + return Err(PreProposeError::Unauthorized {}); + } + COMPLETED_PROPOSALS.save( deps.storage, id, &Proposal { status: ApprovalProposalStatus::Rejected {}, approval_id, + approver, proposer: proposer.clone(), msg: msg.clone(), deposit: deposit.clone(), diff --git a/contracts/pre-propose/dao-pre-propose-approval-multiple/src/tests.rs b/contracts/pre-propose/dao-pre-propose-approval-multiple/src/tests.rs index 3c357fcc4..308f781b1 100644 --- a/contracts/pre-propose/dao-pre-propose-approval-multiple/src/tests.rs +++ b/contracts/pre-propose/dao-pre-propose-approval-multiple/src/tests.rs @@ -1257,10 +1257,10 @@ fn test_approval_and_rejection_permissions() { &coins(10, "ujuno"), ); - // Only approver can propose + // Only approver can approve let err: PreProposeError = app .execute_contract( - Addr::unchecked("nonmember"), + Addr::unchecked("nonapprover"), pre_propose.clone(), &ExecuteMsg::Extension { msg: ExecuteExt::Approve { id: pre_propose_id }, @@ -1272,11 +1272,11 @@ fn test_approval_and_rejection_permissions() { .unwrap(); assert_eq!(err, PreProposeError::Unauthorized {}); - // Only approver can propose + // Only approver can reject let err: PreProposeError = app .execute_contract( - Addr::unchecked("nonmember"), - pre_propose, + Addr::unchecked("nonapprover"), + pre_propose.clone(), &ExecuteMsg::Extension { msg: ExecuteExt::Reject { id: pre_propose_id }, }, @@ -1286,6 +1286,94 @@ fn test_approval_and_rejection_permissions() { .downcast() .unwrap(); assert_eq!(err, PreProposeError::Unauthorized {}); + + // Updating approver after proposal created does not change old proposal's + // approver + app.execute_contract( + Addr::unchecked("approver"), + pre_propose.clone(), + &ExecuteMsg::Extension { + msg: ExecuteExt::UpdateApprover { + address: "newapprover".to_string(), + }, + }, + &[], + ) + .unwrap(); + + let err: PreProposeError = app + .execute_contract( + Addr::unchecked("newapprover"), + pre_propose.clone(), + &ExecuteMsg::Extension { + msg: ExecuteExt::Approve { id: pre_propose_id }, + }, + &[], + ) + .unwrap_err() + .downcast() + .unwrap(); + assert_eq!(err, PreProposeError::Unauthorized {}); + + // Old approver can still approve. + app.execute_contract( + Addr::unchecked("approver"), + pre_propose.clone(), + &ExecuteMsg::Extension { + msg: ExecuteExt::Approve { id: pre_propose_id }, + }, + &[], + ) + .unwrap(); + + // Non-member proposes. + mint_natives(&mut app, "nonmember", coins(10, "ujuno")); + let pre_propose_id = make_pre_proposal( + &mut app, + pre_propose.clone(), + "nonmember", + &coins(10, "ujuno"), + ); + + // Old approver cannot approve nor reject. + let err: PreProposeError = app + .execute_contract( + Addr::unchecked("approver"), + pre_propose.clone(), + &ExecuteMsg::Extension { + msg: ExecuteExt::Approve { id: pre_propose_id }, + }, + &[], + ) + .unwrap_err() + .downcast() + .unwrap(); + assert_eq!(err, PreProposeError::Unauthorized {}); + + let err: PreProposeError = app + .execute_contract( + Addr::unchecked("approver"), + pre_propose.clone(), + &ExecuteMsg::Extension { + msg: ExecuteExt::Reject { id: pre_propose_id }, + }, + &[], + ) + .unwrap_err() + .downcast() + .unwrap(); + assert_eq!(err, PreProposeError::Unauthorized {}); + + // New approver can now approve. + app.execute_contract( + Addr::unchecked("newapprover"), + pre_propose.clone(), + &ExecuteMsg::Extension { + msg: ExecuteExt::Approve { id: pre_propose_id }, + }, + &[], + ) + .unwrap(); } #[test] diff --git a/contracts/pre-propose/dao-pre-propose-approval-single/Cargo.toml b/contracts/pre-propose/dao-pre-propose-approval-single/Cargo.toml index 0b44a0621..2d0b6b902 100644 --- a/contracts/pre-propose/dao-pre-propose-approval-single/Cargo.toml +++ b/contracts/pre-propose/dao-pre-propose-approval-single/Cargo.toml @@ -1,6 +1,9 @@ [package] name = "dao-pre-propose-approval-single" -authors = ["ekez ", "Jake Hartnell "] +authors = [ + "ekez ", + "Jake Hartnell ", +] description = "A DAO DAO pre-propose module handling a proposal approval flow for for dao-proposal-single." edition = { workspace = true } license = { workspace = true } @@ -21,6 +24,7 @@ cosmwasm-std = { workspace = true } cosmwasm-schema = { workspace = true } cw-storage-plus = { workspace = true } cw2 = { workspace = true } +cw-denom = { workspace = true } cw-paginate-storage = { workspace = true } dao-pre-propose-base = { workspace = true } dao-voting = { workspace = true } @@ -28,7 +32,6 @@ thiserror = { workspace = true } dao-interface = { workspace = true } [dev-dependencies] -cw-denom = { workspace = true } cw-multi-test = { workspace = true } cw-utils = { workspace = true } cw4 = { workspace = true } @@ -47,6 +50,6 @@ dao-proposal-single = { workspace = true } dao-dao-core-v241 = { workspace = true } dao-interface-v241 = { workspace = true } dao-pre-propose-approval-single-v241 = { workspace = true } -dao-proposal-single-v241 = { workspace = true } +dao-proposal-single-v241 = { workspace = true } dao-voting-cw4-v241 = { workspace = true } dao-voting-v241 = { workspace = true } diff --git a/contracts/pre-propose/dao-pre-propose-approval-single/src/contract.rs b/contracts/pre-propose/dao-pre-propose-approval-single/src/contract.rs index ddec7116d..4e9d47230 100644 --- a/contracts/pre-propose/dao-pre-propose-approval-single/src/contract.rs +++ b/contracts/pre-propose/dao-pre-propose-approval-single/src/contract.rs @@ -1,17 +1,21 @@ +use cosmwasm_schema::cw_serde; #[cfg(not(feature = "library"))] use cosmwasm_std::entry_point; use cosmwasm_std::{ - to_json_binary, Binary, Deps, DepsMut, Empty, Env, MessageInfo, Order, Response, StdResult, - SubMsg, WasmMsg, + to_json_binary, Addr, Binary, CosmosMsg, Deps, DepsMut, Empty, Env, MessageInfo, Order, + Response, StdError, StdResult, SubMsg, Uint128, WasmMsg, }; use cw2::set_contract_version; +use cw_denom::CheckedDenom; use cw_paginate_storage::paginate_map_values; +use cw_storage_plus::Map; use dao_pre_propose_base::{ error::PreProposeError, msg::ExecuteMsg as ExecuteBase, state::PreProposeContract, }; use dao_voting::approval::{ApprovalProposalStatus, ApproverProposeMessage}; -use dao_voting::deposit::DepositRefundPolicy; +use dao_voting::deposit::{CheckedDepositInfo, DepositRefundPolicy}; use dao_voting::proposal::SingleChoiceProposeMsg as ProposeMsg; +use dao_voting::voting::{SingleChoiceAutoVote, Vote}; use crate::msg::{ ExecuteExt, ExecuteMsg, InstantiateExt, InstantiateMsg, MigrateMsg, ProposeMessage, @@ -125,6 +129,8 @@ pub fn execute_propose( Ok(SubMsg::new(execute_msg)) })?; + let approver = APPROVER.load(deps.storage)?; + // Save the proposal and its information as pending. PENDING_PROPOSALS.save( deps.storage, @@ -132,6 +138,7 @@ pub fn execute_propose( &Proposal { status: ApprovalProposalStatus::Pending {}, approval_id, + approver: approver.clone(), proposer: info.sender, msg: propose_msg_internal, deposit: config.deposit_info, @@ -142,7 +149,8 @@ pub fn execute_propose( .add_messages(deposit_messages) .add_submessages(hooks_msgs) .add_attribute("method", "pre-propose") - .add_attribute("id", approval_id.to_string())) + .add_attribute("id", approval_id.to_string()) + .add_attribute("approver", approver.to_string())) } pub fn execute_approve( @@ -150,16 +158,15 @@ pub fn execute_approve( info: MessageInfo, id: u64, ) -> Result { - // Check sender is the approver - let approver = APPROVER.load(deps.storage)?; - if approver != info.sender { - return Err(PreProposeError::Unauthorized {}); - } - // Load proposal and send propose message to the proposal module let proposal = PENDING_PROPOSALS.may_load(deps.storage, id)?; match proposal { Some(proposal) => { + // Check sender is the approver + if proposal.approver != info.sender { + return Err(PreProposeError::Unauthorized {}); + } + let proposal_module = PrePropose::default().proposal_module.load(deps.storage)?; // Snapshot the deposit for the proposal that we're about @@ -188,6 +195,7 @@ pub fn execute_approve( created_proposal_id: proposal_id, }, approval_id: proposal.approval_id, + approver: proposal.approver, proposer: proposal.proposer, msg: proposal.msg, deposit: proposal.deposit, @@ -211,14 +219,9 @@ pub fn execute_reject( info: MessageInfo, id: u64, ) -> Result { - // Check sender is the approver - let approver = APPROVER.load(deps.storage)?; - if approver != info.sender { - return Err(PreProposeError::Unauthorized {}); - } - let Proposal { approval_id, + approver, proposer, msg, deposit, @@ -227,12 +230,18 @@ pub fn execute_reject( .may_load(deps.storage, id)? .ok_or(PreProposeError::ProposalNotFound {})?; + // Check sender is the approver + if approver != info.sender { + return Err(PreProposeError::Unauthorized {}); + } + COMPLETED_PROPOSALS.save( deps.storage, id, &Proposal { status: ApprovalProposalStatus::Rejected {}, approval_id, + approver, proposer: proposer.clone(), msg: msg.clone(), deposit: deposit.clone(), @@ -407,7 +416,234 @@ pub fn query(deps: Deps, env: Env, msg: QueryMsg) -> StdResult { #[cfg_attr(not(feature = "library"), entry_point)] pub fn migrate(mut deps: DepsMut, _env: Env, msg: MigrateMsg) -> Result { - let res = PrePropose::default().migrate(deps.branch(), msg); + let res: Result = + PrePropose::default().migrate(deps.branch(), msg.clone()); + match msg { + MigrateMsg::FromUnderV250 { .. } => { + // the default migrate function above ensures >= v2.4.1 and < v2.5.0 + + #[cw_serde] + struct ProposalV241 { + /// The status of a completed proposal. + pub status: ProposalStatusV241, + /// The approval ID used to identify this pending proposal. + pub approval_id: u64, + /// The address that created the proposal. + pub proposer: Addr, + /// The propose message that ought to be executed on the + /// proposal message if this proposal is approved. + pub msg: SingleChoiceProposeMsgV241, + /// Snapshot of the deposit info at the time of proposal + /// submission. + pub deposit: Option, + } + + #[cw_serde] + enum ProposalStatusV241 { + /// The proposal is pending approval. + Pending {}, + /// The proposal has been approved. + Approved { + /// The created proposal ID. + created_proposal_id: u64, + }, + /// The proposal has been rejected. + Rejected {}, + } + + #[cw_serde] + struct SingleChoiceProposeMsgV241 { + /// The title of the proposal. + pub title: String, + /// A description of the proposal. + pub description: String, + /// The messages that should be executed in response to this + /// proposal passing. + pub msgs: Vec>, + /// The address creating the proposal. If no pre-propose + /// module is attached to this module this must always be None + /// as the proposer is the sender of the propose message. If a + /// pre-propose module is attached, this must be Some and will + /// set the proposer of the proposal it creates. + pub proposer: Option, + /// An optional vote cast by the proposer. + pub vote: Option, + } + + #[cw_serde] + #[derive(Copy)] + #[repr(u8)] + enum VoteV241 { + /// Marks support for the proposal. + Yes, + /// Marks opposition to the proposal. + No, + /// Marks participation but does not count towards the ratio of + /// support / opposed. + Abstain, + } + + #[cw_serde] + struct SingleChoiceAutoVoteV241 { + /// The proposer's position on the proposal. + pub vote: VoteV241, + /// An optional rationale for why this vote was cast. This can + /// be updated, set, or removed later by the address casting + /// the vote. + pub rationale: Option, + } + + #[cw_serde] + enum DepositRefundPolicyV241 { + /// Deposits should always be refunded. + Always, + /// Deposits should only be refunded for passed proposals. + OnlyPassed, + /// Deposits should never be refunded. + Never, + } + + /// Counterpart to the `DepositInfo` struct which has been + /// processed. This type should never be constructed literally and + /// should always by built by calling `into_checked` on a + /// `DepositInfo` instance. + #[cw_serde] + struct CheckedDepositInfoV241 { + /// The address of the cw20 token to be used for proposal + /// deposits. + pub denom: CheckedDenomV241, + /// The number of tokens that must be deposited to create a + /// proposal. This is validated to be non-zero if this struct is + /// constructed by converted via the `into_checked` method on + /// `DepositInfo`. + pub amount: Uint128, + /// The policy used for refunding proposal deposits. + pub refund_policy: DepositRefundPolicyV241, + } + + #[cw_serde] + enum CheckedDenomV241 { + /// A native (bank module) asset. + Native(String), + /// A cw20 asset. + Cw20(Addr), + } + + let pending_proposals_v241: Map = Map::new("pending_proposals"); + let completed_proposals_v241: Map = Map::new("completed_proposals"); + + // migrate proposals to add approver + + let approver = APPROVER.load(deps.storage)?; + + let pending_proposals = pending_proposals_v241 + .range(deps.storage, None, None, Order::Ascending) + .collect::>>()?; + for (id, proposal) in pending_proposals { + PENDING_PROPOSALS.save( + deps.storage, + id, + &Proposal { + status: ApprovalProposalStatus::Pending {}, + approval_id: proposal.approval_id, + approver: approver.clone(), + proposer: proposal.proposer, + msg: ProposeMsg { + title: proposal.msg.title, + description: proposal.msg.description, + msgs: proposal.msg.msgs, + proposer: proposal.msg.proposer, + vote: proposal.msg.vote.map(|vote| SingleChoiceAutoVote { + vote: match vote.vote { + VoteV241::Yes => Vote::Yes, + VoteV241::No => Vote::No, + VoteV241::Abstain => Vote::Abstain, + }, + rationale: vote.rationale, + }), + }, + deposit: proposal.deposit.map(|deposit| CheckedDepositInfo { + denom: match deposit.denom { + CheckedDenomV241::Native(denom) => CheckedDenom::Native(denom), + CheckedDenomV241::Cw20(addr) => CheckedDenom::Cw20(addr), + }, + amount: deposit.amount, + refund_policy: match deposit.refund_policy { + DepositRefundPolicyV241::Always => DepositRefundPolicy::Always, + DepositRefundPolicyV241::OnlyPassed => { + DepositRefundPolicy::OnlyPassed + } + DepositRefundPolicyV241::Never => DepositRefundPolicy::Never, + }, + }), + }, + )?; + } + + let completed_proposals = completed_proposals_v241 + .range(deps.storage, None, None, Order::Ascending) + .collect::>>()?; + for (id, proposal) in completed_proposals { + COMPLETED_PROPOSALS.save( + deps.storage, + id, + &Proposal { + status: match proposal.status { + ProposalStatusV241::Approved { + created_proposal_id, + } => ApprovalProposalStatus::Approved { + created_proposal_id, + }, + ProposalStatusV241::Rejected {} => ApprovalProposalStatus::Rejected {}, + // should not be possible since these are completed + // proposals only + ProposalStatusV241::Pending {} => { + return Err(PreProposeError::Std(StdError::generic_err( + "unexpected proposal status", + ))) + } + }, + approval_id: proposal.approval_id, + approver: approver.clone(), + proposer: proposal.proposer, + msg: ProposeMsg { + title: proposal.msg.title, + description: proposal.msg.description, + msgs: proposal.msg.msgs, + proposer: proposal.msg.proposer, + vote: proposal.msg.vote.map(|vote| SingleChoiceAutoVote { + vote: match vote.vote { + VoteV241::Yes => Vote::Yes, + VoteV241::No => Vote::No, + VoteV241::Abstain => Vote::Abstain, + }, + rationale: vote.rationale, + }), + }, + deposit: proposal.deposit.map(|deposit| CheckedDepositInfo { + denom: match deposit.denom { + CheckedDenomV241::Native(denom) => CheckedDenom::Native(denom), + CheckedDenomV241::Cw20(addr) => CheckedDenom::Cw20(addr), + }, + amount: deposit.amount, + refund_policy: match deposit.refund_policy { + DepositRefundPolicyV241::Always => DepositRefundPolicy::Always, + DepositRefundPolicyV241::OnlyPassed => { + DepositRefundPolicy::OnlyPassed + } + DepositRefundPolicyV241::Never => DepositRefundPolicy::Never, + }, + }), + }, + )?; + } + } + _ => { + return Err(PreProposeError::Std(StdError::generic_err( + "not implemented", + ))) + } + } set_contract_version(deps.storage, CONTRACT_NAME, CONTRACT_VERSION)?; res } diff --git a/contracts/pre-propose/dao-pre-propose-approval-single/src/tests.rs b/contracts/pre-propose/dao-pre-propose-approval-single/src/tests.rs index fd4a8d20f..1df4ac65a 100644 --- a/contracts/pre-propose/dao-pre-propose-approval-single/src/tests.rs +++ b/contracts/pre-propose/dao-pre-propose-approval-single/src/tests.rs @@ -1230,10 +1230,10 @@ fn test_approval_and_rejection_permissions() { &coins(10, "ujuno"), ); - // Only approver can propose + // Only approver can approve let err: PreProposeError = app .execute_contract( - Addr::unchecked("nonmember"), + Addr::unchecked("nonapprover"), pre_propose.clone(), &ExecuteMsg::Extension { msg: ExecuteExt::Approve { id: pre_propose_id }, @@ -1245,11 +1245,11 @@ fn test_approval_and_rejection_permissions() { .unwrap(); assert_eq!(err, PreProposeError::Unauthorized {}); - // Only approver can propose + // Only approver can reject let err: PreProposeError = app .execute_contract( - Addr::unchecked("nonmember"), - pre_propose, + Addr::unchecked("nonapprover"), + pre_propose.clone(), &ExecuteMsg::Extension { msg: ExecuteExt::Reject { id: pre_propose_id }, }, @@ -1259,6 +1259,94 @@ fn test_approval_and_rejection_permissions() { .downcast() .unwrap(); assert_eq!(err, PreProposeError::Unauthorized {}); + + // Updating approver after proposal created does not change old proposal's + // approver + app.execute_contract( + Addr::unchecked("approver"), + pre_propose.clone(), + &ExecuteMsg::Extension { + msg: ExecuteExt::UpdateApprover { + address: "newapprover".to_string(), + }, + }, + &[], + ) + .unwrap(); + + let err: PreProposeError = app + .execute_contract( + Addr::unchecked("newapprover"), + pre_propose.clone(), + &ExecuteMsg::Extension { + msg: ExecuteExt::Approve { id: pre_propose_id }, + }, + &[], + ) + .unwrap_err() + .downcast() + .unwrap(); + assert_eq!(err, PreProposeError::Unauthorized {}); + + // Old approver can still approve. + app.execute_contract( + Addr::unchecked("approver"), + pre_propose.clone(), + &ExecuteMsg::Extension { + msg: ExecuteExt::Approve { id: pre_propose_id }, + }, + &[], + ) + .unwrap(); + + // Non-member proposes. + mint_natives(&mut app, "nonmember", coins(10, "ujuno")); + let pre_propose_id = make_pre_proposal( + &mut app, + pre_propose.clone(), + "nonmember", + &coins(10, "ujuno"), + ); + + // Old approver cannot approve nor reject. + let err: PreProposeError = app + .execute_contract( + Addr::unchecked("approver"), + pre_propose.clone(), + &ExecuteMsg::Extension { + msg: ExecuteExt::Approve { id: pre_propose_id }, + }, + &[], + ) + .unwrap_err() + .downcast() + .unwrap(); + assert_eq!(err, PreProposeError::Unauthorized {}); + + let err: PreProposeError = app + .execute_contract( + Addr::unchecked("approver"), + pre_propose.clone(), + &ExecuteMsg::Extension { + msg: ExecuteExt::Reject { id: pre_propose_id }, + }, + &[], + ) + .unwrap_err() + .downcast() + .unwrap(); + assert_eq!(err, PreProposeError::Unauthorized {}); + + // New approver can now approve. + app.execute_contract( + Addr::unchecked("newapprover"), + pre_propose.clone(), + &ExecuteMsg::Extension { + msg: ExecuteExt::Approve { id: pre_propose_id }, + }, + &[], + ) + .unwrap(); } #[test] diff --git a/packages/dao-voting/src/approval.rs b/packages/dao-voting/src/approval.rs index 7565aff8d..68587e2ac 100644 --- a/packages/dao-voting/src/approval.rs +++ b/packages/dao-voting/src/approval.rs @@ -37,10 +37,12 @@ pub enum ApprovalProposalStatus { #[cw_serde] pub struct ApprovalProposal { - /// The status of a completed proposal. + /// The status of an approval proposal. pub status: ApprovalProposalStatus, /// The approval ID used to identify this pending proposal. pub approval_id: u64, + /// The address that can approve/reject this proposal. + pub approver: Addr, /// The address that created the proposal. pub proposer: Addr, /// The propose message that ought to be executed on the proposal