Astra: add option to act_proposal to not execute the proposal (#5)

* add flag and action

* add tests

* add readme

* fix

* Apply suggestions from code review

Co-authored-by: Robert Zaremba <robert@zaremba.ch>
Co-authored-by: sczembor <43810037+sczembor@users.noreply.github.com>

* updates

* update tests

* update integration tests

* Apply suggestions from code review

Co-authored-by: Robert Zaremba <robert@zaremba.ch>

* Apply suggestions from code review

Co-authored-by: Robert Zaremba <robert@zaremba.ch>

* more updates

* fix  test

* fix

* update integration tests

---------

Co-authored-by: Robert Zaremba <robert@zaremba.ch>
Co-authored-by: sczembor <43810037+sczembor@users.noreply.github.com>

README.md

@@ -173,15 +173,15 @@ 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
 {
   "roles": [
     {
       "name": "all",
       "kind": "Everyone",
-      "permissions": ["*:AddProposal"],
+      "permissions": ["*:AddProposal", "*:Execute"],
       "vote_policy": {}
     },
     {
@@ -237,6 +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 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.
 

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);
+        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);
+        contract.act_proposal(1, Action::VoteApprove, None, Some(false));
         testing_env!(
             context.build(),
             near_sdk::VMConfig::test(),
@@ -284,7 +284,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(),

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();

astra/src/lib.rs

@@ -343,7 +343,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() {
@@ -359,18 +371,18 @@ 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
+            ProposalStatus::Executed
         );
 
         let id = create_proposal(&mut context, &mut contract);
         // proposal expired, finalize.
         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
@@ -402,7 +414,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]
@@ -416,7 +428,7 @@ mod tests {
         let mut contract = Contract::new(Config::test_config(), policy, accounts(1));
         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);
     }
 
@@ -433,7 +445,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]
@@ -447,8 +459,8 @@ mod tests {
             ndc_trust()
         );
         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]
@@ -468,12 +480,45 @@ 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);
     }
 
+    #[test]
+    fn test_proposal_execution() {
+        let (_, mut contract, id) = setup_for_proposals();
+
+        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::Approved
+        );
+
+        contract.act_proposal(id, Action::Execute, None, None);
+        assert_eq!(
+            contract.get_proposal(id).proposal.status,
+            ProposalStatus::Executed
+        );
+    }
+
+    #[test]
+    #[should_panic(expected = "ERR_PROPOSAL_ALREADY_EXECUTED")]
+    fn test_proposal_double_execution() {
+        let (_, mut contract, id) = setup_for_proposals();
+        contract.act_proposal(id, Action::VoteApprove, None, Some(false));
+        // verify proposal was approved and executed
+        assert_eq!(
+            contract.get_proposal(id).proposal.status,
+            ProposalStatus::Executed
+        );
+
+        // panics if we try to execute again
+        contract.act_proposal(id, Action::Execute, None, None);
+    }
+
     #[test]
     #[should_panic(expected = "ERR_INVALID_POLICY")]
     fn test_fails_adding_invalid_policy() {
@@ -531,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();
@@ -561,12 +606,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
@@ -577,7 +622,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);
     }
 
 

astra/src/policy.rs

@@ -182,7 +182,7 @@ pub fn default_policy(council: Vec<AccountId>) -> 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 {
@@ -400,7 +400,7 @@ impl Policy {
         assert!(
             matches!(
                 proposal.status,
-                ProposalStatus::InProgress | ProposalStatus::Failed
+                ProposalStatus::InProgress | ProposalStatus::Failed | ProposalStatus::Approved
             ),
             "ERR_PROPOSAL_NOT_IN_PROGRESS"
         );

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::{
@@ -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.
@@ -540,10 +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.
-    pub fn act_proposal(&mut self, id: u64, action: Action, memo: Option<String>) {
+    pub fn act_proposal(&mut self, id: u64, action: Action, memo: Option<String>, skip_execution: Option<bool>) {
         if self.status == ContractStatus::Dissolved {
             panic!("Cannot perform this action, dao is dissolved!")
         }
+        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.
@@ -570,11 +572,13 @@ impl Contract {
                     &policy,
                     self.get_user_weight(&sender_id),
                 );
+
                 // 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 {
+                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);
@@ -604,6 +608,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);
@@ -615,8 +620,19 @@ impl Contract {
                 true
             }
             Action::MoveToHub => false,
+            Action::Execute => {
+                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);
+                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"),
             Action::Dissolve => panic!("Operation not allowed"),
+
         };
         if update {
             self.proposals

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
+    Execute,
     /// Veto hook
     VetoProposal,
     /// Dissovle hook

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,22 +372,22 @@ 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?;
     assert!(res.is_success(), "{:?}", res);
     // 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?;
@@ -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()?;