here() should be a self-reserve

[stechu]

Under the new re-anchoring logic, it is advised to use here() to be the sender location, in this case, ORML should allow here() to be a self-reserve.

The related test code on Manta:
https://github.com/Manta-Network/Manta/blob/xcm-v4/runtime/dolphin/tests/xcm_tests.rs#L286

[codecov-commenter]

Codecov Report

Merging #697 (7e2e420) into master (4a66b29) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #697      +/-   ##
==========================================
- Coverage   74.63%   74.62%   -0.01%     
==========================================
  Files          83       83              
  Lines        7261     7264       +3     
==========================================
+ Hits         5419     5421       +2     
- Misses       1842     1843       +1     

Impacted Files	Coverage Δ
traits/src/location.rs	98.18% <100.00%> (+0.03%)	⬆️
xtokens/src/lib.rs	57.62% <100.00%> (-0.06%)	⬇️
xtokens/src/tests.rs	99.17% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4a66b29...7e2e420. Read the comment docs.

[xlc]

Can you add some tests here

[zqhxuyuan]

the SelfLocation config is (1, Parachain(id)) could optimized to Here

open-runtime-module-library/xtokens/src/mock/para.rs

Line 268 in 4a66b29

pub SelfLocation: MultiLocation = MultiLocation::new(1, X1(Parachain(ParachainInfo::get().into())));

then Here case could be SelfReserve:

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

Lines 750 to 754 in 4a66b29

	let self_location = T::SelfLocation::get();
	ensure!(dest != self_location, Error::<T>::NotCrossChainTransfer);
	let reserve = asset.reserve().ok_or(Error::<T>::AssetHasNoReserve)?;
	let transfer_kind = if reserve == self_location {

[stechu]

@zqhxuyuan I am not sure changing

 pub SelfLocation: MultiLocation = MultiLocation::new(1, X1(Parachain(ParachainInfo::get().into()))); 

to

 pub SelfLocation: MultiLocation = MultiLocation::here();

is not a super good idea given how does the current tests are written. For example:

pub struct CurrencyIdConvert;
impl Convert<CurrencyId, Option<MultiLocation>> for CurrencyIdConvert {
	fn convert(id: CurrencyId) -> Option<MultiLocation> {
		match id {
			CurrencyId::R => Some(Parent.into()),
			CurrencyId::A => Some((Parent, Parachain(1), GeneralKey("A".into())).into()),
			CurrencyId::A1 => Some((Parent, Parachain(1), GeneralKey("A1".into())).into()),
			CurrencyId::B => Some((Parent, Parachain(2), GeneralKey("B".into())).into()),
			CurrencyId::B1 => Some((Parent, Parachain(2), GeneralKey("B1".into())).into()),
		}
	}
}

The way how to convert CurrencyId to MultiLocation in the test code is converting to an "absolute path". As a result, there is no easy way to make the tests work without significantly changing the test code.

[zqhxuyuan]

@stechu yeah, you're right, there'll be failed testcases if change SelfLocation to Here.
But in your testcase the SelfLocation is set to Here

https://github.com/Manta-Network/Manta/blob/072c7f52349e071ce8db46331d7285b031329fd8/runtime/dolphin/tests/xcm_mock/parachain.rs#L572-L585

in this PR you add another gate to chain_part, so that chain_part equal to self_location

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

Lines 753 to 754 in 4a66b29

	let reserve = asset.reserve().ok_or(Error::<T>::AssetHasNoReserve)?;
	let transfer_kind = if reserve == self_location {

could you tried set SelfLocation to (1, Parachain(id)) in your testcase and validate if still failed?

PS: I've add another testcase with parachain(3), seems it's ok without your PR here.

https://github.com/open-web3-stack/open-runtime-module-library/pull/699/files

[shaunxw]

One of chain_part use case is to check user errors on dest param in xtokens calls.

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

Line 729 in 4a66b29

if let (Some(dest), Some(recipient)) = (dest.chain_part(), dest.non_chain_part()) {

Changes in this PR will make any random dest has valid chain part.

It will also make any multi assets has valid reserve.

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

Line 753 in 4a66b29

let reserve = asset.reserve().ok_or(Error::<T>::AssetHasNoReserve)?;

[stechu]

The core issue here is that the new reanchoring logic encourage a "relative path" point of view of MultiLocation. For example, if the current working parachain ID is 1. Then, the following two MultiLocations should be equivalent:

../Parachain(1)/xxx

xxx

The previous ORML design and tests are leaning towards always using the first case. This is fine, but I think using the second case should be encouraged since it simpler.

I think at end of the day, it really matters what is the opinionated view of how a local asset's location should be represented. In my view (seems also by Parity), your own parachain balance should be:

here()

instead of

../Parachain(your_own_id)

The asset on your own should be

./asset_location

instead of:

../Parachain(your_own_id)/asset_location

PS:

It will also make any multi assets has valid reserve.

Not really, see the updated new test.

[shaunxw]

ORML uses "absolute" location path on purpose. One of the benefit is to be compatible with token location register module we planned. It's a design choice, instead of which point of view we preferred to see assets on our own parachain.

I think using the second case should be encouraged since it simpler.

I don't see how it makes implementation and integration simpler, but it needs to cover more edge cases.

I think at end of the day, it really matters what is the opinionated view of how a local asset's location should be represented. In my view (seems also by Parity), your own parachain balance should be:

XCM executor is designed to be flexible and in the end it's parachains choice to decide how to convert between multi location and their local currency. Not sure about what you mean by Parity's view.

If ORML impl changed to relative location path design, the way how params are used in xtokens would be very different. It's not just how local asset are represent but also foreign locations in dest and assets. It would be a breaking change.

[girazoki]

One way that we could fullfil both requests is to introduce a new pub trait ReserveCalculator like

pub trait ReserveCalculator {
	/// Calculates a reserve location of a multiasset.
	fn calculate_reserve(&MultiAsset) -> Option<MultiLocation>;
}

And add a new associated type in the config of orml_xtokens

/// Reserve Calculator
type ReserveCalculator: ReserveCalculator;

That way xtokens is not tied to a specific way of seeing self tokens (absolute vs relative) and each runtime can configure it indepentently

[zqhxuyuan]

we got Reserve trait already, if we want expose a trait, I think we could use Reserve and make some adjustment.

open-runtime-module-library/traits/src/location.rs

Lines 43 to 46 in 4a66b29

	pub trait Reserve {
	/// Returns assets reserve location.
	fn reserve(&self) -> Option<MultiLocation>;
	}

[girazoki]

That works too

[stechu]

@zqhxuyuan @shaunxw @xlc Thanks for the code review. I think we should reach the consensus of the design decision here first. For example:

1. should MultiLocation::new(0, X1(GeneralKey("DOT".into()))) be treated NoReserve. There is explicit negative test here: https://github.com/open-web3-stack/open-runtime-module-library/blob/master/traits/src/location.rs#L104 . I think under the new re-anchoring logic, this should be treated as self-reserved asset.
2. What is the preferred Asset location for local asset (details see my reply to @shaunxw in the code review).

For example, it is perfectly fine to set SelfLocation in the parachain mock runtime to here() and makes the customized assets works as well. see the manta test here:
https://github.com/Manta-Network/Manta/blob/xcm-v4/runtime/dolphin/tests/xcm_tests.rs#L381

However, it does requires change the test of xtoken quite a bit. The current way of representing local assets need to be revisited. I am happy to add more tests if a consensus on the design decision can be made.