Distinguish when MinXcmFee is not registered and FeeNotEnough

[ghzlatarev]

Signed-off-by: Georgi Zlatarev georgi.zlatarev@manta.network

A few small changes to improve debugging and readability:

• Added a specific error to specify MinXcmFee is not registered, to distinguish between this case and FeeNotEnough
• Refactored MinXcmFee return an Option, so implementers don't have to use u128::Max for unregistered locations.
• Adjusted existing test cases

[xlc]

please use cargo fmt to format

[ghzlatarev]

please use cargo fmt to format

please use cargo fmt to format

should be clean now.

[xlc]

Test is failing

[ghzlatarev]

Btw I have a related test case that I'm not sure if it's an issue in my setup or the xtokens code. The case is the following:
Alice has 100 of ParaB's native token on ParaA and wants to send it back to ParaB, but wants the fee to be paid with 50 of the relay chain asset. The MinXcmFee for ParaB's native token reserve is set to 40 on ParaA.
On the sender side, the fee > min_xcm_fee so we don't hit FeeNotEnough error and both xcm messages get dispatched to ParaB.
Then the first message arrives on ParaB (via the relay chain), and eventually deposits assets - min_xcm_fee ( 50 - 40 = 10 with free execution) of the relay chain asset into ParaA's sovereign account on ParaB.
Then the second message arrives on ParaB and tries to withdraw the rest of the fee (40, which is the min xcm fee) from ParaA's sovereign account on ParaB, but it only has 10, so it fails.

[xlc]

I don't really get it. Can you share the failing test?

[ghzlatarev]

I don't really get it. Can you share the failing test?

Yeah take a look at this test in our repo - https://github.com/Manta-Network/Manta/blob/bac7e321b14d4abcfb2ca1f5809a7e9f0cb8f065/runtime/common/tests/xcm_tests.rs#L2599-L2776.

It's a little long but essentially register ParaA native asset, ParaB native asset, and relay asset on both ParaA and and ParaB. Also registers min_xcm_fee for ParaB on ParaA.
Then Alice sends some of ParaB asset to herself on ParaA and mints some of the relay asset to herself on ParaA.
Then Alice sends some of the ParaB tokens back from ParaA to ParaB, using relay chain asset as fee.

You can see from the asserts on both ParaA and ParaB that things work correctly when fee_amount is 50 and min_xcm_fee is 10. The execution cost on parachains is set up to be 0, and execution cost on relay chain is set up to be 1.

If you change min_xcm_fee to 40, the asserts on ParaA (sender chain) will remain ok, but the asserts on ParaB (receiver) will fail. This is because the ParaA sovereign account received only 10 of the relay chain asset, as we encoded min_xcm_fee as fee_to_dest when we sent the second to-reserve xcm message, so there's not enough to withdraw into Holding and execution stops at the withdraw_asset instruction.

[zqhxuyuan]

I think your testcase is same as:

open-runtime-module-library/xtokens/src/tests.rs

Lines 717 to 778 in 3576772

	fn sending_sibling_asset_to_reserve_sibling_with_relay_fee_not_enough() {
	TestNet::reset();
	ParaA::execute_with(|| {
	assert_ok!(ParaTokens::deposit(CurrencyId::B, &ALICE, 1_000));
	});
	ParaB::execute_with(|| {
	assert_ok!(ParaTokens::deposit(CurrencyId::B, &sibling_a_account(), 1_000));
	});
	Relay::execute_with(|| {
	let _ = RelayBalances::deposit_creating(&para_a_account(), 1_000);
	});
	let fee_amount: u128 = 159;
	let weight: u128 = 50;
	let dest_weight: u128 = 40;
	ParaA::execute_with(|| {
	assert_ok!(ParaXTokens::transfer_multicurrencies(
	Some(ALICE).into(),
	vec![(CurrencyId::B, 450), (CurrencyId::R, fee_amount)],
	1,
	Box::new(
	(
	Parent,
	Parachain(2),
	Junction::AccountId32 {
	network: NetworkId::Any,
	id: BOB.into(),
	},
	)
	.into()
	),
	weight as u64,
	));
	assert_eq!(550, ParaTokens::free_balance(CurrencyId::B, &ALICE));
	assert_eq!(1000 - fee_amount, ParaTokens::free_balance(CurrencyId::R, &ALICE));
	});
	Relay::execute_with(|| {
	assert_eq!(
	1000 - (fee_amount - dest_weight),
	RelayBalances::free_balance(&para_a_account())
	);
	assert_eq!(
	fee_amount - dest_weight * 2,
	RelayBalances::free_balance(&para_b_account())
	);
	});
	ParaB::execute_with(|| {
	// after first xcm succeed, sibling_a amount = 159-120=39
	// second xcm failed, so sibling_a amount stay same.
	assert_eq!(39, ParaTokens::free_balance(CurrencyId::R, &sibling_a_account()));
	// second xcm failed, so recipient account don't receive any token of B and R.
	assert_eq!(0, ParaTokens::free_balance(CurrencyId::B, &BOB));
	assert_eq!(0, ParaTokens::free_balance(CurrencyId::R, &BOB));
	});
	}

sender should make sure using enough relaychain token as fee.

[ghzlatarev]

I think your testcase is same as:

open-runtime-module-library/xtokens/src/tests.rs

