Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Add GenesisConfig to identity pallet #14774

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

grw-ms
Copy link

@grw-ms grw-ms commented Aug 15, 2023

Description

This change adds a GenesisConfig to the identity pallet, allowing downstream users to automatically seed Identities via chainspec config.

Checklist

  • My PR includes a detailed description as outlined in the "Description" section above
  • My PR follows the labeling requirements of this project (at minimum one label for each A, B, C and D required)
  • I have made corresponding changes to the documentation (if applicable)
  • I have added tests that prove my fix is effective or that my feature works (if applicable)
  • If this PR alters any external APIs or interfaces used by Polkadot, the corresponding Polkadot PR is ready as well as the corresponding Cumulus PR (optional)

@cla-bot-2021
Copy link

cla-bot-2021 bot commented Aug 15, 2023

User @grw-ms, please sign the CLA here.

@grw-ms grw-ms marked this pull request as ready for review August 15, 2023 15:08
@grw-ms grw-ms requested review from a team August 15, 2023 15:08
Copy link
Member

@liamaharon liamaharon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the pr.

On the right track but needs some changes and tests.

@@ -201,6 +202,44 @@ pub mod pallet {
ValueQuery,
>;

#[pallet::genesis_config]
pub struct GenesisConfig<T: Config> {
pub identities: Vec<(T::AccountId, String)>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you want this

Suggested change
pub identities: Vec<(T::AccountId, String)>,
pub identities: Vec<(T::AccountId, IdentityInfo)>,

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume this was done to not require trait bounds on the IdentityInfo type?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even so, it seems much more preferable to be able to specify all other identity-related fields in genesis config, so let's figure out what the trait bounds should be here...

Oh, it's just T::MaxAdditionalFields, so we should just use that:

Suggested change
pub identities: Vec<(T::AccountId, String)>,
pub identities: Vec<(T::AccountId, IdentityInfo<T::MaxAdditionalFields>)>,

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for comments both, I didn't use IdentityInfo initially as it wanted Deserialize<'_>, I agree it is preferable to support this though, I will try to add serde::[Des|S]erialize on these types

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is what i meant with trait bounds. Normally we just use POD types here to not having to use Serde from within the runtime.
I think its fine.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A Vec<u8> would be a bit less biased than a String - otherwise the encoding is not fixed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll probably need BoundedVec then, no reason to not make this derive MEL.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thx, uses BoundedVec now

frame/identity/src/lib.rs Outdated Show resolved Hide resolved
@@ -52,6 +52,9 @@ pub fn config_endowed(code: Option<&[u8]>, extra_endowed: Vec<AccountId>) -> Run
code: code.map(|x| x.to_vec()).unwrap_or_else(|| wasm_binary_unwrap().to_vec()),
..Default::default()
},
identity: IdentityConfig {
identities: vec![(alice(), "Alice".to_string()), (bob(), "Bob".to_string())],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Be mindful that Data in IdentityInfo is truncated to 32 bytes (see docs), so it is recommended to pass a hash of the data rather than the data itself for stuff like display.

@paritytech-ci paritytech-ci requested a review from a team August 15, 2023 16:17
@liamaharon liamaharon added A0-please_review Pull request needs code review. C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit B1-note_worthy Changes should be noted in the release notes labels Aug 15, 2023
@grw-ms grw-ms force-pushed the grw-add-identity-genesisconfig branch from 035b752 to 232fdf0 Compare August 16, 2023 15:12
@paritytech-cicd-pr
Copy link

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: cargo-check-benches
Logs: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/3406456

@paritytech-cicd-pr
Copy link

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: cargo-check-each-crate-macos
Logs: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/3406473

web: Data::None,
additional: BoundedVec::default(),
},
judgements: BoundedVec::default(),
Copy link
Member

@ggwpez ggwpez Aug 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How much sense does it make not to provide any judgments?
Having no judgement is as good as having no identity at all.
But not sure if its possible without also adding a genesis judge…

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since you suggested it, we are now depending upon you to implement a genesis identity judge :)

@paritytech-ci paritytech-ci requested a review from a team August 17, 2023 09:46
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. B1-note_worthy Changes should be noted in the release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants