Girazoki avoid statemine prefix breaking change #1159

girazoki · 2022-01-10T15:42:48Z

What does it do?

It performs three migrations to avoid breaking when statemine changes to its new way of representing assets.

In particular, three new migrations have been added to asset-manager:

M1: Indexes units_per_second by AssetType instead of by assetId. This allows us to decouple assetIds and AssetTypes, and therefore, avoid migrating pallet-assets whenever we have a breaking change like we do in statemine
M2: Populates a new storage item called AssetTypeId, which holds the AssetType->AssetId mapping. This new storage item is needed if we decouple assetIds and AssetTypes, as now one cannot be calculated hashing the other.
M3: Looks for already registered statemine assets of the form X2(Parachain(id), GeneralIndex(index)), and changes them to the new form X3(Parachain(id), PalletInstance(instance), GeneralIndex(index)) . This means that we store the new prefix.

The Runtime has been modified so that when we receive X2(Parachain(id), GeneralIndex(index)), we transalate it to X3(Parachain(id), PalletInstance(instance), GeneralIndex(index)) so that the correct assetId is minted. This wont be necessary once we are sure Statemine has updated, but has been added to avoid interruption of service.

All tests have been adapted to these changes, and new tests have been added to prove that both prefixes mint the same assetId.

This should avoid interrumption from the upgrade that statemine is planning to do including
paritytech/cumulus#831. In a future upgrade, we can remove this hack and migrate our storage item.

I have also added a new extrinsic in pallet-asset-manager to support changing the multilocation associated to an assetId, so that in the future we can adaptively change associations between assetTypes and assetIds

What important points reviewers should know?

Is there something left for follow-up PRs?

What alternative implementations were considered?

Are there relevant PRs or issues in other repositories (Substrate, Polkadot, Frontier, Cumulus)?

What value does it bring to the blockchain users?

…ic to change the sending prefix

…atemine-prefix-breaking-change

notlesh · 2022-01-14T18:24:17Z

pallets/asset-manager/src/benchmarks.rs

+
+	}: _(RawOrigin::Root, asset_id, asset_type.clone())
+	verify {
+		assert_eq!(Pallet::<T>::asset_id_type(asset_id), Some(asset_type));


This looks like it's (1) changing from one asset type to the same asset type, and (2) expecting the implementation of change_existing_asset_type() to not short-circuit this case.

This works for now because that is actually how change_existing_asset_type() is implemented, but it's probably more future-proof to write this test so that it actually changes the type (that way a future "optimization" or similar won't cause the benchmark to become ineffective).

I wanted to write this benchmark like you described, but I have two associated types (assetId, assetType) that I need to generate and in order to be generic (over the mock runtime and over the proper runtime where this is included) I can only generate the default one. Still I think the benchmarking is correct

One option is to require that AssetType: From<MultiLocation> in the config, and in that case I can generate several. But to be honest the pallet is so simple that I did not even bother requiring this. Let me know if this is something we think we should add

Agreed, I don't think my concern is worth a lot of effort...

pallets/asset-manager/src/tests.rs

notlesh · 2022-01-14T18:27:26Z

