fix: asset metadata xcm v3 migration cleanup other migrations #1528

wischli · 2023-09-04T19:52:14Z

Adds missing asset metadata registrations to Centrifuge and Altair runtimes
Refactors and fixes asset metadata migration to xcm v3
- We cannot read the number of LocationToAssetId storage keys via the iter_keys().count() API as all prod chains contain keys which cannot be decoded (XCMv1 MultiLocations)
- By moving the migration to runtime-common, we can reduce the code heavily
Removes unnecessary migrations
Cleans up imports

`try-runtime` output for Centrifuge

Both the weight as well as PoV are low so that we don't need to be worried applying this migration

2023-09-05 14:00:57.802 DEBUG main try-runtime::cli: proof: 0x502705db4aa834e900c8... / 58 nodes
2023-09-05 14:00:57.802 DEBUG main try-runtime::cli: proof size: 7.08 KB (7254 bytes)
2023-09-05 14:00:57.802 DEBUG main try-runtime::cli: compact proof size: 5.29 KB (5415 bytes)
2023-09-05 14:00:57.802 DEBUG main try-runtime::cli: zstd-compressed compact proof 4.92 KB (5040 bytes)
2023-09-05 14:00:57.802 DEBUG main try-runtime::cli: TryRuntime_on_runtime_upgrade executed without errors.
2023-09-05 14:00:57.846  INFO main try-runtime::cli: TryRuntime_on_runtime_upgrade executed without errors. Consumed weight = (5200000000 ps, 0 byte), total weight = (500000000000 ps, 5242880 byte) (1.04 %, 0.00 %).

mustermeiszer

Will take a final look once Altair is in! Migrations looks like it was really tricky, thanks that looks great now!

mustermeiszer · 2023-09-04T20:42:58Z

runtime/centrifuge/src/migrations.rs

+							// TODO(@review): Should probably be LiquidityPoolsTransferability?
+							transferability: CrossChainTransferability::Xcm(XcmMetadata {
+								fee_per_second: None,
+							}),


No, that is perfect as is. We only want to receive that via xcm into accounts for now. I think we first need to go over the implications of having both transferabilities enabled for a single asset.

mustermeiszer · 2023-09-04T20:46:03Z

runtime/common/src/migrations/asset_registry_xcmv3.rs

+
+		// Complexity: O(loc_count) writes
+		let result =
+			orml_asset_registry::LocationToAssetId::<T>::clear(loc_count.saturating_add(2), None);


Why the plus 2?

Sorry, this can be removed. It was needed when we read the counts via the iter_keys API due to key decoding failures. However, the current approach ignores decoding and thus catches all keys.

mustermeiszer · 2023-09-04T20:46:39Z

runtime/common/src/migrations/asset_registry_xcmv3.rs

+        );
+
+		// Complexity: O(meta_count) writes
+		let result = orml_asset_registry::Metadata::<T>::clear(meta_count.saturating_add(1), None);


Why the plus 1?

mustermeiszer · 2023-09-04T20:48:55Z

runtime/common/src/migrations/asset_registry_xcmv3.rs

mustermeiszer

Native currency needs absolut path.

runtime/centrifuge/src/migrations.rs

mustermeiszer · 2023-09-04T21:02:22Z

runtime/centrifuge/src/migrations.rs

