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

pallet-treasury | port to frame v2#8196

Closed
shamb0 wants to merge 19 commits intoparitytech:masterfrom
shamb0:shamb0-br2-pallet-treasury-port-frame-v2
Closed

pallet-treasury | port to frame v2#8196
shamb0 wants to merge 19 commits intoparitytech:masterfrom
shamb0:shamb0-br2-pallet-treasury-port-frame-v2

Conversation

@shamb0
Copy link
Contributor

@shamb0 shamb0 commented Feb 24, 2021 •
edited
Loading

pallet-treasury | port to frame v2

related #7882

polkadot companion: paritytech/polkadot#2730

⚠️ 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 treasury 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 treasury pallet.

@shamb0 shamb0 mentioned this pull request Feb 24, 2021
Closed
@shamb0 shamb0 marked this pull request as ready for review February 24, 2021 14:02
@gui1117 gui1117 mentioned this pull request Feb 24, 2021
Merged
@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 25, 2021
@gui1117
Copy link
Contributor

gui1117 commented Feb 25, 2021

some tests in bounties needs to be fix it seems, because treasury no longer export DefaultInstance.
Instead bounties should use the default instance () or just removing the instance generic

@shamb0 shamb0 mentioned this pull request Feb 25, 2021
Closed
@gui1117
Copy link
Contributor

gui1117 commented Mar 4, 2021

would be good to make use of frame_support::storage::migration::move_storage_from_pallet in a migration function to move all storages, and then call this function either here in on_runtime_upgrade or in a polkadot companion at the runtime level

@shamb0
Copy link
Contributor Author

shamb0 commented Mar 7, 2021

would be good to make use of frame_support::storage::migration::move_storage_from_pallet in a migration function to move all storages, and then call this function either here in on_runtime_upgrade or in a polkadot companion at the runtime level

Thanks @thiolliere , will pick up for this week

gui1117
gui1117 reviewed Mar 25, 2021
View reviewed changes
Copy link
Contributor

@gui1117 gui1117 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

you need to compare the metadata to catch the miss like the constant.
Also master needs to be merged.

But otherwise looks good to me

@gui1117 gui1117 added the D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit label Mar 25, 2021
shamb0 and others added 3 commits March 26, 2021 16:23
@shamb0 @gui1117
Update frame/treasury/src/lib.rs
7a1743a 
Co-authored-by: Guillaume Thiolliere <gui.thiolliere@gmail.com>
@shamb0 @gui1117
Update frame/treasury/src/lib.rs
751a853 
Co-authored-by: Guillaume Thiolliere <gui.thiolliere@gmail.com>
@shamb0 @gui1117
Update frame/treasury/src/lib.rs
45f4412 
Co-authored-by: Guillaume Thiolliere <gui.thiolliere@gmail.com>
shamb0 and others added 7 commits March 26, 2021 16:24
@shamb0 @gui1117
Update frame/treasury/src/lib.rs
5b81c70 
Co-authored-by: Guillaume Thiolliere <gui.thiolliere@gmail.com>
@shamb0 @gui1117
Update frame/treasury/src/lib.rs
bf97e3c 
Co-authored-by: Guillaume Thiolliere <gui.thiolliere@gmail.com>
@shamb0 @gui1117
Update frame/treasury/src/lib.rs
4201465 
Co-authored-by: Guillaume Thiolliere <gui.thiolliere@gmail.com>
@shamb0 @gui1117
Update frame/treasury/src/lib.rs
73b2d77 
Co-authored-by: Guillaume Thiolliere <gui.thiolliere@gmail.com>
@shamb0 @gui1117
Update frame/treasury/src/lib.rs
d445e17 
Co-authored-by: Guillaume Thiolliere <gui.thiolliere@gmail.com>
@shamb0
frame v2 port upstream merge
167658a
@shamb0
Merge branch 'shamb0-br2-pallet-treasury-port-frame-v2' of github.com…
4e9b38a 
…:shamb0/substrate into shamb0-br2-pallet-treasury-port-frame-v2

review updates
@shamb0
Copy link
Contributor Author

shamb0 commented Mar 26, 2021

Thanks @thiolliere, pulled in review suggested comments & merged to master 👍

@gui1117
Copy link
Contributor

gui1117 commented Mar 26, 2021

CI is failing https://gitlab.parity.io/parity/substrate/-/jobs/868957

@gui1117
Copy link
Contributor

gui1117 commented Mar 26, 2021

CI is failing https://gitlab.parity.io/parity/substrate/-/jobs/868957

@shamb0 shamb0 mentioned this pull request Mar 26, 2021
Closed
@shamb0
Copy link
Contributor Author

shamb0 commented Mar 27, 2021

Hello @thiolliere, extremely, sorry. Overlooked at CI builds & dependency. today will have a look on "continuous-integration/gitlab-check-polkadot-companion-build"

@shamb0 shamb0 mentioned this pull request Mar 27, 2021
Closed
@shamb0
Merge remote-tracking branch 'upstream/master' into shamb0-br2-pallet…
e522479 
…-treasury-port-frame-v2

merge to upstream master
gui1117
gui1117 approved these changes Mar 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.

looks good to me, some indentation are wrong though

shamb0 and others added 2 commits March 27, 2021 15:28
@shamb0 @gui1117
Update frame/treasury/src/lib.rs
31a1cf8 
Co-authored-by: Guillaume Thiolliere <gui.thiolliere@gmail.com>
@shamb0
frame v2 port fixing review remarks & indent correction
c526643
@gui1117
Copy link
Contributor

gui1117 commented Mar 29, 2021

some deprecated usage of RawEvent must be fixed: https://gitlab.parity.io/parity/substrate/-/jobs/870164

@gui1117 gui1117 requested a review from ascjones March 29, 2021 08:55
@shamb0
Copy link
Contributor Author

shamb0 commented Mar 29, 2021

Fixed "some deprecated usage of RawEvent"

@shawntabrizi
Copy link
Member

shawntabrizi commented Mar 29, 2021

@thiolliere @shamb0 you must be careful here.

Parts of treasury, the Tips and Bounties sub-pallets use an old Storage Key, which would mean some kind of migration is needed. Maybe not relevant to this exact PR, but def something to watch out for.

@gui1117
Copy link
Contributor

gui1117 commented Mar 29, 2021

yes tips and bounty pallets will need migration.
For treasury both old pallet and new pallet use the prefix "Treasury" in kusama and polkadot.

@shawntabrizi
Copy link
Member

shawntabrizi commented May 3, 2021

@shamb0 this also went stale

Feel free to reopen if you update to master

@gui1117 gui1117 reopened this May 3, 2021
@gui1117
Copy link
Contributor

gui1117 commented May 3, 2021

needs to merge master, and awaiting for another review.

I should also review the merge of master once done.

@gui1117 gui1117 closed this May 3, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@ascjones ascjones Awaiting requested review from ascjones

1 more reviewer

@gui1117 gui1117 gui1117 approved these changes

Reviewers whose approvals may not affect merge requirements

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. D2-breaksapi D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@shamb0 @gui1117 @shawntabrizi @paritytech-ci