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

pallet-collective | port to frame v2 #8193

Closed

Conversation

shamb0
Copy link
Contributor

@shamb0 shamb0 commented Feb 24, 2021

pallet-collective | port to frame v2

related #7882

⚠️ 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: 
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 (moreover for instantiable pallet). 
If pallet is published, make sure to warn about this breaking change.

So users of the collective 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 collective pallet.

@shamb0 shamb0 marked this pull request as ready for review February 24, 2021 11:49
@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. labels Feb 24, 2021
frame/collective/src/lib.rs Outdated Show resolved Hide resolved
frame/collective/src/lib.rs Show resolved Hide resolved
shamb0 and others added 2 commits February 24, 2021 19:49
Co-authored-by: Guillaume Thiolliere <[email protected]>
Co-authored-by: Guillaume Thiolliere <[email protected]>
@gui1117
Copy link
Contributor

gui1117 commented Mar 4, 2021

so if master merged and a runtime migration either in the pallet or in a polkadot companion at the runtime level and it should be good

@shamb0
Copy link
Contributor Author

shamb0 commented Mar 7, 2021

so if master merged and a runtime migration either in the pallet or in a polkadot companion at the runtime level and it should be good

Thanks @thiolliere , will pick up for this week

@gnunicorn
Copy link
Contributor

anyone working on this?

@gui1117
Copy link
Contributor

gui1117 commented Mar 29, 2021

it needs a migration code in polkadot or here. Because the old pallet prefix is different from the pallet name used in polkadot and kusama.

@shamb0
Copy link
Contributor Author

shamb0 commented Mar 29, 2021

Hello @thiolliere, will pick up run time migration part immediately.

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.

migration looks good to me, name must be changed, and genesis build is wrong

A test would be good, something like:

  • apply migration
  • ensure no keys are remaining at the old prefix, so we haven't miss any storages.

frame/collective/src/lib.rs Outdated Show resolved Hide resolved
frame/collective/src/lib.rs Outdated Show resolved Hide resolved
frame/collective/src/lib.rs Outdated Show resolved Hide resolved
frame/collective/src/lib.rs Outdated Show resolved Hide resolved
@gui1117 gui1117 added D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed. and removed Z3-stale labels Mar 29, 2021
@gui1117 gui1117 requested a review from ascjones March 29, 2021 15:21
@gui1117
Copy link
Contributor

gui1117 commented Mar 30, 2021

you need to merge master because rust version in CI has changed.

But last changes are good to me

frame/system/src/lib.rs Outdated Show resolved Hide resolved
frame/system/src/lib.rs Outdated Show resolved Hide resolved
@gui1117
Copy link
Contributor

gui1117 commented Apr 14, 2021

can you merge master ? Once this is done I'll take a second look and it needs another approval maybe @ascjones or somebody else. Then it is good to go.

@shamb0
Copy link
Contributor Author

shamb0 commented Apr 15, 2021

Hello @thiolliere, merge to upstream master Done 👍

frame/collective/src/lib.rs Outdated Show resolved Hide resolved
frame/collective/src/lib.rs Show resolved Hide resolved
frame/collective/src/lib.rs Show resolved Hide resolved
@shawntabrizi
Copy link
Member

@shamb0 looks like collective got updated since this was done.

Care to update this for latest master?

If so, feel free to reopen

@gui1117
Copy link
Contributor

gui1117 commented May 3, 2021

yes need master merge, and a second review

@louismerlin louismerlin removed the D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed. label Oct 13, 2022
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants