Identity module enhancements (#4401)

* Updates; not yet tested.

* Fix and add tests

* Add test

* Update a few comments

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<BalanceOf<Self>>;
 
-	/// 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<BalanceOf<Self>>;
 
-	/// The amount held on deposit for a registered subaccount.
+	/// The maximum number of sub-accounts allowed per identified account.
 	type MaximumSubAccounts: Get<u32>;
 
 	/// 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<Registration<BalanceOf<T>>>;
 
+		/// 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
+		/// The first item is the deposit, the second is a vector of the accounts.
-		/// their "local" name (i.e. in the context of the identity).
+		pub SubsOf get(fn subs): map T::AccountId => (BalanceOf<T>, Vec<T::AccountId>);
-		pub SubsOf get(fn subs): map T::AccountId => (BalanceOf<T>, Vec<(T::AccountId, Data)>);
 
 		/// 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! {
 		/// # <weight>
 		/// - `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.
 		/// # </weight>
 		fn set_subs(origin, subs: Vec<(T::AccountId, Data)>) {
 			let sender = ensure_signed(origin)?;
 			ensure!(<IdentityOf<T>>::exists(&sender), "not found");
 			ensure!(subs.len() <= T::MaximumSubAccounts::get() as usize, "too many subs");
 
-			let old_deposit = <SubsOf<T>>::get(&sender).0;
+			let (old_deposit, old_ids) = <SubsOf<T>>::get(&sender);
 			let new_deposit = T::SubAccountDeposit::get() * <BalanceOf<T>>::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() {
+				<SuperOf<T>>::remove(s);
+			}
+			let ids = subs.into_iter().map(|(id, name)| {
+				<SuperOf<T>>::insert(&id, (sender.clone(), name));
+				id
+			}).collect::<Vec<_>>();
+
+			if ids.is_empty() {
 				<SubsOf<T>>::remove(&sender);
 			} else {
-				<SubsOf<T>>::insert(&sender, (new_deposit, subs));
+				<SubsOf<T>>::insert(&sender, (new_deposit, ids));
 			}
 		}
 
@@ -525,14 +539,18 @@ decl_module! {
 		/// # <weight>
 		/// - `O(R + S + X)`.
 		/// - One balance-reserve operation.
-		/// - Two storage mutations.
+		/// - `S + 2` storage deletions.
 		/// - One event.
 		/// # </weight>
 		fn clear_identity(origin) {
 			let sender = ensure_signed(origin)?;
 
+			let (subs_deposit, sub_ids) = <SubsOf<T>>::take(&sender);
 			let deposit = <IdentityOf<T>>::take(&sender).ok_or("not named")?.total_deposit()
-				+ <SubsOf<T>>::take(&sender).0;
+				+ subs_deposit;
+			for sub in sub_ids.iter() {
+				<SuperOf<T>>::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.
+		///
+		/// # <weight>
+		/// - `O(R)`.
+		/// - One storage mutation `O(R)`.
+		/// # </weight>
+		#[weight = SimpleDispatchInfo::FixedNormal(50_000)]
+		fn set_account_id(origin,
+			#[compact] index: RegistrarIndex,
+			new: T::AccountId,
+		) -> Result {
+			let who = ensure_signed(origin)?;
+
+			<Registrars<T>>::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! {
 		/// # <weight>
 		/// - `O(R + S + X)`.
 		/// - One balance-reserve operation.
-		/// - Two storage mutations.
+		/// - `S + 2` storage mutations.
 		/// - One event.
 		/// # </weight>
 		#[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) = <SubsOf<T>>::take(&target);
 			let deposit = <IdentityOf<T>>::take(&target).ok_or("not named")?.total_deposit()
-				+ <SubsOf<T>>::take(&target).0;
+				+ subs_deposit;
+			for sub in sub_ids.iter() {
+				<SuperOf<T>>::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((20, Data::Raw(vec![40; 1])));
-			subs.push((40, Data::Raw(vec![42; 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));
+		});
+	}
 }