feat: check if application already exists for key

pallets/governance/src/application.rs

@@ -1,5 +1,5 @@
 use crate::frame::traits::ExistenceRequirement;
-use crate::{whitelist, AccountIdOf, BalanceOf, Block};
+use crate::{whitelist, AccountIdOf, AgentApplications, BalanceOf, Block};
 use codec::{Decode, Encode, MaxEncodedLen};
 use polkadot_sdk::frame_election_provider_support::Get;
 use polkadot_sdk::frame_support::dispatch::DispatchResult;
@@ -44,11 +44,19 @@ pub fn submit_application<T: crate::Config>(
 ) -> DispatchResult {
     if !removing && whitelist::is_whitelisted::<T>(&agent_key) {
         return Err(crate::Error::<T>::AlreadyWhitelisted.into());
-    } else if whitelist::is_whitelisted::<T>(&agent_key) {
+    } else if removing && !whitelist::is_whitelisted::<T>(&agent_key) {
         return Err(crate::Error::<T>::NotWhitelisted.into());
     }
 
-    // TODO: check if there is an application for the agent key already?
+    let action = if removing {
+        ApplicationAction::Remove
+    } else {
+        ApplicationAction::Add
+    };
+
+    if exists_for_agent_key::<T>(&agent_key, &action) {
+        return Err(crate::Error::<T>::ApplicationKeyAlreadyUsed.into());
+    }
 
     let config = crate::GlobalGovernanceConfig::<T>::get();
     let cost = config.agent_application_cost;
@@ -78,17 +86,10 @@ pub fn submit_application<T: crate::Config>(
 
     let expires_at = current_block + config.agent_application_expiration;
 
-    let next_id: u32 = crate::AgentApplicationId::<T>::mutate(|id| {
-        let last_id = *id;
-        *id = id.saturating_add(1);
-        last_id
-    });
-
-    let action = if removing {
-        ApplicationAction::Remove
-    } else {
-        ApplicationAction::Add
-    };
+    let next_id = AgentApplications::<T>::iter()
+        .count()
+        .try_into()
+        .map_err(|_| crate::Error::<T>::InternalError)?;
 
     let application = AgentApplication::<T> {
         id: next_id,
@@ -113,10 +114,16 @@ pub fn accept_application<T: crate::Config>(application_id: u32) -> DispatchResu
 
     match application.action {
         ApplicationAction::Add => {
-            whitelist::add_to_whitelist::<T>(application.agent_key.clone())?;
+            crate::Whitelist::<T>::insert(application.agent_key.clone(), ());
+            crate::Pallet::<T>::deposit_event(crate::Event::<T>::WhitelistAdded(
+                application.agent_key,
+            ));
         }
         ApplicationAction::Remove => {
-            whitelist::remove_from_whitelist::<T>(application.agent_key.clone())?;
+            crate::Whitelist::<T>::remove(&application.agent_key);
+            crate::Pallet::<T>::deposit_event(crate::Event::<T>::WhitelistRemoved(
+                application.agent_key,
+            ));
         }
     }
 
@@ -130,7 +137,6 @@ pub fn accept_application<T: crate::Config>(application_id: u32) -> DispatchResu
         <T as crate::Config>::Currency::deposit_creating(&application.payer_key, application.cost);
 
     crate::Pallet::<T>::deposit_event(crate::Event::<T>::ApplicationAccepted(application.id));
-    crate::Pallet::<T>::deposit_event(crate::Event::<T>::WhitelistAdded(application.agent_key));
 
     Ok(())
 }
@@ -166,8 +172,13 @@ pub(crate) fn resolver_expired_applications<T: crate::Config>(current_block: Blo
     }
 }
 
-pub(crate) fn exists_for_agent_key<T: crate::Config>(key: &AccountIdOf<T>) -> bool {
+pub(crate) fn exists_for_agent_key<T: crate::Config>(
+    key: &AccountIdOf<T>,
+    action: &ApplicationAction,
+) -> bool {
     crate::AgentApplications::<T>::iter().any(|(_, application)| {
-        application.agent_key == *key && application.status == ApplicationStatus::Open
+        application.agent_key == *key
+            && application.status == ApplicationStatus::Open
+            && application.action == *action
     })
 }

pallets/governance/src/lib.rs

@@ -64,9 +64,6 @@ pub mod pallet {
     #[pallet::storage]
     pub type AgentApplications<T: Config> = StorageMap<_, Identity, u32, AgentApplication<T>>;
 
-    #[pallet::storage]
-    pub type AgentApplicationId<T: Config> = StorageValue<_, u32, ValueQuery>;
-
     #[pallet::storage]
     pub type Whitelist<T: Config> = StorageMap<_, Identity, AccountIdOf<T>, ()>;
 

pallets/governance/src/whitelist.rs

@@ -1,12 +1,16 @@
 use polkadot_sdk::frame_support::dispatch::DispatchResult;
 
-use crate::AccountIdOf;
+use crate::{application, AccountIdOf};
 
 pub fn add_to_whitelist<T: crate::Config>(key: AccountIdOf<T>) -> DispatchResult {
     if is_whitelisted::<T>(&key) {
         return Err(crate::Error::<T>::AlreadyWhitelisted.into());
     }
 
+    if application::exists_for_agent_key::<T>(&key, &application::ApplicationAction::Add) {
+        return Err(crate::Error::<T>::ApplicationKeyAlreadyUsed.into());
+    }
+
     crate::Whitelist::<T>::insert(key.clone(), ());
     crate::Pallet::<T>::deposit_event(crate::Event::<T>::WhitelistAdded(key));
     Ok(())
@@ -17,6 +21,10 @@ pub fn remove_from_whitelist<T: crate::Config>(key: AccountIdOf<T>) -> DispatchR
         return Err(crate::Error::<T>::NotWhitelisted.into());
     }
 
+    if application::exists_for_agent_key::<T>(&key, &application::ApplicationAction::Remove) {
+        return Err(crate::Error::<T>::ApplicationKeyAlreadyUsed.into());
+    }
+
     crate::Whitelist::<T>::remove(&key);
     crate::Pallet::<T>::deposit_event(crate::Event::<T>::WhitelistRemoved(key));
     Ok(())

pallets/governance/tests/application.rs

@@ -1,10 +1,64 @@
 use pallet_governance::application::ApplicationStatus;
 use pallet_governance::AgentApplications;
 use pallet_governance::GlobalGovernanceConfig;
+use polkadot_sdk::frame_support::assert_err;
 use test_utils::*;
 
 #[test]
-fn whitelist_executes_application_correctly() {
+fn error_is_thrown_on_applying_add_already_whitelisted() {
+    new_test_ext().execute_with(|| {
+        zero_min_burn();
+
+        let key = 0;
+        let adding_key = 1;
+        pallet_governance::Curators::<Test>::insert(key, ());
+        pallet_governance::Whitelist::<Test>::insert(adding_key, ());
+
+        let proposal_cost = GlobalGovernanceConfig::<Test>::get().agent_application_cost;
+        let data = "test".as_bytes().to_vec();
+
+        add_balance(key, proposal_cost + 1);
+
+        assert_err!(
+            pallet_governance::Pallet::<Test>::submit_application(
+                get_origin(key),
+                adding_key,
+                data.clone(),
+                false
+            ),
+            pallet_governance::Error::<Test>::AlreadyWhitelisted
+        );
+    });
+}
+
+#[test]
+fn error_is_thrown_on_applying_remove_not_whitelisted() {
+    new_test_ext().execute_with(|| {
+        zero_min_burn();
+
+        let key = 0;
+        let adding_key = 1;
+        pallet_governance::Curators::<Test>::insert(key, ());
+
+        let proposal_cost = GlobalGovernanceConfig::<Test>::get().agent_application_cost;
+        let data = "test".as_bytes().to_vec();
+
+        add_balance(key, proposal_cost + 1);
+
+        assert_err!(
+            pallet_governance::Pallet::<Test>::submit_application(
+                get_origin(key),
+                adding_key,
+                data.clone(),
+                true
+            ),
+            pallet_governance::Error::<Test>::NotWhitelisted
+        );
+    });
+}
+
+#[test]
+fn whitelist_executes_application_correctly_add() {
     new_test_ext().execute_with(|| {
         zero_min_burn();
 
@@ -23,6 +77,7 @@ fn whitelist_executes_application_correctly() {
             get_origin(key),
             adding_key,
             data.clone(),
+            false
         ));
 
         let balance_after = get_balance(key);
@@ -57,6 +112,94 @@ fn whitelist_executes_application_correctly() {
     });
 }
 
+#[test]
+fn whitelist_executes_application_correctly_remove() {
+    new_test_ext().execute_with(|| {
+        zero_min_burn();
+
+        let key = 0;
+        let adding_key = 1;
+        pallet_governance::Curators::<Test>::insert(key, ());
+
+        let proposal_cost = GlobalGovernanceConfig::<Test>::get().agent_application_cost;
+        let data = "test".as_bytes().to_vec();
+
+        add_balance(key, proposal_cost + 1);
+        // first submit an application
+        let balance_before = get_balance(key);
+
+        assert_ok!(pallet_governance::Pallet::<Test>::submit_application(
+            get_origin(key),
+            adding_key,
+            data.clone(),
+            false
+        ));
+
+        let balance_after = get_balance(key);
+        assert_eq!(balance_after, balance_before - proposal_cost);
+
+        let mut application_id: u32 = u32::MAX;
+        for (_, value) in AgentApplications::<Test>::iter() {
+            assert_eq!(value.agent_key, adding_key);
+            assert_eq!(value.data, data);
+            application_id = value.id;
+        }
+
+        assert_ok!(pallet_governance::Pallet::<Test>::accept_application(
+            get_origin(key),
+            application_id
+        ));
+
+        let balance_after_accept = get_balance(key);
+
+        assert_eq!(balance_after_accept, balance_before);
+
+        assert!(pallet_governance::whitelist::is_whitelisted::<Test>(
+            &adding_key
+        ));
+
+        let application =
+            pallet_governance::AgentApplications::<Test>::get(application_id).unwrap();
+        assert_eq!(
+            application.status,
+            ApplicationStatus::Resolved { accepted: true }
+        );
+    });
+}
+
+#[test]
+fn error_is_thrown_on_multiple_applications_same_key() {
+    new_test_ext().execute_with(|| {
+        zero_min_burn();
+
+        let key = 0;
+        let adding_key = 1;
+        pallet_governance::Curators::<Test>::insert(key, ());
+
+        let proposal_cost = GlobalGovernanceConfig::<Test>::get().agent_application_cost;
+        let data = "test".as_bytes().to_vec();
+
+        add_balance(key, (proposal_cost * 2) + 1);
+
+        assert_ok!(pallet_governance::Pallet::<Test>::submit_application(
+            get_origin(key),
+            adding_key,
+            data.clone(),
+            false
+        ));
+
+        assert_err!(
+            pallet_governance::Pallet::<Test>::submit_application(
+                get_origin(key),
+                adding_key,
+                data.clone(),
+                false
+            ),
+            pallet_governance::Error::<Test>::ApplicationKeyAlreadyUsed
+        );
+    });
+}
+
 #[test]
 fn application_denied_doesnt_add_to_whitelist() {
     new_test_ext().execute_with(|| {
@@ -74,6 +217,7 @@ fn application_denied_doesnt_add_to_whitelist() {
             get_origin(key),
             adding_key,
             data.clone(),
+            false
         ));
 
         let balance_after = get_balance(key);
@@ -124,6 +268,7 @@ fn application_expires() {
             get_origin(key),
             adding_key,
             data.clone(),
+            false
         ));
 
         let mut application_id: u32 = u32::MAX;