[Identity] Decouple usernames from identities #5554

georgepisaltu · 2024-09-02T16:46:13Z

This PR refactors pallet-identity to decouple usernames from identities.

Main changes in this PR:

Separate usernames from identities in storage, allowing for correct deposit accounting
Introduce the option for username authorities to put up a deposit to issue a username
Allow authorities to remove usernames by declaring the intent to do so, then removing the username after the grace period expires
Refactor the authority storage to be keyed by suffix rather than owner account.
Introduce the concept of a system provider for a username, different from a governance allocation, allowing for usernames set by the system and not a specific authority
Implement multi-block migration to enable all of the changes described above

Signed-off-by: georgepisaltu <[email protected]>

…-refactor

paritytech-cicd-pr · 2024-09-02T17:05:45Z

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: test-linux-stable-int
Logs: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/7222927

Signed-off-by: georgepisaltu <[email protected]>

…-refactor

substrate/frame/identity/src/migration.rs

Signed-off-by: georgepisaltu <[email protected]>

…-refactor

Signed-off-by: georgepisaltu <[email protected]>

substrate/frame/identity/README.md

cumulus/parachains/runtimes/people/people-rococo/src/lib.rs

cumulus/parachains/runtimes/people/people-westend/src/lib.rs

polkadot/runtime/westend/src/lib.rs

gui1117 · 2024-09-12T09:00:57Z

substrate/frame/identity/src/lib.rs

