diff --git a/pallets/authority-members/src/lib.rs b/pallets/authority-members/src/lib.rs index b69c0d72993f568c507dbad2b5f9e773eb791f8b..b6c1bd13c23e32253b38eac3ffbd823bc45ec9ce 100644 --- a/pallets/authority-members/src/lib.rs +++ b/pallets/authority-members/src/lib.rs @@ -258,17 +258,17 @@ pub mod pallet { if !Members::<T>::contains_key(member_id) { return Err(Error::<T>::MemberNotFound.into()); } - if Self::is_outgoing(member_id) { + if Self::is_outgoing(&member_id) { return Err(Error::<T>::AlreadyOutgoing.into()); } - let is_incoming = Self::is_incoming(member_id); - if !is_incoming && !Self::is_online(member_id) { + let is_incoming = Self::is_incoming(&member_id); + if !is_incoming && !Self::is_online(&member_id) { return Err(Error::<T>::NotOnlineNorIncoming.into()); } // Apply phase // if is_incoming { - Self::remove_in(member_id); + Self::remove_in(&member_id); } else { Self::insert_out(member_id); } @@ -284,7 +284,7 @@ pub mod pallet { let who = ensure_signed(origin)?; let member_id = Self::verify_ownership_and_membership(&who)?; - if Self::is_blacklisted(member_id) { + if Self::is_blacklisted(&member_id) { return Err(Error::<T>::MemberBlacklisted.into()); } if !Members::<T>::contains_key(member_id) { @@ -296,11 +296,11 @@ pub mod pallet { return Err(Error::<T>::SessionKeysNotProvided.into()); } - if Self::is_incoming(member_id) { + if Self::is_incoming(&member_id) { return Err(Error::<T>::AlreadyIncoming.into()); } - let is_outgoing = Self::is_outgoing(member_id); - if Self::is_online(member_id) && !is_outgoing { + let is_outgoing = Self::is_outgoing(&member_id); + if Self::is_online(&member_id) && !is_outgoing { return Err(Error::<T>::AlreadyOnline.into()); } if Self::authorities_counter() >= T::MaxAuthorities::get() { @@ -309,7 +309,7 @@ pub mod pallet { // Apply phase // if is_outgoing { - Self::remove_out(member_id); + Self::remove_out(&member_id); } else { Self::insert_in(member_id); } @@ -378,25 +378,25 @@ pub mod pallet { // Execute the annotated function in a new storage transaction. // The return type of the annotated function must be `Result`. All changes to storage performed // by the annotated function are discarded if it returns `Err`, or committed if `Ok`. - #[frame_support::transactional] /// Change the owner key of an authority member. + #[frame_support::transactional] pub fn change_owner_key( member_id: T::MemberId, new_owner_key: T::AccountId, ) -> DispatchResultWithPostInfo { - let old_owner_key = Members::<T>::mutate_exists(member_id, |maybe_member_data| { - if let Some(ref mut member_data) = maybe_member_data { - Ok(core::mem::replace( - &mut member_data.owner_key, - new_owner_key.clone(), - )) - } else { - Err(Error::<T>::MemberNotFound) - } + let old_owner_key = Members::<T>::try_mutate_exists(member_id, |maybe_member_data| { + let member_data = maybe_member_data + .as_mut() + .ok_or(Error::<T>::MemberNotFound)?; + Ok::<T::AccountId, Error<T>>(core::mem::replace( + &mut member_data.owner_key, + new_owner_key.clone(), + )) })?; let validator_id = T::ValidatorIdOf::convert(old_owner_key.clone()) .ok_or(pallet_session::Error::<T>::NoAssociatedValidatorId)?; + let session_keys = pallet_session::NextKeys::<T>::get(validator_id) .ok_or(Error::<T>::SessionKeysNotProvided)?; @@ -405,7 +405,7 @@ pub mod pallet { .dispatch_bypass_filter(frame_system::RawOrigin::Signed(old_owner_key).into())?; // Set session keys - let _post_info = pallet_session::Call::<T>::set_keys { + pallet_session::Call::<T>::set_keys { keys: session_keys, proof: vec![], } @@ -427,14 +427,14 @@ pub mod pallet { impl<T: Config> Pallet<T> { /// Perform authority member removal. fn do_remove_member(member_id: T::MemberId, owner_key: T::AccountId) { - if Self::is_online(member_id) { + if Self::is_online(&member_id) { // Trigger the member deletion for next session Self::insert_out(member_id); } // remove all member data - Self::remove_in(member_id); - Self::remove_online(member_id); + Self::remove_in(&member_id); + Self::remove_online(&member_id); Members::<T>::remove(member_id); // Purge session keys @@ -485,53 +485,53 @@ pub mod pallet { } /// Check if member is incoming. - fn is_incoming(member_id: T::MemberId) -> bool { + fn is_incoming(member_id: &T::MemberId) -> bool { IncomingAuthorities::<T>::get() - .binary_search(&member_id) + .binary_search(member_id) .is_ok() } - /// C&heck if member is online. - fn is_online(member_id: T::MemberId) -> bool { + /// Check if member is online. + fn is_online(member_id: &T::MemberId) -> bool { OnlineAuthorities::<T>::get() - .binary_search(&member_id) + .binary_search(member_id) .is_ok() } /// Check if member is outgoing. - fn is_outgoing(member_id: T::MemberId) -> bool { + fn is_outgoing(member_id: &T::MemberId) -> bool { OutgoingAuthorities::<T>::get() - .binary_search(&member_id) + .binary_search(member_id) .is_ok() } /// Check if member is blacklisted. - fn is_blacklisted(member_id: T::MemberId) -> bool { - Blacklist::<T>::get().contains(&member_id) + fn is_blacklisted(member_id: &T::MemberId) -> bool { + Blacklist::<T>::get().contains(member_id) } /// Perform removal from incoming authorities. - fn remove_in(member_id: T::MemberId) { + fn remove_in(member_id: &T::MemberId) { IncomingAuthorities::<T>::mutate(|members_ids| { - if let Ok(index) = members_ids.binary_search(&member_id) { + if let Ok(index) = members_ids.binary_search(member_id) { members_ids.remove(index); } }) } /// Perform removal from online authorities. - fn remove_online(member_id: T::MemberId) { + fn remove_online(member_id: &T::MemberId) { OnlineAuthorities::<T>::mutate(|members_ids| { - if let Ok(index) = members_ids.binary_search(&member_id) { + if let Ok(index) = members_ids.binary_search(member_id) { members_ids.remove(index); } }); } /// Perform removal from outgoing authorities. - fn remove_out(member_id: T::MemberId) { + fn remove_out(member_id: &T::MemberId) { OutgoingAuthorities::<T>::mutate(|members_ids| { - if let Ok(index) = members_ids.binary_search(&member_id) { + if let Ok(index) = members_ids.binary_search(member_id) { members_ids.remove(index); } }); diff --git a/pallets/distance/src/mock.rs b/pallets/distance/src/mock.rs index 06b89976b8ef40c36cb03ad5cf1718a023eeac2a..23f8652124bfcdaf381c892400322dd74920916d 100644 --- a/pallets/distance/src/mock.rs +++ b/pallets/distance/src/mock.rs @@ -262,9 +262,9 @@ impl pallet_identity::Config for Test { type IdtyData = (); type IdtyIndex = u32; type IdtyNameValidator = IdtyNameValidatorTestImpl; + type OnKeyChange = (); type OnNewIdty = (); type OnRemoveIdty = (); - type OwnerKeyChangePermission = (); type RuntimeEvent = RuntimeEvent; type Signature = TestSignature; type Signer = UintAuthorityId; diff --git a/pallets/duniter-wot/src/mock.rs b/pallets/duniter-wot/src/mock.rs index e246133a0353c68fbeda9c3566dd20b5485de6a1..0685fc5d3466da6cabec09f14d0402b73cbdecce 100644 --- a/pallets/duniter-wot/src/mock.rs +++ b/pallets/duniter-wot/src/mock.rs @@ -148,9 +148,9 @@ impl pallet_identity::Config for Test { type IdtyData = (); type IdtyIndex = IdtyIndex; type IdtyNameValidator = IdtyNameValidatorTestImpl; + type OnKeyChange = (); type OnNewIdty = DuniterWot; type OnRemoveIdty = DuniterWot; - type OwnerKeyChangePermission = (); type RuntimeEvent = RuntimeEvent; type Signature = TestSignature; type Signer = UintAuthorityId; diff --git a/pallets/identity/src/lib.rs b/pallets/identity/src/lib.rs index 0e00c7e7a061ca8d70c7c716dddc116d3a045802..4a2c808770a81ecc9e795dc10110fbb057342f50 100644 --- a/pallets/identity/src/lib.rs +++ b/pallets/identity/src/lib.rs @@ -137,8 +137,8 @@ pub mod pallet { /// The type used to check account worthiness. type CheckAccountWorthiness: CheckAccountWorthiness<Self>; - /// Handler that checks the necessary permissions for an identity's owner key change. - type OwnerKeyChangePermission: CheckKeyChangeAllowed<Self>; + /// A handler called when an identity's owner key change. + type OnKeyChange: KeyChange<Self>; /// Custom data to store in each identity. type IdtyData: Clone @@ -461,12 +461,6 @@ pub mod pallet { Error::<T>::OwnerKeyAlreadyUsed ); - // Ensure that the key is not currently as a validator - ensure!( - T::OwnerKeyChangePermission::check_allowed(&idty_index), - Error::<T>::OwnerKeyUsedAsValidator - ); - let block_number = frame_system::Pallet::<T>::block_number(); let maybe_old_old_owner_key = if let Some((old_owner_key, last_change)) = idty_value.old_owner_key { @@ -508,6 +502,7 @@ pub mod pallet { IdentityIndexOf::<T>::insert(&idty_value.owner_key, idty_index); Identities::<T>::insert(idty_index, idty_value); T::AccountLinker::link_identity(&new_key, idty_index)?; + T::OnKeyChange::on_changed(idty_index, new_key.clone())?; Self::deposit_event(Event::IdtyChangedOwnerKey { idty_index, new_owner_key: new_key, @@ -793,6 +788,10 @@ pub mod pallet { IdtyNotFound, /// Invalid payload signature. InvalidSignature, + /// Key used as validator. + OwnerKeyUsedAsValidator, + /// Key in bound period. + OwnerKeyInBound, /// Invalid revocation key. InvalidRevocationKey, /// Issuer is not member and can not perform this action. @@ -815,8 +814,6 @@ pub mod pallet { AccountNotExist, /// Insufficient balance to create an identity. InsufficientBalance, - /// Owner key currently used as validator. - OwnerKeyUsedAsValidator, /// Legacy revocation document format is invalid InvalidLegacyRevocationFormat, } diff --git a/pallets/identity/src/mock.rs b/pallets/identity/src/mock.rs index de48f0a2e24cf7ba19dc423a52bf03b306a64dee..f38afd0975a3022e223ffaa47a4e8ba74a406807 100644 --- a/pallets/identity/src/mock.rs +++ b/pallets/identity/src/mock.rs @@ -101,9 +101,9 @@ impl pallet_identity::Config for Test { type IdtyData = (); type IdtyIndex = u64; type IdtyNameValidator = IdtyNameValidatorTestImpl; + type OnKeyChange = (); type OnNewIdty = (); type OnRemoveIdty = (); - type OwnerKeyChangePermission = (); type RuntimeEvent = RuntimeEvent; type Signature = Signature; type Signer = AccountPublic; diff --git a/pallets/identity/src/traits.rs b/pallets/identity/src/traits.rs index 8366d1cff76022599feb2d118e622128cc12a59e..9aca1dc64539c05edcd971b9ba07a9737830e5f1 100644 --- a/pallets/identity/src/traits.rs +++ b/pallets/identity/src/traits.rs @@ -95,14 +95,14 @@ impl<AccountId, IdtyIndex> LinkIdty<AccountId, IdtyIndex> for () { } } -/// Trait for checking whether a key change is allowed for a given identity. -pub trait CheckKeyChangeAllowed<T: Config> { - /// Determines if a key change is allowed for the given identity. - fn check_allowed(account_id: &T::IdtyIndex) -> bool; +/// A trait for handling identity owner key changes. +pub trait KeyChange<T: Config> { + fn on_changed(id: T::IdtyIndex, account_id: T::AccountId) -> Result<(), DispatchError>; } -impl<T: Config> CheckKeyChangeAllowed<T> for () { - fn check_allowed(_: &T::IdtyIndex) -> bool { - true +impl<T: Config> KeyChange<T> for () { + /// Called when an identity's owner key has changed. + fn on_changed(_id: T::IdtyIndex, _account_id: T::AccountId) -> Result<(), DispatchError> { + Ok(()) } } diff --git a/pallets/quota/src/mock.rs b/pallets/quota/src/mock.rs index dcfd30f778936b5d9cc55b5e2195da82ffa1c806..029192f0befc9075f5d080834c51c6112cacc743 100644 --- a/pallets/quota/src/mock.rs +++ b/pallets/quota/src/mock.rs @@ -145,9 +145,9 @@ impl pallet_identity::Config for Test { type IdtyData = (); type IdtyIndex = u64; type IdtyNameValidator = IdtyNameValidatorTestImpl; + type OnKeyChange = (); type OnNewIdty = (); type OnRemoveIdty = (); - type OwnerKeyChangePermission = (); type RuntimeEvent = RuntimeEvent; type Signature = Signature; type Signer = AccountPublic; diff --git a/pallets/smith-members/src/lib.rs b/pallets/smith-members/src/lib.rs index d9246b0cc506c6f644cacd2048aef11a6d6d104c..25e44c058bf230e723a8e773ea5ac7b469b6034d 100644 --- a/pallets/smith-members/src/lib.rs +++ b/pallets/smith-members/src/lib.rs @@ -59,7 +59,10 @@ use frame_support::{ ensure, pallet_prelude::{Get, RuntimeDebug, Weight}, }; -use frame_system::{ensure_signed, pallet_prelude::OriginFor}; +use frame_system::{ + ensure_signed, + pallet_prelude::{BlockNumberFor, OriginFor}, +}; use scale_info::{ prelude::{collections::BTreeMap, fmt::Debug, vec, vec::Vec}, TypeInfo, @@ -239,6 +242,7 @@ pub mod pallet { }, issued_certs: vec![], received_certs: issuers_, + last_online: None, }, ); // if smith is offline, schedule expire @@ -264,8 +268,13 @@ pub mod pallet { /// The Smith metadata for each identity. #[pallet::storage] #[pallet::getter(fn smiths)] - pub type Smiths<T: Config> = - StorageMap<_, Twox64Concat, T::IdtyIndex, SmithMeta<T::IdtyIndex>, OptionQuery>; + pub type Smiths<T: Config> = StorageMap< + _, + Twox64Concat, + T::IdtyIndex, + SmithMeta<T::IdtyIndex, BlockNumberFor<T>>, + OptionQuery, + >; /// The indexes of Smith to remove at a given session. #[pallet::storage] @@ -580,44 +589,39 @@ impl<T: Config> Pallet<T> { /// Handle the event when a Smith goes online. pub fn on_smith_goes_online(idty_index: T::IdtyIndex) { - if let Some(smith_meta) = Smiths::<T>::get(idty_index) { - if smith_meta.expires_on.is_some() { - Smiths::<T>::mutate(idty_index, |maybe_smith_meta| { - if let Some(smith_meta) = maybe_smith_meta { - // As long as the smith is online, it cannot expire - smith_meta.expires_on = None; - // FIXME: unschedule old expiry (#182) - } - }); + Smiths::<T>::mutate(idty_index, |maybe_smith_meta| { + if let Some(smith_meta) = maybe_smith_meta { + if smith_meta.expires_on.is_some() { + // As long as the smith is online, it cannot expire + smith_meta.expires_on = None; + smith_meta.last_online = None; + } } - } + }); } /// Handle the event when a Smith goes offline. pub fn on_smith_goes_offline(idty_index: T::IdtyIndex) { - if let Some(smith_meta) = Smiths::<T>::get(idty_index) { - // Smith can go offline after main membership expiry - // in this case, there is no scheduled expiry since it is already excluded - if smith_meta.status != SmithStatus::Excluded { - Smiths::<T>::mutate(idty_index, |maybe_smith_meta| { - if let Some(smith_meta) = maybe_smith_meta { - // schedule expiry - let new_expires_on = - CurrentSession::<T>::get() + T::SmithInactivityMaxDuration::get(); - smith_meta.expires_on = Some(new_expires_on); - ExpiresOn::<T>::append(new_expires_on, idty_index); - } - }); + Smiths::<T>::mutate(idty_index, |maybe_smith_meta| { + if let Some(smith_meta) = maybe_smith_meta { + // Smith can go offline after main membership expiry + // in this case, there is no scheduled expiry since it is already excluded + if smith_meta.status != SmithStatus::Excluded { + // schedule expiry + let new_expires_on = + CurrentSession::<T>::get() + T::SmithInactivityMaxDuration::get(); + smith_meta.expires_on = Some(new_expires_on); + let block_number = frame_system::pallet::Pallet::<T>::block_number(); + smith_meta.last_online = Some(block_number); + ExpiresOn::<T>::append(new_expires_on, idty_index); + } } - } + }); } /// Provide whether the given identity index is a Smith. fn provide_is_member(idty_id: &T::IdtyIndex) -> bool { - let Some(smith) = Smiths::<T>::get(idty_id) else { - return false; - }; - smith.status == SmithStatus::Smith + Smiths::<T>::get(idty_id).map_or(false, |smith| smith.status == SmithStatus::Smith) } } diff --git a/pallets/smith-members/src/tests.rs b/pallets/smith-members/src/tests.rs index c92bffb680caedd2e006ad2b7c8378ce04e529d6..068821a4eb8a88a093f40c690964ea761671d744 100644 --- a/pallets/smith-members/src/tests.rs +++ b/pallets/smith-members/src/tests.rs @@ -62,6 +62,7 @@ fn process_to_become_a_smith_and_lose_it() { expires_on: Some(5), issued_certs: vec![], received_certs: vec![], + last_online: None, } ); // Then certification 1/2 @@ -80,6 +81,7 @@ fn process_to_become_a_smith_and_lose_it() { expires_on: Some(5), issued_certs: vec![], received_certs: vec![1], + last_online: None, } ); // Then certification 2/2 @@ -101,6 +103,7 @@ fn process_to_become_a_smith_and_lose_it() { expires_on: Some(5), issued_certs: vec![], received_certs: vec![1, 2], + last_online: None, } ); // Go online to be able to invite+certify @@ -161,7 +164,8 @@ fn process_to_become_a_smith_and_lose_it() { status: SmithStatus::Excluded, expires_on: None, issued_certs: vec![], - received_certs: vec![] + received_certs: vec![], + last_online: Some(1), }) ); assert_eq!( @@ -170,7 +174,8 @@ fn process_to_become_a_smith_and_lose_it() { status: SmithStatus::Excluded, expires_on: None, issued_certs: vec![], - received_certs: vec![] + received_certs: vec![], + last_online: Some(1), }) ); assert_eq!( @@ -179,7 +184,8 @@ fn process_to_become_a_smith_and_lose_it() { status: SmithStatus::Excluded, expires_on: None, issued_certs: vec![], - received_certs: vec![] + received_certs: vec![], + last_online: None, }) ); }); @@ -252,6 +258,7 @@ fn should_have_checks_on_certify() { expires_on: None, issued_certs: vec![4], received_certs: vec![2, 3, 4], + last_online: None, } ); assert_eq!( @@ -261,6 +268,7 @@ fn should_have_checks_on_certify() { expires_on: Some(5), issued_certs: vec![1, 4], received_certs: vec![3, 4], + last_online: None, } ); assert_eq!( @@ -270,6 +278,7 @@ fn should_have_checks_on_certify() { expires_on: Some(5), issued_certs: vec![1, 2], received_certs: vec![4], + last_online: None, } ); assert_eq!( @@ -279,6 +288,7 @@ fn should_have_checks_on_certify() { expires_on: Some(5), issued_certs: vec![1, 2, 3], received_certs: vec![1, 2], + last_online: None, } ); @@ -312,6 +322,7 @@ fn should_have_checks_on_certify() { expires_on: Some(5), issued_certs: vec![1, 2], received_certs: vec![4], + last_online: None, } ); // Try to certify #3 @@ -327,6 +338,7 @@ fn should_have_checks_on_certify() { expires_on: Some(5), issued_certs: vec![1, 2], received_certs: vec![1, 4], + last_online: None, } ); }); @@ -359,7 +371,8 @@ fn smith_activity_postpones_expiration() { status: SmithStatus::Excluded, expires_on: None, issued_certs: vec![], - received_certs: vec![] + received_certs: vec![], + last_online: None, }) ); // issued_certs is empty because #1 was excluded @@ -370,6 +383,7 @@ fn smith_activity_postpones_expiration() { expires_on: None, issued_certs: vec![], received_certs: vec![3, 4], + last_online: None, }) ); @@ -383,6 +397,7 @@ fn smith_activity_postpones_expiration() { expires_on: Some(11), issued_certs: vec![], received_certs: vec![3, 4], + last_online: Some(0), }) ); // Still not expired on session 10 @@ -394,6 +409,7 @@ fn smith_activity_postpones_expiration() { expires_on: Some(11), issued_certs: vec![], received_certs: vec![3, 4], + last_online: Some(0), }) ); // But expired on session 11 @@ -404,7 +420,8 @@ fn smith_activity_postpones_expiration() { status: SmithStatus::Excluded, expires_on: None, issued_certs: vec![], - received_certs: vec![] + received_certs: vec![], + last_online: None, }) ); assert_eq!( @@ -413,7 +430,8 @@ fn smith_activity_postpones_expiration() { status: SmithStatus::Excluded, expires_on: None, issued_certs: vec![], - received_certs: vec![] + received_certs: vec![], + last_online: Some(0), }) ); }); @@ -443,7 +461,8 @@ fn smith_coming_back_recovers_its_issued_certs() { status: Excluded, expires_on: None, issued_certs: vec![1], - received_certs: vec![] + received_certs: vec![], + last_online: None, }) ); // Smith #2 comes back @@ -466,7 +485,8 @@ fn smith_coming_back_recovers_its_issued_certs() { status: Smith, expires_on: Some(10), issued_certs: vec![1], - received_certs: vec![1, 3] + received_certs: vec![1, 3], + last_online: None, }) ); Pallet::<Runtime>::on_smith_goes_online(2); @@ -590,7 +610,8 @@ fn certifying_an_online_smith() { status: Smith, expires_on: Some(8), issued_certs: vec![], - received_certs: vec![1, 2] + received_certs: vec![1, 2], + last_online: None, }) ); assert_eq!(ExpiresOn::<Runtime>::get(7), Some(vec![5])); @@ -604,7 +625,8 @@ fn certifying_an_online_smith() { status: Smith, expires_on: None, issued_certs: vec![], - received_certs: vec![1, 2] + received_certs: vec![1, 2], + last_online: None, }) ); // ExpiresOn is not unscheduled, but as expires_on has switched to None it's not a problem @@ -622,7 +644,8 @@ fn certifying_an_online_smith() { status: Smith, expires_on: None, issued_certs: vec![], - received_certs: vec![1, 2, 3] + received_certs: vec![1, 2, 3], + last_online: None, }) ); }); @@ -648,7 +671,8 @@ fn expires_on_cleans_up() { status: Smith, expires_on: Some(5), issued_certs: vec![2, 3], - received_certs: vec![2, 3] + received_certs: vec![2, 3], + last_online: Some(0), }) ); // It is also present in ExpiresOn schedule @@ -662,7 +686,8 @@ fn expires_on_cleans_up() { status: Excluded, expires_on: None, issued_certs: vec![2, 3], - received_certs: vec![] + received_certs: vec![], + last_online: Some(0), }) ); // ExpiresOn is clean @@ -713,6 +738,7 @@ fn losing_wot_membership_cascades_to_smith_members() { expires_on: Some(5), issued_certs: vec![3], received_certs: vec![2, 3, 4], + last_online: None, }) ); assert_eq!( @@ -742,6 +768,7 @@ fn losing_wot_membership_cascades_to_smith_members() { expires_on: None, issued_certs: vec![3], received_certs: vec![], + last_online: None, }) ); // Issued certifications updated for certifiers of 1 diff --git a/pallets/smith-members/src/types.rs b/pallets/smith-members/src/types.rs index 8399226517f42337ebc36b031d07d76c39dbdd0f..396e0334181616e3dbee0ce8aa039bf12c227d81 100644 --- a/pallets/smith-members/src/types.rs +++ b/pallets/smith-members/src/types.rs @@ -24,7 +24,7 @@ use sp_staking::SessionIndex; /// Represents a certification metadata attached to a Smith identity. #[derive(Encode, Decode, Clone, PartialEq, Eq, RuntimeDebug, TypeInfo)] -pub struct SmithMeta<IdtyIndex> { +pub struct SmithMeta<IdtyIndex, BlockNumber> { /// Current status of the Smith. pub status: SmithStatus, /// The session at which the Smith will expire (for lack of validation activity). @@ -33,16 +33,19 @@ pub struct SmithMeta<IdtyIndex> { pub issued_certs: Vec<IdtyIndex>, /// Certifications received from other Smiths. pub received_certs: Vec<IdtyIndex>, + /// Last online time. + pub last_online: Option<BlockNumber>, } /// By default, a smith has the least possible privileges -impl<IdtyIndex> Default for SmithMeta<IdtyIndex> { +impl<IdtyIndex, BlockNumber> Default for SmithMeta<IdtyIndex, BlockNumber> { fn default() -> Self { Self { status: SmithStatus::Excluded, expires_on: None, issued_certs: Vec::<IdtyIndex>::new(), received_certs: Vec::<IdtyIndex>::new(), + last_online: Default::default(), } } } diff --git a/resources/metadata.scale b/resources/metadata.scale index 430bae89f08fbcf6bcf656bfc6f26516d767119b..566b876a228174b3294280f96249b9b01095bd10 100644 Binary files a/resources/metadata.scale and b/resources/metadata.scale differ diff --git a/runtime/common/src/handlers.rs b/runtime/common/src/handlers.rs index 3fdd7b2d395730e3bb4f45cbb12ae1bc7f3f1817..0029cb4e7c2c68db97176e03534d12aeafc1725c 100644 --- a/runtime/common/src/handlers.rs +++ b/runtime/common/src/handlers.rs @@ -19,6 +19,7 @@ use frame_support::{ pallet_prelude::Weight, traits::{Imbalance, UnfilteredDispatchable}, }; +use frame_system::pallet_prelude::BlockNumberFor; use pallet_smith_members::SmithRemovalReason; use sp_core::Get; @@ -171,23 +172,54 @@ where } /// Runtime handler OwnerKeyChangePermission. -pub struct OwnerKeyChangePermissionHandler<Runtime>(core::marker::PhantomData<Runtime>); +pub struct KeyChangeHandler<Runtime, ReportLongevity>( + core::marker::PhantomData<(Runtime, ReportLongevity)>, +); impl< - Runtime: frame_system::Config + Runtime: frame_system::Config<AccountId = AccountId> + pallet_identity::Config<IdtyIndex = IdtyIndex> - + pallet_authority_members::Config<MemberId = IdtyIndex>, - > pallet_identity::traits::CheckKeyChangeAllowed<Runtime> - for OwnerKeyChangePermissionHandler<Runtime> + + pallet_authority_members::Config<MemberId = IdtyIndex> + + pallet_smith_members::Config<IdtyIndex = IdtyIndex>, + ReportLongevity: Get<BlockNumberFor<Runtime>>, + > pallet_identity::traits::KeyChange<Runtime> for KeyChangeHandler<Runtime, ReportLongevity> { - fn check_allowed(idty_index: &IdtyIndex) -> bool { - !pallet_authority_members::Pallet::<Runtime>::online().contains(idty_index) + /// Handles the event when an identity's owner key is changed. + /// + /// # Errors + /// * Returns `OwnerKeyInBound` if the smith was a validator and the bond period is not finished, meaning it can still be punished for past actions. + /// * Returns `OwnerKeyUsedAsValidator` if the owner key is currently used as a validator. + /// + /// # Behavior + /// * If the smith is online, the operation is rejected. + /// * If the smith was a validator and is still within the bond period, the operation is rejected. It means they can still be punished for past actions. + /// * If the smith is neither online nor within the bond period, the owner key is changed successfully and the change is reflected in the validator member data if available. + fn on_changed( + idty_index: IdtyIndex, + account_id: AccountId, + ) -> Result<(), sp_runtime::DispatchError> { + if let Some(smith) = pallet_smith_members::Pallet::<Runtime>::smiths(&idty_index) { + if let Some(last_online) = smith.last_online { + if last_online + ReportLongevity::get() + > frame_system::pallet::Pallet::<Runtime>::block_number() + { + return Err(pallet_identity::Error::<Runtime>::OwnerKeyInBound.into()); + } else { + pallet_authority_members::Pallet::<Runtime>::change_owner_key( + idty_index, account_id, + ) + .map_err(|e| e.error)?; + } + } else { + return Err(pallet_identity::Error::<Runtime>::OwnerKeyUsedAsValidator.into()); + } + } + Ok(()) } } /// Runtime handler for managing fee handling by transferring unbalanced amounts to a treasury account. pub struct HandleFees<TreasuryAccount, Balances>( - frame_support::pallet_prelude::PhantomData<TreasuryAccount>, - frame_support::pallet_prelude::PhantomData<Balances>, + frame_support::pallet_prelude::PhantomData<(TreasuryAccount, Balances)>, ); type CreditOf<Balances> = frame_support::traits::tokens::fungible::Credit<AccountId, Balances>; impl<TreasuryAccount, Balances> frame_support::traits::OnUnbalanced<CreditOf<Balances>> diff --git a/runtime/common/src/pallets_config.rs b/runtime/common/src/pallets_config.rs index c3b6e728fcb533c49189598ed05a840cef2016f5..8cc6b927dfcdcbe253bd7cf59857489b03bf5ba8 100644 --- a/runtime/common/src/pallets_config.rs +++ b/runtime/common/src/pallets_config.rs @@ -444,9 +444,9 @@ macro_rules! pallets_config { type IdtyData = IdtyData; type IdtyIndex = IdtyIndex; type IdtyNameValidator = IdtyNameValidatorImpl; + type OnKeyChange = KeyChangeHandler<Runtime, ReportLongevity>; type OnNewIdty = OnNewIdtyHandler<Runtime>; type OnRemoveIdty = OnRemoveIdtyHandler<Runtime>; - type OwnerKeyChangePermission = OwnerKeyChangePermissionHandler<Runtime>; type RuntimeEvent = RuntimeEvent; type Signature = Signature; type Signer = <Signature as sp_runtime::traits::Verify>::Signer; diff --git a/runtime/g1/src/parameters.rs b/runtime/g1/src/parameters.rs index d787eb5bf38e58c37ed0bc6d078f449c9617d983..5fe8b963cebf556a7be2e5f0ab9ad5b6ed4c1b8e 100644 --- a/runtime/g1/src/parameters.rs +++ b/runtime/g1/src/parameters.rs @@ -58,7 +58,7 @@ pub const EPOCH_DURATION_IN_SLOTS: BlockNumber = 4 * HOURS; parameter_types! { pub const EpochDuration: u64 = EPOCH_DURATION_IN_SLOTS as u64; pub const ExpectedBlockTime: u64 = MILLISECS_PER_BLOCK; - pub const ReportLongevity: u64 = 168 * EpochDuration::get(); + pub const ReportLongevity: BlockNumber = 168 * EPOCH_DURATION_IN_SLOTS; } // ImOnline diff --git a/runtime/gdev/src/parameters.rs b/runtime/gdev/src/parameters.rs index bcca8e7d2315b7d331a026fc53174199fbe31571..7fc7e9ba55d2e4c95044972f003def9421b80751 100644 --- a/runtime/gdev/src/parameters.rs +++ b/runtime/gdev/src/parameters.rs @@ -58,9 +58,8 @@ parameter_types! { // Babe parameter_types! { pub const ExpectedBlockTime: u64 = MILLISECS_PER_BLOCK; - pub const ReportLongevity: u64 = 168 * HOURS as u64; + pub const ReportLongevity: BlockNumber = 168 * HOURS; } - // ImOnline parameter_types! { pub const ImOnlineUnsignedPriority: TransactionPriority = TransactionPriority::MAX; diff --git a/runtime/gdev/tests/integration_tests.rs b/runtime/gdev/tests/integration_tests.rs index 34d3150dff821b7f137e74652c172a7d42c6bbee..4031f377f9bd11cdf86ae4c744232819ee21ebc8 100644 --- a/runtime/gdev/tests/integration_tests.rs +++ b/runtime/gdev/tests/integration_tests.rs @@ -1020,6 +1020,7 @@ fn test_smith_process() { expires_on: Some(48), issued_certs: vec![], received_certs: vec![1, 2, 3], + last_online: None, }) ); @@ -1053,6 +1054,7 @@ fn test_expired_smith_has_null_expires_on() { expires_on: None, // because online issued_certs: vec![1, 3], received_certs: vec![1, 3], + last_online: None, }) ); @@ -1085,6 +1087,7 @@ fn test_expired_smith_has_null_expires_on() { expires_on: None, // because excluded, no expiry is scheduled issued_certs: vec![1, 3], received_certs: vec![], // received certs are deleted + last_online: None, }) ); // Alice smith cert to Bob has been deleted @@ -1095,6 +1098,7 @@ fn test_expired_smith_has_null_expires_on() { expires_on: None, // because online issued_certs: vec![3], // cert to Bob has been deleted received_certs: vec![2, 3], + last_online: None, }) ); @@ -1116,6 +1120,7 @@ fn test_expired_smith_has_null_expires_on() { expires_on: None, // should be still None issued_certs: vec![1, 3], received_certs: vec![], + last_online: None, }) ); @@ -1516,124 +1521,256 @@ fn test_link_account() { }) } -/// test change owner key +/// test change owner key was validator is online #[test] fn test_change_owner_key_validator_online() { - ExtBuilder::new(1, 3, 4).build().execute_with(|| { - let genesis_hash = System::block_hash(0); - let alice = AccountKeyring::Alice.to_account_id(); - let ferdie = AccountKeyring::Ferdie.to_account_id(); - let payload = (b"icok", genesis_hash, 1u32, alice.clone()).encode(); - let signature = AccountKeyring::Alice.sign(&payload); + ExtBuilder::new(1, 3, 4) + .with_initial_balances(vec![(AccountKeyring::Ferdie.to_account_id(), 8888)]) + .build() + .execute_with(|| { + let genesis_hash = System::block_hash(0); + let alice = AccountKeyring::Alice.to_account_id(); + let ferdie = AccountKeyring::Ferdie.to_account_id(); + let payload = (b"icok", genesis_hash, 1u32, alice.clone()).encode(); + let signature = AccountKeyring::Ferdie.sign(&payload); - // Alice is an online validator - assert!(pallet_authority_members::OnlineAuthorities::<Runtime>::get().contains(&1)); + // Alice is an online validator + assert!(pallet_authority_members::OnlineAuthorities::<Runtime>::get().contains(&1)); + assert_eq!( + pallet_authority_members::Members::<Runtime>::get(1) + .unwrap() + .owner_key, + alice + ); - // As an online validator she cannot change key - assert_noop!( - Identity::change_owner_key( - RuntimeOrigin::signed(alice.clone()), + // As an online validator she cannot change key + assert_noop!( + Identity::change_owner_key( + RuntimeOrigin::signed(alice.clone()), + ferdie.clone(), + signature.into() + ), + pallet_identity::Error::<gdev_runtime::Runtime>::OwnerKeyUsedAsValidator + ); + }) +} + +/// test change owner key between set_key and go online +#[test] +#[ignore = "long to go to ReportLongevity"] +fn test_change_owner_key_offline() { + ExtBuilder::new(1, 3, 4) + .with_initial_balances(vec![(AccountKeyring::Ferdie.to_account_id(), 8888)]) + .build() + .execute_with(|| { + let genesis_hash = System::block_hash(0); + let charlie = AccountKeyring::Charlie.to_account_id(); + let ferdie = AccountKeyring::Ferdie.to_account_id(); + let payload = (b"icok", genesis_hash, 3u32, charlie.clone()).encode(); + let signature = AccountKeyring::Ferdie.sign(&payload); + + // Charlie is an offline smith + SmithMembers::on_smith_goes_offline(3); + assert_eq!( + SmithMembers::smiths(3), + Some(SmithMeta { + status: SmithStatus::Smith, + expires_on: Some(48), + issued_certs: vec![1, 2], + received_certs: vec![1, 2], + last_online: Some(1), + }) + ); + assert_eq!( + frame_system::Pallet::<Runtime>::get(&charlie).linked_idty, + Some(3) + ); + assert_eq!( + frame_system::Pallet::<Runtime>::get(&ferdie).linked_idty, + None + ); + + // We run after the bound period + // Keeping members intact + for i in 1..4 { + let expiration = pallet_membership::Membership::<Runtime>::get(i) + .unwrap() + .expire_on; + pallet_membership::MembershipsExpireOn::<Runtime>::take(expiration); + pallet_smith_members::Smiths::<Runtime>::mutate(i, |data| { + if let Some(ref mut data) = data { + data.expires_on = None; + } + }); + } + pallet_certification::CertsRemovableOn::<Runtime>::take(ValidityPeriod::get()); + run_to_block(ReportLongevity::get() + 1); + + // Charlie can set its session_keys + assert_ok!(AuthorityMembers::set_session_keys( + RuntimeOrigin::signed(AccountKeyring::Charlie.to_account_id()), + create_dummy_session_keys() + )); + assert_eq!( + pallet_authority_members::Members::<Runtime>::get(3) + .unwrap() + .owner_key, + charlie.clone() + ); + + // Charlie can change his owner key to Ferdie's a valid account + // with providers and balance + assert_ok!(Identity::change_owner_key( + RuntimeOrigin::signed(charlie.clone()), ferdie.clone(), signature.into() - ), - pallet_identity::Error::<gdev_runtime::Runtime>::OwnerKeyUsedAsValidator - ); - }) + )); + // The change is reflected on the authority member data + assert_eq!( + pallet_authority_members::Members::<Runtime>::get(3) + .unwrap() + .owner_key, + ferdie.clone() + ); + assert_eq!( + frame_system::Pallet::<Runtime>::get(&ferdie).linked_idty, + Some(3) + ); + }) } /// test change owner key #[test] +#[ignore = "long to go to ReportLongevity"] fn test_change_owner_key() { - ExtBuilder::new(1, 3, 4).build().execute_with(|| { - let genesis_hash = System::block_hash(0); - let charlie = AccountKeyring::Charlie.to_account_id(); - let ferdie = AccountKeyring::Ferdie.to_account_id(); - let payload = (b"icok", genesis_hash, 3u32, charlie.clone()).encode(); - let signature = AccountKeyring::Ferdie.sign(&payload); - - SmithMembers::on_smith_goes_offline(3); - // Charlie is now offline smith - assert_eq!( - SmithMembers::smiths(3), - Some(SmithMeta { - status: SmithStatus::Smith, - expires_on: Some(48), - issued_certs: vec![1, 2], - received_certs: vec![1, 2] - }) - ); + ExtBuilder::new(1, 3, 4) + .with_initial_balances(vec![(AccountKeyring::Ferdie.to_account_id(), 8888)]) + .build() + .execute_with(|| { + let genesis_hash = System::block_hash(0); + let charlie = AccountKeyring::Charlie.to_account_id(); + let ferdie = AccountKeyring::Ferdie.to_account_id(); + let payload = (b"icok", genesis_hash, 3u32, charlie.clone()).encode(); + let signature = AccountKeyring::Ferdie.sign(&payload); - assert_eq!( - frame_system::Pallet::<Runtime>::get(&charlie).linked_idty, - Some(3) - ); - assert_eq!( - frame_system::Pallet::<Runtime>::get(&ferdie).linked_idty, - None - ); - // Dave can change his owner key to Ferdie's - assert_ok!(Identity::change_owner_key( - RuntimeOrigin::signed(charlie.clone()), - ferdie.clone(), - signature.into() - )); - assert_eq!( - frame_system::Pallet::<Runtime>::get(&ferdie).linked_idty, - Some(3) - ); + SmithMembers::on_smith_goes_offline(3); + // Charlie is now offline smith + assert_eq!( + SmithMembers::smiths(3), + Some(SmithMeta { + status: SmithStatus::Smith, + expires_on: Some(48), + issued_certs: vec![1, 2], + received_certs: vec![1, 2], + last_online: Some(1), + }) + ); - // Charlie is still an offline smith - assert_eq!( - SmithMembers::smiths(3), - Some(SmithMeta { - status: SmithStatus::Smith, - expires_on: Some(48), - issued_certs: vec![1, 2], - received_certs: vec![1, 2] - }) - ); + assert_eq!( + frame_system::Pallet::<Runtime>::get(&charlie).linked_idty, + Some(3) + ); + assert_eq!( + frame_system::Pallet::<Runtime>::get(&ferdie).linked_idty, + None + ); - // Ferdie can set its session_keys and go online - frame_system::Pallet::<Runtime>::inc_providers(&ferdie); - assert_ok!(AuthorityMembers::set_session_keys( - RuntimeOrigin::signed(AccountKeyring::Ferdie.to_account_id()), - create_dummy_session_keys() - )); - assert_ok!(AuthorityMembers::go_online(RuntimeOrigin::signed( - AccountKeyring::Ferdie.to_account_id() - ))); + run_to_block(5); - // Charlie is still an offline smith - assert_eq!( - SmithMembers::smiths(3), - Some(SmithMeta { - status: SmithStatus::Smith, - expires_on: Some(48), - issued_certs: vec![1, 2], - received_certs: vec![1, 2] - }) - ); + // Charlie cannot change his owner key because he is in bound period + // and can be punished for past actions + assert_noop!( + Identity::change_owner_key( + RuntimeOrigin::signed(charlie.clone()), + ferdie.clone(), + signature.into() + ), + pallet_identity::Error::<gdev_runtime::Runtime>::OwnerKeyInBound + ); - run_to_block(25); + // We run after the bound period + // Keeping members intact + let smith_expire_on = ReportLongevity::get() * 2; + for i in 1..4 { + let expiration = pallet_membership::Membership::<Runtime>::get(i) + .unwrap() + .expire_on; + pallet_membership::MembershipsExpireOn::<Runtime>::take(expiration); + pallet_smith_members::Smiths::<Runtime>::mutate(i, |data| { + if let Some(ref mut data) = data { + data.expires_on = Some(smith_expire_on); + } + }); + } + pallet_certification::CertsRemovableOn::<Runtime>::take(ValidityPeriod::get()); + pallet_smith_members::ExpiresOn::<Runtime>::take(SmithInactivityMaxDuration::get() + 1); + run_to_block(SmithInactivityMaxDuration::get() * 25); + pallet_smith_members::ExpiresOn::<Runtime>::take(SmithInactivityMaxDuration::get() + 2); + run_to_block(ReportLongevity::get() + 1); + + // Charlie can change his owner key to Ferdie's + assert_ok!(Identity::change_owner_key( + RuntimeOrigin::signed(charlie.clone()), + ferdie.clone(), + signature.into() + )); + assert_eq!( + frame_system::Pallet::<Runtime>::get(&ferdie).linked_idty, + Some(3) + ); - System::assert_has_event(RuntimeEvent::AuthorityMembers( - pallet_authority_members::Event::IncomingAuthorities { members: vec![3] }, - )); - System::assert_has_event(RuntimeEvent::AuthorityMembers( - pallet_authority_members::Event::OutgoingAuthorities { members: vec![1] }, - )); + // Charlie is still an offline smith + assert_eq!( + SmithMembers::smiths(3), + Some(SmithMeta { + status: SmithStatus::Smith, + expires_on: Some(smith_expire_on), + issued_certs: vec![1, 2], + received_certs: vec![1, 2], + last_online: Some(1), + }) + ); - // "Charlie" (idty 3) is now online because its identity is mapped to Ferdies's key - assert_eq!( - SmithMembers::smiths(3), - Some(SmithMeta { - status: SmithStatus::Smith, - expires_on: None, - issued_certs: vec![1, 2], - received_certs: vec![1, 2] - }) - ); - }) + // Ferdie can set its session_keys and go online + frame_system::Pallet::<Runtime>::inc_providers(&ferdie); + assert_ok!(AuthorityMembers::set_session_keys( + RuntimeOrigin::signed(AccountKeyring::Ferdie.to_account_id()), + create_dummy_session_keys() + )); + assert_ok!(AuthorityMembers::go_online(RuntimeOrigin::signed( + AccountKeyring::Ferdie.to_account_id() + ))); + + // Charlie is still an offline smith + assert_eq!( + SmithMembers::smiths(3), + Some(SmithMeta { + status: SmithStatus::Smith, + expires_on: Some(smith_expire_on), + issued_certs: vec![1, 2], + received_certs: vec![1, 2], + last_online: Some(1), + }) + ); + + run_to_block(((ReportLongevity::get() + 1 + 24) / 25) * 25); + + System::assert_has_event(RuntimeEvent::AuthorityMembers( + pallet_authority_members::Event::IncomingAuthorities { members: vec![3] }, + )); + + // "Charlie" (idty 3) is now online because its identity is mapped to Ferdies's key + assert_eq!( + SmithMembers::smiths(3), + Some(SmithMeta { + status: SmithStatus::Smith, + expires_on: None, + issued_certs: vec![1, 2], + received_certs: vec![1, 2], + last_online: None, + }) + ); + }) } /// members of the smith subwot can revoke their identity diff --git a/runtime/gtest/src/parameters.rs b/runtime/gtest/src/parameters.rs index 307f59b19733e1ad99bfb62bb656e9dc51bb70f1..8ac8aeb0aae0a874137ea2660a56b7e2f20dbcde 100644 --- a/runtime/gtest/src/parameters.rs +++ b/runtime/gtest/src/parameters.rs @@ -58,7 +58,7 @@ pub const EPOCH_DURATION_IN_SLOTS: BlockNumber = HOURS; parameter_types! { pub const EpochDuration: u64 = EPOCH_DURATION_IN_SLOTS as u64; pub const ExpectedBlockTime: u64 = MILLISECS_PER_BLOCK; - pub const ReportLongevity: u64 = 168 * EpochDuration::get(); + pub const ReportLongevity: BlockNumber = 168 * EPOCH_DURATION_IN_SLOTS; } // ImOnline