Fix account linking (!213)

* fix syntax

* fix change_owner_key test

* fix #153

* fix #147

* add account reaping test

pallets/duniter-account/src/lib.rs

@@ -32,7 +32,6 @@ use frame_support::pallet_prelude::*;
 use frame_support::traits::{Currency, ExistenceRequirement, StorageVersion};
 use frame_support::traits::{OnUnbalanced, StoredMap};
 use frame_system::pallet_prelude::*;
-use pallet_identity::IdtyEvent;
 use pallet_provide_randomness::RequestId;
 use pallet_quota::traits::RefundFee;
 use pallet_transaction_payment::OnChargeTransaction;
@@ -219,7 +218,16 @@ pub mod pallet {
         }
 
         /// link account to identity
-        pub fn do_link_identity(account_id: T::AccountId, idty_id: IdtyIdOf<T>) {
+        pub fn do_link_identity(
+            account_id: T::AccountId,
+            idty_id: IdtyIdOf<T>,
+        ) -> Result<(), DispatchError> {
+            // Check that account exist
+            ensure!(
+                (frame_system::Account::<T>::get(&account_id).providers >= 1)
+                    || (frame_system::Account::<T>::get(&account_id).sufficients >= 1),
+                pallet_identity::Error::<T>::AccountNotExist
+            );
             // no-op if identity does not change
             if frame_system::Account::<T>::get(&account_id)
                 .data
@@ -234,6 +242,7 @@ pub mod pallet {
                     });
                 })
             }
+            Ok(())
         }
     }
 
@@ -326,8 +335,8 @@ impl<T> pallet_identity::traits::LinkIdty<T::AccountId, IdtyIdOf<T>> for Pallet<
 where
     T: Config,
 {
-    fn link_identity(account_id: T::AccountId, idty_id: IdtyIdOf<T>) {
-        Self::do_link_identity(account_id, idty_id);
+    fn link_identity(account_id: T::AccountId, idty_id: IdtyIdOf<T>) -> Result<(), DispatchError> {
+        Self::do_link_identity(account_id, idty_id)
     }
 }
 
@@ -478,16 +487,3 @@ where
         Ok(())
     }
 }
-
-// implement identity event handler
-impl<T: Config> pallet_identity::traits::OnIdtyChange<T> for Pallet<T> {
-    fn on_idty_change(idty_id: IdtyIdOf<T>, idty_event: &IdtyEvent<T>) {
-        match idty_event {
-            // link account to newly created identity
-            IdtyEvent::Created { owner_key, .. } => {
-                Self::do_link_identity(owner_key.clone(), idty_id);
-            }
-            IdtyEvent::Removed { .. } => {}
-        }
-    }
-}

pallets/identity/src/benchmarking.rs

