Make `IdentityInfo` generic in `pallet-identity` #1661

georgepisaltu · 2023-09-21T11:03:15Z

Fixes #179

Description

This PR makes the structure containing identity information used in pallet-identity generic through the pallet Config. Additionally, the old structure is now available in a separate module called simple (pending rename) and is compatible with the new interface.

Another change in this PR is that while the additional field in IdentityInfo stays for backwards compatibility reasons, the associated costs are stil present in the pallet through the additional function in the IdentityInformationProvider interface. This function is marked as deprecated as it is only a temporary solution to the backwards compatibility problem we had. In short, we could have removed the additional fields in the struct and done a migration, but we chose to wait and do it off-chain through the genesis of the system parachain. After we move the identity pallet to the parachain, additional fields will be migrated into the existing fields and the additional key-value store will be removed. Until that happens, this interface will provide the necessary information to properly account for the associated costs.

Additionally, this PR fixes an unrelated issue; the IdentityField enum used to represent the fields as bitflags couldn't store more than 8 fields, even though it was marked as #[repr(u64)]. This was because of the derive implementation of TypeInfo, which assumed u8 semantics. The custom implementation of this trait in 0105cc0 fixes the issue.

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

georgepisaltu · 2023-09-27T14:30:38Z

Currently the changes in this PR keep the additional fields in the IdentityInfo struct, so no migration would be needed. However, the purpose of this PR was to make the identity information generic and, because additional fields in a Vec inside the IdentityInfo struct is an implementation detail, the costs in terms of deposit and weight associated with these additional fields are no longer accounted for in the extrinsics of this pallet. Below are the options on how to deal with the additional fields problem:

Add a function in the IdentityInformationProvider trait to somehow query for added costs besides some agreed upon standard cost. It could be bytes stored, number of "unnecessary" entries or something similar. Convert the output of that function to an amount reserved for the storage used and some extra weight for processing.
Use one dedicated field for a hash/image. Any information that is not considered useful by the system but is still wanted by users is currently stored in this additional field. While a dedicated field for this "extra" information verifiably stores a "summary", the actual info is stored somewhere else, off-chain maybe.
Remove it altogether as the purpose of the pallet is to judge information publicly. It's arguable non-standardized, custom fields and their judgements:
a. do not have the same relevance as the standardized ones
b. are more prone to typos and errors
c. are more difficult to interact with from a UX perspective

Options 2 and 3 will require migrations of the IdentityOf map while option 1 would not require any. However, with a transition of the identity pallet to a system parachain we can do the migration at genesis and we would not need to worry about the additional fields costs not being accounted for if we freeze any calls to the pallet so that users can't change their identities anymore (which would be required in the build up of the move). IdentityFields itself doesn't require a migration since it's just a BitFlag of a u64 and as long as we don't remove/change existing fields in IdentityField, it's backwards compatible. If we're looking for the best time to get rid of additional fields in the identity information, the genesis of the system chain would be it.

In order to safely merge this PR without accounting for the additional fields costs, we must merge the freeze of the calls to the pallet extrinsics before the next release or remove them altogether.

My personal choice is option 3 because it leaves the door open to add new fields in the future (such as the hash/image field described in option 2) in a backwards compatible way.

sam0x17 · 2023-10-05T16:19:24Z

Just a note @georgepisaltu I was planning to migrate the identity pallet as per #226 but just noticed your PR so happy to make a PR off of your branch here or work on a different one instead if you'd prefer

georgepisaltu · 2023-10-05T16:34:31Z

Just a note @georgepisaltu I was planning to migrate the identity pallet as per #226 but just noticed your PR so happy to make a PR off of your branch here or work on a different one instead if you'd prefer

Work on this branch, the changes are kind of related. I'll rebase the branch and resolve the conflicts as soon as possible.

kianenigma · 2023-10-07T15:09:37Z

Just a note @georgepisaltu I was planning to migrate the identity pallet as per #226 but just noticed your PR so happy to make a PR off of your branch here or work on a different one instead if you'd prefer

From just reading the description, the changes that you are making are pretty orthogonal to making the identity type generic so it should be save. Feel free to open a draft PR regardless of the target branch when you have something :)

sam0x17 · 2023-10-09T13:48:27Z

