Asset conversion nits #2860

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 Outdated 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
4 checks passed

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Asset conversion nits #2860

Asset conversion nits #2860

bkontur commented Jul 13, 2023 •

edited

Loading

joepetrowski left a comment

bkontur commented Jul 14, 2023

command-bot bot commented Jul 14, 2023 •

edited

Loading

command-bot bot commented Jul 14, 2023

command-bot bot commented Jul 17, 2023 •

edited

Loading

command-bot bot commented Jul 17, 2023

joepetrowski left a comment

bkontur commented Jul 19, 2023 •

edited

Loading

bkontur commented Jul 19, 2023

bkontur commented Jul 21, 2023

paritytech-processbot bot commented Jul 21, 2023

paritytech-processbot bot commented Jul 21, 2023

Asset conversion nits #2860

Asset conversion nits #2860

Conversation

bkontur commented Jul 13, 2023 • edited Loading

joepetrowski left a comment

Choose a reason for hiding this comment

bkontur commented Jul 14, 2023

command-bot bot commented Jul 14, 2023 • edited Loading

command-bot bot commented Jul 14, 2023

command-bot bot commented Jul 17, 2023 • edited Loading

command-bot bot commented Jul 17, 2023

joepetrowski left a comment

Choose a reason for hiding this comment

bkontur commented Jul 19, 2023 • edited Loading

bkontur commented Jul 19, 2023

bkontur commented Jul 21, 2023

paritytech-processbot bot commented Jul 21, 2023

paritytech-processbot bot commented Jul 21, 2023

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