fix(identity): handle sufficients counter when owner key change (!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

@@ -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 {
+            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,

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);
     });

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 }),