Girazoki configurable reserve provider

[girazoki]

This is a proposal to configure the way the reserve part is calculated for a MultiAsset in xtokens. It includes adding an associated type called ReserveProvider, that chains can configure the way they prefer. For instance, while Acala might be willing to keep the absolute view for Self Tokens, while other chains can implement this the way they want, e.g., changing it to a relative view.

This is just a proposal that can be dropped in case the suggestion is not well received. I understand that this implies some changes in the runtimes importing xtokens, but minimal changes to the xtokens code/tests itself

[shaunxw]

I'm not against the direction to make absolute/relative location flexible, though a bit concerned that this might not be able to bring actual benefits for cross-chain transfer abilities, but just to meet different parachains developers' preferences. From code maintenance point of view, I would prefer to keep it simple.

The reserve provider needs more tests coverages. Mocks in xtokens should be updated to include both absolute and relative provider in different parachains.

[girazoki]

I agree it does not bring other benefit than flexibility when configuring xcm in your runtime with xtokens.

From the test perspective, I am tempted if we are willing to explore this idea to add a new parachain runtime where orml_xtokens is constructed with relative provider. This should maintain the tests that the pallet has so far, and add a few more (although the ones added will probably cover only transferring the token of the new para runtime, since this is the case we want to test)

[girazoki]

With respect to offering a public RelativeReserveProvider in orml-traits, I am not that sure. I know there was some hesitancy with respect to the potential issues this could bring with respect to the validity of tokens in #697.

I dont think this is that much of an issue as the xcm transactor would fail in case of inserting a non-existing self location asset, but I might be missing something.

[codecov-commenter]

Codecov Report

Merging #708 (2dd1bd2) into master (fecbe01) will decrease coverage by 0.16%.
The diff coverage is 69.90%.

@@            Coverage Diff             @@
##           master     #708      +/-   ##
==========================================
- Coverage   75.46%   75.30%   -0.17%     
==========================================
  Files          83       84       +1     
  Lines        7476     7673     +197     
==========================================
+ Hits         5642     5778     +136     
- Misses       1834     1895      +61     

Impacted Files	Coverage Δ
xtokens/src/mock/para.rs	75.75% <ø> (ø)
xtokens/src/mock/para_relative_view.rs	18.91% <18.91%> (ø)
xtokens/src/lib.rs	64.48% <50.00%> (-0.21%)	⬇️
xtokens/src/mock/mod.rs	90.32% <85.71%> (+2.82%)	⬆️
traits/src/location.rs	97.22% <96.00%> (-0.93%)	⬇️
xcm-support/src/lib.rs	85.71% <100.00%> (ø)
xcm-support/src/tests.rs	95.00% <100.00%> (ø)
xtokens/src/tests.rs	99.46% <100.00%> (+0.08%)	⬆️

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

[girazoki]

Is this something you guys would consider adding? @shaunxw @xlc

[shaunxw]

LGTM

Could you please also add unit tests to cover the following scenarios?

• Transfer currency D from ParaA to ParaB
• Transfer currency R from ParaA to ParaD

[girazoki]

Absolutely, added!

[xlc]

Needs a resolve conflicts and good to merge @girazoki