Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add optional executor restriction to cw3-flex #741

Merged
merged 5 commits into from
Jun 19, 2022
Merged

Add optional executor restriction to cw3-flex #741

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c154dc3
cw3-flex: Add implementation for optional executor
ueco-jb Jun 18, 2022
57c11d6
cw3-flex: Adjust existing test to new executor parameter
ueco-jb Jun 18, 2022
467c472
cw3-flex: Add testcases that confirms executor works
ueco-jb Jun 18, 2022
ad6a889
cw3-flex: Create authorize method for Config structure
ueco-jb Jun 19, 2022
1e1f6e5
cw3-flex: Adjust comments to match authorize methods better
ueco-jb Jun 19, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
181 changes: 171 additions & 10 deletions contracts/cw3-flex-multisig/src/contract.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ use cw_utils::{maybe_addr, Expiration, ThresholdResponse};

use crate::error::ContractError;
use crate::msg::{ExecuteMsg, InstantiateMsg, QueryMsg};
use crate::state::{Config, CONFIG};
use crate::state::{Config, Executor, CONFIG};

// version info for migration info
const CONTRACT_NAME: &str = "crates.io:cw3-flex-multisig";
Expand All @@ -46,6 +46,7 @@ pub fn instantiate(
threshold: msg.threshold,
max_voting_period: msg.max_voting_period,
group_addr,
executor: msg.executor,
};
CONFIG.save(deps.storage, &cfg)?;

