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

Update Identity pallet to Frame V2 #8697

Merged
10 commits merged into from
May 4, 2021

Conversation

ferrell-code
Copy link
Contributor

Updated Identity pallet to Frame V2 🍣

relates #7882

@ferrell-code
Copy link
Contributor Author

⚠️ Breaking Change ⚠️

Following the upgrade guidelines here: https://crates.parity.io/frame_support/attr.pallet.html#upgrade-guidelines.

storages now use PalletInfo for module_prefix instead of the one given to decl_storage: Thus any use of this pallet in construct_runtime! should be careful to update name in order not to break storage or to upgrade storage. If pallet is published, make sure to warn about this breaking change.

So users of the Identity pallet must be careful about the name they used in construct_runtime!. Hence the runtime-migration label, which might not be needed depending on the configuration of the Identity pallet.

let sender = ensure_signed(origin)?;
let sub = T::Lookup::lookup(sub)?;
ensure!(IdentityOf::<T>::contains_key(&sender), Error::<T>::NoIdentity);
ensure!(SuperOf::<T>::get(&sub).map_or(false, |x| x.0 == sender), Error::<T>::NotOwned);
SuperOf::<T>::insert(&sub, (sender, data));
Ok(())
}

Copy link
Contributor Author

@ferrell-code ferrell-code Apr 29, 2021

Choose a reason for hiding this comment

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

Not sure why new macro wanted me to add dispatch.

Just wanted to highlight to make sure this is sensible and not incorrect

Copy link
Contributor

Choose a reason for hiding this comment

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

yes new macro doesn't accept to return nothing, we could enhance the macro though

Copy link
Member

Choose a reason for hiding this comment

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

no, lets make returns explicit. less macro magic :)

@ferrell-code
Copy link
Contributor Author

Kusama and Polkadot both have Identity in construct runtime! no need for migration. Rococo does no appear to use Identity which makes sense as it is a test net

@shawntabrizi shawntabrizi added this to In progress in Runtime via automation Apr 29, 2021
@ferrell-code
Copy link
Contributor Author

/benchmark runtime pallet pallet_identity

@parity-benchapp
Copy link

parity-benchapp bot commented May 1, 2021

Error running benchmark: pallet-identity-framev2

stdoutfatal: ambiguous argument 'origin/pallet-identity-framev2': unknown revision or path not in the working tree. Use '--' to separate paths from revisions, like this: 'git [...] -- [...]'

@shawntabrizi
Copy link
Member

@ferrell-code not compiling?

@shawntabrizi shawntabrizi added B0-silent Changes should not be mentioned in any release notes B7-runtimenoteworthy C1-low PR touches the given topic and has a low impact on builders. A0-please_review Pull request needs code review. D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed. labels May 3, 2021
@ferrell-code
Copy link
Contributor Author

oops my bad forgot to push, should be good now :)

@ferrell-code
Copy link
Contributor Author

/benchmark runtime pallet pallet_identity

@parity-benchapp
Copy link

parity-benchapp bot commented May 3, 2021

Error running benchmark: pallet-identity-framev2

stdoutfatal: ambiguous argument 'origin/pallet-identity-framev2': unknown revision or path not in the working tree. Use '--' to separate paths from revisions, like this: 'git [...] -- [...]'

Copy link
Contributor

@thiolliere thiolliere left a comment

Choose a reason for hiding this comment

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

looks good to me

frame/identity/src/benchmarking.rs Outdated Show resolved Hide resolved
let sender = ensure_signed(origin)?;
let sub = T::Lookup::lookup(sub)?;
ensure!(IdentityOf::<T>::contains_key(&sender), Error::<T>::NoIdentity);
ensure!(SuperOf::<T>::get(&sub).map_or(false, |x| x.0 == sender), Error::<T>::NotOwned);
SuperOf::<T>::insert(&sub, (sender, data));
Ok(())
}

Copy link
Contributor

Choose a reason for hiding this comment

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

yes new macro doesn't accept to return nothing, we could enhance the macro though

frame/identity/src/lib.rs Show resolved Hide resolved
@thiolliere thiolliere added B3-apinoteworthy and removed B0-silent Changes should not be mentioned in any release notes B7-runtimenoteworthy labels May 3, 2021
ferrell-code and others added 2 commits May 3, 2021 13:46
Co-authored-by: Guillaume Thiolliere <gui.thiolliere@gmail.com>
Runtime automation moved this from In progress to Needs Audit May 3, 2021
@thiolliere
Copy link
Contributor

bot merge

@ghost
Copy link

ghost commented May 4, 2021

Trying merge.

@ghost ghost merged commit 4993b70 into paritytech:master May 4, 2021
Runtime automation moved this from Needs Audit to Done May 4, 2021
@ferrell-code ferrell-code deleted the pallet-identity-framev2 branch May 5, 2021 17:16
@shawntabrizi shawntabrizi moved this from Done to Archive in Runtime May 27, 2021
nazar-pc pushed a commit to subspace/substrate that referenced this pull request Aug 8, 2021
* bump pallet to frame v2

* line width

* get benchmarking ot compile

* fix benchmarking now

* should actually fix benchmark

* make docs prettier

* add dependency

* add metadata

* Update frame/identity/src/benchmarking.rs

Co-authored-by: Guillaume Thiolliere <gui.thiolliere@gmail.com>

Co-authored-by: Shawn Tabrizi <shawntabrizi@gmail.com>
Co-authored-by: Guillaume Thiolliere <gui.thiolliere@gmail.com>
This pull request was closed.
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. C1-low PR touches the given topic and has a low impact on builders. D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants