chore: extend v1.7.2 transfer location migration #1858

wischli · 2024-06-04T14:45:03Z

Description

Extends #1853 by

Adding pre- and post-hooks
Adding a migration for Dev and Demo
Moving from hardcoded account id migration to account list per runtime
- Dev has no storage entries which need to be migrated
- Demo has 2 storage entries for the same account id which need to be migrated but since we don't use XCM messages on Demo, we can just purge them IMO

To be discussed

This PR is more aggressive with storage removal than the base one: It removes any old storage entry which cannot be decoded versus only removing, if it can be decoded and a new v4 location can be created.

My argument is that undecodable storage keys are always a pain to clean up. If new storage entries pop up in between proposing the RU and executing it, we can either push a hotfix PR afterwards migrating those keys or communicating that these users need to re-do their transfer allowlist call manually.

Another option would be exposing a migrate extrinsic in the pallet-transfer-allowlist for undecodable keys and keeping them in storage.

Checklist:

I have added Rust doc comments to structs, enums, traits and functions
I have made corresponding changes to the documentation
I have performed a self-review of my code
I have added tests that prove my fix is effective or that my feature works

codecov · 2024-06-04T15:59:54Z

Codecov Report

Attention: Patch coverage is 0% with 60 lines in your changes missing coverage. Please review.

Project coverage is 46.55%. Comparing base (b95b098) to head (995887e).
Report is 2 commits behind head on polkadot-v1.7.2-transfer-location.

Files	Patch %	Lines
...ntime/common/src/migrations/restricted_location.rs	0.00%	60 Missing ⚠️

Additional details and impacted files

@@                          Coverage Diff                          @@
##           polkadot-v1.7.2-transfer-location    #1858      +/-   ##
=====================================================================
- Coverage                              46.74%   46.55%   -0.20%     
=====================================================================
  Files                                    166      167       +1     
  Lines                                  13044    13076      +32     
=====================================================================
- Hits                                    6097     6087      -10     
- Misses                                  6947     6989      +42

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

lemunozm · 2024-06-05T05:51:17Z

My argument is that undecodable storage keys are always a pain to clean up. If new storage entries pop up in between proposing the RU and executing it, we can either push a hotfix PR afterwards migrating those keys or communicating that these users need to re-do their transfer allowlist call manually.

But how can we know we need to migrate them or re-do them if we remove all of them? Just because the transfer filter is not filtering as expected?

lemunozm

Great improvement over the base migration @wischli! All cases are well tied 💯

lemunozm · 2024-06-05T05:57:06Z

runtime/common/src/migrations/restricted_location.rs

