From 171086076e75e0254071b53d9f8d2055989530b7 Mon Sep 17 00:00:00 2001 From: Amit Yadav Date: Wed, 20 Sep 2023 00:19:01 +0530 Subject: [PATCH 01/14] add flag and action --- astra/src/lib.rs | 16 ++++++------ astra/src/proposals.rs | 56 +++++++++++++++++++++++++++++++----------- astra/src/types.rs | 2 ++ 3 files changed, 52 insertions(+), 22 deletions(-) diff --git a/astra/src/lib.rs b/astra/src/lib.rs index 50da8a8b..340732c5 100644 --- a/astra/src/lib.rs +++ b/astra/src/lib.rs @@ -207,7 +207,7 @@ mod tests { assert_eq!(contract.get_proposals(0, 10).len(), 1); let id = create_proposal(&mut context, &mut contract); - contract.act_proposal(id, Action::VoteApprove, None); + contract.act_proposal(id, Action::VoteApprove, None, None); assert_eq!( contract.get_proposal(id).proposal.status, ProposalStatus::Approved @@ -218,7 +218,7 @@ mod tests { testing_env!(context .block_timestamp(1_000_000_000 * 24 * 60 * 60 * 8) .build()); - contract.act_proposal(id, Action::Finalize, None); + contract.act_proposal(id, Action::Finalize, None, None); assert_eq!( contract.get_proposal(id).proposal.status, ProposalStatus::Expired @@ -249,7 +249,7 @@ mod tests { ); let id = create_proposal(&mut context, &mut contract); assert_eq!(contract.get_proposal(id).proposal.description, "test"); - contract.act_proposal(id, Action::RemoveProposal, None); + contract.act_proposal(id, Action::RemoveProposal, None, None); } #[test] @@ -263,7 +263,7 @@ mod tests { let mut contract = Contract::new(Config::test_config(), policy); let id = create_proposal(&mut context, &mut contract); assert_eq!(contract.get_proposal(id).proposal.description, "test"); - contract.act_proposal(id, Action::RemoveProposal, None); + contract.act_proposal(id, Action::RemoveProposal, None, None); assert_eq!(contract.get_proposals(0, 10).len(), 0); } @@ -279,7 +279,7 @@ mod tests { testing_env!(context .block_timestamp(1_000_000_000 * 24 * 60 * 60 * 8) .build()); - contract.act_proposal(id, Action::VoteApprove, None); + contract.act_proposal(id, Action::VoteApprove, None, None); } #[test] @@ -292,8 +292,8 @@ mod tests { VersionedPolicy::Default(vec![accounts(1), accounts(2)]), ); let id = create_proposal(&mut context, &mut contract); - contract.act_proposal(id, Action::VoteApprove, None); - contract.act_proposal(id, Action::VoteApprove, None); + contract.act_proposal(id, Action::VoteApprove, None, None); + contract.act_proposal(id, Action::VoteApprove, None, None); } #[test] @@ -312,7 +312,7 @@ mod tests { role: "missing".to_string(), }, }); - contract.act_proposal(id, Action::VoteApprove, None); + contract.act_proposal(id, Action::VoteApprove, None, None); let x = contract.get_policy(); // still 2 roles: all and council. assert_eq!(x.roles.len(), 2); diff --git a/astra/src/proposals.rs b/astra/src/proposals.rs index 45a240d9..9034d0e9 100644 --- a/astra/src/proposals.rs +++ b/astra/src/proposals.rs @@ -537,7 +537,8 @@ impl Contract { /// Act on given proposal by id, if permissions allow. /// Memo is logged but not stored in the state. Can be used to leave notes or explain the action. - pub fn act_proposal(&mut self, id: u64, action: Action, memo: Option) { + pub fn act_proposal(&mut self, id: u64, action: Action, memo: Option, no_execute: Option) { + let execute = no_execute.unwrap_or(true); let mut proposal: Proposal = self.proposals.get(&id).expect("ERR_NO_PROPOSAL").into(); let policy = self.policy.get().unwrap().to_policy(); // Check permissions for the given action. @@ -564,21 +565,26 @@ impl Contract { &policy, self.get_user_weight(&sender_id), ); - // Updates proposal status with new votes using the policy. - proposal.status = + + if execute { + // Updates proposal status with new votes using the policy. + proposal.status = policy.proposal_status(&proposal, roles, self.total_delegation_amount); - if proposal.status == ProposalStatus::Approved { - self.internal_execute_proposal(&policy, &proposal, id); - true - } else if proposal.status == ProposalStatus::Removed { - self.internal_reject_proposal(&policy, &proposal, false); - self.proposals.remove(&id); - false - } else if proposal.status == ProposalStatus::Rejected { - self.internal_reject_proposal(&policy, &proposal, true); - true + if proposal.status == ProposalStatus::Approved && execute { + self.internal_execute_proposal(&policy, &proposal, id); + true + } else if proposal.status == ProposalStatus::Removed && execute { + self.internal_reject_proposal(&policy, &proposal, false); + self.proposals.remove(&id); + false + } else if proposal.status == ProposalStatus::Rejected && execute { + self.internal_reject_proposal(&policy, &proposal, true); + true + } else { + // Still in progress or expired. + true + } } else { - // Still in progress or expired. true } } @@ -609,6 +615,28 @@ impl Contract { true } Action::MoveToHub => false, + Action::ExecuteProposal => { + // Note: If proposal was executed already `proposal_status` will throw error + proposal.status = policy.proposal_status(&proposal, roles, self.total_delegation_amount); + let mut update = true; + match proposal.status { + ProposalStatus::Approved => { + self.internal_execute_proposal(&policy, &proposal, id); + } + ProposalStatus::Removed => { + self.internal_reject_proposal(&policy, &proposal, false); + self.proposals.remove(&id); + update = false; + } + ProposalStatus::Rejected => { + self.internal_reject_proposal(&policy, &proposal, true); + } + _ => { + // do nothing + } + } + update + } }; if update { self.proposals diff --git a/astra/src/types.rs b/astra/src/types.rs index b9f3f7d4..df9b444e 100644 --- a/astra/src/types.rs +++ b/astra/src/types.rs @@ -60,6 +60,8 @@ pub enum Action { Finalize, /// Move a proposal to the hub to shift into another DAO. MoveToHub, + /// Execute proposal and update proposal status + ExecuteProposal } impl Action { From 0b6b6a15a7401ac0600d4c4aaedd7cf158f7d086 Mon Sep 17 00:00:00 2001 From: Amit Yadav Date: Wed, 20 Sep 2023 00:27:52 +0530 Subject: [PATCH 02/14] add tests --- astra/src/bounties.rs | 6 +++--- astra/src/lib.rs | 46 +++++++++++++++++++++++++++++++++++++++++++ astra/src/policy.rs | 1 + 3 files changed, 50 insertions(+), 3 deletions(-) diff --git a/astra/src/bounties.rs b/astra/src/bounties.rs index f1b87670..d31b5aa2 100644 --- a/astra/src/bounties.rs +++ b/astra/src/bounties.rs @@ -228,7 +228,7 @@ mod tests { }, }); assert_eq!(contract.get_last_bounty_id(), id); - contract.act_proposal(id, Action::VoteApprove, None); + contract.act_proposal(id, Action::VoteApprove, None, None); id } @@ -267,7 +267,7 @@ mod tests { "bounty_done" ); - contract.act_proposal(1, Action::VoteApprove, None); + contract.act_proposal(1, Action::VoteApprove, None, None); testing_env!( context.build(), near_sdk::VMConfig::test(), @@ -282,7 +282,7 @@ mod tests { contract.bounty_claim(0, U64::from(500)); contract.bounty_done(0, None, "Bounty is done 2".to_string()); - contract.act_proposal(2, Action::VoteApprove, None); + contract.act_proposal(2, Action::VoteApprove, None, None); testing_env!( context.build(), near_sdk::VMConfig::test(), diff --git a/astra/src/lib.rs b/astra/src/lib.rs index 340732c5..73417cfe 100644 --- a/astra/src/lib.rs +++ b/astra/src/lib.rs @@ -318,6 +318,52 @@ mod tests { assert_eq!(x.roles.len(), 2); } + #[test] + fn test_proposal_execution() { + let mut context = VMContextBuilder::new(); + testing_env!(context.predecessor_account_id(accounts(1)).build()); + let mut contract = Contract::new( + Config::test_config(), + VersionedPolicy::Default(vec![accounts(1)]), + ); + + let id = create_proposal(&mut context, &mut contract); + contract.act_proposal(id, Action::VoteApprove, None, Some(false)); + // verify proposal wasn't approved during final vote + assert_eq!( + contract.get_proposal(id).proposal.status, + ProposalStatus::InProgress + ); + + contract.act_proposal(id, Action::ExecuteProposal, None, None); + assert_eq!( + contract.get_proposal(id).proposal.status, + ProposalStatus::Approved + ); + } + + #[test] + #[should_panic(expected = "ERR_PROPOSAL_NOT_IN_PROGRESS")] + fn test_proposal_double_execution() { + let mut context = VMContextBuilder::new(); + testing_env!(context.predecessor_account_id(accounts(1)).build()); + let mut contract = Contract::new( + Config::test_config(), + VersionedPolicy::Default(vec![accounts(1)]), + ); + + let id = create_proposal(&mut context, &mut contract); + contract.act_proposal(id, Action::VoteApprove, None, None); + // verify proposal was approved and executed + assert_eq!( + contract.get_proposal(id).proposal.status, + ProposalStatus::Approved + ); + + // panics if we try to execute again + contract.act_proposal(id, Action::ExecuteProposal, None, None); + } + #[test] #[should_panic(expected = "ERR_INVALID_POLICY")] fn test_fails_adding_invalid_policy() { diff --git a/astra/src/policy.rs b/astra/src/policy.rs index 55566537..f707d363 100644 --- a/astra/src/policy.rs +++ b/astra/src/policy.rs @@ -195,6 +195,7 @@ pub fn default_policy(council: Vec) -> Policy { "*:VoteReject".to_string(), "*:VoteRemove".to_string(), "*:Finalize".to_string(), + "*:ExecuteProposal".to_string(), ] .into_iter() .collect(), From e259c4bada21205cc3c6226d168f6d163ee1959c Mon Sep 17 00:00:00 2001 From: Amit Yadav Date: Wed, 20 Sep 2023 00:30:22 +0530 Subject: [PATCH 03/14] add readme --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 0831a9db..25a0ecc7 100644 --- a/README.md +++ b/README.md @@ -235,7 +235,7 @@ near view $ASTRA_ID get_policy - `VoteRemove` - _Votes to remove given proposal or bounty (this may be because the proposal is spam or otherwise invalid)._ - `Finalize` - _Finalizes proposal which is cancelled when proposal has expired (this action also returns funds)._ - `MoveToHub` - _Moves a proposal to the hub (this is used to move a proposal into another DAO)._ - +- `ExecuteProposal` - Execute a proposal if it was not executed in final voting. --- ## Proposals From 6f4369d5423e4c021478adf5c27903ac39639207 Mon Sep 17 00:00:00 2001 From: Amit Yadav Date: Wed, 20 Sep 2023 11:17:14 +0530 Subject: [PATCH 04/14] fix --- astra/src/policy.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/astra/src/policy.rs b/astra/src/policy.rs index f707d363..f778998f 100644 --- a/astra/src/policy.rs +++ b/astra/src/policy.rs @@ -495,6 +495,7 @@ mod tests { "*:VoteReject".to_string(), "*:VoteRemove".to_string(), "*:Finalize".to_string(), + "*:ExecuteProposal".to_string(), ] .into_iter() .collect(); From e0e08c13f88c42042dffdc6b1d0e4d41d8b7c3fa Mon Sep 17 00:00:00 2001 From: Amit Yadav Date: Thu, 21 Sep 2023 13:57:32 +0530 Subject: [PATCH 05/14] Apply suggestions from code review Co-authored-by: Robert Zaremba Co-authored-by: sczembor <43810037+sczembor@users.noreply.github.com> --- astra/src/proposals.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/astra/src/proposals.rs b/astra/src/proposals.rs index 9034d0e9..75c13b46 100644 --- a/astra/src/proposals.rs +++ b/astra/src/proposals.rs @@ -570,7 +570,7 @@ impl Contract { // Updates proposal status with new votes using the policy. proposal.status = policy.proposal_status(&proposal, roles, self.total_delegation_amount); - if proposal.status == ProposalStatus::Approved && execute { + if proposal.status == ProposalStatus::Approved { self.internal_execute_proposal(&policy, &proposal, id); true } else if proposal.status == ProposalStatus::Removed && execute { @@ -615,7 +615,7 @@ impl Contract { true } Action::MoveToHub => false, - Action::ExecuteProposal => { + Action::Execute => { // Note: If proposal was executed already `proposal_status` will throw error proposal.status = policy.proposal_status(&proposal, roles, self.total_delegation_amount); let mut update = true; From 0508af12636d436802f9c7a1e29db9dcdfb01cdb Mon Sep 17 00:00:00 2001 From: Amit Yadav Date: Thu, 21 Sep 2023 14:13:06 +0530 Subject: [PATCH 06/14] updates --- README.md | 6 ++--- astra/src/lib.rs | 18 +++++++------- astra/src/policy.rs | 7 +++--- astra/src/proposals.rs | 53 ++++++++++++++++++------------------------ astra/src/types.rs | 2 +- 5 files changed, 39 insertions(+), 47 deletions(-) diff --git a/README.md b/README.md index 25a0ecc7..c1a4682a 100644 --- a/README.md +++ b/README.md @@ -173,7 +173,7 @@ export ASTRA_ID=genesis.$CONTRACT_ID near view $ASTRA_ID get_policy ``` -- Verify that the name, purpose, metadata, and council are all configured correctly. Also note the following default values: +- Verify that the name, purpose, metadata, and council are all configured correctly. Also note the following default values for roles and permissions: ```json { @@ -181,7 +181,7 @@ near view $ASTRA_ID get_policy { "name": "all", "kind": "Everyone", - "permissions": ["*:AddProposal"], + "permissions": ["*:AddProposal", "*:Execute"], "vote_policy": {} }, { @@ -235,7 +235,7 @@ near view $ASTRA_ID get_policy - `VoteRemove` - _Votes to remove given proposal or bounty (this may be because the proposal is spam or otherwise invalid)._ - `Finalize` - _Finalizes proposal which is cancelled when proposal has expired (this action also returns funds)._ - `MoveToHub` - _Moves a proposal to the hub (this is used to move a proposal into another DAO)._ -- `ExecuteProposal` - Execute a proposal if it was not executed in final voting. +- `Execute` - Execute a proposal if it was not executed in final voting. --- ## Proposals diff --git a/astra/src/lib.rs b/astra/src/lib.rs index 73417cfe..aa3a709f 100644 --- a/astra/src/lib.rs +++ b/astra/src/lib.rs @@ -328,22 +328,22 @@ mod tests { ); let id = create_proposal(&mut context, &mut contract); - contract.act_proposal(id, Action::VoteApprove, None, Some(false)); - // verify proposal wasn't approved during final vote + contract.act_proposal(id, Action::VoteApprove, None, Some(true)); + // verify proposal wasn't executed during final vote assert_eq!( contract.get_proposal(id).proposal.status, - ProposalStatus::InProgress + ProposalStatus::Approved ); - contract.act_proposal(id, Action::ExecuteProposal, None, None); + contract.act_proposal(id, Action::Execute, None, None); assert_eq!( contract.get_proposal(id).proposal.status, - ProposalStatus::Approved + ProposalStatus::Executed ); } #[test] - #[should_panic(expected = "ERR_PROPOSAL_NOT_IN_PROGRESS")] + #[should_panic(expected = "ERR_PROPOSAL_ALREADY_EXECUTED")] fn test_proposal_double_execution() { let mut context = VMContextBuilder::new(); testing_env!(context.predecessor_account_id(accounts(1)).build()); @@ -353,15 +353,15 @@ mod tests { ); let id = create_proposal(&mut context, &mut contract); - contract.act_proposal(id, Action::VoteApprove, None, None); + contract.act_proposal(id, Action::VoteApprove, None, Some(false)); // verify proposal was approved and executed assert_eq!( contract.get_proposal(id).proposal.status, - ProposalStatus::Approved + ProposalStatus::Executed ); // panics if we try to execute again - contract.act_proposal(id, Action::ExecuteProposal, None, None); + contract.act_proposal(id, Action::Execute, None, None); } #[test] diff --git a/astra/src/policy.rs b/astra/src/policy.rs index f778998f..6937d608 100644 --- a/astra/src/policy.rs +++ b/astra/src/policy.rs @@ -182,7 +182,7 @@ pub fn default_policy(council: Vec) -> Policy { RolePermission { name: "all".to_string(), kind: RoleKind::Everyone, - permissions: vec!["*:AddProposal".to_string()].into_iter().collect(), + permissions: vec!["*:AddProposal".to_string(), "*:Execute".to_string()].into_iter().collect(), vote_policy: HashMap::default(), }, RolePermission { @@ -195,7 +195,6 @@ pub fn default_policy(council: Vec) -> Policy { "*:VoteReject".to_string(), "*:VoteRemove".to_string(), "*:Finalize".to_string(), - "*:ExecuteProposal".to_string(), ] .into_iter() .collect(), @@ -386,7 +385,7 @@ impl Policy { assert!( matches!( proposal.status, - ProposalStatus::InProgress | ProposalStatus::Failed + ProposalStatus::InProgress | ProposalStatus::Failed | ProposalStatus::Approved ), "ERR_PROPOSAL_NOT_IN_PROGRESS" ); @@ -495,7 +494,7 @@ mod tests { "*:VoteReject".to_string(), "*:VoteRemove".to_string(), "*:Finalize".to_string(), - "*:ExecuteProposal".to_string(), + "*:Execute".to_string(), ] .into_iter() .collect(); diff --git a/astra/src/proposals.rs b/astra/src/proposals.rs index 75c13b46..805d926c 100644 --- a/astra/src/proposals.rs +++ b/astra/src/proposals.rs @@ -31,6 +31,7 @@ pub enum ProposalStatus { Moved, /// If proposal has failed when finalizing. Allowed to re-finalize again to either expire or approved. Failed, + Executed, } /// Function call arguments. @@ -537,8 +538,8 @@ impl Contract { /// Act on given proposal by id, if permissions allow. /// Memo is logged but not stored in the state. Can be used to leave notes or explain the action. - pub fn act_proposal(&mut self, id: u64, action: Action, memo: Option, no_execute: Option) { - let execute = no_execute.unwrap_or(true); + pub fn act_proposal(&mut self, id: u64, action: Action, memo: Option, skip_execution: Option) { + let execute = !skip_execution.unwrap_or(true); let mut proposal: Proposal = self.proposals.get(&id).expect("ERR_NO_PROPOSAL").into(); let policy = self.policy.get().unwrap().to_policy(); // Check permissions for the given action. @@ -566,25 +567,22 @@ impl Contract { self.get_user_weight(&sender_id), ); - if execute { - // Updates proposal status with new votes using the policy. - proposal.status = + // Updates proposal status with new votes using the policy. + proposal.status = policy.proposal_status(&proposal, roles, self.total_delegation_amount); - if proposal.status == ProposalStatus::Approved { - self.internal_execute_proposal(&policy, &proposal, id); - true - } else if proposal.status == ProposalStatus::Removed && execute { - self.internal_reject_proposal(&policy, &proposal, false); - self.proposals.remove(&id); - false - } else if proposal.status == ProposalStatus::Rejected && execute { - self.internal_reject_proposal(&policy, &proposal, true); - true - } else { - // Still in progress or expired. - true - } + if proposal.status == ProposalStatus::Approved && execute { + self.internal_execute_proposal(&policy, &proposal, id); + proposal.status = ProposalStatus::Executed; + true + } else if proposal.status == ProposalStatus::Removed { + self.internal_reject_proposal(&policy, &proposal, false); + self.proposals.remove(&id); + false + } else if proposal.status == ProposalStatus::Rejected { + self.internal_reject_proposal(&policy, &proposal, true); + true } else { + // Still in progress or expired. true } } @@ -604,6 +602,7 @@ impl Contract { match proposal.status { ProposalStatus::Approved => { self.internal_execute_proposal(&policy, &proposal, id); + proposal.status = ProposalStatus::Executed; } ProposalStatus::Expired => { self.internal_reject_proposal(&policy, &proposal, true); @@ -616,26 +615,20 @@ impl Contract { } Action::MoveToHub => false, Action::Execute => { - // Note: If proposal was executed already `proposal_status` will throw error + if proposal.status == ProposalStatus::Executed { + env::panic_str("ERR_PROPOSAL_ALREADY_EXECUTED"); + } proposal.status = policy.proposal_status(&proposal, roles, self.total_delegation_amount); - let mut update = true; match proposal.status { ProposalStatus::Approved => { self.internal_execute_proposal(&policy, &proposal, id); - } - ProposalStatus::Removed => { - self.internal_reject_proposal(&policy, &proposal, false); - self.proposals.remove(&id); - update = false; - } - ProposalStatus::Rejected => { - self.internal_reject_proposal(&policy, &proposal, true); + proposal.status = ProposalStatus::Executed; } _ => { // do nothing } } - update + true } }; if update { diff --git a/astra/src/types.rs b/astra/src/types.rs index df9b444e..c5264a96 100644 --- a/astra/src/types.rs +++ b/astra/src/types.rs @@ -61,7 +61,7 @@ pub enum Action { /// Move a proposal to the hub to shift into another DAO. MoveToHub, /// Execute proposal and update proposal status - ExecuteProposal + Execute } impl Action { From 068be65293da2b3df41e7316c8302acb5dbdb593 Mon Sep 17 00:00:00 2001 From: Amit Yadav Date: Thu, 21 Sep 2023 14:25:34 +0530 Subject: [PATCH 07/14] update tests --- astra/src/bounties.rs | 4 ++-- astra/src/lib.rs | 8 +++++--- astra/src/policy.rs | 1 - astra/src/proposals.rs | 6 +----- astra/src/types.rs | 2 +- astra/tests/utils/mod.rs | 2 +- 6 files changed, 10 insertions(+), 13 deletions(-) diff --git a/astra/src/bounties.rs b/astra/src/bounties.rs index 1cd04e2f..7014b436 100644 --- a/astra/src/bounties.rs +++ b/astra/src/bounties.rs @@ -229,7 +229,7 @@ mod tests { }, }); assert_eq!(contract.get_last_bounty_id(), id); - contract.act_proposal(id, Action::VoteApprove, None, None); + contract.act_proposal(id, Action::VoteApprove, None, Some(false)); id } @@ -269,7 +269,7 @@ mod tests { "bounty_done" ); - contract.act_proposal(1, Action::VoteApprove, None, None); + contract.act_proposal(1, Action::VoteApprove, None, Some(false)); testing_env!( context.build(), near_sdk::VMConfig::test(), diff --git a/astra/src/lib.rs b/astra/src/lib.rs index e117333a..41dc5c22 100644 --- a/astra/src/lib.rs +++ b/astra/src/lib.rs @@ -477,6 +477,7 @@ mod tests { let mut contract = Contract::new( Config::test_config(), VersionedPolicy::Default(vec![accounts(1)]), + ndc_trust() ); let id = create_proposal(&mut context, &mut contract); @@ -502,6 +503,7 @@ mod tests { let mut contract = Contract::new( Config::test_config(), VersionedPolicy::Default(vec![accounts(1)]), + ndc_trust() ); let id = create_proposal(&mut context, &mut contract); @@ -596,12 +598,12 @@ mod tests { // Other members vote context.predecessor_account_id = council(2); testing_env!(context.clone()); - contract.act_proposal(id, Action::VoteApprove, Some("vote on prosposal".to_string())); + contract.act_proposal(id, Action::VoteApprove, Some("vote on prosposal".to_string()), None); assert!(contract.get_proposal(id).proposal.votes.contains_key(&council(2))); context.predecessor_account_id = council(3); testing_env!(context.clone()); - contract.act_proposal(id, Action::VoteReject, Some("vote on prosposal".to_string())); + contract.act_proposal(id, Action::VoteReject, Some("vote on prosposal".to_string()), None); assert!(contract.get_proposal(id).proposal.votes.contains_key(&council(3))); // Voting body vetos @@ -612,7 +614,7 @@ mod tests { // no more members should be able to vote context.predecessor_account_id = council(4); testing_env!(context); - contract.act_proposal(id, Action::VoteApprove, Some("vote on prosposal".to_string())); + contract.act_proposal(id, Action::VoteApprove, Some("vote on prosposal".to_string()), None); } diff --git a/astra/src/policy.rs b/astra/src/policy.rs index 88d1922c..b4bed5b3 100644 --- a/astra/src/policy.rs +++ b/astra/src/policy.rs @@ -509,7 +509,6 @@ mod tests { "*:VoteReject".to_string(), "*:VoteRemove".to_string(), "*:Finalize".to_string(), - "*:Execute".to_string(), ] .into_iter() .collect(); diff --git a/astra/src/proposals.rs b/astra/src/proposals.rs index b545d9aa..a01c1891 100644 --- a/astra/src/proposals.rs +++ b/astra/src/proposals.rs @@ -541,15 +541,11 @@ impl Contract { /// Act on given proposal by id, if permissions allow. /// Memo is logged but not stored in the state. Can be used to leave notes or explain the action. -<<<<<<< HEAD pub fn act_proposal(&mut self, id: u64, action: Action, memo: Option, skip_execution: Option) { - let execute = !skip_execution.unwrap_or(true); -======= - pub fn act_proposal(&mut self, id: u64, action: Action, memo: Option) { if self.status == ContractStatus::Dissolved { panic!("Cannot perform this action, dao is dissolved!") } ->>>>>>> c5fac1d2368e58353dbf6ee3520886889121b9c2 + let execute = !skip_execution.unwrap_or(true); let mut proposal: Proposal = self.proposals.get(&id).expect("ERR_NO_PROPOSAL").into(); let policy = self.policy.get().unwrap().to_policy(); // Check permissions for the given action. diff --git a/astra/src/types.rs b/astra/src/types.rs index e41c4cce..ffffa70f 100644 --- a/astra/src/types.rs +++ b/astra/src/types.rs @@ -61,7 +61,7 @@ pub enum Action { /// Move a proposal to the hub to shift into another DAO. MoveToHub, /// Execute proposal and update proposal status - Execute + Execute, /// Veto hook VetoProposal, /// Dissovle hook diff --git a/astra/tests/utils/mod.rs b/astra/tests/utils/mod.rs index b5f6ddc5..50842d18 100644 --- a/astra/tests/utils/mod.rs +++ b/astra/tests/utils/mod.rs @@ -140,7 +140,7 @@ pub async fn vote(users: Vec, dao: &Contract, proposal_id: u64) -> anyh for user in users.into_iter() { let res = user .call(dao.id(), "act_proposal") - .args_json(json!({"id": proposal_id, "action": Action::VoteApprove})) + .args_json(json!({"id": proposal_id, "action": Action::VoteApprove, "skip_execution": false})) .max_gas() .transact() .await?; From 7d3514eed902c22dd3e47fe4ac393a9d69945916 Mon Sep 17 00:00:00 2001 From: Amit Yadav Date: Thu, 21 Sep 2023 14:42:49 +0530 Subject: [PATCH 08/14] update integration tests --- astra/tests/test_general.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/astra/tests/test_general.rs b/astra/tests/test_general.rs index 765d2b54..2d0baeaf 100644 --- a/astra/tests/test_general.rs +++ b/astra/tests/test_general.rs @@ -344,7 +344,7 @@ async fn proposal_tests() -> anyhow::Result<()> { let prop: Proposal = dao.call("get_proposal").args_json(json!({"id": 1})).view().await?.json()?; assert_eq!( prop.status, - ProposalStatus::Approved + ProposalStatus::Executed ); Ok(()) @@ -372,14 +372,14 @@ async fn test_create_dao_and_use_token() -> anyhow::Result<()> { // Voting by user who is not a member should fail. let res = user2.clone() .call(dao.id(), "act_proposal") - .args_json(json!({"id": 0, "action": Action::VoteApprove})) + .args_json(json!({"id": 0, "action": Action::VoteApprove, "skip_execution": false})) .max_gas() .transact() .await?; assert!(res.is_failure(), "{:?}", res); let res = root.clone() .call(dao.id(), "act_proposal") - .args_json(json!({"id": 0, "action": Action::VoteApprove})) + .args_json(json!({"id": 0, "action": Action::VoteApprove, "skip_execution": false})) .max_gas() .transact() .await?; @@ -387,7 +387,7 @@ async fn test_create_dao_and_use_token() -> anyhow::Result<()> { // Voting second time on the same proposal should fail. let res = root.clone() .call(dao.id(), "act_proposal") - .args_json(json!({"id": 0, "action": Action::VoteApprove})) + .args_json(json!({"id": 0, "action": Action::VoteApprove, "skip_execution": false})) .max_gas() .transact() .await?; From 68223159d1c6be4cf8260fcb2065e8d909461631 Mon Sep 17 00:00:00 2001 From: Amit Yadav Date: Thu, 21 Sep 2023 14:49:04 +0530 Subject: [PATCH 09/14] Apply suggestions from code review Co-authored-by: Robert Zaremba --- astra/src/proposals.rs | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/astra/src/proposals.rs b/astra/src/proposals.rs index a01c1891..0efc8504 100644 --- a/astra/src/proposals.rs +++ b/astra/src/proposals.rs @@ -545,7 +545,7 @@ impl Contract { if self.status == ContractStatus::Dissolved { panic!("Cannot perform this action, dao is dissolved!") } - let execute = !skip_execution.unwrap_or(true); + let execute = !skip_execution.unwrap_or(false); let mut proposal: Proposal = self.proposals.get(&id).expect("ERR_NO_PROPOSAL").into(); let policy = self.policy.get().unwrap().to_policy(); // Check permissions for the given action. @@ -621,9 +621,8 @@ impl Contract { } Action::MoveToHub => false, Action::Execute => { - if proposal.status == ProposalStatus::Executed { - env::panic_str("ERR_PROPOSAL_ALREADY_EXECUTED"); - } + require!(proposal.status == ProposalStatus::Executed, "ERR_PROPOSAL_ALREADY_EXECUTED"); + // recompute status to check if the proposal is not expired. proposal.status = policy.proposal_status(&proposal, roles, self.total_delegation_amount); match proposal.status { ProposalStatus::Approved => { From a96d09bceeea08a64e9fb0ce4b32aad4c8afd044 Mon Sep 17 00:00:00 2001 From: Amit Yadav Date: Thu, 21 Sep 2023 14:49:14 +0530 Subject: [PATCH 10/14] Apply suggestions from code review Co-authored-by: Robert Zaremba --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index a98f26e8..9db27735 100644 --- a/README.md +++ b/README.md @@ -237,7 +237,7 @@ near view $ASTRA_ID get_policy - `VoteRemove` - _Votes to remove given proposal or bounty (this may be because the proposal is spam or otherwise invalid)._ - `Finalize` - _Finalizes proposal which is cancelled when proposal has expired (this action also returns funds)._ - `MoveToHub` - _Moves a proposal to the hub (this is used to move a proposal into another DAO)._ -- `Execute` - Execute a proposal if it was not executed in final voting. +- `Execute` - Execute a proposal if it was not executed in final vote. - `VetoProposal` - Veto a proposal. Veto is instant, it will `reject` the proposal and return bond. - `Dissolve` - Dissolve this DAO: remove all members of the DAO, and sending the remaining funds back to the trust. From 381502db729b66726ae1f07605389dc86182f09d Mon Sep 17 00:00:00 2001 From: Amit Yadav Date: Thu, 21 Sep 2023 14:58:32 +0530 Subject: [PATCH 11/14] more updates --- astra/src/lib.rs | 33 +++++++++++++++------------------ astra/src/proposals.rs | 17 ++++++----------- 2 files changed, 21 insertions(+), 29 deletions(-) diff --git a/astra/src/lib.rs b/astra/src/lib.rs index 41dc5c22..7a04f50a 100644 --- a/astra/src/lib.rs +++ b/astra/src/lib.rs @@ -339,7 +339,19 @@ mod tests { } let id = create_proposal(&mut context, &mut contract); (context.build(), contract, id) - } + } + + fn setup_for_proposals() -> (VMContext, Contract, u64) { + let mut context = VMContextBuilder::new(); + testing_env!(context.predecessor_account_id(accounts(1)).build()); + let mut contract = Contract::new( + Config::test_config(), + VersionedPolicy::Default(vec![accounts(1)]), + ndc_trust() + ); + let id = create_proposal(&mut context, &mut contract); + return (context.build(), contract, id) + } #[test] fn test_basics() { @@ -472,15 +484,8 @@ mod tests { #[test] fn test_proposal_execution() { - let mut context = VMContextBuilder::new(); - testing_env!(context.predecessor_account_id(accounts(1)).build()); - let mut contract = Contract::new( - Config::test_config(), - VersionedPolicy::Default(vec![accounts(1)]), - ndc_trust() - ); + let (_, mut contract, id) = setup_for_proposals(); - let id = create_proposal(&mut context, &mut contract); contract.act_proposal(id, Action::VoteApprove, None, Some(true)); // verify proposal wasn't executed during final vote assert_eq!( @@ -498,15 +503,7 @@ mod tests { #[test] #[should_panic(expected = "ERR_PROPOSAL_ALREADY_EXECUTED")] fn test_proposal_double_execution() { - let mut context = VMContextBuilder::new(); - testing_env!(context.predecessor_account_id(accounts(1)).build()); - let mut contract = Contract::new( - Config::test_config(), - VersionedPolicy::Default(vec![accounts(1)]), - ndc_trust() - ); - - let id = create_proposal(&mut context, &mut contract); + let (_, mut contract, id) = setup_for_proposals(); contract.act_proposal(id, Action::VoteApprove, None, Some(false)); // verify proposal was approved and executed assert_eq!( diff --git a/astra/src/proposals.rs b/astra/src/proposals.rs index 0efc8504..79ba9df6 100644 --- a/astra/src/proposals.rs +++ b/astra/src/proposals.rs @@ -3,7 +3,7 @@ use std::collections::HashMap; use near_contract_standards::fungible_token::core::ext_ft_core; use near_sdk::borsh::{self, BorshDeserialize, BorshSerialize}; use near_sdk::json_types::{Base64VecU8, U128, U64}; -use near_sdk::{log, AccountId, Balance, Gas, PromiseOrValue}; +use near_sdk::{log, AccountId, Balance, Gas, PromiseOrValue, require}; use crate::policy::UserInfo; use crate::types::{ @@ -621,18 +621,13 @@ impl Contract { } Action::MoveToHub => false, Action::Execute => { - require!(proposal.status == ProposalStatus::Executed, "ERR_PROPOSAL_ALREADY_EXECUTED"); + require!(proposal.status != ProposalStatus::Executed, "ERR_PROPOSAL_ALREADY_EXECUTED"); // recompute status to check if the proposal is not expired. proposal.status = policy.proposal_status(&proposal, roles, self.total_delegation_amount); - match proposal.status { - ProposalStatus::Approved => { - self.internal_execute_proposal(&policy, &proposal, id); - proposal.status = ProposalStatus::Executed; - } - _ => { - // do nothing - } - } + require!(proposal.status == ProposalStatus::Approved, "ERR_PROPOSAL_NOT_APPROVED"); + + self.internal_execute_proposal(&policy, &proposal, id); + proposal.status = ProposalStatus::Executed; true }, Action::VetoProposal => panic!("Operation not allowed"), From 997c3c6c5f3485753d2b19b38a5e2666e54d317b Mon Sep 17 00:00:00 2001 From: Amit Yadav Date: Thu, 21 Sep 2023 19:02:08 +0530 Subject: [PATCH 12/14] fix test --- astra/src/events.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/astra/src/events.rs b/astra/src/events.rs index d6cd0a4e..f55ebc80 100644 --- a/astra/src/events.rs +++ b/astra/src/events.rs @@ -35,7 +35,7 @@ mod unit_tests { #[test] fn log_hooks() { let expected1 = r#"EVENT_JSON:{"standard":"astra++","version":"1.0.0","event":"veto","data":{"prop_id":21}}"#; - let expected2 = r#"EVENT_JSON:{"standard":"astra++","version":"1.0.0","event":"dissolve","data":"dao is dissolved"}"#; + let expected2 = r#"EVENT_JSON:{"standard":"astra++","version":"1.0.0","event":"dissolve","data":""}"#; emit_veto(21); assert_eq!(vec![expected1], test_utils::get_logs()); emit_dissolve(); From 2da2ae667cfa361506a144d6fed66331b08766ea Mon Sep 17 00:00:00 2001 From: Amit Yadav Date: Thu, 21 Sep 2023 19:03:52 +0530 Subject: [PATCH 13/14] fix --- astra/src/lib.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/astra/src/lib.rs b/astra/src/lib.rs index d1369a3d..23a00262 100644 --- a/astra/src/lib.rs +++ b/astra/src/lib.rs @@ -374,7 +374,7 @@ mod tests { contract.act_proposal(id, Action::VoteApprove, None, None); assert_eq!( contract.get_proposal(id).proposal.status, - ProposalStatus::Approved + ProposalStatus::Executed ); let id = create_proposal(&mut context, &mut contract); @@ -576,7 +576,7 @@ mod tests { testing_env!(context.clone()); contract.dissolve_hook(); - let expected = r#"EVENT_JSON:{"standard":"astra++","version":"1.0.0","event":"dissolve","data":"dao is dissolved"}"#; + let expected = r#"EVENT_JSON:{"standard":"astra++","version":"1.0.0","event":"dissolve","data":""}"#; assert_eq!(vec![expected], get_logs()); res = contract.policy.get().unwrap().to_policy(); From b00fadcba4c6074f5f365659e6fd21fc679c6540 Mon Sep 17 00:00:00 2001 From: Amit Yadav Date: Thu, 21 Sep 2023 20:09:16 +0530 Subject: [PATCH 14/14] update integration tests --- astra/tests/test_general.rs | 2 +- astra/tests/utils/mod.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/astra/tests/test_general.rs b/astra/tests/test_general.rs index 2d0baeaf..d385dd3b 100644 --- a/astra/tests/test_general.rs +++ b/astra/tests/test_general.rs @@ -436,7 +436,7 @@ async fn test_create_dao_and_use_token() -> anyhow::Result<()> { let prop: Proposal = dao.call("get_proposal").args_json(json!({"id": 2})).view().await?.json()?; assert_eq!( prop.status, - ProposalStatus::Approved + ProposalStatus::Executed ); let supply: U128 = staking.call("ft_total_supply").view().await?.json()?; diff --git a/astra/tests/utils/mod.rs b/astra/tests/utils/mod.rs index 50842d18..b5f6ddc5 100644 --- a/astra/tests/utils/mod.rs +++ b/astra/tests/utils/mod.rs @@ -140,7 +140,7 @@ pub async fn vote(users: Vec, dao: &Contract, proposal_id: u64) -> anyh for user in users.into_iter() { let res = user .call(dao.id(), "act_proposal") - .args_json(json!({"id": proposal_id, "action": Action::VoteApprove, "skip_execution": false})) + .args_json(json!({"id": proposal_id, "action": Action::VoteApprove})) .max_gas() .transact() .await?;