@georgepisaltu I'm happy to try to rebase if you're busy with other stuff

georgepisaltu · 2023-10-09T13:56:31Z

@georgepisaltu I'm happy to try to rebase if you're busy with other stuff

I pushed the merge, I'll have to move the polkadot/kusama pallet configuration and weights to a different PR in the fellowship I think. The rest of it should be pretty much the same. Let me know if it helps.

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

substrate/frame/identity/src/simple.rs

jsidorenko · 2023-10-11T08:42:53Z

substrate/frame/identity/src/types.rs

+	}
+}
+
+impl<IdField: U64BitFlag> MaxEncodedLen for IdentityFields<IdField>


You can remove the code below and use the impl_codec_bitflags macro from there
P. S. Previously we had a different enumflags2 version for the identity pallet and nfts, but since we synced the versions now, we could extract that macro into a common folder and re-use the code

Unfortunately it doesn't work because of the generics, but I don't think it's too ugly either way.

hmm.. is it possible to avoid that generic? I think it could work without it, but maybe I'm missing smth

ggwpez · 2023-10-12T10:34:34Z

Summing up a recent discussion:

Adding the additional_fields method either as "deprecated" or "experimental" as intermediate solution.
Migrating a few fields like "github" or "discord" over once we have the proper fields and deleting all the other fields.
Removing the additional_fields after the transition period.

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

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

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

juangirini · 2023-12-26T19:47:04Z

Just a note @georgepisaltu I was planning to migrate the identity pallet as per #226 but just noticed your PR so happy to make a PR off of your branch here or work on a different one instead if you'd prefer

@sam0x17 have you started working on it? If so are you planning to continue working on it or it is something I can take over?

bkchr · 2023-12-26T22:27:37Z

@juangirini you can do this.

Introduced in paritytech/polkadot-sdk#1661

* Get `entropy-shared` compiling with updated deps * Fix `entropy-shared` build for `wasm-no-std` * Make `pallet-staking-extension` compile Starts introducing some of the changes brought in by paritytech/polkadot-sdk#1484. * Introduce staking changes from paritytech/substrate#12970 * Add RuntimeFreezeReason from paritytech/polkadot-sdk#1900 * Re-map errors in staking extension pallet * Bump `pallet-programs` * Missed a crate dep for staking extension * Remove commented out code * Get `pallet-relayer` compiling * Fix relayer test compilation * Fix `pallet-free-tx`'s tests * Reorder config types * Bump `pallet-propagation` dependencies * Bump `pallet-slashing` dependencies * Bump `pallet-transaction-pause` This includes the deprecation of `Balances::transfer` from paritytech/polkadot-sdk#1226. * TaploFmt * Use `StakingAccount` instead of `.into()` * Import `vec` macro in `pallet-slashing` * Import `vec` in other pallets * Bump TOML dependencies for runtime Haven't fixed the Rust code yet though * Self define types instead of using `node-primitives` * Update staking related configs Values were mostly taken from the Polkadot runtime config * Update `pallet-preimage` config Changes introduced in paritytech/polkadot-sdk#1363 * Add `MaxNominators` to `pallet_babe` Introduced in paritytech/substrate#14471 * Add `MaxNominators` to `pallet_grandpa` Also introduced in paritytech/substrate#14471 * Add `RuntimeFreezeReason` to `pallet_nomination_pools` * Add `MaxTipAmount` to `pallet_tips` Introduced in paritytech/polkadot-sdk#1709 * Add `IdentityInformation` to `pallet_identity` Introduced in paritytech/polkadot-sdk#1661 * finish runtime * Use `crates.io` packages in `entropy` TOML This doesn't compile yet * Remove reference to `node_primitives` * Move imports under correct dependency section These shouldn't be built-deps, my bad * Change `WarpSyncParams` import to come from `sc_network_sync` Change introduced here: paritytech/polkadot-sdk#1912 * Add missing `sc-consensus-babe` package * Remove second generic from `DefaultImportQueue` This was removed in paritytech/substrate#14612 * Add temporary cursed RPC bounds * Switch `sp-consensus-grandpa` to be from Crates * Add Grandpa justification period to service * Rename `justification_period to` `justification_generation_period` * Add missing `block_relay` param Introduced in paritytech/polkadot-sdk#1524 * Try to clean up runtime types * Add `opaque` types to runtime These match what was in `node-primitives`. In particular we're interested in the `Block` type which we were using in a lot of places, and I incorrectly changed to be `Unchechecked` instead of `Opaque`. * Use opaque block type in service * Add full session key impls * Remove cursed trait bounds for RPC * Convert some tabs to spaces Sorry, I had to * Import `Vec` from `sp_std` in benchmarks Turns out the issue responsible for this change is this: paritytech/polkadot-sdk#172 * Pull `TrackedStorageKey` from `sp_storage` From here: paritytech/substrate#14787 * Add `BenchmarkHelper` to treasury pallet * TaploFmt * change max freezes * Sort TOML imports in runtime manifest * Move `version` field to be first in runtime manifest * Organize imports in pallet manifests * Sort and organize imports for client * Address TODOs in `entropy-shared` * Remove TODOs related to `node-primitives` * Pull `substrate-wasm-builder` from crates.io * TaploFmt * Fix benchmarking import post-merge * Fix another program import port-merge * Import `Vec` to programs benches * Wrong vec import 🙃 * Poke CI * Bump node metadata --------- Co-authored-by: Jesse Abramowitz <jesse@entropy.xyz>