Expand Down Expand Up @@ -192,15 +193,33 @@ pub fn execute_execute(
info: MessageInfo,
proposal_id: u64,
) -> Result<Response, ContractError> {
// anyone can trigger this if the vote passed

let mut prop = PROPOSALS.load(deps.storage, proposal_id)?;
// we allow execution even after the proposal "expiration" as long as all vote come in before
// that point. If it was approved on time, it can be executed any time.
if prop.current_status(&env.block) != Status::Passed {
return Err(ContractError::WrongExecuteStatus {});
}

// Executor can be set in 3 ways:
// - Member: any member of the voting group can execute
// - Only: only passed address is able to execute
// - None: Anyone can execute message
let cfg = CONFIG.load(deps.storage)?;
if let Some(executor) = cfg.executor {
match executor {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this something that could be pulled out to a method on Executor?
Already thinking of reusing it for who can make proposals. (but default is Member)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I moved it under Config struct. I think it's more versatile this way.
What do you think?

Executor::Member => {
cfg.group_addr
.is_member(&deps.querier, &info.sender, None)?
.ok_or(ContractError::Unauthorized {})?;
}
Executor::Only(addr) => {
if addr != info.sender {
return Err(ContractError::Unauthorized {});
}
}
}
}

// set it to executed
prop.status = Status::Executed;
PROPOSALS.save(deps.storage, proposal_id, &prop)?;
Expand Down Expand Up @@ -497,12 +516,14 @@ mod tests {
group: Addr,
threshold: Threshold,
max_voting_period: Duration,
executor: Option<crate::state::Executor>,
) -> Addr {
let flex_id = app.store_code(contract_flex());
let msg = crate::msg::InstantiateMsg {
group_addr: group.to_string(),
threshold,
max_voting_period,
executor,
};
app.instantiate_contract(flex_id, Addr::unchecked(OWNER), &msg, &[], "flex", None)
.unwrap()
Expand All @@ -527,6 +548,7 @@ mod tests {
max_voting_period,
init_funds,
multisig_as_group_admin,
None,
)
}

Expand All @@ -537,6 +559,7 @@ mod tests {
max_voting_period: Duration,
init_funds: Vec<Coin>,
multisig_as_group_admin: bool,
executor: Option<crate::state::Executor>,
) -> (Addr, Addr) {
// 1. Instantiate group contract with members (and OWNER as admin)
let members = vec![
Expand All @@ -551,7 +574,13 @@ mod tests {
app.update_block(next_block);

// 2. Set up Multisig backed by this group
let flex_addr = instantiate_flex(app, group_addr.clone(), threshold, max_voting_period);
let flex_addr = instantiate_flex(
app,
group_addr.clone(),
threshold,
max_voting_period,
executor,
);
app.update_block(next_block);

// 3. (Optional) Set the multisig as the group owner
Expand Down Expand Up @@ -616,6 +645,7 @@ mod tests {
quorum: Decimal::percent(1),
},
max_voting_period,
executor: None,
};
let err = app
.instantiate_contract(
Expand All @@ -637,6 +667,7 @@ mod tests {
group_addr: group_addr.to_string(),
threshold: Threshold::AbsoluteCount { weight: 100 },
max_voting_period,
executor: None,
};
let err = app
.instantiate_contract(
Expand All @@ -658,6 +689,7 @@ mod tests {
group_addr: group_addr.to_string(),
threshold: Threshold::AbsoluteCount { weight: 1 },
max_voting_period,
executor: None,
};
let flex_addr = app
.instantiate_contract(
Expand Down Expand Up @@ -815,7 +847,8 @@ mod tests {
threshold: Decimal::percent(80),
quorum: Decimal::percent(20),
};
let (flex_addr, _) = setup_test_case(&mut app, threshold, voting_period, init_funds, false);
let (flex_addr, _) =
setup_test_case(&mut app, threshold, voting_period, init_funds, false, None);

// create proposal with 1 vote power
let proposal = pay_somebody_proposal();
Expand Down Expand Up @@ -910,7 +943,8 @@ mod tests {
quorum: Decimal::percent(1),
};
let voting_period = Duration::Time(2000000);
let (flex_addr, _) = setup_test_case(&mut app, threshold, voting_period, init_funds, false);
let (flex_addr, _) =
setup_test_case(&mut app, threshold, voting_period, init_funds, false, None);

// create proposal with 0 vote power
let proposal = pay_somebody_proposal();
Expand Down Expand Up @@ -1101,7 +1135,8 @@ mod tests {
quorum: Decimal::percent(1),
};
let voting_period = Duration::Time(2000000);
let (flex_addr, _) = setup_test_case(&mut app, threshold, voting_period, init_funds, true);
let (flex_addr, _) =
setup_test_case(&mut app, threshold, voting_period, init_funds, true, None);

// ensure we have cash to cover the proposal
let contract_bal = app.wrap().query_balance(&flex_addr, "BTC").unwrap();
Expand Down Expand Up @@ -1191,6 +1226,128 @@ mod tests {
);
}

#[test]
fn execute_with_executor_member() {
let init_funds = coins(10, "BTC");
let mut app = mock_app(&init_funds);

let threshold = Threshold::ThresholdQuorum {
threshold: Decimal::percent(51),
quorum: Decimal::percent(1),
};
let voting_period = Duration::Time(2000000);
let (flex_addr, _) = setup_test_case(
&mut app,
threshold,
voting_period,
init_funds,
true,
Some(crate::state::Executor::Member),
);

// create proposal with 0 vote power
let proposal = pay_somebody_proposal();
let res = app
.execute_contract(Addr::unchecked(OWNER), flex_addr.clone(), &proposal, &[])
.unwrap();

// Get the proposal id from the logs
let proposal_id: u64 = res.custom_attrs(1)[2].value.parse().unwrap();

// Vote it, so it passes
let vote = ExecuteMsg::Vote {
proposal_id,
vote: Vote::Yes,
};
app.execute_contract(Addr::unchecked(VOTER4), flex_addr.clone(), &vote, &[])
.unwrap();

let execution = ExecuteMsg::Execute { proposal_id };
let err = app
.execute_contract(
Addr::unchecked(Addr::unchecked("anyone")),
flex_addr.clone(),
&execution,
&[],
)
.unwrap_err();
assert_eq!(ContractError::Unauthorized {}, err.downcast().unwrap());

app.execute_contract(
Addr::unchecked(Addr::unchecked(VOTER2)),
flex_addr,
&execution,
&[],
)
.unwrap();
}

#[test]
fn execute_with_executor_only() {
ueco-jb marked this conversation as resolved.
Show resolved Hide resolved
let init_funds = coins(10, "BTC");
let mut app = mock_app(&init_funds);

let threshold = Threshold::ThresholdQuorum {
threshold: Decimal::percent(51),
quorum: Decimal::percent(1),
};
let voting_period = Duration::Time(2000000);
let (flex_addr, _) = setup_test_case(
&mut app,
threshold,
voting_period,
init_funds,
true,
Some(crate::state::Executor::Only(Addr::unchecked(VOTER3))),
);

// create proposal with 0 vote power
let proposal = pay_somebody_proposal();
let res = app
.execute_contract(Addr::unchecked(OWNER), flex_addr.clone(), &proposal, &[])
.unwrap();

// Get the proposal id from the logs
let proposal_id: u64 = res.custom_attrs(1)[2].value.parse().unwrap();

// Vote it, so it passes
let vote = ExecuteMsg::Vote {
proposal_id,
vote: Vote::Yes,
};
app.execute_contract(Addr::unchecked(VOTER4), flex_addr.clone(), &vote, &[])
.unwrap();

let execution = ExecuteMsg::Execute { proposal_id };
let err = app
.execute_contract(
Addr::unchecked(Addr::unchecked("anyone")),
flex_addr.clone(),
&execution,
&[],
)
.unwrap_err();
assert_eq!(ContractError::Unauthorized {}, err.downcast().unwrap());

let err = app
.execute_contract(
Addr::unchecked(Addr::unchecked(VOTER1)),
flex_addr.clone(),
&execution,
&[],
)
.unwrap_err();
assert_eq!(ContractError::Unauthorized {}, err.downcast().unwrap());

app.execute_contract(
Addr::unchecked(Addr::unchecked(VOTER3)),
flex_addr,
&execution,
&[],
)
.unwrap();
}

#[test]
fn proposal_pass_on_expiration() {
let init_funds = coins(10, "BTC");
Expand All @@ -1207,6 +1364,7 @@ mod tests {
Duration::Time(voting_period),
init_funds,
true,
None,
);

// ensure we have cash to cover the proposal
Expand Down Expand Up @@ -1282,7 +1440,8 @@ mod tests {
quorum: Decimal::percent(1),
};
let voting_period = Duration::Height(2000000);
let (flex_addr, _) = setup_test_case(&mut app, threshold, voting_period, init_funds, true);
let (flex_addr, _) =
setup_test_case(&mut app, threshold, voting_period, init_funds, true, None);

// create proposal with 0 vote power
let proposal = pay_somebody_proposal();
Expand Down Expand Up @@ -1334,7 +1493,7 @@ mod tests {
};
let voting_period = Duration::Time(20000);
let (flex_addr, group_addr) =
setup_test_case(&mut app, threshold, voting_period, init_funds, false);
setup_test_case(&mut app, threshold, voting_period, init_funds, false, None);

// VOTER1 starts a proposal to send some tokens (1/4 votes)
let proposal = pay_somebody_proposal();
Expand Down Expand Up @@ -1580,7 +1739,7 @@ mod tests {
};
let voting_period = Duration::Time(20000);
let (flex_addr, group_addr) =
setup_test_case(&mut app, threshold, voting_period, init_funds, false);
setup_test_case(&mut app, threshold, voting_period, init_funds, false, None);

// VOTER3 starts a proposal to send some tokens (3/12 votes)
let proposal = pay_somebody_proposal();
Expand Down Expand Up @@ -1663,6 +1822,7 @@ mod tests {
voting_period,
init_funds,
false,
None,
);

// VOTER3 starts a proposal to send some tokens (3 votes)
Expand Down Expand Up @@ -1733,6 +1893,7 @@ mod tests {
voting_period,
init_funds,
false,
None,
);

// create proposal
Expand Down
5 changes: 5 additions & 0 deletions contracts/cw3-flex-multisig/src/msg.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,17 @@ use cw3::Vote;
use cw4::MemberChangedHookMsg;
use cw_utils::{Duration, Expiration, Threshold};

use crate::state::Executor;

#[derive(Serialize, Deserialize, Clone, PartialEq, JsonSchema, Debug)]
pub struct InstantiateMsg {
// this is the group contract that contains the member list
pub group_addr: String,
pub threshold: Threshold,
pub max_voting_period: Duration,
// who is able to execute passed proposals
// None means that anyone can execute
pub executor: Option<Executor>,
}

// TODO: add some T variants? Maybe good enough as fixed Empty for now
Expand Down
13 changes: 13 additions & 0 deletions contracts/cw3-flex-multisig/src/state.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,16 +1,29 @@
use schemars::JsonSchema;
use serde::{Deserialize, Serialize};

use cosmwasm_std::Addr;
use cw4::Cw4Contract;
use cw_storage_plus::Item;
use cw_utils::{Duration, Threshold};

/// Defines who is able to execute proposals once passed
#[derive(Serialize, Deserialize, Clone, PartialEq, JsonSchema, Debug)]
pub enum Executor {
ueco-jb marked this conversation as resolved.
Show resolved Hide resolved
/// Any member of the voting group, even with 0 points
Member,
/// Only the given address
Only(Addr),
}

#[derive(Serialize, Deserialize, Clone, PartialEq, JsonSchema, Debug)]
pub struct Config {
pub threshold: Threshold,
pub max_voting_period: Duration,
// Total weight and voters are queried from this contract
pub group_addr: Cw4Contract,
// who is able to execute passed proposals
// None means that anyone can execute
pub executor: Option<Executor>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Disregard the above comment. This is great as it is drop-in state compatible with previous version, not requiring an explicit migration (we should have some placeholder migrate function, but no state change needed). No need to make breaking changes for some aesthetic opinion of mine.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I considered that but as you pointed out - this way it's completely backward compatible.

}

// unique items
Expand Down