+					},
+					Provider::Allocation => {
+						// We don't refund the allocation, it is lost, but we refund some weight.
+						T::WeightInfo::remove_expired_approval(1)


maybe having some const or enum instead of 0 and 1 would make it more readable.

But it is good to me.

Could just pass the Provider Enum? Similarly in other places.

Signed-off-by: georgepisaltu <[email protected]>

substrate/frame/identity/src/lib.rs

Ank4n · 2024-09-13T11:06:29Z

substrate/frame/identity/src/lib.rs

@@ -265,33 +297,47 @@ pub mod pallet {

 	/// A map of the accounts who are authorized to grant usernames.
 	#[pallet::storage]
-	pub type UsernameAuthorities<T: Config> =
-		StorageMap<_, Twox64Concat, T::AccountId, AuthorityPropertiesOf<T>, OptionQuery>;
+	pub type AuthorityOf<T: Config> =


This implies for a given suffix, there can be only one authority? Wasn't clear from the docs.

Yes, one authority per suffix. I will update the docs.

Ank4n · 2024-09-13T11:13:30Z

substrate/frame/identity/src/lib.rs

+		/// A username has been removed.
+		UsernameRemoved { username: Username<T> },
+		/// A username has been killed.
+		UsernameKilled { username: Username<T> },


UsernameRemoved and UsernameKilled sounds very synonymous. May be should call it as ForceUsernameRemoved?

From pallet terminology in docs, "removed" and "killed" are different things. I'd prefer to stick to even naming convention of 2 word events and defer to call documentation. I could write more comprehensive docs for the event enum variants.

Ank4n · 2024-09-13T11:18:37Z

substrate/frame/identity/src/lib.rs

 			// The authority may already exist, but we don't need to check. They might be changing
 			// their suffix or adding allocation, so we just want to overwrite whatever was there.


Comment needs to be updated.
Do we still don't need to check for overwrite? This might be overwriting authority for a given suffix.

I'll rethink this section.

Ank4n · 2024-09-13T11:21:40Z

substrate/frame/identity/src/lib.rs

@@ -1041,11 +1087,15 @@ pub mod pallet {
 		#[pallet::weight(T::WeightInfo::remove_username_authority())]
 		pub fn remove_username_authority(
 			origin: OriginFor<T>,
+			suffix: Vec<u8>,
 			authority: AccountIdLookupOf<T>,


Don't strictly need authority as input anymore. But fine either way.

Ank4n · 2024-09-13T11:28:38Z

substrate/frame/identity/src/lib.rs

 		pub fn set_username_for(
 			origin: OriginFor<T>,
 			who: AccountIdLookupOf<T>,
 			username: Vec<u8>,
 			signature: Option<T::OffchainSignature>,
+			use_allocation: bool,


Update fn rustdoc about use_allocation flag? Currently it only says authority must have an allocation.

Ank4n · 2024-09-13T11:31:39Z

substrate/frame/identity/src/lib.rs

+					},
+					Provider::Allocation => {
+						// We don't refund the allocation, it is lost, but we refund some weight.
+						T::WeightInfo::remove_expired_approval(1)


Could just pass the Provider Enum? Similarly in other places.

substrate/frame/identity/src/benchmarking.rs

substrate/frame/identity/src/migration.rs

Signed-off-by: georgepisaltu <[email protected]>

substrate/frame/identity/src/migration.rs

Signed-off-by: georgepisaltu <[email protected]>

gui1117

Looks good to me

substrate/frame/identity/src/lib.rs

…-refactor

Signed-off-by: georgepisaltu <[email protected]>

…-refactor

georgepisaltu · 2024-09-30T08:45:47Z

I implemented the try-runtime checks.

There is a unit test for the migration and I manually tested the try-runtime checks with the changes in paritytech/try-runtime-cli#90. They probably don't pass in the CI here but they passed for me locally.

georgepisaltu added 23 commits August 24, 2024 00:51

WIP username refactor

288d351

Signed-off-by: georgepisaltu <[email protected]>

Minor fixes

06f8472

Signed-off-by: georgepisaltu <[email protected]>

Fix test compilation

ebdd61d

Signed-off-by: georgepisaltu <[email protected]>

Fix a couple of tests

e42fb1b

Signed-off-by: georgepisaltu <[email protected]>

All tests pass

cfa7406

Signed-off-by: georgepisaltu <[email protected]>

Add tests related to username deletion

7d351ae

Signed-off-by: georgepisaltu <[email protected]>

Fix tests for accepting usernames

5119afe

Signed-off-by: georgepisaltu <[email protected]>

Add the rest of unit tests

6b64952

Signed-off-by: georgepisaltu <[email protected]>

Fix benches for new changes to usernames

083b168

Signed-off-by: georgepisaltu <[email protected]>

Add initial migration code

6269619

Signed-off-by: georgepisaltu <[email protected]>

Finish writing migration code

8e326af

Signed-off-by: georgepisaltu <[email protected]>

Add benchmarked weight for migration steps

4b3ee47

Signed-off-by: georgepisaltu <[email protected]>

WIP fix test

987fc4e

Signed-off-by: georgepisaltu <[email protected]>

WIP some other stuff

831c556

Signed-off-by: georgepisaltu <[email protected]>

WIP test almost fixed

f28afed

Signed-off-by: georgepisaltu <[email protected]>

Fix first part of migration

34611fb

Signed-off-by: georgepisaltu <[email protected]>

Fix compilation fail

ac22ee8

Signed-off-by: georgepisaltu <[email protected]>

Fix tests

717439a

Signed-off-by: georgepisaltu <[email protected]>

Finish migration

2736a05

Signed-off-by: georgepisaltu <[email protected]>

Get rid of benchmarks

db04e86

Signed-off-by: georgepisaltu <[email protected]>

Some renames

040cd62

Signed-off-by: georgepisaltu <[email protected]>

Update docs

36b474e

Signed-off-by: georgepisaltu <[email protected]>

Merge remote-tracking branch 'upstream/master' into identity-username…

c33ff74

…-refactor

georgepisaltu added the T2-pallets This PR/Issue is related to a particular pallet. label Sep 2, 2024

georgepisaltu self-assigned this Sep 2, 2024

georgepisaltu requested a review from a team as a code owner September 2, 2024 16:46

georgepisaltu requested review from ggwpez, seadanda and joepetrowski September 2, 2024 16:47

georgepisaltu added 3 commits September 10, 2024 18:58

Refactor storage alias types

1035f6f

Signed-off-by: georgepisaltu <[email protected]>

Add dummy weights

448c034

Signed-off-by: georgepisaltu <[email protected]>

Merge remote-tracking branch 'upstream/master' into identity-username…

0acaffd

…-refactor

paritytech-review-bot bot requested a review from a team September 10, 2024 16:03

ggwpez reviewed Sep 10, 2024

View reviewed changes

substrate/frame/identity/src/migration.rs Show resolved Hide resolved

substrate/frame/identity/src/migration.rs Outdated Show resolved Hide resolved

substrate/frame/identity/src/migration.rs Show resolved Hide resolved

georgepisaltu added 3 commits September 11, 2024 11:22

Small refactor

35bcc68

Signed-off-by: georgepisaltu <[email protected]>

Add prdoc

54c67b6

Signed-off-by: georgepisaltu <[email protected]>

Merge remote-tracking branch 'upstream/master' into identity-username…

7a33cd4

…-refactor

paritytech-review-bot bot requested a review from a team September 11, 2024 09:38

ggwpez approved these changes Sep 11, 2024

View reviewed changes

georgepisaltu added 4 commits September 11, 2024 15:43

Enable migration

7a9d48c

Signed-off-by: georgepisaltu <[email protected]>

Fix compilation

4acddb0

Signed-off-by: georgepisaltu <[email protected]>

Manually set the storage version

a49211e

Signed-off-by: georgepisaltu <[email protected]>

Fix bench compile

e44d6d2

Signed-off-by: georgepisaltu <[email protected]>

gui1117 reviewed Sep 12, 2024

View reviewed changes

Actually enable MBMs for omitted runtimes

bb0153c

Signed-off-by: georgepisaltu <[email protected]>

gui1117 reviewed Sep 12, 2024

View reviewed changes

substrate/frame/identity/src/lib.rs Outdated Show resolved Hide resolved

Ank4n reviewed Sep 13, 2024

View reviewed changes

Merge branch 'master' into identity-username-refactor

b39175f

gui1117 reviewed Sep 16, 2024

View reviewed changes

substrate/frame/identity/src/benchmarking.rs Outdated Show resolved Hide resolved

substrate/frame/identity/src/migration.rs Show resolved Hide resolved

substrate/frame/identity/src/migration.rs Show resolved Hide resolved

georgepisaltu added 2 commits September 16, 2024 10:46

Remove obsolete dangling username call

af85fc7

Signed-off-by: georgepisaltu <[email protected]>

Update docs

7fe2d82

Signed-off-by: georgepisaltu <[email protected]>

gui1117 reviewed Sep 16, 2024

View reviewed changes

substrate/frame/identity/src/migration.rs Show resolved Hide resolved

Truncate keys

c3d22fe

Signed-off-by: georgepisaltu <[email protected]>

gui1117 approved these changes Sep 16, 2024

View reviewed changes

substrate/frame/identity/src/lib.rs Outdated Show resolved Hide resolved

georgepisaltu added 4 commits September 26, 2024 17:32

Merge remote-tracking branch 'upstream/master' into identity-username…

112d870

…-refactor

Add try runtime checks

a11ef4a

Signed-off-by: georgepisaltu <[email protected]>

Fix docs

976e0ff

Signed-off-by: georgepisaltu <[email protected]>

Merge remote-tracking branch 'upstream/master' into identity-username…

a3ed0b1

…-refactor

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Identity] Decouple usernames from identities #5554

[Identity] Decouple usernames from identities #5554

georgepisaltu commented Sep 2, 2024

paritytech-cicd-pr commented Sep 2, 2024

gui1117 Sep 12, 2024

Ank4n Sep 13, 2024

Ank4n Sep 13, 2024

georgepisaltu Sep 13, 2024

Ank4n Sep 13, 2024

georgepisaltu Sep 13, 2024

Ank4n Sep 13, 2024

georgepisaltu Sep 13, 2024

Ank4n Sep 13, 2024

Ank4n Sep 13, 2024

Ank4n Sep 13, 2024

gui1117 left a comment

georgepisaltu commented Sep 30, 2024

		// The authority may already exist, but we don't need to check. They might be changing
		// their suffix or adding allocation, so we just want to overwrite whatever was there.

[Identity] Decouple usernames from identities #5554

Are you sure you want to change the base?

[Identity] Decouple usernames from identities #5554

Conversation

georgepisaltu commented Sep 2, 2024

paritytech-cicd-pr commented Sep 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gui1117 left a comment

Choose a reason for hiding this comment

georgepisaltu commented Sep 30, 2024