From 037f149459beff15e51f44093247760786ae047a Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=C3=89lo=C3=AFs?= <c@elo.tf>
Date: Wed, 24 Aug 2022 17:15:58 +0200
Subject: [PATCH] fix(identity): handle sufficients counter when owner key
change (nodes/rust/duniter-v2s!97)
* fix(identity): add a special call to fix sufficients counter
* fix(identity): handle sufficients counter when owner key change
* test: handle sufficients properly when owner key change
---
pallets/identity/src/lib.rs | 49 +++++++++++++++++++++++----
pallets/identity/src/mock.rs | 1 +
pallets/identity/src/tests.rs | 62 +++++++++++++++++++++++++++++++++--
3 files changed, 103 insertions(+), 9 deletions(-)
diff --git a/pallets/identity/src/lib.rs b/pallets/identity/src/lib.rs
index c8f8b4f43..77cba1b93 100644
--- a/pallets/identity/src/lib.rs
+++ b/pallets/identity/src/lib.rs
@@ -402,6 +402,7 @@ pub mod pallet {
new_key: T::AccountId,
new_key_sig: T::NewOwnerKeySignature,
) -> DispatchResultWithPostInfo {
+ // verification phase
let who = ensure_signed(origin)?;
let idty_index =
@@ -415,12 +416,20 @@ pub mod pallet {
);
let block_number = frame_system::Pallet::<T>::block_number();
- if let Some((_, last_change)) = idty_value.old_owner_key {
- ensure!(
- block_number >= last_change + T::ChangeOwnerKeyPeriod::get(),
- Error::<T>::OwnerKeyAlreadyRecentlyChanged
- );
- }
+ let maybe_old_old_owner_key =
+ if let Some((old_owner_key, last_change)) = idty_value.old_owner_key {
+ ensure!(
+ block_number >= last_change + T::ChangeOwnerKeyPeriod::get(),
+ Error::<T>::OwnerKeyAlreadyRecentlyChanged
+ );
+ ensure!(
+ old_owner_key != new_key,
+ Error::<T>::ProhibitedToRevertToAnOldKey
+ );
+ Some(old_owner_key)
+ } else {
+ None
+ };
let genesis_hash = frame_system::Pallet::<T>::block_hash(T::BlockNumber::zero());
let new_key_payload = NewOwnerKeyPayload {
@@ -435,9 +444,15 @@ pub mod pallet {
Error::<T>::InvalidNewOwnerKeySig
);
+ // Apply phase
+ if let Some(old_old_owner_key) = maybe_old_old_owner_key {
+ frame_system::Pallet::<T>::dec_sufficients(&old_old_owner_key);
+ }
IdentityIndexOf::<T>::remove(&idty_value.owner_key);
+
idty_value.old_owner_key = Some((idty_value.owner_key.clone(), block_number));
idty_value.owner_key = new_key.clone();
+ frame_system::Pallet::<T>::inc_sufficients(&idty_value.owner_key);
IdentityIndexOf::<T>::insert(&idty_value.owner_key, idty_index);
Identities::<T>::insert(idty_index, idty_value);
Self::deposit_event(Event::IdtyChangedOwnerKey {
@@ -527,6 +542,23 @@ pub mod pallet {
Ok(().into())
}
+
+ #[pallet::weight(1_000_000_000)]
+ pub fn fix_sufficients(
+ origin: OriginFor<T>,
+ owner_key: T::AccountId,
+ inc: bool,
+ ) -> DispatchResultWithPostInfo {
+ ensure_root(origin)?;
+
+ if inc {
+ frame_system::Pallet::<T>::inc_sufficients(&owner_key);
+ } else {
+ frame_system::Pallet::<T>::dec_sufficients(&owner_key);
+ }
+
+ Ok(().into())
+ }
}
// ERRORS //
@@ -577,6 +609,8 @@ pub mod pallet {
OwnerKeyAlreadyRecentlyChanged,
/// Owner key already used
OwnerKeyAlreadyUsed,
+ /// Prohibited to revert to an old key
+ ProhibitedToRevertToAnOldKey,
/// Right already added
RightAlreadyAdded,
/// Right does not exist
@@ -601,6 +635,9 @@ pub mod pallet {
// Identity should be removed after the consumers of the identity
Identities::<T>::remove(idty_index);
frame_system::Pallet::<T>::dec_sufficients(&idty_val.owner_key);
+ if let Some((old_owner_key, _last_change)) = idty_val.old_owner_key {
+ frame_system::Pallet::<T>::dec_sufficients(&old_owner_key);
+ }
Self::deposit_event(Event::IdtyRemoved { idty_index });
T::OnIdtyChange::on_idty_change(
idty_index,
diff --git a/pallets/identity/src/mock.rs b/pallets/identity/src/mock.rs
index 082cb28a1..38c358510 100644
--- a/pallets/identity/src/mock.rs
+++ b/pallets/identity/src/mock.rs
@@ -117,6 +117,7 @@ pub fn new_test_ext(gen_conf: pallet_identity::GenesisConfig<Test>) -> sp_io::Te
frame_support::BasicExternalities::execute_with_storage(&mut t, || {
// Some dedicated test account
+ frame_system::Pallet::<Test>::inc_sufficients(&1);
frame_system::Pallet::<Test>::inc_providers(&2);
frame_system::Pallet::<Test>::inc_providers(&3);
});
diff --git a/pallets/identity/src/tests.rs b/pallets/identity/src/tests.rs
index be17d9068..77624e10c 100644
--- a/pallets/identity/src/tests.rs
+++ b/pallets/identity/src/tests.rs
@@ -198,7 +198,7 @@ fn test_change_owner_key() {
.execute_with(|| {
let genesis_hash = System::block_hash(0);
let old_owner_key = 1u64;
- let new_key_payload = NewOwnerKeyPayload {
+ let mut new_key_payload = NewOwnerKeyPayload {
genesis_hash: &genesis_hash,
idty_index: 1u64,
old_owner_key: &old_owner_key,
@@ -207,7 +207,11 @@ fn test_change_owner_key() {
// We need to initialize at least one block before any call
run_to_block(1);
- // Caller should have an asoociated identity
+ // Verify genesis data
+ assert_eq!(System::sufficients(&1), 1);
+ assert_eq!(System::sufficients(&10), 0);
+
+ // Caller should have an associated identity
assert_err!(
Identity::change_owner_key(
Origin::signed(42),
@@ -264,10 +268,15 @@ fn test_change_owner_key() {
status: crate::IdtyStatus::Validated,
})
);
+ // Alice still sufficient
+ assert_eq!(System::sufficients(&1), 1);
+ // New owner key should become a sufficient account
+ assert_eq!(System::sufficients(&10), 1);
run_to_block(2);
// Alice can't re-change her owner key too early
+ new_key_payload.old_owner_key = &10;
assert_err!(
Identity::change_owner_key(
Origin::signed(10),
@@ -279,6 +288,45 @@ fn test_change_owner_key() {
),
Error::<Test>::OwnerKeyAlreadyRecentlyChanged
);
+
+ // Alice can re-change her owner key after ChangeOwnerKeyPeriod blocs
+ run_to_block(2 + <Test as crate::Config>::ChangeOwnerKeyPeriod::get());
+ assert_ok!(Identity::change_owner_key(
+ Origin::signed(10),
+ 100,
+ TestSignature(
+ 100,
+ (NEW_OWNER_KEY_PAYLOAD_PREFIX, new_key_payload).encode()
+ )
+ ));
+ // Old old owner key should not be sufficient anymore
+ assert_eq!(System::sufficients(&1), 0);
+ // Old owner key should still sufficient
+ assert_eq!(System::sufficients(&10), 1);
+ // New owner key should become a sufficient account
+ assert_eq!(System::sufficients(&100), 1);
+
+ // Revoke identity 1
+ assert_ok!(Identity::revoke_identity(
+ Origin::signed(42),
+ 1,
+ 100,
+ TestSignature(
+ 100,
+ (
+ REVOCATION_PAYLOAD_PREFIX,
+ RevocationPayload {
+ idty_index: 1u64,
+ genesis_hash: System::block_hash(0),
+ }
+ )
+ .encode()
+ )
+ ));
+ // Old owner key should not be sufficient anymore
+ assert_eq!(System::sufficients(&10), 0);
+ // Last owner key should not be sufficient anymore
+ assert_eq!(System::sufficients(&100), 0);
});
}
@@ -327,9 +375,17 @@ fn test_idty_revocation() {
));
let events = System::events();
- assert_eq!(events.len(), 1);
+ assert_eq!(events.len(), 2);
assert_eq!(
events[0],
+ EventRecord {
+ phase: Phase::Initialization,
+ event: Event::System(frame_system::Event::KilledAccount { account: 1 }),
+ topics: vec![],
+ }
+ );
+ assert_eq!(
+ events[1],
EventRecord {
phase: Phase::Initialization,
event: Event::Identity(crate::Event::IdtyRemoved { idty_index: 1 }),
--
GitLab