From f14d98a439392dbfbc17ac02f1c5c6cd00fa52b3 Mon Sep 17 00:00:00 2001 From: Gavin Wood Date: Mon, 16 Dec 2019 20:36:08 +0800 Subject: [PATCH] Identity module enhancements (#4401) * Updates; not yet tested. * Fix and add tests * Add test * Update a few comments --- substrate/frame/identity/src/lib.rs | 136 ++++++++++++++++++++++++---- 1 file changed, 120 insertions(+), 16 deletions(-) diff --git a/substrate/frame/identity/src/lib.rs b/substrate/frame/identity/src/lib.rs index a4e6a46b6b..904ab8cf2e 100644 --- a/substrate/frame/identity/src/lib.rs +++ b/substrate/frame/identity/src/lib.rs @@ -93,10 +93,12 @@ pub trait Trait: system::Trait { /// The amount held on deposit per additional field for a registered identity. type FieldDeposit: Get>; - /// The amount held on deposit for a registered subaccount. + /// The amount held on deposit for a registered subaccount. This should account for the fact + /// that one storage item's value will increase by the size of an account ID, and there will be + /// another trie item whose value is the size of an account ID plus 32 bytes. type SubAccountDeposit: Get>; - /// The amount held on deposit for a registered subaccount. + /// The maximum number of sub-accounts allowed per identified account. type MaximumSubAccounts: Get; /// What to do with slashed funds. @@ -363,11 +365,14 @@ decl_storage! { /// Information that is pertinent to identify the entity behind an account. pub IdentityOf get(fn identity): map T::AccountId => Option>>; + /// The super-identity of an alternative "sub" identity together with its name, within that + /// context. If the account is not some other account's sub-identity, then just `None`. + pub SuperOf get(fn super_of): map T::AccountId => Option<(T::AccountId, Data)>; + /// Alternative "sub" identities of this account. /// - /// The first item is the deposit, the second is a vector of the accounts together with - /// their "local" name (i.e. in the context of the identity). - pub SubsOf get(fn subs): map T::AccountId => (BalanceOf, Vec<(T::AccountId, Data)>); + /// The first item is the deposit, the second is a vector of the accounts. + pub SubsOf get(fn subs): map T::AccountId => (BalanceOf, Vec); /// The set of registrars. Not expected to get very big as can only be added through a /// special origin (likely a council motion). @@ -488,14 +493,15 @@ decl_module! { /// # /// - `O(S)` where `S` subs-count (hard- and deposit-bounded). /// - At most two balance operations. - /// - One storage mutation (codec `O(S)`); one storage-exists. + /// - At most O(2 * S + 1) storage mutations; codec complexity `O(1 * S + S * 1)`); + /// one storage-exists. /// # fn set_subs(origin, subs: Vec<(T::AccountId, Data)>) { let sender = ensure_signed(origin)?; ensure!(>::exists(&sender), "not found"); ensure!(subs.len() <= T::MaximumSubAccounts::get() as usize, "too many subs"); - let old_deposit = >::get(&sender).0; + let (old_deposit, old_ids) = >::get(&sender); let new_deposit = T::SubAccountDeposit::get() * >::from(subs.len() as u32); if old_deposit < new_deposit { @@ -506,10 +512,18 @@ decl_module! { let _ = T::Currency::unreserve(&sender, old_deposit - new_deposit); } - if subs.is_empty() { + for s in old_ids.iter() { + >::remove(s); + } + let ids = subs.into_iter().map(|(id, name)| { + >::insert(&id, (sender.clone(), name)); + id + }).collect::>(); + + if ids.is_empty() { >::remove(&sender); } else { - >::insert(&sender, (new_deposit, subs)); + >::insert(&sender, (new_deposit, ids)); } } @@ -525,14 +539,18 @@ decl_module! { /// # /// - `O(R + S + X)`. /// - One balance-reserve operation. - /// - Two storage mutations. + /// - `S + 2` storage deletions. /// - One event. /// # fn clear_identity(origin) { let sender = ensure_signed(origin)?; + let (subs_deposit, sub_ids) = >::take(&sender); let deposit = >::take(&sender).ok_or("not named")?.total_deposit() - + >::take(&sender).0; + + subs_deposit; + for sub in sub_ids.iter() { + >::remove(sub); + } let _ = T::Currency::unreserve(&sender, deposit.clone()); @@ -654,6 +672,33 @@ decl_module! { ) } + /// Change the account associated with a registrar. + /// + /// The dispatch origin for this call must be _Signed_ and the sender must be the account + /// of the registrar whose index is `index`. + /// + /// - `index`: the index of the registrar whose fee is to be set. + /// - `new`: the new account ID. + /// + /// # + /// - `O(R)`. + /// - One storage mutation `O(R)`. + /// # + #[weight = SimpleDispatchInfo::FixedNormal(50_000)] + fn set_account_id(origin, + #[compact] index: RegistrarIndex, + new: T::AccountId, + ) -> Result { + let who = ensure_signed(origin)?; + + >::mutate(|rs| + rs.get_mut(index as usize) + .and_then(|x| x.as_mut()) + .and_then(|r| if r.account == who { r.account = new; Some(()) } else { None }) + .ok_or("invalid index") + ) + } + /// Set the field information for a registrar. /// /// The dispatch origin for this call must be _Signed_ and the sender must be the account @@ -746,7 +791,7 @@ decl_module! { /// # /// - `O(R + S + X)`. /// - One balance-reserve operation. - /// - Two storage mutations. + /// - `S + 2` storage mutations. /// - One event. /// # #[weight = SimpleDispatchInfo::FreeOperational] @@ -759,8 +804,12 @@ decl_module! { // Figure out who we're meant to be clearing. let target = T::Lookup::lookup(target)?; // Grab their deposit (and check that they have one). + let (subs_deposit, sub_ids) = >::take(&target); let deposit = >::take(&target).ok_or("not named")?.total_deposit() - + >::take(&target).0; + + subs_deposit; + for sub in sub_ids.iter() { + >::remove(sub); + } // Slash their deposit from them. T::Slashed::on_unbalanced(T::Currency::slash_reserved(&target, deposit).0); @@ -967,18 +1016,60 @@ mod tests { assert_ok!(Identity::set_identity(Origin::signed(10), ten())); assert_ok!(Identity::set_subs(Origin::signed(10), subs.clone())); assert_eq!(Balances::free_balance(10), 80); - assert_eq!(Identity::subs(10), (10, subs.clone())); + assert_eq!(Identity::subs(10), (10, vec![20])); + assert_eq!(Identity::super_of(20), Some((10, Data::Raw(vec![40; 1])))); + // push another item and re-set it. + subs.push((30, Data::Raw(vec![50; 1]))); + assert_ok!(Identity::set_subs(Origin::signed(10), subs.clone())); + assert_eq!(Balances::free_balance(10), 70); + assert_eq!(Identity::subs(10), (20, vec![20, 30])); + assert_eq!(Identity::super_of(20), Some((10, Data::Raw(vec![40; 1])))); + assert_eq!(Identity::super_of(30), Some((10, Data::Raw(vec![50; 1])))); + + // switch out one of the items and re-set. + subs[0] = (40, Data::Raw(vec![60; 1])); + assert_ok!(Identity::set_subs(Origin::signed(10), subs.clone())); + assert_eq!(Balances::free_balance(10), 70); // no change in the balance + assert_eq!(Identity::subs(10), (20, vec![40, 30])); + assert_eq!(Identity::super_of(20), None); + assert_eq!(Identity::super_of(30), Some((10, Data::Raw(vec![50; 1])))); + assert_eq!(Identity::super_of(40), Some((10, Data::Raw(vec![60; 1])))); + + // clear assert_ok!(Identity::set_subs(Origin::signed(10), vec![])); assert_eq!(Balances::free_balance(10), 90); assert_eq!(Identity::subs(10), (0, vec![])); + assert_eq!(Identity::super_of(30), None); + assert_eq!(Identity::super_of(40), None); - subs.push((30, Data::Raw(vec![41; 1]))); - subs.push((40, Data::Raw(vec![42; 1]))); + subs.push((20, Data::Raw(vec![40; 1]))); assert_noop!(Identity::set_subs(Origin::signed(10), subs.clone()), "too many subs"); }); } + #[test] + fn clearing_account_should_remove_subaccounts_and_refund() { + new_test_ext().execute_with(|| { + assert_ok!(Identity::set_identity(Origin::signed(10), ten())); + assert_ok!(Identity::set_subs(Origin::signed(10), vec![(20, Data::Raw(vec![40; 1]))])); + assert_ok!(Identity::clear_identity(Origin::signed(10))); + assert_eq!(Balances::free_balance(10), 100); + assert!(Identity::super_of(20).is_none()); + }); + } + + #[test] + fn killing_account_should_remove_subaccounts_and_not_refund() { + new_test_ext().execute_with(|| { + assert_ok!(Identity::set_identity(Origin::signed(10), ten())); + assert_ok!(Identity::set_subs(Origin::signed(10), vec![(20, Data::Raw(vec![40; 1]))])); + assert_ok!(Identity::kill_identity(Origin::ROOT, 10)); + assert_eq!(Balances::free_balance(10), 80); + assert!(Identity::super_of(20).is_none()); + }); + } + #[test] fn cancelling_requested_judgement_should_work() { new_test_ext().execute_with(|| { @@ -1040,4 +1131,17 @@ mod tests { assert_eq!(Balances::free_balance(10), 70); }); } + + #[test] + fn setting_account_id_should_work() { + new_test_ext().execute_with(|| { + assert_ok!(Identity::add_registrar(Origin::signed(1), 3)); + // account 4 cannot change the first registrar's identity since it's owned by 3. + assert_noop!(Identity::set_account_id(Origin::signed(4), 0, 3), "invalid index"); + // account 3 can, because that's the registrar's current account. + assert_ok!(Identity::set_account_id(Origin::signed(3), 0, 4)); + // account 4 can now, because that's their new ID. + assert_ok!(Identity::set_account_id(Origin::signed(4), 0, 3)); + }); + } }