Lines 717 to 778 in 3576772

	fn sending_sibling_asset_to_reserve_sibling_with_relay_fee_not_enough() {
	TestNet::reset();
	ParaA::execute_with(|| {
	assert_ok!(ParaTokens::deposit(CurrencyId::B, &ALICE, 1_000));
	});
	ParaB::execute_with(|| {
	assert_ok!(ParaTokens::deposit(CurrencyId::B, &sibling_a_account(), 1_000));
	});
	Relay::execute_with(|| {
	let _ = RelayBalances::deposit_creating(&para_a_account(), 1_000);
	});
	let fee_amount: u128 = 159;
	let weight: u128 = 50;
	let dest_weight: u128 = 40;
	ParaA::execute_with(|| {
	assert_ok!(ParaXTokens::transfer_multicurrencies(
	Some(ALICE).into(),
	vec![(CurrencyId::B, 450), (CurrencyId::R, fee_amount)],
	1,
	Box::new(
	(
	Parent,
	Parachain(2),
	Junction::AccountId32 {
	network: NetworkId::Any,
	id: BOB.into(),
	},
	)
	.into()
	),
	weight as u64,
	));
	assert_eq!(550, ParaTokens::free_balance(CurrencyId::B, &ALICE));
	assert_eq!(1000 - fee_amount, ParaTokens::free_balance(CurrencyId::R, &ALICE));
	});
	Relay::execute_with(|| {
	assert_eq!(
	1000 - (fee_amount - dest_weight),
	RelayBalances::free_balance(&para_a_account())
	);
	assert_eq!(
	fee_amount - dest_weight * 2,
	RelayBalances::free_balance(&para_b_account())
	);
	});
	ParaB::execute_with(|| {
	// after first xcm succeed, sibling_a amount = 159-120=39
	// second xcm failed, so sibling_a amount stay same.
	assert_eq!(39, ParaTokens::free_balance(CurrencyId::R, &sibling_a_account()));
	// second xcm failed, so recipient account don't receive any token of B and R.
	assert_eq!(0, ParaTokens::free_balance(CurrencyId::B, &BOB));
	assert_eq!(0, ParaTokens::free_balance(CurrencyId::R, &BOB));
	});
	}

sender should make sure using enough relaychain token as fee.
Right, that's the same case yeah.
Maybe some of the edge cases can be limited if this check

open-runtime-module-library/xtokens/src/lib.rs

Line 545 in 3576772

ensure!(fee_to_dest < fee, Error::<T>::FeeNotEnough);

is changed to ensure!(fee_to_dest < fee - fee_to_dest , Error::<T>::FeeNotEnough); ?
Even with free execution if this doesn't hold, there's no point continuing.

[ghzlatarev]

Also why do we need to subtract min-xcm-fee here instead of sending the full fee amout to the relay (and rerouted to parachain) https://github.com/open-web3-stack/open-runtime-module-library/blob/master/xtokens/src/lib.rs#L558
Take for example case of sending USDT back to Statemine with free execution everywhere to illustrate the point. After both messages get received, the sender's parachain sovereign account will receive fee_amount - 2 * min_xcm_fee and the receiver account will receive min_xcm_fee , so 1 min_xcm_fee amount seems lost.

[zqhxuyuan]

Also why do we need to subtract min-xcm-fee here instead of sending the full fee amout to the relay (and rerouted to parachain) https://github.com/open-web3-stack/open-runtime-module-library/blob/master/xtokens/src/lib.rs#L558 Take for example case of sending USDT back to Statemine with free execution everywhere to illustrate the point. After both messages get received, the sender's parachain sovereign account will receive fee_amount - 2 * min_xcm_fee and the receiver account will receive min_xcm_fee , so 1 min_xcm_fee amount seems lost.

If we send all fee_amount to relaychain, then there're none left relaychain token used to send direct to Statemine, But Statemine current only support relaychain token to buy execution fee, so this xcm execution will failed.
Current implementation is split into two xcm, first one is (fee_amount-min_xcm_fee) KSM to relaychain then to Statemine, second one is min_xcm_fee KSM & USDT direct to Stemine.
If we send fee_amount KSM to relaychain then to Statemine(first one), then second one is USDT direct to Statemine, it'll failed in Statemine side.

[ghzlatarev]

Also why do we need to subtract min-xcm-fee here instead of sending the full fee amout to the relay (and rerouted to parachain) https://github.com/open-web3-stack/open-runtime-module-library/blob/master/xtokens/src/lib.rs#L558 Take for example case of sending USDT back to Statemine with free execution everywhere to illustrate the point. After both messages get received, the sender's parachain sovereign account will receive fee_amount - 2 * min_xcm_fee and the receiver account will receive min_xcm_fee , so 1 min_xcm_fee amount seems lost.

If we send all fee_amount to relaychain, then there're none left relaychain token used to send direct to Statemine, But Statemine current only support relaychain token to buy execution fee, so this xcm execution will failed. Current implementation is split into two xcm, first one is (fee_amount-min_xcm_fee) KSM to relaychain then to Statemine, second one is min_xcm_fee KSM & USDT direct to Stemine. If we send fee_amount KSM to relaychain then to Statemine(first one), then second one is USDT direct to Statemine, it'll failed in Statemine side.

Yes, of-course, thanks for re-explaining the code. For a minute there I though that assets_to_dest did not include the fee_to_dest and that it was magically being withdrawn into the holding register of Statemine without being included in the WithdrawAsset instruction... :D