Identity pallet improvements (paritytech#2048)

This PR is a follow up to paritytech#1661 

- [x] rename the `simple` module to `legacy`
- [x] fix benchmarks to disregard the number of additional fields
- [x] change the storage deposits to charge per encoded byte of the
identity information instance, removing the need for `fn
additional(&self) -> usize` in `IdentityInformationProvider`
- [x] ~add an extrinsic to rejig deposits to account for the change
above~
- [ ] ~ensure through proper configuration that the new byte-based
deposit is always lower than whatever is reserved now~
- [x] remove `IdentityFields` from the `set_fields` extrinsic signature,
as per [this
discussion](paritytech#1661 (comment))

> ensure through proper configuration that the new byte-based deposit is
always lower than whatever is reserved now

Not sure this is needed anymore. If the new deposits are higher than
what is currently on chain and users don't have enough funds to reserve
what is needed, the extrinisc fails and they're basically grandfathered
and frozen until they add more funds and/or make a change to their
identity. This behavior seems fine to me. Original idea
[here](paritytech#1661 (comment)).

> add an extrinsic to rejig deposits to account for the change above

This was initially implemented but now removed from this PR in favor of
the implementation detailed
[here](paritytech#2088).

---------

Signed-off-by: georgepisaltu <george.pisaltu@parity.io>
Co-authored-by: joepetrowski <joe@parity.io>

substrate/bin/node/runtime/src/impls.rs

@@ -26,6 +26,7 @@ use frame_support::{
 };
 use pallet_alliance::{IdentityVerifier, ProposalIndex, ProposalProvider};
 use pallet_asset_tx_payment::HandleCredit;
+use pallet_identity::legacy::IdentityField;
 use sp_std::prelude::*;
 
 use crate::{
@@ -56,8 +57,8 @@ impl HandleCredit<AccountId, Assets> for CreditToBlockAuthor {
 
 pub struct AllianceIdentityVerifier;
 impl IdentityVerifier<AccountId> for AllianceIdentityVerifier {
-	fn has_identity(who: &AccountId, fields: u64) -> bool {
-		crate::Identity::has_identity(who, fields)
+	fn has_required_identities(who: &AccountId) -> bool {
+		crate::Identity::has_identity(who, (IdentityField::Display | IdentityField::Web).bits())
 	}
 
 	fn has_good_judgement(who: &AccountId) -> bool {

substrate/bin/node/runtime/src/lib.rs

@@ -60,7 +60,7 @@ use node_primitives::{AccountIndex, Balance, BlockNumber, Hash, Moment, Nonce};
 use pallet_asset_conversion::{NativeOrAssetId, NativeOrAssetIdConverter};
 use pallet_broker::{CoreAssignment, CoreIndex, CoretimeInterface, PartsOf57600};
 use pallet_election_provider_multi_phase::{GeometricDepositBase, SolutionAccuracyOf};
-use pallet_identity::simple::IdentityInfo;
+use pallet_identity::legacy::IdentityInfo;
 use pallet_im_online::sr25519::AuthorityId as ImOnlineId;
 use pallet_nfts::PalletFeatures;
 use pallet_nis::WithMaximumOf;
@@ -1459,8 +1459,10 @@ impl pallet_grandpa::Config for Runtime {
 }
 
 parameter_types! {
-	pub const BasicDeposit: Balance = 10 * DOLLARS;       // 258 bytes on-chain
-	pub const FieldDeposit: Balance = 250 * CENTS;        // 66 bytes on-chain
+	// difference of 26 bytes on-chain for the registration and 9 bytes on-chain for the identity
+	// information, already accounted for by the byte deposit
+	pub const BasicDeposit: Balance = deposit(1, 17);
+	pub const ByteDeposit: Balance = deposit(0, 1);
 	pub const SubAccountDeposit: Balance = 2 * DOLLARS;   // 53 bytes on-chain
 	pub const MaxSubAccounts: u32 = 100;
 	pub const MaxAdditionalFields: u32 = 100;
@@ -1471,10 +1473,9 @@ impl pallet_identity::Config for Runtime {
 	type RuntimeEvent = RuntimeEvent;
 	type Currency = Balances;
 	type BasicDeposit = BasicDeposit;
-	type FieldDeposit = FieldDeposit;
+	type ByteDeposit = ByteDeposit;
 	type SubAccountDeposit = SubAccountDeposit;
 	type MaxSubAccounts = MaxSubAccounts;
-	type MaxAdditionalFields = MaxAdditionalFields;
 	type IdentityInformation = IdentityInfo<MaxAdditionalFields>;
 	type MaxRegistrars = MaxRegistrars;
 	type Slashed = Treasury;

substrate/frame/alliance/src/lib.rs

@@ -112,7 +112,6 @@ use frame_support::{
 	},
 	weights::Weight,
 };
-use pallet_identity::simple::IdentityField;
 use scale_info::TypeInfo;
 
 pub use pallet::*;
@@ -135,9 +134,9 @@ type NegativeImbalanceOf<T, I> = <<T as Config<I>>::Currency as Currency<
 
 /// Interface required for identity verification.
 pub trait IdentityVerifier<AccountId> {
-	/// Function that returns whether an account has an identity registered with the identity
-	/// provider.
-	fn has_identity(who: &AccountId, fields: u64) -> bool;
+	/// Function that returns whether an account has the required identities registered with the
+	/// identity provider.
+	fn has_required_identities(who: &AccountId) -> bool;
 
 	/// Whether an account has been deemed "good" by the provider.
 	fn has_good_judgement(who: &AccountId) -> bool;
@@ -149,7 +148,7 @@ pub trait IdentityVerifier<AccountId> {
 
 /// The non-provider. Imposes no restrictions on account identity.
 impl<AccountId> IdentityVerifier<AccountId> for () {
-	fn has_identity(_who: &AccountId, _fields: u64) -> bool {
+	fn has_required_identities(_who: &AccountId) -> bool {
 		true
 	}
 
@@ -339,7 +338,7 @@ pub mod pallet {
 		/// Balance is insufficient for the required deposit.
 		InsufficientFunds,
 		/// The account's identity does not have display field and website field.
-		WithoutIdentityDisplayAndWebsite,
+		WithoutRequiredIdentityFields,
 		/// The account's identity has no good judgement.
 		WithoutGoodIdentityJudgement,
 		/// The proposal hash is not found.
@@ -1082,13 +1081,10 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
 	}
 
 	fn has_identity(who: &T::AccountId) -> DispatchResult {
-		const IDENTITY_FIELD_DISPLAY: u64 = IdentityField::Display as u64;
-		const IDENTITY_FIELD_WEB: u64 = IdentityField::Web as u64;
-
 		let judgement = |who: &T::AccountId| -> DispatchResult {
 			ensure!(
-				T::IdentityVerifier::has_identity(who, IDENTITY_FIELD_DISPLAY | IDENTITY_FIELD_WEB),
-				Error::<T, I>::WithoutIdentityDisplayAndWebsite
+				T::IdentityVerifier::has_required_identities(who),
+				Error::<T, I>::WithoutRequiredIdentityFields
 			);
 			ensure!(
 				T::IdentityVerifier::has_good_judgement(who),

substrate/frame/alliance/src/mock.rs

@@ -31,7 +31,10 @@ pub use frame_support::{
 	BoundedVec,
 };
 use frame_system::{EnsureRoot, EnsureSignedBy};
-use pallet_identity::{simple::IdentityInfo, Data, Judgement};
+use pallet_identity::{
+	legacy::{IdentityField, IdentityInfo},
+	Data, Judgement,
+};
 
 pub use crate as pallet_alliance;
 
@@ -96,9 +99,9 @@ impl pallet_collective::Config<AllianceCollective> for Test {
 }
 
 parameter_types! {
-	pub const BasicDeposit: u64 = 10;
-	pub const FieldDeposit: u64 = 10;
-	pub const SubAccountDeposit: u64 = 10;
+	pub const BasicDeposit: u64 = 100;
+	pub const ByteDeposit: u64 = 10;
+	pub const SubAccountDeposit: u64 = 100;
 	pub const MaxSubAccounts: u32 = 2;
 	pub const MaxAdditionalFields: u32 = 2;
 	pub const MaxRegistrars: u32 = 20;
@@ -117,10 +120,9 @@ impl pallet_identity::Config for Test {
 	type RuntimeEvent = RuntimeEvent;
 	type Currency = Balances;
 	type BasicDeposit = BasicDeposit;
-	type FieldDeposit = FieldDeposit;
+	type ByteDeposit = ByteDeposit;
 	type SubAccountDeposit = SubAccountDeposit;
 	type MaxSubAccounts = MaxSubAccounts;
-	type MaxAdditionalFields = MaxAdditionalFields;
 	type IdentityInformation = IdentityInfo<MaxAdditionalFields>;
 	type MaxRegistrars = MaxRegistrars;
 	type Slashed = ();
@@ -131,8 +133,8 @@ impl pallet_identity::Config for Test {
 
 pub struct AllianceIdentityVerifier;
 impl IdentityVerifier<AccountId> for AllianceIdentityVerifier {
-	fn has_identity(who: &AccountId, fields: u64) -> bool {
-		Identity::has_identity(who, fields)
+	fn has_required_identities(who: &AccountId) -> bool {
+		Identity::has_identity(who, (IdentityField::Display | IdentityField::Web).bits())
 	}
 
 	fn has_good_judgement(who: &AccountId) -> bool {
@@ -232,20 +234,40 @@ frame_support::construct_runtime!(
 	}
 );
 
+fn test_identity_info() -> IdentityInfo<MaxAdditionalFields> {
+	IdentityInfo {
+		additional: BoundedVec::default(),
+		display: Data::Raw(b"name".to_vec().try_into().unwrap()),
+		legal: Data::default(),
+		web: Data::Raw(b"website".to_vec().try_into().unwrap()),
+		riot: Data::default(),
+		email: Data::default(),
+		pgp_fingerprint: None,
+		image: Data::default(),
+		twitter: Data::default(),
+	}
+}
+
+pub(super) fn test_identity_info_deposit() -> <Test as pallet_balances::Config>::Balance {
+	let basic_deposit: u64 = <Test as pallet_identity::Config>::BasicDeposit::get();
+	let byte_deposit: u64 = <Test as pallet_identity::Config>::ByteDeposit::get();
+	byte_deposit * test_identity_info().encoded_size() as u64 + basic_deposit
+}
+
 pub fn new_test_ext() -> sp_io::TestExternalities {
 	let mut t = frame_system::GenesisConfig::<Test>::default().build_storage().unwrap();
 
 	pallet_balances::GenesisConfig::<Test> {
 		balances: vec![
-			(1, 50),
-			(2, 50),
-			(3, 50),
-			(4, 50),
-			(5, 30),
-			(6, 50),
-			(7, 50),
-			(8, 50),
-			(9, 50),
+			(1, 1000),
+			(2, 1000),
+			(3, 1000),
+			(4, 1000),
+			(5, test_identity_info_deposit() + 10),
+			(6, 1000),
+			(7, 1000),
+			(8, 1000),
+			(9, 1000),
 		],
 	}
 	.assimilate_storage(&mut t)
@@ -263,17 +285,7 @@ pub fn new_test_ext() -> sp_io::TestExternalities {
 	ext.execute_with(|| {
 		assert_ok!(Identity::add_registrar(RuntimeOrigin::signed(1), 1));
 
-		let info = IdentityInfo {
-			additional: BoundedVec::default(),
-			display: Data::Raw(b"name".to_vec().try_into().unwrap()),
-			legal: Data::default(),
-			web: Data::Raw(b"website".to_vec().try_into().unwrap()),
-			riot: Data::default(),
-			email: Data::default(),
-			pgp_fingerprint: None,
-			image: Data::default(),
-			twitter: Data::default(),
-		};
+		let info = test_identity_info();
 		assert_ok!(Identity::set_identity(RuntimeOrigin::signed(1), Box::new(info.clone())));
 		assert_ok!(Identity::provide_judgement(
 			RuntimeOrigin::signed(1),

substrate/frame/alliance/src/tests.rs

@@ -105,6 +105,9 @@ fn init_members_works() {
 #[test]
 fn disband_works() {
 	new_test_ext().execute_with(|| {
+		let id_deposit = test_identity_info_deposit();
+		let expected_join_deposit = <Test as Config>::AllyDeposit::get();
+		assert_eq!(Balances::free_balance(9), 1000 - id_deposit);
 		// ensure alliance is set
 		assert_eq!(Alliance::voting_members(), vec![1, 2, 3]);
 
@@ -113,10 +116,10 @@ fn disband_works() {
 		assert!(Alliance::is_member_of(&2, MemberRole::Retiring));
 
 		// join alliance and reserve funds
-		assert_eq!(Balances::free_balance(9), 40);
+		assert_eq!(Balances::free_balance(9), 1000 - id_deposit);
 		assert_ok!(Alliance::join_alliance(RuntimeOrigin::signed(9)));
-		assert_eq!(Alliance::deposit_of(9), Some(25));
-		assert_eq!(Balances::free_balance(9), 15);
+		assert_eq!(Alliance::deposit_of(9), Some(expected_join_deposit));
+		assert_eq!(Balances::free_balance(9), 1000 - id_deposit - expected_join_deposit);
 		assert!(Alliance::is_member_of(&9, MemberRole::Ally));
 
 		// fails without root
@@ -146,7 +149,7 @@ fn disband_works() {
 		// assert a retiring member from the previous Alliance not removed
 		assert!(Alliance::is_member_of(&2, MemberRole::Retiring));
 		// deposit unreserved
-		assert_eq!(Balances::free_balance(9), 40);
+		assert_eq!(Balances::free_balance(9), 1000 - id_deposit);
 
 		System::assert_last_event(mock::RuntimeEvent::Alliance(crate::Event::AllianceDisbanded {
 			fellow_members: 2,
@@ -358,6 +361,9 @@ fn remove_announcement_works() {
 #[test]
 fn join_alliance_works() {
 	new_test_ext().execute_with(|| {
+		let id_deposit = test_identity_info_deposit();
+		let join_deposit = <Test as Config>::AllyDeposit::get();
+		assert_eq!(Balances::free_balance(9), 1000 - id_deposit);
 		// check already member
 		assert_noop!(
 			Alliance::join_alliance(RuntimeOrigin::signed(1)),
@@ -384,8 +390,10 @@ fn join_alliance_works() {
 			Error::<Test, ()>::InsufficientFunds
 		);
 
+		assert_eq!(Balances::free_balance(4), 1000 - id_deposit);
 		// success to submit
 		assert_ok!(Alliance::join_alliance(RuntimeOrigin::signed(4)));
+		assert_eq!(Balances::free_balance(4), 1000 - id_deposit - join_deposit);
 		assert_eq!(Alliance::deposit_of(4), Some(25));
 		assert_eq!(Alliance::members(MemberRole::Ally), vec![4]);
 
@@ -405,7 +413,7 @@ fn join_alliance_works() {
 		#[cfg(not(feature = "runtime-benchmarks"))]
 		assert_noop!(
 			Alliance::join_alliance(RuntimeOrigin::signed(7)),
-			Error::<Test, ()>::WithoutIdentityDisplayAndWebsite
+			Error::<Test, ()>::WithoutRequiredIdentityFields
 		);
 	});
 }
@@ -460,7 +468,7 @@ fn nominate_ally_works() {
 		#[cfg(not(feature = "runtime-benchmarks"))]
 		assert_noop!(
 			Alliance::join_alliance(RuntimeOrigin::signed(7)),
-			Error::<Test, ()>::WithoutIdentityDisplayAndWebsite
+			Error::<Test, ()>::WithoutRequiredIdentityFields
 		);
 	});
 }