Asset conversion nits by bkontur · Pull Request #2860 · paritytech/cumulus

bkontur · 2023-07-13T09:56:39Z

PR:

fixes benchmarks for pallet_asset_conversion - create_pool error
small refactor to reuse conversion stuff for assetId vs MultiLocation
Allow set_storage for AllowMultiAssetPools / LiquidityWithdrawalFee
small nits

TODOs

native asset - migration or storage set for westend - Asset conversion nits #2860 (review)
set correct weight instead pallet_asset_conversion::Config -> type WeightInfo = (); when benchmarks start to work
(can be separate PR) fix benchmarks - [asset-conversion] Fix benchmarks swap_exact_tokens_for_tokens / swap_tokens_for_exact_tokens #2874
~~change AssetConversionApi to use VersionedMultiLocation~~ continues here [asset-conversion] Runtime api AssetConversionApi for AssetHubs uses Box<MultiLocation> for public interface polkadot-sdk#1129

joepetrowski

We already have some pools with parents: 0. I don't mind nuking them on Westend, but could you either provide a migration or a set_storage call to sudo?

…_assets in pool

…ound it)

bkontur · 2023-07-14T23:50:56Z

bot fmt

command-bot · 2023-07-14T23:51:01Z

@bkontur https://gitlab.parity.io/parity/mirrors/cumulus/-/jobs/3194070 was started for your command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh". Check out https://gitlab.parity.io/parity/mirrors/cumulus/-/pipelines?page=1&scope=all&username=group_605_bot to know what else is being executed currently.

Comment bot cancel 1-4748247b-48e4-4946-a07c-2569cc0cff3a to cancel this command or bot cancel to cancel all commands in this pull request.

command-bot · 2023-07-14T23:51:41Z

@bkontur Command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh" has finished. Result: https://gitlab.parity.io/parity/mirrors/cumulus/-/jobs/3194070 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/cumulus/-/jobs/3194070/artifacts/download.

… `MultiLocation { parents: 1, interior: Here }`

command-bot · 2023-07-17T15:24:28Z

@bkontur https://gitlab.parity.io/parity/mirrors/cumulus/-/jobs/3203923 was started for your command "$PIPELINE_SCRIPTS_DIR/commands/bench/bench.sh" pallet dev pallet_asset_conversion. Check out https://gitlab.parity.io/parity/mirrors/cumulus/-/pipelines?page=1&scope=all&username=group_605_bot to know what else is being executed currently.

Comment bot cancel 10-91ba7436-8498-4481-a1a3-3f9de0540048 to cancel this command or bot cancel to cancel all commands in this pull request.

command-bot · 2023-07-17T15:26:14Z

@bkontur Command "$PIPELINE_SCRIPTS_DIR/commands/bench/bench.sh" pallet dev pallet_asset_conversion has finished. Result: https://gitlab.parity.io/parity/mirrors/cumulus/-/jobs/3203923 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/cumulus/-/jobs/3203923/artifacts/download.