+			let mut tranche_id = [0u8; 16];
+			tranche_id[..tranche_id_bytes.len()].copy_from_slice(tranche_id_bytes);
+
+			vec![


Can we also register Native here? Wdyt? No hard requirement.

Sure! I suppose we want to register all Centrifuge assets actually?

I think Native is sufficient, no?

Not sure TBH. Catalyst is not connected to anything and we control Council. So I would say we can also do it later. Are there any other reasons I am missing? Whatever you think is best and easiest.

Co-authored-by: Frederik Gartenmeister <[email protected]>

NunoAlexandre

Look good! I was thinking that we would "just" basically call omrl_asset_registry::update_asset for each known CurrencyId with the updated, xcm v3 multilocation, instead of clearing all the different storages and registering the tokens anew. That still seems much simpler but now we have this already in place so that's not a problem for me 👍

NunoAlexandre · 2023-09-05T07:03:07Z

runtime/altair/src/xcm.rs

-			RuntimeCall::System(
-				frame_system::Call::kill_prefix { .. } | frame_system::Call::set_heap_pages { .. },
-			) | RuntimeCall::Timestamp(..)
+			RuntimeCall::Timestamp(..)
 				| RuntimeCall::Balances(..)
-				| RuntimeCall::Session(pallet_session::Call::purge_keys { .. })
-				| RuntimeCall::Treasury(..)
 				| RuntimeCall::Utility(pallet_utility::Call::as_derivative { .. })
-				| RuntimeCall::Vesting(..)
 				| RuntimeCall::PolkadotXcm(
 					pallet_xcm::Call::limited_reserve_transfer_assets { .. }
-				) | RuntimeCall::CollatorSelection(
-				pallet_collator_selection::Call::set_desired_candidates { .. }
-					| pallet_collator_selection::Call::set_candidacy_bond { .. }
-					| pallet_collator_selection::Call::register_as_candidate { .. }
-					| pallet_collator_selection::Call::leave_intent { .. },
-			) | RuntimeCall::XcmpQueue(..)
+				) | RuntimeCall::XcmpQueue(..)
 				| RuntimeCall::DmpQueue(..)
 				| RuntimeCall::Proxy(..)
+				| RuntimeCall::LiquidityPoolsGateway(


These changes to the other calls are unrelated to this PR and could introduce undesired behaviour given that we don't have tests in place to properly assess this. Not a blocker tho

I honestly don't know why this diff still exists. This change was merged yesterday already with #1526. I previously had this PR based against the #1526 branch.

mustermeiszer

Removing change request as it is addressed. Just so that I am not the blocking person anymore.

wischli · 2023-09-05T08:04:23Z

@NunoAlexandre

I was thinking that we would "just" basically call omrl_asset_registry::update_asset for each known CurrencyId with the updated, xcm v3 multilocation, instead of clearing all the different storages and registering the tokens anew. That still seems much simpler but now we have this already in place so that's not a problem for me 👍

Same here. I took over this approach from @mikiquantum and wanted to change as little as possible apart from making it runtime agnostic.

I suppose we don't need to run the CrossChainTransferabilityMigration then as we hardcode all metadata. Sorry for the wasted time on that beatiful migration which is superseeded by this ugly hardcoded one.

NunoAlexandre · 2023-09-05T08:20:44Z

@NunoAlexandre

I was thinking that we would "just" basically call omrl_asset_registry::update_asset for each known CurrencyId with the updated, xcm v3 multilocation, instead of clearing all the different storages and registering the tokens anew. That still seems much simpler but now we have this already in place so that's not a problem for me 👍

Same here. I took over this approach from @mikiquantum and wanted to change as little as possible apart from making it runtime agnostic.

I suppose we don't need to run the CrossChainTransferabilityMigration then as we hardcode all metadata. Sorry for the wasted time on that beatiful migration which is superseeded by this ugly hardcoded one.

Ah, that makes sense! :)

And no worries, what is important is that we get this right 🏄‍♂️

wischli · 2023-09-05T10:34:56Z

FYI, I cleaned up and checked all migrations against Algol, Altair, Catalyst and Centrifuge with try-runtime.

wischli · 2023-09-05T11:49:35Z

runtime/centrifuge/src/migrations.rs

 	pallet_rewards::migrations::new_instance::FundExistentialDeposit<
 		crate::Runtime,
 		pallet_rewards::Instance2,
 		crate::NativeToken,
 		crate::ExistentialDeposit,
 	>,
-	asset_registry::AssetRegistryMultilocationToXCMV3<crate::Runtime>,


Note: Replaces with runtime_common::migrations::asset_registry_xcmv3::Migration

wischli · 2023-09-05T11:49:54Z

