[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 <george.pisaltu@parity.io>

…-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 <george.pisaltu@parity.io>

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 <george.pisaltu@parity.io>

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 <george.pisaltu@parity.io>

substrate/frame/identity/src/migration.rs

Signed-off-by: georgepisaltu <george.pisaltu@parity.io>

gui1117

Looks good to me

substrate/frame/identity/src/lib.rs

…-refactor

Signed-off-by: georgepisaltu <george.pisaltu@parity.io>

…-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.

…-refactor Signed-off-by: georgepisaltu <george.pisaltu@parity.io>

Signed-off-by: georgepisaltu <george.pisaltu@parity.io>

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 <george.pisaltu@parity.io> Co-authored-by: Ankan <10196091+Ank4n@users.noreply.github.com>

georgepisaltu added 23 commits August 24, 2024 00:51

WIP username refactor

288d351

Signed-off-by: georgepisaltu <george.pisaltu@parity.io>

Minor fixes

06f8472

Signed-off-by: georgepisaltu <george.pisaltu@parity.io>

Fix test compilation

ebdd61d

Signed-off-by: georgepisaltu <george.pisaltu@parity.io>

Fix a couple of tests

e42fb1b

Signed-off-by: georgepisaltu <george.pisaltu@parity.io>

All tests pass

cfa7406

Signed-off-by: georgepisaltu <george.pisaltu@parity.io>

Add tests related to username deletion

7d351ae

Signed-off-by: georgepisaltu <george.pisaltu@parity.io>

Fix tests for accepting usernames

5119afe

Signed-off-by: georgepisaltu <george.pisaltu@parity.io>

Add the rest of unit tests

6b64952

Signed-off-by: georgepisaltu <george.pisaltu@parity.io>

Fix benches for new changes to usernames

083b168

Signed-off-by: georgepisaltu <george.pisaltu@parity.io>

Add initial migration code

6269619

Signed-off-by: georgepisaltu <george.pisaltu@parity.io>

Finish writing migration code

8e326af

Signed-off-by: georgepisaltu <george.pisaltu@parity.io>

Add benchmarked weight for migration steps

4b3ee47

Signed-off-by: georgepisaltu <george.pisaltu@parity.io>

WIP fix test

987fc4e

Signed-off-by: georgepisaltu <george.pisaltu@parity.io>

WIP some other stuff

831c556

Signed-off-by: georgepisaltu <george.pisaltu@parity.io>

WIP test almost fixed

f28afed

Signed-off-by: georgepisaltu <george.pisaltu@parity.io>

Fix first part of migration

34611fb

Signed-off-by: georgepisaltu <george.pisaltu@parity.io>

Fix compilation fail

ac22ee8

Signed-off-by: georgepisaltu <george.pisaltu@parity.io>

Fix tests

717439a

Signed-off-by: georgepisaltu <george.pisaltu@parity.io>

Finish migration

2736a05

Signed-off-by: georgepisaltu <george.pisaltu@parity.io>

Get rid of benchmarks

db04e86

Signed-off-by: georgepisaltu <george.pisaltu@parity.io>

Some renames

040cd62

Signed-off-by: georgepisaltu <george.pisaltu@parity.io>

Update docs

36b474e

Signed-off-by: georgepisaltu <george.pisaltu@parity.io>

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

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 <george.pisaltu@parity.io>

Fix compilation

4acddb0

Signed-off-by: georgepisaltu <george.pisaltu@parity.io>

Manually set the storage version

a49211e

Signed-off-by: georgepisaltu <george.pisaltu@parity.io>

Fix bench compile

e44d6d2

Signed-off-by: georgepisaltu <george.pisaltu@parity.io>

gui1117 reviewed Sep 12, 2024

View reviewed changes

Actually enable MBMs for omitted runtimes

bb0153c

Signed-off-by: georgepisaltu <george.pisaltu@parity.io>

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 <george.pisaltu@parity.io>

Update docs

7fe2d82

Signed-off-by: georgepisaltu <george.pisaltu@parity.io>

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 <george.pisaltu@parity.io>

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 <george.pisaltu@parity.io>

Fix docs

976e0ff

Signed-off-by: georgepisaltu <george.pisaltu@parity.io>

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

a3ed0b1

…-refactor

georgepisaltu added 2 commits October 29, 2024 10:27

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

f31fdd6

…-refactor Signed-off-by: georgepisaltu <george.pisaltu@parity.io>

Update prdoc

ae07a64

Signed-off-by: georgepisaltu <george.pisaltu@parity.io>

georgepisaltu added this pull request to the merge queue Oct 29, 2024

Merged via the queue into paritytech:master with commit cc4fe1e Oct 29, 2024
195 checks passed

georgepisaltu deleted the identity-username-refactor branch October 29, 2024 14:58

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

[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