From 428b398d6daaaf5d09618d29dce62980aed40174 Mon Sep 17 00:00:00 2001 From: Kelvin Steiner Date: Fri, 3 Jan 2025 11:23:02 -0300 Subject: [PATCH] fix: persist Applications instead of deleting and Remove applications (#33) Applications that are resolved (accepted/rejected) or expired will be persisted in the chain state instead of being deleted to persist the changes on the ledger and improve the consumption workflow of this data. Also, now proposals can be used to _remove_ a key from the whitelist. # Pull Request Checklist Before submitting this PR, please make sure: - [x] You have run `cargo clippy` and addressed any warnings - [ ] You have added appropriate tests (if applicable) - [ ] You have updated the documentation (if applicable) - [x] You have reviewed your own code - [ ] You have updated changelog (if applicable) ## Description Please provide a brief description of the changes in this PR. ## Related Issues Please link any related issues here --- pallets/governance/src/application.rs | 99 ++++++++++-- pallets/governance/src/lib.rs | 8 +- pallets/governance/src/whitelist.rs | 10 +- pallets/governance/tests/application.rs | 197 +++++++++++++++++++++++- 4 files changed, 291 insertions(+), 23 deletions(-) diff --git a/pallets/governance/src/application.rs b/pallets/governance/src/application.rs index 771176a..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; @@ -19,15 +19,43 @@ 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 removing && !whitelist::is_whitelisted::(&agent_key) { + return Err(crate::Error::::NotWhitelisted.into()); + } + + 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(); @@ -58,23 +86,24 @@ pub fn submit_application( let expires_at = current_block + config.agent_application_expiration; - let application_id: u32 = crate::AgentApplicationId::::mutate(|id| { - let last_id = *id; - *id = id.saturating_add(1); - last_id - }); + let next_id = AgentApplications::::iter() + .count() + .try_into() + .map_err(|_| crate::Error::::InternalError)?; 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,15 +112,32 @@ 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 => { + crate::Whitelist::::insert(application.agent_key.clone(), ()); + crate::Pallet::::deposit_event(crate::Event::::WhitelistAdded( + application.agent_key, + )); + } + ApplicationAction::Remove => { + crate::Whitelist::::remove(&application.agent_key); + crate::Pallet::::deposit_event(crate::Event::::WhitelistRemoved( + application.agent_key, + )); + } + } - 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); crate::Pallet::::deposit_event(crate::Event::::ApplicationAccepted(application.id)); - crate::Pallet::::deposit_event(crate::Event::::WhitelistAdded(application.agent_key)); Ok(()) } @@ -100,19 +146,40 @@ 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)); } } + +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 1b191de..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, ()>; @@ -150,7 +147,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 +230,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/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 6dd2605..4e06c9a 100644 --- a/pallets/governance/tests/application.rs +++ b/pallets/governance/tests/application.rs @@ -1,9 +1,119 @@ +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(); + + 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 whitelist_executes_application_correctly_remove() { new_test_ext().execute_with(|| { zero_min_burn(); @@ -22,6 +132,7 @@ fn whitelist_executes_application_correctly() { get_origin(key), adding_key, data.clone(), + false )); let balance_after = get_balance(key); @@ -46,6 +157,46 @@ 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 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 + ); }); } @@ -66,6 +217,7 @@ fn application_denied_doesnt_add_to_whitelist() { get_origin(key), adding_key, data.clone(), + false, )); let balance_after = get_balance(key); @@ -90,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); }); }