From 973f230dde51fe70e9c0116bbabe12b8ec864918 Mon Sep 17 00:00:00 2001 From: Salman Pathan Date: Fri, 24 Nov 2023 01:17:08 +0530 Subject: [PATCH] Update role metadata structure and submit remove job request (#312) * submit clear job request to jobs handler * update metadata struct & submit remove job request * send pending job details and error --- pallets/roles/src/impls.rs | 19 +------ pallets/roles/src/lib.rs | 64 ++++++++++++++++++----- pallets/roles/src/mock.rs | 17 ++++++ pallets/roles/src/tests.rs | 97 ++++++++++++++++++----------------- primitives/src/types/jobs.rs | 14 +++++ primitives/src/types/roles.rs | 35 +++++++++---- 6 files changed, 161 insertions(+), 85 deletions(-) diff --git a/pallets/roles/src/impls.rs b/pallets/roles/src/impls.rs index dff5eb2f1..c64a1bf35 100644 --- a/pallets/roles/src/impls.rs +++ b/pallets/roles/src/impls.rs @@ -31,23 +31,8 @@ impl RolesHandler for Pallet { /// Returns `true` if the validator is permitted to work with this job type, otherwise `false`. fn is_validator(address: T::AccountId, job_key: JobKey) -> bool { let assigned_roles = AccountRolesMapping::::get(address); - - let mut found_role = false; - - for assigned_role in assigned_roles { - match job_key { - JobKey::DKG | JobKey::DKGSignature => - if assigned_role.is_tss() { - found_role = true; - }, - JobKey::ZkSaasPhaseOne | JobKey::ZkSaasPhaseTwo => - if assigned_role.is_zksaas() { - found_role = true; - }, - } - } - - return found_role + let job_role = job_key.get_role_type(); + assigned_roles.contains(&job_role) } /// Slash validator stake for the reported offence. The function should be a best effort diff --git a/pallets/roles/src/lib.rs b/pallets/roles/src/lib.rs index 77b758780..826b4f51d 100644 --- a/pallets/roles/src/lib.rs +++ b/pallets/roles/src/lib.rs @@ -30,7 +30,10 @@ use parity_scale_codec::{Decode, Encode}; use scale_info::TypeInfo; use sp_runtime::{codec, traits::Zero, Saturating}; use sp_std::{convert::TryInto, prelude::*, vec}; -use tangle_primitives::roles::RoleType; +use tangle_primitives::{ + roles::{RoleType, RoleTypeMetadata}, + traits::jobs::JobsHandler, +}; mod impls; #[cfg(test)] pub(crate) mod mock; @@ -66,8 +69,8 @@ pub struct RoleStakingLedger { #[derive(PartialEqNoBound, EqNoBound, Encode, Decode, RuntimeDebugNoBound, TypeInfo, Clone)] #[scale_info(skip_type_params(T))] pub struct RoleStakingRecord { - /// Role type - pub role: RoleType, + /// Metadata associated with the role. + pub metadata: RoleTypeMetadata, /// The total amount of the stash's balance that is re-staked for selected role. #[codec(compact)] pub re_staked: BalanceOf, @@ -94,7 +97,10 @@ pub mod pallet { use super::*; use frame_support::pallet_prelude::*; use frame_system::pallet_prelude::*; - use tangle_primitives::traits::jobs::MPCHandler; + use tangle_primitives::{ + jobs::{JobId, JobKey}, + traits::jobs::MPCHandler, + }; #[pallet::pallet] #[pallet::without_storage_info] @@ -106,6 +112,10 @@ pub mod pallet { /// The overarching event type. type RuntimeEvent: From> + IsType<::RuntimeEvent>; + /// The job manager mechanism. + type JobsHandler: JobsHandler; + + /// Max roles per account. #[pallet::constant] type MaxRolesPerAccount: Get; @@ -124,6 +134,8 @@ pub mod pallet { RoleRemoved { account: T::AccountId, role: RoleType }, /// Slashed validator. Slashed { account: T::AccountId, amount: BalanceOf }, + /// Pending jobs,that cannot be opted out at the moment. + PendingJobs { pending_jobs: Vec<(JobKey, JobId)> }, } #[pallet::error] @@ -144,6 +156,8 @@ pub mod pallet { AccountAlreadyPaired, /// Stash controller account not found in Roles Ledger. AccountNotPaired, + /// Role clear request failed due to pending jobs, which can't be opted out at the moment. + RoleClearRequestFailed, } /// Map from all "controller" accounts to the info regarding the staking. @@ -204,7 +218,7 @@ pub mod pallet { // Validate role staking records. for record in records.clone() { - let role = record.role; + let role = record.metadata.get_role_type(); let re_stake_amount = record.re_staked; // Check if role is already assigned. ensure!( @@ -215,7 +229,7 @@ pub mod pallet { // validate the metadata T::MPCHandler::validate_authority_key( stash_account.clone(), - role.clone().get_authority_key(), + record.metadata.get_authority_key(), )?; // Re-staking amount of record should meet min re-staking amount requirement. @@ -232,16 +246,16 @@ pub mod pallet { ); ledger.total = ledger.total.saturating_add(re_stake_amount); - let role_info = RoleStakingRecord { role, re_staked: re_stake_amount }; - ledger.roles.push(role_info); + ledger.roles.push(record); } // Now that records are validated we can add them and update ledger for record in records { - Self::add_role(stash_account.clone(), record.role.clone())?; + let role = record.metadata.get_role_type(); + Self::add_role(stash_account.clone(), role.clone())?; Self::deposit_event(Event::::RoleAssigned { account: stash_account.clone(), - role: record.role, + role, }); } Self::update_ledger(&stash_account, &ledger); @@ -276,9 +290,33 @@ pub mod pallet { Error::::NoRoleAssigned ); - // TODO: Call jobs manager to remove the services. - // On successful removal of services, remove the role from the mapping. - // Issue link for reference : https://github.com/webb-tools/tangle/issues/292 + // Get active jobs for the role. + let active_jobs = T::JobsHandler::get_active_jobs(stash_account.clone()); + let mut role_cleared = true; + let mut pending_jobs = Vec::new(); + for job in active_jobs { + let job_key = job.0; + if job_key.get_role_type() == role { + // Submit request to exit from the known set. + let res = T::JobsHandler::exit_from_known_set( + stash_account.clone(), + job_key.clone(), + job.1, + ); + + if res.is_err() { + role_cleared = false; + pending_jobs.push((job_key.clone(), job.1)); + } + } + } + + if !role_cleared { + // Role clear request failed due to pending jobs, which can't be opted out at the + // moment. + Self::deposit_event(Event::::PendingJobs { pending_jobs }); + return Err(Error::::RoleClearRequestFailed.into()) + }; // Remove role from the mapping. Self::remove_role(stash_account.clone(), role.clone())?; diff --git a/pallets/roles/src/mock.rs b/pallets/roles/src/mock.rs index 1cf3de7fb..760badba1 100644 --- a/pallets/roles/src/mock.rs +++ b/pallets/roles/src/mock.rs @@ -227,8 +227,25 @@ impl pallet_staking::Config for Runtime { type WeightInfo = (); } +pub struct MockJobsHandler; + +impl JobsHandler for MockJobsHandler { + fn get_active_jobs(_validator: AccountId) -> Vec<(JobKey, JobId)> { + Default::default() + } + + fn exit_from_known_set( + _validator: AccountId, + _job_key: JobKey, + _job_id: JobId, + ) -> sp_runtime::DispatchResult { + Ok(()) + } +} + impl Config for Runtime { type RuntimeEvent = RuntimeEvent; + type JobsHandler = MockJobsHandler; type MaxRolesPerAccount = ConstU32<2>; type MPCHandler = MockMPCHandler; type WeightInfo = (); diff --git a/pallets/roles/src/tests.rs b/pallets/roles/src/tests.rs index 511715642..8dbfe0b2d 100644 --- a/pallets/roles/src/tests.rs +++ b/pallets/roles/src/tests.rs @@ -24,29 +24,24 @@ fn test_assign_roles() { // Initially account if funded with 10000 tokens and we are trying to bond 5000 tokens // Roles user is interested in re-staking. - let role_records = - vec![RoleStakingRecord { role: RoleType::Tss(Default::default()), re_staked: 5000 }]; + let role_records = vec![RoleStakingRecord { + metadata: RoleTypeMetadata::Tss(Default::default()), + re_staked: 5000, + }]; - assert_ok!(Roles::assign_roles(RuntimeOrigin::signed(1), role_records)); + assert_ok!(Roles::assign_roles(RuntimeOrigin::signed(1), role_records.clone())); assert_events(vec![RuntimeEvent::Roles(crate::Event::RoleAssigned { account: 1, - role: RoleType::Tss(Default::default()), + role: RoleType::Tss, })]); // Lets verify role assigned to account. - assert_eq!(Roles::has_role(1, RoleType::Tss(Default::default())), true); + assert_eq!(Roles::has_role(1, RoleType::Tss), true); // Verify ledger mapping assert_eq!( Roles::ledger(1), - Some(RoleStakingLedger { - stash: 1, - total: 5000, - roles: vec![RoleStakingRecord { - role: RoleType::Tss(Default::default()), - re_staked: 5000 - }] - }) + Some(RoleStakingLedger { stash: 1, total: 5000, roles: role_records }) ); }); } @@ -59,31 +54,27 @@ fn test_assign_multiple_roles() { // Roles user is interested in re-staking. let role_records = vec![ - RoleStakingRecord { role: RoleType::Tss(Default::default()), re_staked: 2500 }, - RoleStakingRecord { role: RoleType::ZkSaas(Default::default()), re_staked: 2500 }, + RoleStakingRecord { + metadata: RoleTypeMetadata::Tss(Default::default()), + re_staked: 2500, + }, + RoleStakingRecord { + metadata: RoleTypeMetadata::ZkSaas(Default::default()), + re_staked: 2500, + }, ]; - assert_ok!(Roles::assign_roles(RuntimeOrigin::signed(1), role_records)); + assert_ok!(Roles::assign_roles(RuntimeOrigin::signed(1), role_records.clone())); // Lets verify role assigned to account. - assert_eq!(Roles::has_role(1, RoleType::Tss(Default::default())), true); + assert_eq!(Roles::has_role(1, RoleType::Tss), true); // Lets verify role assigned to account. - assert_eq!(Roles::has_role(1, RoleType::ZkSaas(Default::default())), true); + assert_eq!(Roles::has_role(1, RoleType::ZkSaas), true); assert_eq!( Roles::ledger(1), - Some(RoleStakingLedger { - stash: 1, - total: 5000, - roles: vec![ - RoleStakingRecord { role: RoleType::Tss(Default::default()), re_staked: 2500 }, - RoleStakingRecord { - role: RoleType::ZkSaas(Default::default()), - re_staked: 2500 - }, - ] - }) + Some(RoleStakingLedger { stash: 1, total: 5000, roles: role_records }) ); }); } @@ -97,8 +88,14 @@ fn test_assign_roles_should_fail_if_total_re_stake_value_exceeds_max_re_stake_va // Roles user is interested in re-staking. let role_records = vec![ - RoleStakingRecord { role: RoleType::Tss(Default::default()), re_staked: 5000 }, - RoleStakingRecord { role: RoleType::ZkSaas(Default::default()), re_staked: 5000 }, + RoleStakingRecord { + metadata: RoleTypeMetadata::Tss(Default::default()), + re_staked: 5000, + }, + RoleStakingRecord { + metadata: RoleTypeMetadata::ZkSaas(Default::default()), + re_staked: 5000, + }, ]; // Since max re_stake limit is 5000 it should fail with `ExceedsMaxReStakeValue` error. assert_err!( @@ -114,21 +111,23 @@ fn test_clear_role() { // Initially account if funded with 10000 tokens and we are trying to bond 5000 tokens // Roles user is interested in re-staking. - let role_records = - vec![RoleStakingRecord { role: RoleType::Tss(Default::default()), re_staked: 5000 }]; + let role_records = vec![RoleStakingRecord { + metadata: RoleTypeMetadata::Tss(Default::default()), + re_staked: 5000, + }]; assert_ok!(Roles::assign_roles(RuntimeOrigin::signed(1), role_records)); // Now lets clear the role - assert_ok!(Roles::clear_role(RuntimeOrigin::signed(1), RoleType::Tss(Default::default()))); + assert_ok!(Roles::clear_role(RuntimeOrigin::signed(1), RoleType::Tss)); assert_events(vec![RuntimeEvent::Roles(crate::Event::RoleRemoved { account: 1, - role: RoleType::Tss(Default::default()), + role: RoleType::Tss, })]); // Role should be removed from account role mappings. - assert_eq!(Roles::has_role(1, RoleType::Tss(Default::default())), false); + assert_eq!(Roles::has_role(1, RoleType::Tss), false); // Ledger should be removed from ledger mappings. assert_eq!(Roles::ledger(1), None); @@ -141,8 +140,10 @@ fn test_assign_roles_should_fail_if_not_validator() { // we will use account 5 which is not a validator // Roles user is interested in re-staking. - let role_records = - vec![RoleStakingRecord { role: RoleType::Tss(Default::default()), re_staked: 5000 }]; + let role_records = vec![RoleStakingRecord { + metadata: RoleTypeMetadata::Tss(Default::default()), + re_staked: 5000, + }]; assert_err!( Roles::assign_roles(RuntimeOrigin::signed(5), role_records), @@ -158,19 +159,21 @@ fn test_unbound_funds_should_work() { // for providing TSS services. // Roles user is interested in re-staking. - let role_records = - vec![RoleStakingRecord { role: RoleType::Tss(Default::default()), re_staked: 5000 }]; + let role_records = vec![RoleStakingRecord { + metadata: RoleTypeMetadata::Tss(Default::default()), + re_staked: 5000, + }]; assert_ok!(Roles::assign_roles(RuntimeOrigin::signed(1), role_records)); // Lets verify role is assigned to account. - assert_eq!(Roles::has_role(1, RoleType::Tss(Default::default())), true); + assert_eq!(Roles::has_role(1, RoleType::Tss), true); // Lets clear the role. - assert_ok!(Roles::clear_role(RuntimeOrigin::signed(1), RoleType::Tss(Default::default()))); + assert_ok!(Roles::clear_role(RuntimeOrigin::signed(1), RoleType::Tss)); // Role should be removed from account role mappings. - assert_eq!(Roles::has_role(1, RoleType::Tss(Default::default())), false); + assert_eq!(Roles::has_role(1, RoleType::Tss), false); // unbound funds. assert_ok!(Roles::unbound_funds(RuntimeOrigin::signed(1), 5000)); @@ -195,13 +198,15 @@ fn test_unbound_funds_should_fail_if_role_assigned() { // for providing TSS services. // Roles user is interested in re-staking. - let role_records = - vec![RoleStakingRecord { role: RoleType::Tss(Default::default()), re_staked: 5000 }]; + let role_records = vec![RoleStakingRecord { + metadata: RoleTypeMetadata::Tss(Default::default()), + re_staked: 5000, + }]; assert_ok!(Roles::assign_roles(RuntimeOrigin::signed(1), role_records)); // Lets verify role is assigned to account. - assert_eq!(Roles::has_role(1, RoleType::Tss(Default::default())), true); + assert_eq!(Roles::has_role(1, RoleType::Tss), true); // Lets try to unbound funds. assert_err!( diff --git a/primitives/src/types/jobs.rs b/primitives/src/types/jobs.rs index 520c603ba..345e14b0c 100644 --- a/primitives/src/types/jobs.rs +++ b/primitives/src/types/jobs.rs @@ -17,6 +17,8 @@ use frame_support::{dispatch::Vec, pallet_prelude::*, RuntimeDebug}; #[cfg(feature = "std")] use serde::{Deserialize, Serialize}; +use crate::roles::RoleType; + pub type JobId = u32; /// Represents a job submission with specified `AccountId` and `BlockNumber`. @@ -199,6 +201,18 @@ pub enum JobKey { ZkSaasPhaseTwo, } +impl JobKey { + /// Returns role assigned with the job. + pub fn get_role_type(&self) -> RoleType { + match self { + JobKey::DKG => RoleType::Tss, + JobKey::DKGSignature => RoleType::Tss, + JobKey::ZkSaasPhaseOne => RoleType::ZkSaas, + JobKey::ZkSaasPhaseTwo => RoleType::ZkSaas, + } + } +} + /// Represents a job submission with specified `AccountId` and `BlockNumber`. #[derive(PartialEq, Eq, Encode, Decode, RuntimeDebug, TypeInfo, Clone)] pub struct PhaseOneResult { diff --git a/primitives/src/types/roles.rs b/primitives/src/types/roles.rs index f0fbc53d2..f1d935757 100644 --- a/primitives/src/types/roles.rs +++ b/primitives/src/types/roles.rs @@ -18,25 +18,42 @@ use frame_support::{dispatch::Vec, pallet_prelude::*}; /// Role type to be used in the system. #[derive(Encode, Decode, Clone, Debug, PartialEq, Eq, TypeInfo)] pub enum RoleType { - Tss(TssRoleMetadata), - ZkSaas(ZkSaasRoleMetadata), + Tss, + ZkSaas, } impl RoleType { /// Checks if the role type is a TSS role. - pub fn is_tss(self) -> bool { - matches!(self, RoleType::Tss(_)) + pub fn is_tss(&self) -> bool { + matches!(self, RoleType::Tss) } /// Checks if the role type is a Zk-Saas role. - pub fn is_zksaas(self) -> bool { - matches!(self, RoleType::ZkSaas(_)) + pub fn is_zksaas(&self) -> bool { + matches!(self, RoleType::ZkSaas) + } +} + +/// Metadata associated with a role type. +#[derive(Encode, Decode, Clone, Debug, PartialEq, Eq, TypeInfo)] +pub enum RoleTypeMetadata { + Tss(TssRoleMetadata), + ZkSaas(ZkSaasRoleMetadata), +} + +impl RoleTypeMetadata { + /// Return type of role. + pub fn get_role_type(&self) -> RoleType { + match self { + RoleTypeMetadata::Tss(_) => RoleType::Tss, + RoleTypeMetadata::ZkSaas(_) => RoleType::ZkSaas, + } } - pub fn get_authority_key(self) -> Vec { + pub fn get_authority_key(&self) -> Vec { match self { - RoleType::Tss(metadata) => metadata.authority_key, - RoleType::ZkSaas(metadata) => metadata.authority_key, + RoleTypeMetadata::Tss(metadata) => metadata.authority_key.clone(), + RoleTypeMetadata::ZkSaas(metadata) => metadata.authority_key.clone(), } } }