…Fee`

joepetrowski

Looks OK, @jsidorenko can check for the migration especially?

parachains/runtimes/assets/common/src/local_and_foreign_assets.rs

parachains/runtimes/assets/asset-hub-westend/src/lib.rs

bkontur · 2023-07-19T07:14:20Z

Looks OK, @jsidorenko can check for the migration especially?

for that migration, I have a working test:

pub use asset_hub_westend_runtime::{AssetConversion, PoolAssets};

#[test]
fn test_migration() {
	use frame_support::traits::{
		fungible::Inspect, fungibles::roles::Inspect as InspectRoles, OnRuntimeUpgrade,
	};

	let account = AccountId::from(ALICE);
	let existential_deposit = ExistentialDeposit::get();

	ExtBuilder::<Runtime>::default()
		.with_collators(vec![AccountId::from(ALICE)])
		.with_session_keys(vec![(
			AccountId::from(ALICE),
			AccountId::from(ALICE),
			SessionKeys { aura: AuraId::from(sp_core::sr25519::Public::from_raw(ALICE)) },
		)])
		.with_balances(vec![
			(account.clone(), existential_deposit * AssetDeposit::get() * 100),
		])
		.build()
		.execute_with(|| {
			const ASSET_ID: u32 = 1;
			let asset_native = Box::new(MultiLocation::new(0, Here));
			let asset_one = Box::new(MultiLocation {
				parents: 0,
				interior: X2(PalletInstance(50), GeneralIndex(ASSET_ID.into())),
			});
			assert_ok!(Assets::create(
				RuntimeHelper::<Runtime>::origin_of(account.clone()),
				ASSET_ID.into(),
				account.clone().into(),
				1000,
			));
			assert_ok!(Assets::mint(
				RuntimeHelper::<Runtime>::origin_of(account.clone()),
				ASSET_ID.into(),
				account.clone().into(),
				3_000_000_000_000,
			));
			assert_ok!(AssetConversion::create_pool(
				RuntimeHelper::<Runtime>::origin_of(account.clone()),
				asset_native.clone(),
				asset_one.clone(),
			));
			assert_ok!(AssetConversion::add_liquidity(
				RuntimeHelper::<Runtime>::origin_of(account.clone()),
				asset_native.clone(),
				asset_one.clone(),
				1_000_000_000_000,
				2_000_000_000_000,
				0,
				0,
				account.into()
			));

			let lp_token = PoolAssets::asset_ids().next().unwrap();

			let old_pool_id = AssetConversion::get_pool_id(asset_native.clone(), asset_one.clone());
			let old_pool_account = AssetConversion::get_pool_account(&old_pool_id);
			let new_pool_id = AssetConversion::get_pool_id(Box::new(WestendLocation::get()), asset_one.clone());
			let new_pool_account = AssetConversion::get_pool_account(&new_pool_id);

			assert_ne!(0, Balances::balance(&old_pool_account));
			assert_eq!(0, Balances::balance(&new_pool_account));
			assert_ne!(0, Assets::balance(ASSET_ID, old_pool_account.clone()));
			assert_eq!(0, Assets::balance(ASSET_ID, new_pool_account.clone()));
			assert_ne!(0, PoolAssets::balance(lp_token, old_pool_account.clone()));
			assert_eq!(0, PoolAssets::balance(lp_token, new_pool_account.clone()));
			assert_eq!(Some(old_pool_account.clone()), PoolAssets::owner(lp_token));
                        assert!(pallet_asset_conversion::Pools::<Runtime>::contains_key(&old_pool_id));
			assert!(!pallet_asset_conversion::Pools::<Runtime>::contains_key(&new_pool_id));
			println!("before: {:?}", PoolAssets::owner(lp_token));

			asset_hub_westend_runtime::migrations::NativeAssetParents0ToParents1Migration::<Runtime>::on_runtime_upgrade();

			assert_eq!(0, Balances::balance(&old_pool_account));
			assert_ne!(0, Balances::balance(&new_pool_account));
			assert_eq!(0, Assets::balance(ASSET_ID, old_pool_account.clone()));
			assert_ne!(0, Assets::balance(ASSET_ID, new_pool_account.clone()));
			assert_eq!(0, PoolAssets::balance(lp_token, old_pool_account.clone()));
			assert_ne!(0, PoolAssets::balance(lp_token, new_pool_account.clone()));
			assert_eq!(Some(new_pool_account), PoolAssets::owner(lp_token));
                        assert!(!pallet_asset_conversion::Pools::<Runtime>::contains_key(&old_pool_id));
			assert!(pallet_asset_conversion::Pools::<Runtime>::contains_key(&new_pool_id));
			println!("after: {:?}", PoolAssets::owner(lp_token));
		})
}

but it only works with this hardcoded hack, so I better not pushed it

	fn is_native(asset_id: &Box<MultiLocation>) -> bool {
		*asset_id == Self::get_native() ||
                        // THIS IS HACK FOR MIGRATION
			*asset_id == Box::new(MultiLocation::new(0, xcm::latest::prelude::Here))
	}

bkontur · 2023-07-19T07:25:36Z

Looks OK, @jsidorenko can check for the migration especially?

for that migration, I have a working test:

pub use asset_hub_westend_runtime::{AssetConversion, PoolAssets};

#[test]
fn test_migration() {
	use frame_support::traits::{
		fungible::Inspect, fungibles::roles::Inspect as InspectRoles, OnRuntimeUpgrade,
	};

	let account = AccountId::from(ALICE);
	let existential_deposit = ExistentialDeposit::get();

	ExtBuilder::<Runtime>::default()
		.with_collators(vec![AccountId::from(ALICE)])
		.with_session_keys(vec![(
			AccountId::from(ALICE),
			AccountId::from(ALICE),
			SessionKeys { aura: AuraId::from(sp_core::sr25519::Public::from_raw(ALICE)) },
		)])
		.with_balances(vec![
			(account.clone(), existential_deposit * AssetDeposit::get() * 100),
		])
		.build()
		.execute_with(|| {
			const ASSET_ID: u32 = 1;
			let asset_native = Box::new(MultiLocation::new(0, Here));
			let asset_one = Box::new(MultiLocation {
				parents: 0,
				interior: X2(PalletInstance(50), GeneralIndex(ASSET_ID.into())),
			});
			assert_ok!(Assets::create(
				RuntimeHelper::<Runtime>::origin_of(account.clone()),
				ASSET_ID.into(),
				account.clone().into(),
				1000,
			));
			assert_ok!(Assets::mint(
				RuntimeHelper::<Runtime>::origin_of(account.clone()),
				ASSET_ID.into(),
				account.clone().into(),
				3_000_000_000_000,
			));
			assert_ok!(AssetConversion::create_pool(
				RuntimeHelper::<Runtime>::origin_of(account.clone()),
				asset_native.clone(),
				asset_one.clone(),
			));
			assert_ok!(AssetConversion::add_liquidity(
				RuntimeHelper::<Runtime>::origin_of(account.clone()),
				asset_native.clone(),
				asset_one.clone(),
				1_000_000_000_000,
				2_000_000_000_000,
				0,
				0,
				account.into()
			));

			let lp_token = PoolAssets::asset_ids().next().unwrap();

			let old_pool_id = AssetConversion::get_pool_id(asset_native.clone(), asset_one.clone());
			let old_pool_account = AssetConversion::get_pool_account(&old_pool_id);
			let new_pool_id = AssetConversion::get_pool_id(Box::new(WestendLocation::get()), asset_one.clone());
			let new_pool_account = AssetConversion::get_pool_account(&new_pool_id);

			assert_ne!(0, Balances::balance(&old_pool_account));
			assert_eq!(0, Balances::balance(&new_pool_account));
			assert_ne!(0, Assets::balance(ASSET_ID, old_pool_account.clone()));
			assert_eq!(0, Assets::balance(ASSET_ID, new_pool_account.clone()));
			assert_ne!(0, PoolAssets::balance(lp_token, old_pool_account.clone()));
			assert_eq!(0, PoolAssets::balance(lp_token, new_pool_account.clone()));
			assert_eq!(Some(old_pool_account.clone()), PoolAssets::owner(lp_token));
			println!("before: {:?}", PoolAssets::owner(lp_token));

			asset_hub_westend_runtime::migrations::NativeAssetParents0ToParents1Migration::<Runtime>::on_runtime_upgrade();

			assert_eq!(0, Balances::balance(&old_pool_account));
			assert_ne!(0, Balances::balance(&new_pool_account));
			assert_eq!(0, Assets::balance(ASSET_ID, old_pool_account.clone()));
			assert_ne!(0, Assets::balance(ASSET_ID, new_pool_account.clone()));
			assert_eq!(0, PoolAssets::balance(lp_token, old_pool_account.clone()));
			assert_ne!(0, PoolAssets::balance(lp_token, new_pool_account.clone()));
			assert_eq!(Some(new_pool_account), PoolAssets::owner(lp_token));
			println!("after: {:?}", PoolAssets::owner(lp_token));
		})
}

but it only works with this hardcoded hack, so I better not pushed it

	fn is_native(asset_id: &Box<MultiLocation>) -> bool {
		*asset_id == Self::get_native() ||
                        // THIS IS HACK FOR MIGRATION
			*asset_id == Box::new(MultiLocation::new(0, xcm::latest::prelude::Here))
	}

actually, the migration misses the real pool_id change, omg... I am on it

parachains/runtimes/assets/asset-hub-westend/src/lib.rs

bkontur · 2023-07-21T11:44:15Z

bot rebase

paritytech-processbot · 2023-07-21T11:44:26Z

Rebased

paritytech-processbot · 2023-07-21T13:08:15Z

Merge cancelled due to error. Error: Command 'Command { std: cd "/storage/repositories/cumulus" && "git" "push" "paritytech" "bko-asset-conv", kill_on_drop: false }' failed with status Some(128); output: remote: Internal Server Error
fatal: unable to access 'https://github.com/paritytech/cumulus.git/': The requested URL returned error: 500

bkontur added 2 commits July 13, 2023 11:55

[asset-conv] Unused Balances type

d17f3ce

[asset-conv] native asset change

3ec9dd4

joepetrowski approved these changes Jul 13, 2023

View reviewed changes

bkontur mentioned this pull request Jul 13, 2023

Asset conversion get_pool_id fix (Ord does not count with is_native flag) paritytech/substrate#14572

Merged

bkontur added 2 commits July 14, 2023 11:16

Dedicated AssetBalance type for pallet_assets instances

a249fb4

Merge remote-tracking branch 'origin/master' into bko-asset-conv

fefbed3

bkontur added B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. labels Jul 14, 2023

bkontur added 6 commits July 14, 2023 17:15

Improved local vs foreign asset handling + test for not allowing pool…

691e6df

…_assets in pool

Removed into_multiasset_id

a6e428d

Fix

6636b6b

Refactor

5e404f3

Fixed create_pool for benchmark with LocalAndForeignAssets (finally f…

b8f86cf

…ound it)

Merge remote-tracking branch 'origin/master' into bko-asset-conv

757a07b

".git/.scripts/commands/fmt/fmt.sh"

f2d5086

bkontur added 4 commits July 15, 2023 02:02

Revert

3726ab8

Merge remote-tracking branch 'origin/bko-asset-conv' into bko-asset-conv

c86ec4e

fmt

7733ee3

Migrates pools with MultiLocation { parents: 0, interior: Here } to…

02890dd

… `MultiLocation { parents: 1, interior: Here }`

joepetrowski mentioned this pull request Aug 24, 2023

Asset Conversion on Rococo/Kusama paritytech/polkadot-sdk#102

Closed

4 tasks

Allow set_storage for AllowMultiAssetPools / `LiquidityWithdrawal…

