diff --git a/pallets/governance/src/application.rs b/pallets/governance/src/application.rs index 2e6a2df..c4ad3ac 100644 --- a/pallets/governance/src/application.rs +++ b/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( ) -> DispatchResult { if !removing && whitelist::is_whitelisted::(&agent_key) { return Err(crate::Error::::AlreadyWhitelisted.into()); - } else if whitelist::is_whitelisted::(&agent_key) { + } else if removing && !whitelist::is_whitelisted::(&agent_key) { return Err(crate::Error::::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::(&agent_key, &action) { + return Err(crate::Error::::ApplicationKeyAlreadyUsed.into()); + } let config = crate::GlobalGovernanceConfig::::get(); let cost = config.agent_application_cost; @@ -78,17 +86,10 @@ pub fn submit_application( let expires_at = current_block + config.agent_application_expiration; - let next_id: u32 = crate::AgentApplicationId::::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::::iter() + .count() + .try_into() + .map_err(|_| crate::Error::::InternalError)?; let application = AgentApplication:: { id: next_id, @@ -113,10 +114,16 @@ pub fn accept_application(application_id: u32) -> DispatchResu match application.action { ApplicationAction::Add => { - whitelist::add_to_whitelist::(application.agent_key.clone())?; + crate::Whitelist::::insert(application.agent_key.clone(), ()); + crate::Pallet::::deposit_event(crate::Event::::WhitelistAdded( + application.agent_key, + )); } ApplicationAction::Remove => { - whitelist::remove_from_whitelist::(application.agent_key.clone())?; + crate::Whitelist::::remove(&application.agent_key); + crate::Pallet::::deposit_event(crate::Event::::WhitelistRemoved( + application.agent_key, + )); } } @@ -130,7 +137,6 @@ pub fn accept_application(application_id: u32) -> DispatchResu ::Currency::deposit_creating(&application.payer_key, application.cost); crate::Pallet::::deposit_event(crate::Event::::ApplicationAccepted(application.id)); - crate::Pallet::::deposit_event(crate::Event::::WhitelistAdded(application.agent_key)); Ok(()) } @@ -166,8 +172,13 @@ pub(crate) fn resolver_expired_applications(current_block: Blo } } -pub(crate) fn exists_for_agent_key(key: &AccountIdOf) -> bool { +pub(crate) fn exists_for_agent_key( + key: &AccountIdOf, + action: &ApplicationAction, +) -> bool { crate::AgentApplications::::iter().any(|(_, application)| { - application.agent_key == *key && application.status == ApplicationStatus::Open + application.agent_key == *key + && application.status == ApplicationStatus::Open + && application.action == *action }) } diff --git a/pallets/governance/src/lib.rs b/pallets/governance/src/lib.rs index 9570b8b..14bae96 100644 --- a/pallets/governance/src/lib.rs +++ b/pallets/governance/src/lib.rs @@ -64,9 +64,6 @@ pub mod pallet { #[pallet::storage] pub type AgentApplications = StorageMap<_, Identity, u32, AgentApplication>; - #[pallet::storage] - pub type AgentApplicationId = StorageValue<_, u32, ValueQuery>; - #[pallet::storage] pub type Whitelist = StorageMap<_, Identity, AccountIdOf, ()>; diff --git a/pallets/governance/src/whitelist.rs b/pallets/governance/src/whitelist.rs index fe0353c..0505894 100644 --- a/pallets/governance/src/whitelist.rs +++ b/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(key: AccountIdOf) -> DispatchResult { if is_whitelisted::(&key) { return Err(crate::Error::::AlreadyWhitelisted.into()); } + if application::exists_for_agent_key::(&key, &application::ApplicationAction::Add) { + return Err(crate::Error::::ApplicationKeyAlreadyUsed.into()); + } + crate::Whitelist::::insert(key.clone(), ()); crate::Pallet::::deposit_event(crate::Event::::WhitelistAdded(key)); Ok(()) @@ -17,6 +21,10 @@ pub fn remove_from_whitelist(key: AccountIdOf) -> DispatchR return Err(crate::Error::::NotWhitelisted.into()); } + if application::exists_for_agent_key::(&key, &application::ApplicationAction::Remove) { + return Err(crate::Error::::ApplicationKeyAlreadyUsed.into()); + } + crate::Whitelist::::remove(&key); crate::Pallet::::deposit_event(crate::Event::::WhitelistRemoved(key)); Ok(()) diff --git a/pallets/governance/tests/application.rs b/pallets/governance/tests/application.rs index 786132c..69880b9 100644 --- a/pallets/governance/tests/application.rs +++ b/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::::insert(key, ()); + pallet_governance::Whitelist::::insert(adding_key, ()); + + let proposal_cost = GlobalGovernanceConfig::::get().agent_application_cost; + let data = "test".as_bytes().to_vec(); + + add_balance(key, proposal_cost + 1); + + assert_err!( + pallet_governance::Pallet::::submit_application( + get_origin(key), + adding_key, + data.clone(), + false + ), + pallet_governance::Error::::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::::insert(key, ()); + + let proposal_cost = GlobalGovernanceConfig::::get().agent_application_cost; + let data = "test".as_bytes().to_vec(); + + add_balance(key, proposal_cost + 1); + + assert_err!( + pallet_governance::Pallet::::submit_application( + get_origin(key), + adding_key, + data.clone(), + true + ), + pallet_governance::Error::::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::::insert(key, ()); + + let proposal_cost = GlobalGovernanceConfig::::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::::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::::iter() { + assert_eq!(value.agent_key, adding_key); + assert_eq!(value.data, data); + application_id = value.id; + } + + assert_ok!(pallet_governance::Pallet::::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::( + &adding_key + )); + + let application = + pallet_governance::AgentApplications::::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::::insert(key, ()); + + let proposal_cost = GlobalGovernanceConfig::::get().agent_application_cost; + let data = "test".as_bytes().to_vec(); + + add_balance(key, (proposal_cost * 2) + 1); + + assert_ok!(pallet_governance::Pallet::::submit_application( + get_origin(key), + adding_key, + data.clone(), + false + )); + + assert_err!( + pallet_governance::Pallet::::submit_application( + get_origin(key), + adding_key, + data.clone(), + false + ), + pallet_governance::Error::::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;