From 4859bdd22e8915c0ca121b53e60a0f72ba8bd872 Mon Sep 17 00:00:00 2001 From: Kelvin Steiner Date: Mon, 30 Dec 2024 11:42:36 -0300 Subject: [PATCH 1/2] fix: persist resolved and expired Applications instead of deleting --- pallets/governance/src/application.rs | 70 +++++++++++++++++++++---- pallets/governance/src/lib.rs | 5 +- pallets/governance/tests/application.rs | 2 + 3 files changed, 65 insertions(+), 12 deletions(-) diff --git a/pallets/governance/src/application.rs b/pallets/governance/src/application.rs index 771176a..5600b02 100644 --- a/pallets/governance/src/application.rs +++ b/pallets/governance/src/application.rs @@ -19,17 +19,37 @@ pub struct AgentApplication { pub data: BoundedVec, pub cost: BalanceOf, pub expires_at: Block, + pub action: ApplicationAction, + pub status: ApplicationStatus, +} + +#[derive(DebugNoBound, Decode, Encode, TypeInfo, MaxEncodedLen, PartialEq, Eq)] +pub enum ApplicationAction { + Add, + Remove, +} + +#[derive(DebugNoBound, Decode, Encode, TypeInfo, MaxEncodedLen, PartialEq, Eq)] +pub enum ApplicationStatus { + Open, + Resolved { accepted: bool }, + Expired, } pub fn submit_application( payer: AccountIdOf, agent_key: AccountIdOf, data: Vec, + removing: bool, ) -> DispatchResult { - if whitelist::is_whitelisted::(&agent_key) { + if !removing && whitelist::is_whitelisted::(&agent_key) { return Err(crate::Error::::AlreadyWhitelisted.into()); + } else if whitelist::is_whitelisted::(&agent_key) { + return Err(crate::Error::::NotWhitelisted.into()); } + // TODO: check if there is an application for the agent key already? + let config = crate::GlobalGovernanceConfig::::get(); let cost = config.agent_application_cost; @@ -58,23 +78,31 @@ pub fn submit_application( let expires_at = current_block + config.agent_application_expiration; - let application_id: u32 = crate::AgentApplicationId::::mutate(|id| { + 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 application = AgentApplication:: { - id: application_id, + id: next_id, payer_key: payer, agent_key, data: BoundedVec::truncate_from(data), cost, expires_at, + status: ApplicationStatus::Open, + action, }; - crate::AgentApplications::::insert(application_id, application); - crate::Pallet::::deposit_event(crate::Event::::ApplicationCreated(application_id)); + crate::AgentApplications::::insert(next_id, application); + crate::Pallet::::deposit_event(crate::Event::::ApplicationCreated(next_id)); Ok(()) } @@ -83,10 +111,22 @@ pub fn accept_application(application_id: u32) -> DispatchResu let application = crate::AgentApplications::::get(application_id) .ok_or(crate::Error::::ApplicationNotFound)?; - crate::AgentApplications::::remove(application.id); + match application.action { + ApplicationAction::Add => { + whitelist::add_to_whitelist::(application.agent_key.clone())?; + } + ApplicationAction::Remove => { + whitelist::remove_from_whitelist::(application.agent_key.clone())?; + } + } - whitelist::add_to_whitelist::(application.agent_key.clone())?; + crate::AgentApplications::::mutate(application_id, |application| { + if let Some(app) = application { + app.status = ApplicationStatus::Resolved { accepted: true }; + } + }); + // Pay the application cost back to the applicant let _ = ::Currency::deposit_creating(&application.payer_key, application.cost); @@ -100,19 +140,29 @@ pub fn deny_application(application_id: u32) -> DispatchResult let application = crate::AgentApplications::::get(application_id) .ok_or(crate::Error::::ApplicationNotFound)?; - crate::AgentApplications::::remove(application.id); + crate::AgentApplications::::mutate(application_id, |application| { + if let Some(app) = application { + app.status = ApplicationStatus::Resolved { accepted: false }; + } + }); + crate::Pallet::::deposit_event(crate::Event::::ApplicationDenied(application.id)); Ok(()) } -pub(crate) fn remove_expired_applications(current_block: Block) { +pub(crate) fn resolve_expired_applications(current_block: Block) { for application in crate::AgentApplications::::iter_values() { if current_block < application.expires_at { continue; } - crate::AgentApplications::::remove(application.id); + crate::AgentApplications::::mutate(application.id, |application| { + if let Some(app) = application { + app.status = ApplicationStatus::Expired; + } + }); + crate::Pallet::::deposit_event(crate::Event::::ApplicationExpired(application.id)); } } diff --git a/pallets/governance/src/lib.rs b/pallets/governance/src/lib.rs index 1b191de..9a06869 100644 --- a/pallets/governance/src/lib.rs +++ b/pallets/governance/src/lib.rs @@ -150,7 +150,7 @@ pub mod pallet { .ok() .expect("blockchain won't pass 2 ^ 64 blocks"); - application::remove_expired_applications::(current_block); + application::resolve_expired_applications::(current_block); proposal::tick_proposals::(current_block); proposal::tick_proposal_rewards::(current_block); @@ -233,9 +233,10 @@ pub mod pallet { origin: OriginFor, agent_key: AccountIdOf, metadata: Vec, + removing: bool, ) -> DispatchResult { let payer = ensure_signed(origin)?; - application::submit_application::(payer, agent_key, metadata) + application::submit_application::(payer, agent_key, metadata, removing) } #[pallet::call_index(10)] diff --git a/pallets/governance/tests/application.rs b/pallets/governance/tests/application.rs index 6dd2605..08eccc9 100644 --- a/pallets/governance/tests/application.rs +++ b/pallets/governance/tests/application.rs @@ -22,6 +22,7 @@ fn whitelist_executes_application_correctly() { get_origin(key), adding_key, data.clone(), + false, )); let balance_after = get_balance(key); @@ -66,6 +67,7 @@ fn application_denied_doesnt_add_to_whitelist() { get_origin(key), adding_key, data.clone(), + false, )); let balance_after = get_balance(key); From e252fff5a820e7404a6389d4e6952f899d666445 Mon Sep 17 00:00:00 2001 From: Kelvin Steiner Date: Fri, 3 Jan 2025 10:34:50 -0300 Subject: [PATCH 2/2] fix: check if application already exists for key --- pallets/governance/src/application.rs | 51 ++++--- pallets/governance/src/lib.rs | 3 - pallets/governance/src/whitelist.rs | 10 +- pallets/governance/tests/application.rs | 195 +++++++++++++++++++++++- 4 files changed, 237 insertions(+), 22 deletions(-) diff --git a/pallets/governance/src/application.rs b/pallets/governance/src/application.rs index 5600b02..6974089 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, + )); } } @@ -131,7 +138,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,3 +172,14 @@ pub(crate) fn resolve_expired_applications(current_block: Bloc crate::Pallet::::deposit_event(crate::Event::::ApplicationExpired(application.id)); } } + +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.action == *action + }) +} diff --git a/pallets/governance/src/lib.rs b/pallets/governance/src/lib.rs index 9a06869..0e2588e 100644 --- a/pallets/governance/src/lib.rs +++ b/pallets/governance/src/lib.rs @@ -68,9 +68,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 7610d8b..fadcc1f 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); let _ = pallet_torus0::agent::unregister::(key.clone()); crate::Pallet::::deposit_event(crate::Event::::WhitelistRemoved(key)); diff --git a/pallets/governance/tests/application.rs b/pallets/governance/tests/application.rs index 08eccc9..4e06c9a 100644 --- a/pallets/governance/tests/application.rs +++ b/pallets/governance/tests/application.rs @@ -1,9 +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(); @@ -47,6 +102,101 @@ fn whitelist_executes_application_correctly() { 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 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 + ); }); } @@ -92,5 +242,48 @@ fn application_denied_doesnt_add_to_whitelist() { assert!(!pallet_governance::whitelist::is_whitelisted::( &adding_key )); + + let application = + pallet_governance::AgentApplications::::get(application_id).unwrap(); + assert_eq!( + application.status, + ApplicationStatus::Resolved { accepted: false } + ); + }); +} + +#[test] +fn application_expires() { + new_test_ext().execute_with(|| { + 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_ok!(pallet_governance::Pallet::::submit_application( + get_origin(key), + adding_key, + data.clone(), + false + )); + + 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; + } + + step_block( + pallet_governance::GlobalGovernanceConfig::::get().agent_application_expiration, + ); + + let application = + pallet_governance::AgentApplications::::get(application_id).unwrap(); + assert_eq!(application.status, ApplicationStatus::Expired); }); }