ac8accb

…Fee`

joepetrowski mentioned this pull request Jul 17, 2023

Asset Conversion Roadmap paritytech/roadmap#33

Open

bkontur added 3 commits July 18, 2023 14:25

Merge remote-tracking branch 'origin/master' into bko-asset-conv

636c6ed

Benchmarks work

4ed8483

Merge remote-tracking branch 'origin/master' into bko-asset-conv

899590f

bkontur marked this pull request as ready for review July 18, 2023 20:34

bkontur added the A0-please_review Pull request needs code review. label Jul 18, 2023

joepetrowski reviewed Jul 19, 2023

View reviewed changes

parachains/runtimes/assets/common/src/local_and_foreign_assets.rs Show resolved Hide resolved

parachains/runtimes/assets/asset-hub-westend/src/lib.rs Outdated Show resolved Hide resolved

parachains/runtimes/assets/asset-hub-westend/src/lib.rs Outdated Show resolved Hide resolved

bkontur added 2 commits July 19, 2023 09:10

Removed comment + more defensive migration

6964a27

T::Currency::transfer -> Balances::transfer_all in migration

c01e7d7

Change pool_id in migration

73b4521

jsidorenko reviewed Jul 19, 2023

View reviewed changes

parachains/runtimes/assets/asset-hub-westend/src/lib.rs Outdated Show resolved Hide resolved

Update parachains/runtimes/assets/asset-hub-westend/src/lib.rs

a301fda

jsidorenko mentioned this pull request Jul 19, 2023

Add AssetConversionApi polkadot-js/api#5694

Merged

gilescope approved these changes Jul 20, 2023

View reviewed changes

Merge remote-tracking branch 'origin/master' into bko-asset-conv

45af713

bkontur added 3 commits July 21, 2023 16:09

Merge remote-tracking branch 'origin/master' into bko-asset-conv

8c6275c

Bump substrate (manually)

7fb4784

Bump polkadot

b8f0892

bkontur mentioned this pull request Jul 21, 2023

Change Fixed to WeightInfoBounds for Polkadot paritytech/polkadot#7077

Merged

Fixes from polkadot + clippy

d2f6b85

paritytech-ci requested a review from a team July 21, 2023 14:46

Fix for xcm-emulator tests (thank you Nacho)

d8bcb0b

bkchr approved these changes Jul 21, 2023

View reviewed changes

bkchr merged commit 1ac85ed into master Jul 21, 2023

bkchr deleted the bko-asset-conv branch July 21, 2023 21:53

Conversation

bkontur commented Jul 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joepetrowski left a comment

Choose a reason for hiding this comment

Uh oh!

bkontur commented Jul 14, 2023

Uh oh!

command-bot bot commented Jul 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

command-bot bot commented Jul 14, 2023

Uh oh!

command-bot bot commented Jul 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

command-bot bot commented Jul 17, 2023

Uh oh!

joepetrowski left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bkontur commented Jul 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bkontur commented Jul 19, 2023

Uh oh!

Uh oh!

bkontur commented Jul 21, 2023

Uh oh!

paritytech-processbot bot commented Jul 21, 2023

Uh oh!

paritytech-processbot bot commented Jul 21, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

bkontur commented Jul 13, 2023 •

edited

Loading

command-bot bot commented Jul 14, 2023 •

edited

Loading

command-bot bot commented Jul 17, 2023 •

edited

Loading

bkontur commented Jul 19, 2023 •

edited

Loading