Fixes paritytech#179 # Description This PR makes the structure containing identity information used in `pallet-identity` generic through the pallet `Config`. Additionally, the old structure is now available in a separate module called `simple` (pending rename) and is compatible with the new interface. Another change in this PR is that while the `additional` field in `IdentityInfo` stays for backwards compatibility reasons, the associated costs are stil present in the pallet through the `additional` function in the `IdentityInformationProvider` interface. This function is marked as deprecated as it is only a temporary solution to the backwards compatibility problem we had. In short, we could have removed the additional fields in the struct and done a migration, but we chose to wait and do it off-chain through the genesis of the system parachain. After we move the identity pallet to the parachain, additional fields will be migrated into the existing fields and the `additional` key-value store will be removed. Until that happens, this interface will provide the necessary information to properly account for the associated costs. Additionally, this PR fixes an unrelated issue; the `IdentityField` enum used to represent the fields as bitflags couldn't store more than 8 fields, even though it was marked as `#[repr(u64)]`. This was because of the `derive` implementation of `TypeInfo`, which assumed `u8` semantics. The custom implementation of this trait in paritytech@0105cc0 fixes the issue. --------- Signed-off-by: georgepisaltu <george.pisaltu@parity.io> Co-authored-by: Sam Johnson <sam@durosoft.com> Co-authored-by: joe petrowski <25483142+joepetrowski@users.noreply.github.com>

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>

* Setup deps * Remove Koi from account migration test * paritytech/polkadot-sdk#1495 * Bump * paritytech/polkadot-sdk#1524 * !! paritytech/polkadot-sdk#1363 * paritytech/polkadot-sdk#1492 * paritytech/polkadot-sdk#1911 * paritytech/polkadot-sdk#1900 Signed-off-by: Xavier Lau <xavier@inv.cafe> * paritytech/polkadot-sdk#1661 * paritytech/polkadot-sdk#2144 * paritytech/polkadot-sdk#2048 * paritytech/polkadot-sdk#1672 * paritytech/polkadot-sdk#2303 * paritytech/polkadot-sdk#1256 * Remove identity and vesting * Fixes * paritytech/polkadot-sdk#2657 * paritytech/polkadot-sdk#1313 * paritytech/polkadot-sdk#2331 * paritytech/polkadot-sdk#2409 part.1 * paritytech/polkadot-sdk#2767 * paritytech/polkadot-sdk#2521 Signed-off-by: Xavier Lau <xavier@inv.cafe> * paritytech/polkadot-sdk#1222 * paritytech/polkadot-sdk#1234 part.1 * Satisfy compiler * XCM V4 part.1 * paritytech/polkadot-sdk#1246 * Remove pallet-democracy part.1 * paritytech/polkadot-sdk#2142 * paritytech/polkadot-sdk#2428 * paritytech/polkadot-sdk#3228 * XCM V4 part.2 * Bump * Build all runtimes * Build node * Remove pallet-democracy Signed-off-by: Xavier Lau <xavier@inv.cafe> * Format * Fix pallet tests * Fix precompile tests * Format * Fixes * Async, remove council, common pallet config * Fix `ethtx-forward` test case (#1519) * Fix ethtx-forward tests * Format * Fix following the review * Fixes * Fixes * Use default impl * Benchmark helper * Bench part.1 * Bench part.2 * Bench part.3 * Fix all tests * Typo * Feat * Fix EVM tracing build * Reuse upstream `proof_size_base_cost()` (#1521) * Format issue * Fixes * Fix CI --------- Signed-off-by: Xavier Lau <xavier@inv.cafe> Co-authored-by: Bear Wang <boundless.forest@outlook.com>

