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

migration(tips): unreserve deposits #14241

Merged
merged 19 commits into from
Jun 5, 2023
Merged

migration(tips): unreserve deposits #14241

merged 19 commits into from
Jun 5, 2023
+257 −0

Conversation

liamaharon
Copy link
Contributor

@liamaharon liamaharon commented May 26, 2023 •
edited
Loading

Partial paritytech/polkadot-sdk#485

Overview

Adds a migration that unreserves all deposits made by the tips pallet.

Details

tips contains a Tips storage item containing amounts reserved by open tips.

pre_upgrade records the reserves of all accounts with open tips prior to the migration, for use later in post_upgrade.

on_runtime_upgrade unreserves all balances locked up in tips.

post_upgrade checks that the reserved balances were reduced by the expected amount.

Questions for reviewers / todos:

  • I'm not sure how to calculate the weights in on_runtime_upgrade, would appreciate some guidance there.

@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 T1-runtime This PR/Issue is related to the topic “runtime”. labels May 26, 2023
@liamaharon liamaharon requested review from a team May 26, 2023 16:30
@kianenigma kianenigma mentioned this pull request May 26, 2023
Closed
12 tasks
@liamaharon liamaharon mentioned this pull request May 31, 2023
Merged
11 tasks
ggwpez
ggwpez approved these changes May 31, 2023
View reviewed changes
Copy link
Member

@ggwpez ggwpez left a comment

Choose a reason for hiding this comment

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

Nice!

frame/tips/src/migrations/unreserve_all_funds.rs Outdated Show resolved Hide resolved
frame/tips/src/migrations/unreserve_all_funds.rs Outdated Show resolved Hide resolved
frame/tips/src/migrations/unreserve_all_funds.rs Outdated Show resolved Hide resolved
frame/tips/src/migrations/unreserve_all_funds.rs Outdated Show resolved Hide resolved
frame/tips/src/migrations/unreserve_all_funds.rs Outdated Show resolved Hide resolved
muharem
muharem approved these changes Jun 2, 2023
View reviewed changes
frame/tips/src/migrations/unreserve_all_funds.rs Outdated Show resolved Hide resolved
frame/tips/src/migrations/mod.rs Outdated Show resolved Hide resolved
frame/tips/src/migrations/unreserve_all_funds.rs Outdated Show resolved Hide resolved
@muharem
Copy link
Contributor

muharem commented Jun 2, 2023

I wonder now if these migrations really needs to be part of the pallets.
The pros, it can be reused, but probably very rarely. The cons, it should be maintained. The storage or logic change can make it wrong.
Usually migrations are from one storage schema to another, which stays relevant.

@ggwpez
Copy link
Member

ggwpez commented Jun 2, 2023 •
edited
Loading

I think we can delete pallet-tips from Substrate once it is un-deployed from DotSama, so no maintenance burden.

@liamaharon
Copy link
Contributor Author

I think we can delete pallet-tips from Substrate once it is un-deployed from DotSama, so no maintenance burden.

@ggwpez are we sure there're no parachains using it?

@liamaharon
Copy link
Contributor Author

bot merge

@paritytech-processbot
Copy link

Waiting for commit status.

@liamaharon liamaharon mentioned this pull request Jun 4, 2023
Closed
@paritytech-processbot
Copy link

Merge cancelled due to error. Error: Statuses failed for 7b51e6b

@ggwpez
Copy link
Member

ggwpez commented Jun 4, 2023

I think we can delete pallet-tips from Substrate once it is un-deployed from DotSama, so no maintenance burden.

@ggwpez are we sure there're no parachains using it?

Parachains can use it all they want, but maintaining pallets that we dont use ourselves does not make any sense (to me).
So once we have a proper offboarding, probably containing a multi-block unlock migration, we should be able to delete them. Parachains can then fork the code and maintain if themselfes.

@liamaharon
Copy link
Contributor Author

bot merge

@paritytech-processbot
Copy link

Waiting for commit status.

@paritytech-processbot paritytech-processbot bot merged commit 31b57a1 into master Jun 5, 2023
@paritytech-processbot paritytech-processbot bot deleted the liam-unreserve-tips-migration branch June 5, 2023 09:15
@xlc
Copy link
Contributor