@@ -254,6 +254,7 @@ benchmarks! {
         let alice_origin = RawOrigin::Signed(Identities::<T>::get(T::IdtyIndex::one()).unwrap().owner_key);
         let bob_public = sr25519_generate(0.into(), None);
         let bob: T::AccountId = MultiSigner::Sr25519(bob_public).into_account().into();
+        frame_system::Pallet::<T>::inc_providers(&bob);
         let genesis_hash = frame_system::Pallet::<T>::block_hash(T::BlockNumber::zero());
         let payload = (
             LINK_IDTY_PAYLOAD_PREFIX, genesis_hash, T::IdtyIndex::one(), bob.clone(),

pallets/identity/src/lib.rs

@@ -328,6 +328,7 @@ pub mod pallet {
                 idty_index,
                 owner_key: owner_key.clone(),
             });
+            T::AccountLinker::link_identity(owner_key.clone(), idty_index)?;
             T::OnIdtyChange::on_idty_change(
                 idty_index,
                 &IdtyEvent::Created {
@@ -454,9 +455,10 @@ pub mod pallet {
             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);
+            T::AccountLinker::link_identity(new_key.clone(), idty_index)?;
             Self::deposit_event(Event::IdtyChangedOwnerKey {
                 idty_index,
-                new_owner_key: new_key.clone(),
+                new_owner_key: new_key,
             });
 
             Ok(().into())
@@ -584,7 +586,7 @@ pub mod pallet {
                 Error::<T>::InvalidSignature
             );
             // apply
-            Self::do_link_account(account_id, idty_index);
+            T::AccountLinker::link_identity(account_id, idty_index)?;
 
             Ok(().into())
         }
@@ -614,7 +616,7 @@ pub mod pallet {
         InvalidSignature,
         /// Invalid revocation key.
         InvalidRevocationKey,
-        /// Issuer is not member and can not perform this action
+        /// Issuer is not member and can not perform this action.
         IssuerNotMember,
         /// Identity creation period is not respected.
         NotRespectIdtyCreationPeriod,
@@ -624,12 +626,14 @@ pub mod pallet {
         OwnerKeyAlreadyUsed,
         /// Reverting to an old key is prohibited.
         ProhibitedToRevertToAnOldKey,
-        /// Already revoked
+        /// Already revoked.
         AlreadyRevoked,
-        /// Can not revoke identity that never was member
+        /// Can not revoke identity that never was member.
         CanNotRevokeUnconfirmed,
-        /// Can not revoke identity that never was member
+        /// Can not revoke identity that never was member.
         CanNotRevokeUnvalidated,
+        /// Cannot link to an inexisting account.
+        AccountNotExist,
     }
 
     // INTERNAL FUNCTIONS //
@@ -773,12 +777,6 @@ pub mod pallet {
             total_weight.saturating_add(T::WeightInfo::prune_identities_noop())
         }
 
-        /// link account
-        fn do_link_account(account_id: T::AccountId, idty_index: T::IdtyIndex) {
-            // call account linker
-            T::AccountLinker::link_identity(account_id, idty_index);
-        }
-
         /// change identity status and reschedule next action
         fn update_identity_status(
             idty_index: T::IdtyIndex,

pallets/identity/src/traits.rs

@@ -53,10 +53,12 @@ impl<T: Config> OnIdtyChange<T> for Tuple {
 
 /// trait used to link an account to an identity
 pub trait LinkIdty<AccountId, IdtyIndex> {
-    fn link_identity(account_id: AccountId, idty_index: IdtyIndex);
+    fn link_identity(account_id: AccountId, idty_index: IdtyIndex) -> Result<(), DispatchError>;
 }
 impl<AccountId, IdtyIndex> LinkIdty<AccountId, IdtyIndex> for () {
-    fn link_identity(_: AccountId, _: IdtyIndex) {}
+    fn link_identity(_: AccountId, _: IdtyIndex) -> Result<(), DispatchError> {
+        Ok(())
+    }
 }
 
 /// trait used only in benchmarks to prepare identity for benchmarking

runtime/common/src/pallets_config.rs

@@ -479,7 +479,7 @@ macro_rules! pallets_config {
             type IdtyNameValidator = IdtyNameValidatorImpl;
             type Signer = <Signature as sp_runtime::traits::Verify>::Signer;
 			type Signature = Signature;
-            type OnIdtyChange = (Wot, SmithSubWot, Quota, Account);
+            type OnIdtyChange = (Wot, SmithSubWot, Quota);
             type RuntimeEvent = RuntimeEvent;
             type WeightInfo = common_runtime::weights::pallet_identity::WeightInfo<Runtime>;
             #[cfg(feature = "runtime-benchmarks")]

runtime/gdev/tests/integration_tests.rs

@@ -1196,12 +1196,31 @@ fn test_oneshot_accounts() {
 /// test linking account to identity
 #[test]
 fn test_link_account() {
-    ExtBuilder::new(1, 3, 4).build().execute_with(|| {
+    ExtBuilder::new(1, 3, 4)
+        .with_initial_balances(vec![(AccountKeyring::Alice.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"link", genesis_hash, 1u32, ferdie.clone()).encode();
             let signature = AccountKeyring::Ferdie.sign(&payload);
+
+            // Ferdie's account cannot be linked to Alice identity because the account does not exist
+            assert_noop!(
+                Identity::link_account(
+                    frame_system::RawOrigin::Signed(alice.clone()).into(),
+                    ferdie.clone(),
+                    signature.clone().into()
+                ),
+                pallet_identity::Error::<gdev_runtime::Runtime>::AccountNotExist
+            );
+
+            assert_ok!(Balances::transfer(
+                frame_system::RawOrigin::Signed(alice.clone()).into(),
+                MultiAddress::Id(ferdie.clone()),
+                1_000
+            ));
             // Ferdie's account can be linked to Alice identity
             assert_ok!(Identity::link_account(
                 frame_system::RawOrigin::Signed(alice).into(),
@@ -1220,12 +1239,25 @@ fn test_change_owner_key() {
         let ferdie = AccountKeyring::Ferdie.to_account_id();
         let payload = (b"icok", genesis_hash, 4u32, dave.clone()).encode();
         let signature = AccountKeyring::Ferdie.sign(&payload);
+
+        assert_eq!(
+            frame_system::Pallet::<Runtime>::get(&dave).linked_idty,
+            Some(4)
+        );
+        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(
-            frame_system::RawOrigin::Signed(dave).into(),
-            ferdie,
+            frame_system::RawOrigin::Signed(dave.clone()).into(),
+            ferdie.clone(),
             signature.into()
         ));
+        assert_eq!(
+            frame_system::Pallet::<Runtime>::get(&ferdie).linked_idty,
+            Some(4)
+        );
     })
 }
 
@@ -1306,3 +1338,37 @@ fn smith_data_problem() {
             run_to_block(4);
         });
 }
+
+/// test killed account
+// The only way to kill an account is to kill  the identity
+// and transfer all funds.
+#[test]
+fn test_killed_account() {
+    ExtBuilder::new(1, 2, 4)
+        .with_initial_balances(vec![(AccountKeyring::Bob.to_account_id(), 1_000)])
+        .build()
+        .execute_with(|| {
+            let alice_account = AccountKeyring::Alice.to_account_id();
+            let bob_account = AccountKeyring::Bob.to_account_id();
+            // check that Alice is 1 and Bob 2
+            assert_eq!(Identity::identity_index_of(&alice_account), Some(1));
+            assert_eq!(Identity::identity_index_of(&bob_account), Some(2));
+
+            let _ = Identity::do_remove_identity(2, pallet_identity::RemovalReason::Revoked);
+            assert_eq!(
+                frame_system::Pallet::<Runtime>::get(&bob_account).linked_idty,
+                Some(2)
+            );
+            assert_ok!(Balances::transfer_all(
+                frame_system::RawOrigin::Signed(bob_account.clone()).into(),
+                sp_runtime::MultiAddress::Id(alice_account.clone()),
+                false
+            ));
+
+            // Bob account should have been reaped
+            assert_eq!(
+                frame_system::Pallet::<Runtime>::get(&bob_account),
+                pallet_duniter_account::AccountData::default()
+            );
+        })
+}