runtime/centrifuge/src/migrations.rs


 pub type UpgradeCentrifuge1020 = (
-	asset_registry::CrossChainTransferabilityMigration,


Note: Not necessary due to hardcoded metadata from runtime_common::migrations::asset_registry_xcmv3::Migration

wischli · 2023-09-05T11:50:43Z

runtime/altair/src/migrations.rs

+	// FIXME: This migration fails to decode 4 entries against Altair
 	// orml_tokens_migration::CurrencyIdRefactorMigration,


Note: Needs fixing before Altair upgrade!

wischli · 2023-09-05T11:51:24Z

runtime/altair/src/migrations.rs

 	// orml_tokens_migration::CurrencyIdRefactorMigration,
-	pool_system::MigrateAUSDPools,


Note: Superseeded by runtime_common::migrations::nuke::Migration<crate::PoolSystem, crate::RocksDbWeight, 0>

wischli · 2023-09-05T11:51:47Z

runtime/altair/src/migrations.rs

 	pallet_rewards::migrations::new_instance::FundExistentialDeposit<
 		crate::Runtime,
 		pallet_rewards::Instance2,
 		crate::NativeToken,
 		crate::ExistentialDeposit,
 	>,
-	asset_registry::AssetRegistryMultilocationToXCMV3<crate::Runtime>,


Note: Replaced with runtime_common::migrations::asset_registry_xcmv3::Migration

wischli · 2023-09-05T11:52:17Z

runtime/altair/src/migrations.rs

+	// At minimum, bumps storage version from 0 to 1
+	runtime_common::migrations::nuke::Migration<crate::PoolSystem, crate::RocksDbWeight, 0>,
+	// At minimum, bumps storage version from 0 to 1
+	runtime_common::migrations::nuke::Migration<crate::Investments, crate::RocksDbWeight, 0>,
+	// Funds pallet_rewards::Instance2 account with existential deposit


Note: Storage is empty but this nuke still bumps StorageVersion.

wischli · 2023-09-05T11:53:06Z

runtime/altair/src/migrations.rs

-	// runtime_common::migrations::nuke::Migration<crate::Loans, RocksDbWeight, 1>,
-	// runtime_common::migrations::nuke::Migration<crate::InterestAccrual, RocksDbWeight, 0>,


Note: No nuke required on Algol for Loans and InterestAccrual as these are already at the latest StorageVersion

wischli · 2023-09-05T11:53:39Z

runtime/centrifuge/src/migrations.rs

+	// At minimum, bumps storage version from 0 to 1
+	runtime_common::migrations::nuke::Migration<crate::PoolSystem, crate::RocksDbWeight, 0>,
+	// At minimum, bumps storage version from 0 to 1
+	runtime_common::migrations::nuke::Migration<crate::Investments, crate::RocksDbWeight, 0>,


Note: Storage is empty but this nuke still bumps StorageVersion.

mustermeiszer

Altair native wrong location. Otherwise fine.

mustermeiszer · 2023-09-05T12:24:15Z

runtime/altair/src/migrations.rs

+						location: Some(VersionedMultiLocation::V3(MultiLocation::new(
+							0,
+							Junctions::X1(GeneralKey {
+								length: 2,
+								data: gk,
+							}),


Absolut path needed

mustermeiszer · 2023-09-05T12:25:21Z

runtime/altair/src/migrations.rs

-			None => {
-				break count;
-			}
+		pub fn get_algol_assets() -> Vec<(


missing native.

mustermeiszer

LGTM! Thanks for the effort, that was quite a pice ^^

wischli · 2023-09-05T13:04:57Z

Thanks for the effort, that was quite a pice ^^

Indeed... Migrations always multiply the usual required time by orders of magnitude. At least we can be sure all migrations are now properly configured - 🤞 it works out in practice.

* wip xcm v3 asset-registry migrations * fix: xcm v3 altair, centrifuge * feat: add defensive SafeCallFilter for transact * clean up asset addition for catalyst & centrifuge * add Altair + Algol migration * Add Liquidity Pools related pallets to centrifuge runtime (#1523) * centrifuge: Add liquidity pools pallets to runtime * centrifuge: Add LP OutboundQueue wrapper to runtime * taplo: Obey * runtime: Move OutboundQueue import * centrifuge: Allow both root and half of council as admin origins for LP pallets * centrifuge: Add Axelar Gateway precompile * centrifuge: Use correct HRMP encoder, update HRMP fee to 1 DOT, use XCM V3 imports * centrifuge: Update error message for InboundQueue * clippy: Fix warnings * centrifuge: Rename outbound queue, fix migration import for orml asset registry * Upgrade1020 LP follow-ups (#1525) * centrifuge: Enable LP gateway XCM origin converter * centrifuge: Update LP pallet config to use Quantity for BalanceRatio * runtime: Update XCM origin converter, use Quantity in LP message, add OriginRecovery to LP gateway * feat: safe XCM version migration + extend SafeCallFilter (#1526) * fix: allow gateway process msg for xcm * feat: add safe xcm version migration * fix: Allow process_msg for xcm transact * refactor: remove spec_version check * refactor: stricter SafeCallFilter * Upgrade1020 LP follow-ups (#1525) * centrifuge: Enable LP gateway XCM origin converter * centrifuge: Update LP pallet config to use Quantity for BalanceRatio * runtime: Update XCM origin converter, use Quantity in LP message, add OriginRecovery to LP gateway * fix: rm OrderBook safe call cause missing * fix: OrderBook missing in both runtimes * fix: clippy --------- Co-authored-by: Cosmin Damian <[email protected]> * Feat/upgrade1020 misc 1 (#1529) * fix: asset metadata xcm v3 migration cleanup other migrations (#1528) * fix: centrifuge asset migration * Apply suggestions from code review Co-authored-by: Frederik Gartenmeister <[email protected]> * chore: append cfg assets to catalyst * fix: asset registry migration improvements * feat: apply asset metadata migration to altair runtime * fix: cleanup altair/algol migrations * fix: less aggressive nuke pre-upgrade * fix: finalize migrations, add context * fix: clippy + cleanup imports * fix: native currency altair, algol * fix: Algol native currency name --------- Co-authored-by: Frederik Gartenmeister <[email protected]> * fmt * fixup * Upgrade1020 gateway changes (#1531) * gateway: Use BoundedVec instead of EVMChain enum * gateway: Add Sender type to LP gateway config * gateway: Rename EVM chain size const, emit event when submitting message, adjust gateway sender provider * Feat: upgrade1020 misc 2 (#1532) * feat: account ensuring origin * feat: bumb runtime version develpment * fix: allow council to control gatway on Altair based chains * fix: use EnsureSigned, make use of type and add admin * feat: overestimated weights for lp logic * fix: taplooo * fix: tests and comment * chore: bump weights for 1020 (#1533) * chore: update centrifuge, altair weights * feat: bump and improve dev weights * fix: transfer_allowlist weights * feat: normalize SafeCallFilter across runtimes * fix: native asset key in migration --------- Co-authored-by: William Freudenberger <[email protected]> Co-authored-by: Cosmin Damian <[email protected]> Co-authored-by: Frederik Gartenmeister <[email protected]> Co-authored-by: nuno <[email protected]>

Base automatically changed from wf/upgrade-1020 to upgrade1020 September 4, 2023 20:02

mustermeiszer reviewed Sep 4, 2023

View reviewed changes

mustermeiszer requested changes Sep 4, 2023

View reviewed changes

wischli and others added 3 commits September 5, 2023 08:36

fix: centrifuge asset migration

9d21b61

Apply suggestions from code review

fc5b5fd

Co-authored-by: Frederik Gartenmeister <[email protected]>

chore: append cfg assets to catalyst

b8ea129

NunoAlexandre reviewed Sep 5, 2023

View reviewed changes

mustermeiszer approved these changes Sep 5, 2023

View reviewed changes

wischli added 2 commits September 5, 2023 10:12

fix: asset registry migration improvements

9fe6e83

feat: apply asset metadata migration to altair runtime

ececb2d

wischli added 3 commits September 5, 2023 11:39

fix: cleanup altair/algol migrations

e24f030

fix: less aggressive nuke pre-upgrade

85409e0

fix: finalize migrations, add context

0b64fa1

wischli force-pushed the wf/upgrade-1020-asset-migration branch from a877a4e to 0b64fa1 Compare September 5, 2023 10:29

wischli changed the title ~~fix: asset metadata migration xcm v3~~ fix: asset metadata xcm v3 migration and others Sep 5, 2023

wischli marked this pull request as ready for review September 5, 2023 10:35

wischli requested review from mikiquantum, hieronx and lemunozm as code owners September 5, 2023 10:35

wischli changed the title ~~fix: asset metadata xcm v3 migration and others~~ fix: asset metadata xcm v3 migration cleanup other migrations Sep 5, 2023

fix: clippy + cleanup imports

c195a78

wischli commented Sep 5, 2023

View reviewed changes

wischli requested a review from NunoAlexandre September 5, 2023 12:12

mustermeiszer requested changes Sep 5, 2023

View reviewed changes

fix: native currency altair, algol

a9caf16

wischli requested a review from mustermeiszer September 5, 2023 12:45

fix: Algol native currency name

1e8c597

mustermeiszer approved these changes Sep 5, 2023

View reviewed changes

wischli merged commit 5cde842 into upgrade1020 Sep 5, 2023
3 of 11 checks passed

wischli deleted the wf/upgrade-1020-asset-migration branch September 5, 2023 13:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: asset metadata xcm v3 migration cleanup other migrations #1528

fix: asset metadata xcm v3 migration cleanup other migrations #1528

wischli commented Sep 4, 2023 •

edited

Loading

mustermeiszer left a comment

mustermeiszer Sep 4, 2023

mustermeiszer Sep 4, 2023

wischli Sep 5, 2023

mustermeiszer Sep 4, 2023

wischli Sep 5, 2023

mustermeiszer Sep 4, 2023

mustermeiszer left a comment

mustermeiszer Sep 4, 2023

wischli Sep 5, 2023

NunoAlexandre Sep 5, 2023

mustermeiszer Sep 5, 2023

NunoAlexandre left a comment

NunoAlexandre Sep 5, 2023

wischli Sep 5, 2023

mustermeiszer left a comment

wischli commented Sep 5, 2023 •

edited

Loading

NunoAlexandre commented Sep 5, 2023

wischli commented Sep 5, 2023

wischli Sep 5, 2023

wischli Sep 5, 2023

wischli Sep 5, 2023

wischli Sep 5, 2023

wischli Sep 5, 2023

wischli Sep 5, 2023

wischli Sep 5, 2023

wischli Sep 5, 2023

mustermeiszer left a comment

mustermeiszer Sep 5, 2023

wischli Sep 5, 2023

mustermeiszer Sep 5, 2023

wischli Sep 5, 2023

mustermeiszer left a comment

wischli commented Sep 5, 2023


		pub type UpgradeCentrifuge1020 = (
		asset_registry::CrossChainTransferabilityMigration,

		// FIXME: This migration fails to decode 4 entries against Altair
		// orml_tokens_migration::CurrencyIdRefactorMigration,

		// orml_tokens_migration::CurrencyIdRefactorMigration,
		pool_system::MigrateAUSDPools,

		// runtime_common::migrations::nuke::Migration<crate::Loans, RocksDbWeight, 1>,
		// runtime_common::migrations::nuke::Migration<crate::InterestAccrual, RocksDbWeight, 0>,

fix: asset metadata xcm v3 migration cleanup other migrations #1528

fix: asset metadata xcm v3 migration cleanup other migrations #1528

Conversation

wischli commented Sep 4, 2023 • edited Loading

try-runtime output for Centrifuge

mustermeiszer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mustermeiszer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NunoAlexandre left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mustermeiszer left a comment

Choose a reason for hiding this comment

wischli commented Sep 5, 2023 • edited Loading

NunoAlexandre commented Sep 5, 2023

wischli commented Sep 5, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mustermeiszer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mustermeiszer left a comment

Choose a reason for hiding this comment

wischli commented Sep 5, 2023

wischli commented Sep 4, 2023 •

edited

Loading

`try-runtime` output for Centrifuge

wischli commented Sep 5, 2023 •

edited

Loading