This repository has been archived by the owner on Nov 15, 2023. It is now read-only.
migration(elections-phragmen): unreserve deposits and clear locks #14218
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ections-phragmen-unlock-and-unreserve-funds-migration
liamaharon
added
A0-please_review
liamaharon
changed the title
Closed
12 tasks
liamaharon
changed the title
…ections-phragmen-unlock-and-unreserve-funds-migration
liamaharon
changed the title
liamaharon
changed the title
|
I just found a way to handle the Balances types cleaner. Switching this to a draft while I improve that, also going to add some unit tests. |
liamaharon
added
A3-in_progress
kianenigma
reviewed
May 26, 2023
frame/elections-phragmen/src/migrations/unlock_and_unreserve_all_funds.rs
Outdated
Show resolved
Hide resolved
…ections-phragmen-unlock-and-unreserve-funds-migration
…ections-phragmen-unlock-and-unreserve-funds-migration
kianenigma
reviewed
Jun 13, 2023
| .len() | ||
| .saturating_add(runner_ups.len()) | ||
| .saturating_add(candidates.len()) | ||
| .saturating_add(voters_len.saturating_mul(T::MaxVotesPerVoter::get() as usize)) |
There was a problem hiding this comment.
Not sure why you multiply T::MaxVotesPerVoter?
There was a problem hiding this comment.
Each Voter contains a Vec called votes up to size MaxVotesPerVoter.
I guessed that the weight of the read scales with the size of the Vec, but maybe that's incorrect if it can all be loaded into memory in one read regardless?
kianenigma
reviewed
Jun 13, 2023
|
|
||
| // Get the total amount deposited (Members, RunnerUps, Candidates and Voters all can have | ||
| // deposits). | ||
| let account_deposited_sums: BTreeMap<T::AccountId, BalanceOf<T>> = members |
There was a problem hiding this comment.
to be pedantic about the variable name, we are voting here, not staking.
There was a problem hiding this comment.
It's referred to as a stake in the pallet:
substrate/frame/elections-phragmen/src/lib.rs
Lines 152 to 153 in a57508e
Still happy to change it to locked though if you prefer.
There was a problem hiding this comment.
I'm going to merge this for now in the interests of trying to get the migrations ready for 1.0, but let me know if you'd still like me to change it and I will open another pr :)
kianenigma
approved these changes
Jun 13, 2023
sam0x17
approved these changes
Jun 13, 2023
ggwpez
reviewed
Jun 13, 2023
frame/elections-phragmen/src/migrations/unlock_and_unreserve_all_funds.rs
Outdated
Show resolved
Hide resolved
frame/elections-phragmen/src/migrations/unlock_and_unreserve_all_funds.rs
Show resolved
Hide resolved
frame/elections-phragmen/src/migrations/unlock_and_unreserve_all_funds.rs
Outdated
Show resolved
Hide resolved
frame/elections-phragmen/src/migrations/unlock_and_unreserve_all_funds.rs
Outdated
Show resolved
Hide resolved
frame/elections-phragmen/src/migrations/unlock_and_unreserve_all_funds.rs
Outdated
Show resolved
Hide resolved
frame/elections-phragmen/src/migrations/unlock_and_unreserve_all_funds.rs
Show resolved
Hide resolved
…ll_funds.rs Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
…ll_funds.rs Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
…gration' and 'liam-elections-phragmen-unlock-and-unreserve-funds-migration' of github.com:paritytech/substrate into liam-elections-phragmen-unlock-and-unreserve-funds-migration
…ections-phragmen-unlock-and-unreserve-funds-migration
…ll_funds.rs Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
|
bot merge |
|
Waiting for commit status. |
|
Merge cancelled due to error. Error: Github API says #14218 is not mergeable |
|
bot merge |
paritytech-processbot
bot
deleted the
liam-elections-phragmen-unlock-and-unreserve-funds-migration
branch
nathanwhit
pushed a commit
to nathanwhit/substrate
that referenced
this pull request
Jul 19, 2023
…ritytech#14218) * pre_upgrade hook wip * working pre_upgrade * simplify code * cleanup and document * return reads from get_account_deposited_and_staked_sums * improve comment * on_runtime_upgrade comment * post upgrade comment * use saturating_add * clippy * clean up balances * add tests * fix comment * oops * actually fix comment * fix std build * address pr comments * remove redundant comment * update comment * add comment * oliver comments from tips pallet pr * lint * remove need for do_pre/do_post runtime functions * generic dbweight * Update frame/elections-phragmen/src/migrations/unlock_and_unreserve_all_funds.rs Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io> * Update frame/elections-phragmen/src/migrations/unlock_and_unreserve_all_funds.rs Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io> * pr comments * remove useless check * feature gate log target * lint * Update frame/elections-phragmen/src/migrations/unlock_and_unreserve_all_funds.rs Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io> * add log for unexpected amounts --------- Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Partial paritytech/polkadot-sdk#485
Overview
Adds a migration that unreserves all deposits and unlocks all stake held by the elections-phragmen pallet.
I've verified this works using
try-runtime-cliand the latest Polkadot state (testing with Kusama state blocked by my last Q below).Details
elections-phragmencontains 4 storage items containing information about balances to unreserve and unlock:Members,RunnerUps,Candidates, andVoters.Every storage item stores a deposit to unreserve, and just
Voterscontains a stake to unlock.pre_upgradecollects pre-migration data useful for validating the migration was successful, and also checks the integrity of deposited and reserved balances.on_runtime_upgradeunreserves and unlocks the balances.post_upgradechecks that all expected locks were removed, and reserved balances were reduced by the expected amount.try-runtime-cli on-runtime-upgrade output
Questions for reviewers / todos:
This currently assumes that theelection_phragmenpallet is part of the runtime. We need to run it for Kusama where it has already been removed from the runtime. What's the cleanest way to go about this?How do I calculate the weight of theremove_lockandunreservecalls?remove_locktriggers someERROR main runtime::system: Logic error: Unexpected underflow in reducing consumererror logs. I have verified that a non-zero lock always exists beforeremove_lockis called, even when it results in this log. Are they safe to ignore?