chore: improve proposal and pruning coverage (#83)

Improves coverage for some governance files.

Author: saiintbrisson

.github/workflows/check.yml

@@ -56,7 +56,7 @@ jobs:
       #     RUST_BACKTRACE: 1
       #     SKIP_WASM_BUILD: 1
       #   run: cargo test
-      
+
       - uses: jwalton/gh-find-current-pr@v1
         id: findPr
 
@@ -79,7 +79,7 @@ jobs:
         env:
           RUST_BACKTRACE: 1
           SKIP_WASM_BUILD: 1
-        
+
       - name: Generate coverage summary report
         if: success() && steps.findPr.outputs.number && steps.extractBranch.outputs.branch
         uses: irongut/CodeCoverageSummary@v1.3.0
@@ -98,10 +98,11 @@ jobs:
         env:
           RUST_BACKTRACE: 1
           SKIP_WASM_BUILD: 1
-      
-      - name: Upload html report to S3 Bucket 
+
+      - name: Upload html report to S3 Bucket
         if: success() && steps.findPr.outputs.number && steps.extractBranch.outputs.branch
         id: htmlUpload
+        continue-on-error: true
         run: |
           aws --endpoint-url $ENDPOINT s3 sync ./target/llvm-cov/html s3://$BUCKET_NAME/$BRANCH
           echo "link=$(aws --endpoint-url $ENDPOINT s3 presign s3://$BUCKET_NAME/$BRANCH/index.html)" >> $GITHUB_OUTPUT
@@ -112,7 +113,7 @@ jobs:
           AWS_ACCESS_KEY_ID: ${{ secrets.COV_AWS_ACCESS_KEY_ID }}
           AWS_SECRET_ACCESS_KEY: ${{ secrets.COV_AWS_SECRET_ACCESS_KEY }}
           AWS_DEFAULT_REGION: ${{ vars.COV_DEFAULT_REGION }}
-        
+
       - name: Add coverage PR report comment
         if: success() && steps.findPr.outputs.number
         uses: marocchino/sticky-pull-request-comment@v2
@@ -131,6 +132,3 @@ jobs:
           recreate: true
           message: |
             [Detailed coverage report](${{ steps.htmlUpload.outputs.link }})
-
-
-

pallets/governance/src/roles.rs

@@ -10,7 +10,8 @@ use crate::{ensure, storage::StorageMap, AccountIdOf, Config, Error};
 
 /// Generic function used to manage the Curator and Allocator maps, which behave
 /// similarly.
-pub(super) fn manage_role<T: Config, M: StorageMap<AccountIdOf<T>, ()>>(
+#[doc(hidden)]
+pub fn manage_role<T: Config, M: StorageMap<AccountIdOf<T>, ()>>(
     key: AccountIdOf<T>,
     is_add: bool,
     error: Error<T>,

pallets/governance/tests/application.rs

@@ -255,6 +255,12 @@ fn application_denied_doesnt_add_to_whitelist() {
 #[test]
 fn application_expires() {
     new_test_ext().execute_with(|| {
+        let expiration_blocks =
+            pallet_governance::GlobalGovernanceConfig::<Test>::mutate(|config| {
+                config.agent_application_expiration = 200;
+                config.agent_application_expiration
+            });
+
         let key = 0;
         let adding_key = 1;
         pallet_governance::Curators::<Test>::insert(key, ());
@@ -278,9 +284,7 @@ fn application_expires() {
             application_id = value.id;
         }
 
-        step_block(
-            pallet_governance::GlobalGovernanceConfig::<Test>::get().agent_application_expiration,
-        );
+        step_block(expiration_blocks);
 
         let application =
             pallet_governance::AgentApplications::<Test>::get(application_id).unwrap();
@@ -291,6 +295,12 @@ fn application_expires() {
 #[test]
 fn error_is_thrown_on_resolving_non_open_application() {
     new_test_ext().execute_with(|| {
+        let expiration_blocks =
+            pallet_governance::GlobalGovernanceConfig::<Test>::mutate(|config| {
+                config.agent_application_expiration = 200;
+                config.agent_application_expiration
+            });
+
         let key = 0;
         let adding_key = 1;
         pallet_governance::Curators::<Test>::insert(key, ());
@@ -345,9 +355,7 @@ fn error_is_thrown_on_resolving_non_open_application() {
         let application_id = 1;
 
         // expires the application
-        step_block(
-            pallet_governance::GlobalGovernanceConfig::<Test>::get().agent_application_expiration,
-        );
+        step_block(expiration_blocks);
         let balance_after_expire = get_balance(key);
         assert_eq!(balance_after_expire, 1);
 

pallets/governance/tests/curator.rs

@@ -0,0 +1,114 @@
+use pallet_governance::Curators;
+use polkadot_sdk::frame_support::assert_err;
+use test_utils::*;
+
+use crate::pallet_governance::Error;
+
+#[test]
+fn add_and_remove_curator() {
+    new_test_ext().execute_with(|| {
+        let curator_key = 0;
+
+        assert_ok!(pallet_governance::Pallet::<Test>::add_curator(
+            RuntimeOrigin::root(),
+            curator_key
+        ));
+
+        assert!(pallet_governance::Curators::<Test>::contains_key(
+            curator_key
+        ));
+
+        assert_ok!(pallet_governance::Pallet::<Test>::remove_curator(
+            RuntimeOrigin::root(),
+            curator_key,
+        ));
+
+        assert!(!pallet_governance::Curators::<Test>::contains_key(
+            curator_key
+        ));
+    });
+}
+
+#[test]
+fn add_and_remove_allocator() {
+    new_test_ext().execute_with(|| {
+        let allocator_key = 0;
+
+        assert_ok!(pallet_governance::Pallet::<Test>::add_allocator(
+            RuntimeOrigin::root(),
+            allocator_key
+        ));
+
+        assert!(pallet_governance::Allocators::<Test>::contains_key(
+            allocator_key
+        ));
+
+        assert_ok!(pallet_governance::Pallet::<Test>::remove_allocator(
+            RuntimeOrigin::root(),
+            allocator_key,
+        ));
+
+        assert!(!pallet_governance::Allocators::<Test>::contains_key(
+            allocator_key
+        ));
+    });
+}
+
+#[test]
+fn add_and_remove_from_whitelist() {
+    new_test_ext().execute_with(|| {
+        let curator_key = 0;
+        let module_key = 1;
+        Curators::<Test>::insert(curator_key, ());
+
+        assert_ok!(pallet_governance::Pallet::<Test>::add_to_whitelist(
+            get_origin(curator_key),
+            module_key
+        ));
+
+        assert_err!(
+            pallet_governance::Pallet::<Test>::add_to_whitelist(
+                get_origin(curator_key),
+                module_key
+            ),
+            Error::<Test>::AlreadyWhitelisted
+        );
+
+        assert!(pallet_governance::whitelist::is_whitelisted::<Test>(
+            &module_key
+        ));
+
+        assert_ok!(pallet_governance::Pallet::<Test>::remove_from_whitelist(
+            get_origin(curator_key),
+            module_key
+        ));
+
+        assert_err!(
+            pallet_governance::Pallet::<Test>::remove_from_whitelist(
+                get_origin(curator_key),
+                module_key
+            ),
+            Error::<Test>::NotWhitelisted
+        );
+
+        assert!(!pallet_governance::whitelist::is_whitelisted::<Test>(
+            &module_key
+        ));
+    });
+}
+
+#[test]
+fn only_curators_can_whitelist() {
+    new_test_ext().execute_with(|| {
+        let not_curator_key = 0;
+        let module_key = 1;
+
+        assert_err!(
+            pallet_governance::Pallet::<Test>::add_to_whitelist(
+                get_origin(not_curator_key),
+                module_key
+            ),
+            Error::<Test>::NotCurator
+        );
+    });
+}

pallets/governance/tests/roles.rs

@@ -66,9 +66,7 @@ fn delegate(account: u32) {
 }
 
 pub fn stake(account: u32, module: u32, stake: u128) {
-    // if get_balance(account) <= stake {
     add_balance(account, stake);
-    // }
 
     if get_balance(account) - stake < 1 {
         add_balance(account, 1);
@@ -136,6 +134,24 @@ fn global_proposal_validates_parameters() {
         const KEY: u32 = 0;
         add_balance(KEY, to_nano(100_000));
 
+        assert_err!(
+            pallet_governance::Pallet::<Test>::add_global_params_proposal(
+                get_origin(KEY),
+                Default::default(),
+                b"".to_vec(),
+            ),
+            Error::<Test>::ProposalDataTooSmall
+        );
+
+        assert_err!(
+            pallet_governance::Pallet::<Test>::add_global_params_proposal(
+                get_origin(KEY),
+                Default::default(),
+                b"1".repeat(257),
+            ),
+            Error::<Test>::ProposalDataTooLarge
+        );
+
         let test = |global_params| {
             pallet_governance::Pallet::<Test>::add_global_params_proposal(
                 get_origin(KEY),
@@ -150,6 +166,12 @@ fn global_proposal_validates_parameters() {
         })
         .expect_err("created proposal with invalid max name length");
 
+        test(GlobalParamsData {
+            proposal_cost: 50_000_000_000_000_000_000_001,
+            ..Default::default()
+        })
+        .expect_err("created proposal with invalid proposal cost");
+
         test(GlobalParamsData::default()).expect("failed to create proposal with valid parameters");
     });
 }
@@ -189,6 +211,57 @@ fn global_custom_proposal_is_accepted_correctly() {
                 stake_against: to_nano(5),
             }
         );
+
+        step_block(1);
+
+        assert_eq!(
+            Proposals::<Test>::get(0).unwrap().status,
+            ProposalStatus::Accepted {
+                block: 100,
+                stake_for: to_nano(10),
+                stake_against: to_nano(5),
+            }
+        );
+    });
+}
+
+#[test]
+fn removes_vote_correctly() {
+    new_test_ext().execute_with(|| {
+        zero_min_burn();
+
+        const FOR: u32 = 0;
+        const AGAINST: u32 = 1;
+
+        register(FOR, 0, 0, to_nano(10));
+        register(AGAINST, 0, 1, to_nano(10));
+
+        config(1, 100);
+
+        assert_ok!(
+            pallet_governance::Pallet::<Test>::add_global_custom_proposal(
+                get_origin(FOR),
+                b"metadata".to_vec()
+            )
+        );
+
+        vote(FOR, 0, true);
+        vote(AGAINST, 0, false);
+
+        pallet_governance::voting::remove_vote::<Test>(FOR, 0).unwrap();
+        pallet_governance::voting::remove_vote::<Test>(AGAINST, 0).unwrap();
+
+        match pallet_governance::Proposals::<Test>::get(0).unwrap().status {
+            ProposalStatus::Open {
+                votes_for,
+                votes_against,
+                ..
+            } => {
+                assert!(!votes_for.contains(&FOR));
+                assert!(!votes_against.contains(&AGAINST));
+            }
+            _ => unreachable!(),
+        }
     });
 }
 
@@ -267,20 +340,19 @@ fn global_proposals_counts_delegated_stake() {
         const FOR: u32 = 0;
         const AGAINST: u32 = 1;
         const FOR_DELEGATED: u32 = 2;
-        const AGAINST_DELEGATED: u32 = 3;
 
         let origin = get_origin(0);
 
         register(FOR, 0, FOR, to_nano(5));
-        // delegate(FOR);
         register(AGAINST, 0, AGAINST, to_nano(10));
-        // delegate(AGAINST);
 
         stake(FOR_DELEGATED, FOR, to_nano(10));
         delegate(FOR_DELEGATED);
 
-        stake(AGAINST_DELEGATED, AGAINST, to_nano(3));
-        delegate(AGAINST_DELEGATED);
+        // AGAINST does not delegate voting power, so it doesn't matter
+        // to who it stakes.
+        stake(AGAINST, FOR, to_nano(3));
+        pallet_governance::voting::disable_delegation::<Test>(AGAINST).unwrap();
 
         config(1, 100);