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

Upgrade authorship pallet to Frame-v2 #8663

Merged
10 commits merged into from
May 3, 2021
Merged

Upgrade authorship pallet to Frame-v2 #8663

10 commits merged into from
May 3, 2021

Conversation

ferrell-code
Copy link
Contributor

@ferrell-code ferrell-code commented Apr 24, 2021 •
edited
Loading

bumped authorship pallet to framev2 :)

This PR needs a polkadot companion pr to add Authorship to certain mocks construct-runtime!.

The pallet needs to be part of the pallets in construct_runtime to work properly, thus tests might need to be changed.

polkadot companion: paritytech/polkadot#2945

@ferrell-code
Copy link
Contributor Author

relates #7882

@ferrell-code
Copy link
Contributor Author

⚠️ Breaking Change ⚠️

From https://crates.parity.io/frame_support/attr.pallet.html#checking-upgrade-guidelines

storages now use PalletInfo for module_prefix instead of the one given to decl_storage: use of this pallet in construct_runtime! needs careful updating of the name in order to not break storage or to upgrade storage (moreover for instantiable pallet). If pallet is published, make sure to warn about this breaking change.

Hence, users of the Authorship pallet must check name used in construct_runtime!.

name of the pallet in kusama, polkadot and westend are Authorship

@ferrell-code
Copy link
Contributor Author

I believe it needs a polkadot companion pr to add Authorship to certain mocks construct-runtime!

xlc
xlc reviewed Apr 27, 2021
View reviewed changes
frame/authorship/src/lib.rs Outdated Show resolved Hide resolved
@gui1117 @xlc
Fix upgrade of storage.
4cc2fae 
Co-authored-by: Xiliang Chen <xlchen1291@gmail.com>
gui1117
gui1117 approved these changes Apr 27, 2021
View reviewed changes
Copy link
Contributor

@gui1117 gui1117 left a comment

Choose a reason for hiding this comment

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

can you put this message #8663 (comment) in the top message

And also add that the pallet needs to be part of the pallets in construct_runtime to work properly, thus tests might need to be changed.

Otherwise it looks good to me, but it needs a companion PR on polkadot

frame/authorship/src/lib.rs
@@ -194,50 +173,86 @@ decl_module! {
Self::prune_old_uncles(minimum_height)
}

<Self as Store>::DidSetUncles::put(false);
<DidSetUncles<T>>::put(false);
Copy link
Contributor

Choose a reason for hiding this comment

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

We can still use the trait Store, but I'm fine with the change too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah I realized that after I already changed them ¯_(ツ)_/¯

Copy link
Member

Choose a reason for hiding this comment

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

In general I prefer the syntax like DidSetUncles::<T>::put(false), but its all the same.

@gui1117 gui1117 added A0-please_review Pull request needs code review. B3-apinoteworthy 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. labels Apr 27, 2021
@ferrell-code ferrell-code mentioned this pull request Apr 27, 2021
Merged
kianenigma
kianenigma reviewed Apr 28, 2021
View reviewed changes
frame/babe/src/mock.rs
@@ -52,6 +52,7 @@ frame_support::construct_runtime!(
UncheckedExtrinsic = UncheckedExtrinsic,
{
System: frame_system::{Pallet, Call, Config, Storage, Event<T>},
Authorship: pallet_authorship::{Pallet, Call, Storage, Inherent},
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not used as far as I can see. If so, You should remove the impl block rather than adding it here.

Copy link
Contributor

Choose a reason for hiding this comment

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

pallet authorship is used in the equivocation handler (equivocation handler used in the mock requires pallet authorship)

kianenigma
kianenigma reviewed Apr 28, 2021
View reviewed changes
frame/grandpa/src/mock.rs
@@ -52,6 +52,7 @@ frame_support::construct_runtime!(
UncheckedExtrinsic = UncheckedExtrinsic,
{
System: frame_system::{Pallet, Call, Config, Storage, Event<T>},
Authorship: pallet_authorship::{Pallet, Call, Storage, Inherent},
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

Copy link
Contributor

Choose a reason for hiding this comment

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

same used for equivocation handler

kianenigma
kianenigma reviewed Apr 28, 2021
View reviewed changes
frame/staking/src/mock.rs
@@ -96,6 +96,7 @@ frame_support::construct_runtime!(
UncheckedExtrinsic = UncheckedExtrinsic,
{
System: frame_system::{Pallet, Call, Config, Storage, Event<T>},
Authorship: pallet_authorship::{Pallet, Call, Storage, Inherent},
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto

Copy link
Contributor

Choose a reason for hiding this comment

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

it is used in tests to note uncles and check the reward points I think

kianenigma
kianenigma approved these changes Apr 28, 2021
View reviewed changes
Copy link
Contributor

@kianenigma kianenigma left a comment

Choose a reason for hiding this comment

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

And also add that the pallet needs to be part of the pallets in construct_runtime to work properly, thus tests might need to be changed.

I'm a bit confused about this, but I suppose it is my mistake. LGTM otherwise.

@gui1117
Copy link
Contributor

gui1117 commented Apr 28, 2021

And also add that the pallet needs to be part of the pallets in construct_runtime to work properly, thus tests might need to be changed.

I'm a bit confused about this, but I suppose it is my mistake. LGTM otherwise.

pallet which are implemented for a runtime needs to be part of construct_runtime in order to have the pallet info accessible.
Currently it is not enforced by rust code. Maybe we should think about a way to make rust compilation fail when it is not the case.

@paritytech paritytech deleted a comment Apr 28, 2021
@paritytech paritytech deleted a comment Apr 28, 2021
@shawntabrizi
Copy link
Member

bot merge

@ghost
Copy link

ghost commented May 3, 2021

Trying merge.

@ghost ghost merged commit c96e57d into paritytech:master May 3, 2021
@ferrell-code ferrell-code deleted the upgrade-authorship-framev2 branch May 5, 2021 17:17
nazar-pc pushed a commit to autonomys/substrate that referenced this pull request Aug 8, 2021
@ferrell-code @gui1117
@xlc @nazar-pc
Upgrade authorship pallet to Frame-v2 (paritytech#8663)
5d1adbd 
* first commit

* get to compile

* fix deprecated grandpa

* formatting

* module to pallet

* add authorship pallet to mocks

* Fix upgrade of storage.

Co-authored-by: Xiliang Chen <xlchen1291@gmail.com>

* trigger CI

* put back doc

Co-authored-by: Guillaume Thiolliere <gui.thiolliere@gmail.com>
Co-authored-by: Xiliang Chen <xlchen1291@gmail.com>
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@gui1117 gui1117 gui1117 approved these changes

@xlc xlc xlc left review comments

@kianenigma kianenigma kianenigma approved these changes

@shawntabrizi shawntabrizi shawntabrizi approved these changes

@andresilva andresilva Awaiting requested review from andresilva

Assignees
No one assigned
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@ferrell-code @gui1117 @shawntabrizi @xlc @kianenigma