pallets/asset-manager/src/weights.rs

 }

 /// Weights for pallet_asset_manager using the Substrate node and recommended hardware.
 pub struct SubstrateWeight<T>(PhantomData<T>);
 impl<T: frame_system::Config> WeightInfo for SubstrateWeight<T> {
 	fn register_asset() -> Weight {
-		(52_572_000 as Weight)
+		(50_651_000 as Weight)


Needs proper benchmark. Let me know when you want me to run this.

I actually run it on the target machine, but feel free to redo it

runtime/moonbase/src/lib.rs

notlesh

I don't understand this well enough to approve it, but I have looked through it. I'll try to go back through it again soon.

notlesh · 2022-01-19T14:54:56Z

pallets/asset-manager/src/lib.rs

+		fn get_asset_id(asset_type: T::AssetType) -> Option<T::AssetId> {
+			AssetTypeId::<T>::get(asset_type)
+		}


This implies a 1:1 mapping between an AssetId and an AssetType (instead of a many-to-one). I previously thought it made sense to support multiple AssetIds mapping to a single AssetType, is this categorically wrong? Or is it not the AssetManagers responsibility to handle this?

I think this is wrong. We need the asset-manager to provide one specific multiLocation when a user says: "I want to transact asset X".

The assetType exists mainly to be able to reference distinct type of tokens in the future. Right now its an enum containing XCM(MultiLocation) but in the future we might want to distinguish between, e.g., fungible and non-fungible tokens, which should probably go into different pallets. But since our approach is to retrieve the Id through a hash, there is low risk of collision

Oh, that's right. This constraint is about communicating back to another chain, right?

pallets/asset-manager/src/lib.rs

pallets/asset-manager/src/tests.rs

notlesh · 2022-01-19T15:41:37Z

runtime/moonbase/tests/xcm_mock/parachain.rs

+				parents: 1,
+				interior: X3(
+					Parachain(id),
+					PalletInstance(StatemintAssetPalletInstance::get()),


This seems safe for now. If Kusama were to change their pallet instance (very unlikely) or add a second instance of Statemint (unlikely), this would do bad things, but we will be able to remove this before then anyway -- right?

Yes, the idea is that we remove this once Statemine has upgraded

notlesh · 2022-01-19T15:50:43Z

runtime/moonriver/src/lib.rs

+			CouncilCollective,
+			TechCommitteeCollective,
+		>,
+		runtime_common::migrations::XcmMigrations<


Just to be pedantic, this means we can order CommonMigrations and XcmMigrations independently, and that all XcmMigrations come after all CommonMigrations.

This is possible because of the impl GetMigrations for Tuple.

I believe this will cause a lot of allocation and copies every time get_migrations() is called, but that shouldn't happen outside of runtime upgrades.

Agree. I think in the near future this will dissappear though, just because xcm is now reaching moonbeam. That means xcm migrations will be moved to common.

pallets/asset-manager/src/migrations.rs

notlesh · 2022-01-19T18:35:41Z

pallets/asset-manager/src/migrations.rs

+pub struct PopulateAssetTypeIdStorage<T>(PhantomData<T>);
+impl<T: Config> OnRuntimeUpgrade for PopulateAssetTypeIdStorage<T> {
+	fn on_runtime_upgrade() -> Weight {
+		log::info!(target: "PopulateAssetTypeIdStorage", "actually running it");


I think log statements like this are fine to leave in, but I might change it so that it will have enough context to make sense in the real world

notlesh · 2022-01-19T18:44:25Z

pallets/asset-manager/src/migrations.rs

+			let location: Option<MultiLocation> = asset_type.into();
+
+			match location {
+				Some(MultiLocation {


You could also throw in an assert here that makes sure the From impl can be used as expected with respect to this conversion

Not entirely sure about what you mean by this. Can you elaborate more?

notlesh

This looks good to me. I have some nits which I'll leave to your discretion. I'm asking for changes over just one thing: the assert in on_runtime_upgrade() as I feel pretty strongly that it should go in post_runtime_upgrade().

…atemine-prefix-breaking-change

crystalin · 2022-01-20T13:19:25Z

moonbeam-types-bundle/package-lock.json

-      "version": "7.16.3",
-      "resolved": "https://registry.npmjs.org/@babel/runtime/-/runtime-7.16.3.tgz",
-      "integrity": "sha512-WBwekcqacdY2e9AF/Q7WLFUWmdJGJTkbjqTjoMDgXkVZ3ZRUvOPsLb5KdwISoQVsbP+DQzVZW4Zhci0DvpbNTQ==",
+      "version": "7.16.7",


Please revert those changes if not intended

The package-lock changes were needed. The reason is that I needed version 6.12.1 to be able to decode and insert a message as if it was coming from statemine (in typescript). I will revisit, but I struggled with this file to be able to do that

crystalin · 2022-01-20T13:24:09Z

pallets/asset-manager/src/lib.rs

 	}

-	/// Stores the asset TYPE
+	/// Stores the asset Type with assetId as key


Suggested change

/// Stores the asset Type with assetId as key

/// Mapping from an asset id to asset type. This is mostly used when receiving transaction specifying an asset directly, like transferring an asset from this chain to another.

(We should provide a bit more explanation for our storage variable... I wrote an exemple but feel free to rewrite matching what correspond the best to it)

crystalin · 2022-01-20T13:26:13Z

pallets/asset-manager/src/lib.rs

 	#[pallet::storage]
 	#[pallet::getter(fn asset_id_type)]
 	pub type AssetIdType<T: Config> = StorageMap<_, Blake2_128Concat, T::AssetId, T::AssetType>;

+	/// Stores the asset Id with Asset Type as key


Suggested change

/// Stores the asset Id with Asset Type as key

/// Reverse mapping of AssetIdType. Mapping from an asset type to an asset id. This is mostly used when receiving a multilocation XCM message to retrieve the corresponding asset.

(another exemple)

will do, thanks alan

crystalin · 2022-01-20T13:27:33Z

pallets/asset-manager/src/migrations.rs

+		// There are no entries in the old storage afterward
+
+		// Assert new storage is empty
+		assert!(AssetTypeUnitsPerSecond::<T>::iter().next().is_none());


Out of curiosity what happens if this is failing? Will it break the chain ?

this is only in pre_upgrade. So this only applies for the try-runtime tests. This does not get executed on_runtime()

girazoki added 4 commits January 10, 2022 10:17

Add statemine like runtime for xcm tests

e170f4c

Be able to receive both prefixes from statemine, while add an extrins…

ef6ae21

…ic to change the sending prefix

Add statemine/statemint tests

32cdb42

add statemine runtime test

a15613b

girazoki marked this pull request as draft January 10, 2022 15:43

refactor

9d44c8f

girazoki added A0-pleasereview Pull request needs code review. D5-nicetohaveaudit⚠️ PR contains trivial changes to logic that should be properly reviewed. B7-runtimenoteworthy Changes should be noted in any runtime-upgrade release notes labels Jan 10, 2022

girazoki added 4 commits January 11, 2022 12:43

More rust and typescript tests

bf39e2e

NPX

aeef2b3

Document tests

306bb0e

editorconfig

6bc863c

girazoki added D9-needsaudit👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited and removed D5-nicetohaveaudit⚠️ PR contains trivial changes to logic that should be properly reviewed. labels Jan 11, 2022

girazoki marked this pull request as ready for review January 11, 2022 14:36

girazoki added 8 commits January 11, 2022 16:00

Merge remote-tracking branch 'upstream/master' into girazoki-avoid-st…

01b0618

…atemine-prefix-breaking-change

Update benchmark

bdeaebf

Update weights

14c2599

Update message

8b4dcbb

Merge remote-tracking branch 'upstream/master' into girazoki-avoid-st…

8aa9b70

…atemine-prefix-breaking-change

Update estamite for assets transfer

c7112a7

Adapt test and package-lock to run all tests correctly

1895e6c

Prettier

723dd97

librelois mentioned this pull request Jan 14, 2022

release runtime-1200 #1179

Closed

32 tasks

notlesh reviewed Jan 14, 2022

View reviewed changes

pallets/asset-manager/src/tests.rs Show resolved Hide resolved

notlesh reviewed Jan 14, 2022

View reviewed changes

runtime/moonbase/src/lib.rs Show resolved Hide resolved

notlesh reviewed Jan 14, 2022

View reviewed changes