georgepisaltu added 7 commits September 21, 2023 13:43

Make identity info generic in pallet config

683e44b

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

Fix IdentityField representation

0105cc0

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

Fix UTs and benches

c79531d

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

Make pallet-alliance work with changes

10f071e

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

Adjust runtimes using identity to changes

cbd49b3

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

Move IdentityField to associated type

4f97f9d

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

Update documentation and prepare for migration

a1577ad

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

georgepisaltu requested review from a team September 21, 2023 11:03

georgepisaltu self-assigned this Sep 21, 2023

georgepisaltu added 2 commits September 26, 2023 15:56

Add additional_fields back to the impl

a51abc4

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

Remove unnecessary bounds on IdentityField

63dbc74

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

joepetrowski mentioned this pull request Oct 7, 2023

Identity Deposits Relay to Parachain Migration #1814

Merged

3 tasks

sam0x17 self-assigned this Oct 9, 2023

Merge remote-tracking branch 'origin' into george/identity-pallet-gen…

d303fb9

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

georgepisaltu force-pushed the george/identity-pallet-generic-3 branch from 4129f96 to d303fb9 Compare October 10, 2023 14:34

jsidorenko reviewed Oct 11, 2023

View reviewed changes

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

jsidorenko reviewed Oct 11, 2023

View reviewed changes

ggwpez mentioned this pull request Oct 11, 2023

Refactor Identity to benchmark v2 #1838

Merged

georgepisaltu added 3 commits October 16, 2023 18:09

Add additional field logic back into impl

8d3b22e

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

Merge remote-tracking branch 'origin' into george/identity-pallet-gen…

2104d2b

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

Simplify IdentityField impl

aa7d87d

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

georgepisaltu added the T2-pallets This PR/Issue is related to a particular pallet. label Oct 16, 2023

jasl mentioned this pull request Dec 25, 2023

Upgrade to polkadot-v1.5.0 Phala-Network/phala-blockchain#1494

Merged

dnjscksdn98 mentioned this pull request Jan 12, 2024

NODE-65, deps: polkadot sdk migration bifrost-platform/bifrost-node#36

Merged

13 tasks

HCastano added a commit to entropyxyz/entropy-core that referenced this pull request Jan 19, 2024

Add IdentityInformation to pallet_identity

86045a4

Introduced in paritytech/polkadot-sdk#1661

serban300 pushed a commit to serban300/polkadot-sdk that referenced this pull request Mar 26, 2024