xlc commented Jun 5, 2023

Parachains can use it all they want, but maintaining pallets that we dont use ourselves does not make any sense (to me).
So once we have a proper offboarding, probably containing a multi-block unlock migration, we should be able to delete them. Parachains can then fork the code and maintain if themselfes.

Really????

@ggwpez
Copy link
Member

ggwpez commented Jun 5, 2023

The Gov1 pallets can hopefully also be moved out of Substrate - to ORML for example.

@xlc
Copy link
Contributor

xlc commented Jun 5, 2023 •
edited
Loading

I guess you should talk with ORML maintainers about it first before making a decision / suggestion.

And of course with every parachain team devs because this is going to be another big breaking change. And for what? Move some of your responsibility to someone else so it is no longer your problem? Yeah sure. What is exactly Substrate and FRAME?

@gilescope
Copy link
Contributor

@xlc has a fair point. We could at least move old pallets to an unmaintained/archived dir for a period of time with the hope that others projects might take up the maintenance so that there's a period of time for people to migrate off it to something else. We need to be better at backward compatibility rather than rug pulling pallets out from under people. People can't build on shifting sands.

@ggwpez
Copy link
Member

ggwpez commented Jun 6, 2023 •
edited
Loading

@xlc has a fair point. We could at least move old pallets to an unmaintained/archived dir for a period of time with the hope that others projects might take up the maintenance so that there's a period of time for people to migrate off it to something else.

Yes it should go through the deprecation process (paritytech/polkadot-sdk#182), and in the process of that, some other entity would hopefully pick up the maintenance of that. Could be any entity in the ecosystem that has an interest in their maintenance.

We need to be better at backward compatibility rather than rug pulling pallets out from under people. People can't build on shifting sands.

Yea true. I dont know if this has been talked about publicly yet, but the LTS release will help with that.

I guess you should talk with ORML maintainers about it first before making a decision / suggestion.

I did not intend to decide anything on behalves of ORML maintainers. Sorry if it seemed so.

And of course with every parachain team devs because this is going to be another big breaking change. And for what? Move some of your responsibility to someone else so it is no longer your problem? Yeah sure. What is exactly Substrate and FRAME?

Now this is my personal opinion (and not any kind of decision on behalf of Parity or another party):
I think that the core Substrate repository is not a good spot to host a large collection of random pallets. I think it is better to keep Substrate itself on the smaller side as to provide universal building blocks for FRAME Pallets and the matching node software. The bulk collection of pallets could then live in a more openly managed repo, where the community takes active part in their maintenance.
This could help with the problem of "Parity is constantly breaking our code".

@gavofyork
Copy link
Member

I have to mostly agree with @ggwpez here. Like any collective of individuals, Parity is neither able nor implicitly responsible to indefinitely maintain superseded or deprecated code, even if it was the one who originally wrote it.

@xlc
Copy link
Contributor

xlc commented Jun 6, 2023

I agree this should be a collective effort not just Parity but also there are some work that Parity have to do to make it possible. This isn’t a new topic. #11530

@juangirini juangirini mentioned this pull request Aug 24, 2023
Open
nathanwhit pushed a commit to nathanwhit/substrate that referenced this pull request Jul 19, 2023
@liamaharon @nathanwhit
migration(tips): unreserve deposits (paritytech#14241)
063231e 
* unreserve all tip funds migration

* improve test

* fix comment

* implement weights

* saturating_accrue

* remove unnecessary collect

* prefer ensure

* use assert

* use saturating_add

* use saturating_accrue

* test pre_upgrade and post_upgrade

* remove pallet_treasury bound

* resolve pr comments

* rename migration

* kick ci

* kick ci
@jasl jasl mentioned this pull request Sep 20, 2023
Merged
@crystalin crystalin mentioned this pull request Oct 20, 2023
Closed
@JesseAbram JesseAbram mentioned this pull request Oct 26, 2023
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ggwpez ggwpez ggwpez approved these changes

@muharem muharem muharem approved these changes

Assignees
No one assigned
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 T1-runtime This PR/Issue is related to the topic “runtime”.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@liamaharon @muharem @ggwpez @xlc @gilescope @gavofyork