Improve inactive fund tracking #13009
Conversation
gavofyork
added
B0-silent
| // Remove the old `StorageVersion` type. | ||
| frame_support::storage::unhashed::kill(&frame_support::storage::storage_prefix( | ||
| Pallet::<T, I>::name().as_bytes(), | ||
| "StorageVersion".as_bytes(), | ||
| )); |
There was a problem hiding this comment.
| // Remove the old `StorageVersion` type. | |
| frame_support::storage::unhashed::kill(&frame_support::storage::storage_prefix( | |
| Pallet::<T, I>::name().as_bytes(), | |
| "StorageVersion".as_bytes(), | |
| )); |
Should have a proper storage version now since it is at version 1.
There was a problem hiding this comment.
This is also wrong. The key is not StorageVersion
There was a problem hiding this comment.
This was old code. AFAIK it should do no harm.
| Inactive::<T, I>::put(&pot); | ||
| T::Currency::reactivate(deactivated); | ||
| T::Currency::deactivate(pot); | ||
| Deactivated::<T, I>::put(&pot); |
There was a problem hiding this comment.
Also kill the Inactive here as cleanup?
There was a problem hiding this comment.
I'm actually unsure what the implications are for changing the logic here. Is it simply just reverting code?
There was a problem hiding this comment.
Its undoing and fixing the fund-deactivation since it was faultily implemented the first time.
IIUC on the next run the Deactivated will be zero, so the whole pot will be deactivated, which is fine since the foregoing balances migration will reset all deactivated funds.
There was a problem hiding this comment.
Also kill the
Inactivehere as cleanup?
Let's delete it in a migration so that it happens once, not in on_init.
There was a problem hiding this comment.
since it was faultily implemented the first time.
What's the case case where the old logic was wrong?
There was a problem hiding this comment.
Incorrect logic was in crowdloan where a datatype was being misused. See paritytech/polkadot#6471
| InactiveIssuance::<T, I>::kill(); | ||
|
|
||
| // Set storage version to `0`. | ||
| StorageVersion::new(0).put::<Pallet<T, I>>(); |
There was a problem hiding this comment.
@ggwpez If we're at version 1 now, then it doesn't quite make sense to regress and go back to version 0? Or are we really treating this all as a revert?
There was a problem hiding this comment.
It is a trick so that the MigrateToTrackInactive v0 -> v1 migration can run again.
There was a problem hiding this comment.
Can we not just write a proper migration for this? And introduce storage Version 2?
There was a problem hiding this comment.
0 is defined as "not tracking InactiveIssuance", 1 is defined as "tracking InactiveIssuance". In Polkadot/Kusama mainnet (which are already at version 1), we are migrating back to 0 and thus no longer tracking InactiveIssuance, and then migrating back to 1 (and thus tracking it, this time correctly). There are no new alterations to storage formats or indeed state transition semantics, and thus it makes no sense to introduce new StorageVersions which would in all manner be exactly equivalent to the existing StorageVersions. The only thing we are doing is mending state.
There was a problem hiding this comment.
Yeah, from the PoV of Polkadot/Kusama this makes sense. However, we should write these migrations in a generic way so that other downstream users (e.g. parachain teams) can use them as well.
There could at least have been some docs on what this migration does and that it should be executed before the other migration.
* Improve inactive fund tracking * Resetting migration * Fix * Update frame/balances/src/migration.rs
|
This pull request has been mentioned on Polkadot Forum. There might be relevant details there: https://forum.polkadot.network/t/polkadot-release-analysis-v0-9-37/1736/1 |
* Improve inactive fund tracking * Resetting migration * Fix * Update frame/balances/src/migration.rs
* Improve inactive fund tracking * Resetting migration * Fix * Update frame/balances/src/migration.rs
TODO