Fixes fo ro/wo bridge (paritytech#1661)

80dfb39

serban300 pushed a commit to serban300/polkadot-sdk that referenced this pull request Mar 27, 2024

Fixes fo ro/wo bridge (paritytech#1661)

9924fad

Dinonard mentioned this pull request Mar 28, 2024

Feat/polkadot v1.3.0 uplift AstarNetwork/Astar#1207

Merged

serban300 pushed a commit to serban300/polkadot-sdk that referenced this pull request Apr 8, 2024

Fixes fo ro/wo bridge (paritytech#1661)

d2a7895

serban300 pushed a commit to serban300/polkadot-sdk that referenced this pull request Apr 8, 2024

Fixes fo ro/wo bridge (paritytech#1661)

a2f2c10

serban300 pushed a commit to serban300/polkadot-sdk that referenced this pull request Apr 8, 2024

Fixes fo ro/wo bridge (paritytech#1661)

031ce51

serban300 pushed a commit to serban300/polkadot-sdk that referenced this pull request Apr 8, 2024

Fixes fo ro/wo bridge (paritytech#1661)

2ad973a

serban300 pushed a commit to serban300/polkadot-sdk that referenced this pull request Apr 8, 2024

Fixes fo ro/wo bridge (paritytech#1661)

6a457a9

serban300 pushed a commit to serban300/polkadot-sdk that referenced this pull request Apr 9, 2024

Fixes fo ro/wo bridge (paritytech#1661)

1e87a97

serban300 pushed a commit to serban300/polkadot-sdk that referenced this pull request Apr 9, 2024

Fixes fo ro/wo bridge (paritytech#1661)

6506a18

serban300 pushed a commit to serban300/polkadot-sdk that referenced this pull request Apr 9, 2024

Fixes fo ro/wo bridge (paritytech#1661)

6dadf1e

serban300 pushed a commit to serban300/polkadot-sdk that referenced this pull request Apr 9, 2024

Fixes fo ro/wo bridge (paritytech#1661)

4404eed

serban300 pushed a commit to serban300/polkadot-sdk that referenced this pull request Apr 9, 2024

Fixes fo ro/wo bridge (paritytech#1661)

a77abc1

serban300 pushed a commit to serban300/polkadot-sdk that referenced this pull request Apr 9, 2024

Fixes fo ro/wo bridge (paritytech#1661)

d8ead38

serban300 pushed a commit to serban300/polkadot-sdk that referenced this pull request Apr 10, 2024

Fixes fo ro/wo bridge (paritytech#1661)

0a99ef8

serban300 pushed a commit to serban300/polkadot-sdk that referenced this pull request Apr 10, 2024

Fixes fo ro/wo bridge (paritytech#1661)

c7d0b8c

bkchr pushed a commit that referenced this pull request Apr 10, 2024

Fixes fo ro/wo bridge (#1661)

786db04

ukint-vs mentioned this pull request Apr 25, 2024

feat: Update substrate to v1.3.0 gear-tech/gear#3921

Merged

This was referenced Jun 5, 2024

Update polkadot-sdk from v1.7.0 to v1.11.0 moondance-labs/tanssi#573

Closed

Update polkadot-sdk from v1.10.0 to v1.11.0 moondance-labs/tanssi#577

Closed

AurevoirXavier added a commit to darwinia-network/darwinia that referenced this pull request Jun 21, 2024

paritytech/polkadot-sdk#1661

a7b8a9a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make `IdentityInfo` generic in `pallet-identity` #1661

Make `IdentityInfo` generic in `pallet-identity` #1661

georgepisaltu commented Sep 21, 2023 •

edited

Loading

georgepisaltu commented Sep 27, 2023

sam0x17 commented Oct 5, 2023

georgepisaltu commented Oct 5, 2023

kianenigma commented Oct 7, 2023

sam0x17 commented Oct 9, 2023

georgepisaltu commented Oct 9, 2023

jsidorenko Oct 11, 2023

georgepisaltu Oct 16, 2023

jsidorenko Oct 17, 2023

ggwpez commented Oct 12, 2023

juangirini commented Dec 26, 2023

bkchr commented Dec 26, 2023

Make IdentityInfo generic in pallet-identity #1661

Make IdentityInfo generic in pallet-identity #1661

Conversation

georgepisaltu commented Sep 21, 2023 • edited Loading

Description

georgepisaltu commented Sep 27, 2023

sam0x17 commented Oct 5, 2023

georgepisaltu commented Oct 5, 2023

kianenigma commented Oct 7, 2023

sam0x17 commented Oct 9, 2023

georgepisaltu commented Oct 9, 2023

jsidorenko Oct 11, 2023

Choose a reason for hiding this comment

georgepisaltu Oct 16, 2023

Choose a reason for hiding this comment

jsidorenko Oct 17, 2023

Choose a reason for hiding this comment

ggwpez commented Oct 12, 2023

juangirini commented Dec 26, 2023

bkchr commented Dec 26, 2023

Make `IdentityInfo` generic in `pallet-identity` #1661

Make `IdentityInfo` generic in `pallet-identity` #1661

georgepisaltu commented Sep 21, 2023 •

edited

Loading