+						Self::migrate_location_key(transfer_address.clone(), hash),
+					)),
+					(None, old::RestrictedTransferLocation::Xcm(_)) => {
+						log::warn!("{LOG_PREFIX} Account {account_id:?} missing in AccountMap despite old XCM location storage");


Great check!

lemunozm · 2024-06-05T05:59:28Z

runtime/common/src/migrations/restricted_location.rs

+							currency_id,
+							old_restricted_location,
+							// Leads to storage entry removal
+							// TODO: Discuss whether we are fine with removing such storage entries or whether we want to keep the old undecodable ones or maybe just use same account id per default instead of removing?


removing such storage entries

keep the old undecodable ones

use same account id per default instead of removing

I would vote for 1. and 2.

lemunozm · 2024-06-05T06:03:07Z

runtime/centrifuge/src/migrations.rs

+	// 4dTeMxuPJCK7zQGhFcgCivSJqBs9Wo2SuMSQeYCCuVJ9xrE2 --> 5Fc9NzKzJZwZvgjQBmSKtvZmJ5oP6B49DFC5dXZhTETjrSzo
+	pub AccountMap: Vec<(AccountId, AccountId)> = vec![
+		(
+			AccountId::new(hex_literal::hex!("5dbb2cec05b6bda775f7945827b887b0e7b5245eae8b4ef266c60820c9377185")),


This account is 4dTeMxuPJCK7zQGhFcgCivSJqBs9Wo2SuMSQeYCCuVJ9xrE2 in hex, right?

Yes

❯ subkey inspect 4dTeMxuPJCK7zQGhFcgCivSJqBs9Wo2SuMSQeYCCuVJ9xrE2 Public Key URI `4dTeMxuPJCK7zQGhFcgCivSJqBs9Wo2SuMSQeYCCuVJ9xrE2` is account: Network ID/Version: centrifuge Public key (hex): 0x5dbb2cec05b6bda775f7945827b887b0e7b5245eae8b4ef266c60820c9377185 Account ID: 0x5dbb2cec05b6bda775f7945827b887b0e7b5245eae8b4ef266c60820c9377185 Public key (SS58): 4dTeMxuPJCK7zQGhFcgCivSJqBs9Wo2SuMSQeYCCuVJ9xrE2

wischli · 2024-06-05T07:16:00Z

But how can we know we need to migrate them or re-do them if we remove all of them? Just because the transfer filter is not filtering as expected?

I would say by looking at the logs at the time of the migration which I always do for runtime upgrades and we have access to historic storage by querying it for an older block hash in which it still existed.

The only upside for keeping it is that it is easier to reconstruct and check against on the runtime level.

lemunozm · 2024-06-05T07:29:20Z

Ok, then we can remove them. I think the probability to have new entries is low

lemunozm · 2024-06-05T07:30:00Z

You can merge it when you want 💯

* migration for restricted transfer location * add correct location for centrifuge * minor changes * fix benchmarks * fix IT compilation * chore: extend v1.7.2 transfer location migration (#1858) * feat: add pre + post-upgrade * feat: add nuking migration for dev and demo * fix: remove account list for demo * refactor: Use `VersionedLocation` in `RestrictedTransferLocation` (#1861) * feat: add pre + post-upgrade * feat: add nuking migration for dev and demo * fix: remove account list for demo * refactor: use VersionedLocation * fix: transfer allowlist benchmarks + tests * fix: clippy * fix: benchmark cli * fix: revert tight coupling * fmt * fix IT compilation --------- Co-authored-by: William Freudenberger <[email protected]>

wischli added 3 commits June 4, 2024 16:33

feat: add pre + post-upgrade

66886ca

feat: add nuking migration for dev and demo

8242a08

fix: remove account list for demo

995887e

wischli added the D8-migration Pull request touches storage and needs migration code. label Jun 4, 2024

wischli self-assigned this Jun 4, 2024

wischli requested a review from lemunozm June 4, 2024 14:45

lemunozm approved these changes Jun 5, 2024

View reviewed changes

lemunozm reviewed Jun 5, 2024

View reviewed changes

lemunozm mentioned this pull request Jun 5, 2024

Upgrade to Polkadot-v1.7.2 #1833

Merged

52 tasks

wischli merged commit f457f22 into polkadot-v1.7.2-transfer-location Jun 5, 2024
9 of 15 checks passed

wischli added this to the Centrifuge 1029 milestone Jun 6, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: extend v1.7.2 transfer location migration #1858

chore: extend v1.7.2 transfer location migration #1858

wischli commented Jun 4, 2024 •

edited

Loading

codecov bot commented Jun 4, 2024

lemunozm commented Jun 5, 2024

lemunozm left a comment

lemunozm Jun 5, 2024

lemunozm Jun 5, 2024

lemunozm Jun 5, 2024

wischli Jun 5, 2024

wischli commented Jun 5, 2024

lemunozm commented Jun 5, 2024

lemunozm commented Jun 5, 2024 •

edited

Loading

chore: extend v1.7.2 transfer location migration #1858

chore: extend v1.7.2 transfer location migration #1858

Conversation

wischli commented Jun 4, 2024 • edited Loading

Description

To be discussed

Checklist:

codecov bot commented Jun 4, 2024

Codecov Report

lemunozm commented Jun 5, 2024

lemunozm left a comment

Choose a reason for hiding this comment

lemunozm Jun 5, 2024

Choose a reason for hiding this comment

lemunozm Jun 5, 2024

Choose a reason for hiding this comment

lemunozm Jun 5, 2024

Choose a reason for hiding this comment

wischli Jun 5, 2024

Choose a reason for hiding this comment

wischli commented Jun 5, 2024

lemunozm commented Jun 5, 2024

lemunozm commented Jun 5, 2024 • edited Loading

wischli commented Jun 4, 2024 •

edited

Loading

lemunozm commented Jun 5, 2024 •

edited

Loading