Skip to content

Commit

Permalink
lock approver in pending proposal on propose instead of using most re…
Browse files Browse the repository at this point in the history 
…cent approver
  • Loading branch information
@NoahSaso
NoahSaso committed Sep 18, 2024
1 parent f7bd383 commit 91d5e8b
Show file tree
Hide file tree
Showing 7 changed files with 365 additions and 46 deletions.
1 change: 1 addition & 0 deletions Cargo.lock
View file

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

31 changes: 18 additions & 13 deletions contracts/pre-propose/dao-pre-propose-approval-multiple/src/contract.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -125,13 +125,16 @@ 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,
approval_id,
&Proposal {
status: ApprovalProposalStatus::Pending {},
approval_id,
approver: approver.clone(),
proposer: info.sender,
msg: propose_msg_internal,
deposit: config.deposit_info,
Expand All @@ -142,24 +145,24 @@ 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(
deps: DepsMut,
info: MessageInfo,
id: u64,
) -> Result<Response, PreProposeError> {
// 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
Expand Down Expand Up @@ -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,
Expand All @@ -211,14 +215,9 @@ pub fn execute_reject(
info: MessageInfo,
id: u64,
) -> Result<Response, PreProposeError> {
// 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,
Expand All @@ -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(),
Expand Down
98 changes: 93 additions & 5 deletions contracts/pre-propose/dao-pre-propose-approval-multiple/src/tests.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 },
Expand All @@ -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 },
},
Expand All @@ -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]
Expand Down
16 changes: 11 additions & 5 deletions contracts/pre-propose/dao-pre-propose-approval-single/Cargo.toml
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
[package]
name = "dao-pre-propose-approval-single"
authors = ["ekez <[email protected]>", "Jake Hartnell <[email protected]>"]
authors = [
"ekez <[email protected]>",
"Jake Hartnell <[email protected]>",
]
description = "A DAO DAO pre-propose module handling a proposal approval flow for for dao-proposal-single."
edition = { workspace = true }
license = { workspace = true }
Expand All @@ -21,14 +24,19 @@ 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 }
thiserror = { workspace = true }
dao-interface = { workspace = true }

# v2.4.1 migration
cw-denom-v241 = { workspace = true }
dao-pre-propose-approval-single-v241 = { workspace = true }
dao-voting-v241 = { workspace = true }

[dev-dependencies]
cw-denom = { workspace = true }
cw-multi-test = { workspace = true }
cw-utils = { workspace = true }
cw4 = { workspace = true }
Expand All @@ -46,7 +54,5 @@ dao-proposal-single = { workspace = true }
# v2.4.1 migration
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 }
Loading

0 comments on commit 91d5e8b

Please sign in to comment.