support asset transfer from/to sibling chain #806

yrong · 2021-11-29T03:08:35Z

This PR adds the AsPrefixedGeneralIndexFromLocalOrRemote conversion type to allow accepting assets that were moved to another chain via reserve based transfers back onto Statemint (and friends).

yrong · 2021-11-29T03:30:52Z

@apopiak @KiChjang

not sure it is the proper way, AFAIK the current asset<->multilocation convert in statemine runtime only allows to transfer asset from local and this pr is for from remote parachain with reserve location as X2(parachain:1000,GeneralIndex:asset_id)

KiChjang · 2021-11-29T21:45:11Z

Thanks for your PR! Unfortunately, this is not the right place to add a new item to configure XCM. I believe the right place would be to add it in Polkadot's repository, under xcm/xcm-builder.

Aside from that, I'm not quite sure what this PR is trying to solve. Can you please elaborate the background of your problem and why your solution would solve it? I don't understand anything about this need for a GeneralIndex for example.

yrong · 2021-11-30T02:32:05Z

@KiChjang The requirement is simple, we would like to transfer asset issued in statemine (e.g:usdt) to our chain and then transfer it back. With this pr and some adaptor logic in our chain I can achive that.

The GeneralIndex is to encode asset_id as multiLocation now adopted in statemine runtime.

Btw, with limitedReserveTransferAssets to transfer to our chain

and wrap xcm as following to transfer asset back
https://github.com/bifrost-finance/bifrost/blob/c7eb1bef7abfdeb6562aeda29ab76805308b77f5/pallets/salp/src/lib.rs?_pjax=%23js-repo-pjax-container%2C%20div%5Bitemtype%3D%22http%3A%2F%2Fschema.org%2FSoftwareSourceCode%22%5D%20main%2C%20%5Bdata-pjax-container%5D#L1014-L1031

KiChjang · 2021-11-30T03:40:15Z

So if I'm hearing you correctly, you're saying that the GeneralIndex is used to specify which one of the many assets in statemine that you want to transfer to the destination chain via reserve transfers? In other words, your flow is exactly the same as InititateReserveWithdraw, but with the ability to specify an asset that is not KSM, correct?

If so, then there may need to be a bit more thought into how the MultiLocation of a reserve chain asset should be represented. Right now you have Parachain(para_id)/GeneralIndex(index), which is rather imprecise -- a GeneralIndex can be interpreted in many different ways. To reduce the ambiguity, I propose inserting a PalletIndex(assets_pallet_index) in between, so that the whole path becomes Parachain(para_id)/PalletIndex(assets_pallet_index)/GeneralIndex(index), so that there's even less of a chance to misinterpret what a GeneralIndex means.

yrong · 2021-11-30T03:59:53Z

So if I'm hearing you correctly, you're saying that the GeneralIndex is used to specify which one of the many assets in statemine that you want to transfer to the destination chain via reserve transfers? In other words, your flow is exactly the same as InititateReserveWithdraw, but with the ability to specify an asset that is not KSM, correct?

Exactly

If so, then there may need to be a bit more thought into how the MultiLocation of a reserve chain asset should be represented. Right now you have Parachain(para_id)/GeneralIndex(index), which is rather imprecise -- a GeneralIndex can be interpreted in many different ways. To reduce the ambiguity, I propose inserting a PalletIndex(assets_pallet_index) in between, so that the whole path becomes Parachain(para_id)/PalletIndex(assets_pallet_index)/GeneralIndex(index), so that there's even less of a chance to misinterpret what a GeneralIndex means.

seems you want to add assets_pallet_index in between, I'am not sure fully understand your concern here. AFAIK statemine is a common good chain focus in asset management and GeneralIndex is the only place we use in statemine to reprensent asset. IMHO we may not need another level for pallet index here.

I believe the right place would be to add it in Polkadot's repository, under xcm/xcm-builder.

Agree

KiChjang · 2021-11-30T07:23:03Z

@yrong If for whatever reason, 1000 is suddenly not the parachain ID for statemine, due to it being reorganized or that you're in fact on a relay chain that has a different architecture (or maybe simply a typo), and Parachain(1000) also happens to interpret GeneralIndex(index) in a way that's different from what you'd expect, then you're in trouble because now you don't actually know what the XCM is doing.

Moreover, it's very not obvious what a GeneralIndex does on a parachain. With a prefixed path that contains a PalletIndex, at least you have an idea that the GeneralIndex is meant to be interpreted by the pallet for an operation that's specific to the pallet (the pallet is also versioned, which helps a lot). With a GeneralIndex that follows a Parachain, what sort of useful information can you gather from it? That basically every GeneralIndex should only be interpreted according to which Parachain it was sent to? So when the parachain decides to change its "API" for this MultiLocation, it can just do so without notifying its consumers?

apopiak · 2021-11-30T10:16:26Z

When I set up the asset XCM integration I figured that we need a unique asset identifier within Statemint, so I used the GeneralIndex. When receiving an asset identified by a GeneralIndex Statemint will interpret it as a local asset that it has sovereignty/control over.
Now, I agree that we might want to allow the transaction of other assets in the future, so we will need to define how to identify them and then a PalletIndex or originating Parachain id might come in handy.

@yrong My question to you is: What keeps you from just sending a GeneralIndex as the asset's id? Left a corresponding comment here: https://github.com/bifrost-finance/bifrost/pull/420/files#r759127339

Edit: Context is that the AsPrefixedGeneralIndex is already live in Statemine so I would want a good argument why it needs to be changed.

yrong · 2021-11-30T16:14:58Z

When I set up the asset XCM integration I figured that we need a unique asset identifier within Statemint, so I used the GeneralIndex. When receiving an asset identified by a GeneralIndex Statemint will interpret it as a local asset that it has sovereignty/control over. Now, I agree that we might want to allow the transaction of other assets in the future, so we will need to define how to identify them and then a PalletIndex or originating Parachain id might come in handy.

@yrong My question to you is: What keeps you from just sending a GeneralIndex as the asset's id? Left a corresponding comment here: https://github.com/bifrost-finance/bifrost/pull/420/files#r759127339

Edit: Context is that the AsPrefixedGeneralIndex is already live in Statemine so I would want a good argument why it needs to be changed.

@apopiak some clarification here bifrost-io/bifrost#420 (comment)

apopiak · 2021-12-01T15:12:23Z

Alright, seems like the reanchoring logic in the executor means we should merge (something like) this.

polkadot-parachains/westmint/src/lib.rs

apopiak

Thank you for the contribution from my side as well, btw. 🙏

Here are the adjustments I'd like to see:

Please move this into parachains-common. (Statemint and Statemine will have to use the same.)
This also should have a few tests:
- matching the local prefix
- matching the remote prefix
- the failure cases
Please add docs
Also you skipped the prefix checking from the original here:
https://github.com/paritytech/polkadot/blob/55872676fae63399eb111a60ba27cb735cbaead6/xcm/xcm-builder/src/fungibles_adapter.rs#L41-L46
You should be able to use match_and_split instead, though: https://github.com/paritytech/polkadot/blob/cc24fb87b702db6510d08c689e442b6be384d798/xcm/src/v1/multilocation.rs#L249
We should probably also add (something like) this to the xcm-builder as @KiChjang suggests. (Would be great if you could open that PR as well to upstream this.)

yrong · 2021-12-02T14:35:41Z

@apopiak some refact per your request list. For the xcm-builder upstream refact I would like to do later and we're looking forward this pr merged into polkadot-v0.9.13 release

polkadot-parachains/parachains-common/src/impls.rs

apopiak · 2021-12-02T16:17:10Z

Added a basic description. Feel free to expand it.

apopiak · 2021-12-02T16:17:40Z

I'd like someone else to also have a look before merging this.

gavofyork · 2021-12-02T20:02:18Z

Why is this against 0.9.13? Surely it should be going into master?

gavofyork · 2021-12-02T20:18:42Z

polkadot-parachains/parachains-common/src/impls.rs

@@ -113,6 +114,44 @@ impl<T: Get<MultiLocation>> FilterAssetLocation for AssetsFrom<T> {
 	}
 }

+/// Converter struct with two main usage scenarios:


Documentation needs improvement. This is incomprehensible at present.

gavofyork · 2021-12-02T20:18:54Z

polkadot-parachains/parachains-common/src/impls.rs

+/// 	- with `reverse_ref` to convert a numeric asset ID (must be `TryFrom/TryInto<u128>`) into a `GeneralIndex` junction
+/// 2. Transfer asset back from other para-chain to local
+/// 	- with `convert_ref` to convert multilocation struct `(1,X2(ParaChain(para_id),GeneralIndex(general_index))` to a numeric asset ID
+pub struct AsPrefixedGeneralIndexFromLocalOrRemote<


Wrong place for this - it should be in xcm-builder.

gavofyork

I remain yet to be convinced that this is needed or even a good idea.

gavofyork · 2021-12-02T20:20:51Z

polkadot-parachains/statemint/src/lib.rs

@@ -481,7 +483,7 @@ pub type FungiblesTransactor = FungiblesAdapter<
 	ConvertedConcreteAssetId<
 		AssetId,
 		Balance,
-		AsPrefixedGeneralIndex<Local, AssetId, JustTry>,
+		AsPrefixedGeneralIndexFromLocalOrRemote<Local, Remote, AssetId, JustTry>,


No alterations to Statemint's XCM right now. Stuff like this is EXTREMELY dangerous to change and must a) be completely understood by me; and b) be in place in Kusama for some time prior.

KiChjang · 2021-12-03T00:12:53Z

polkadot-parachains/parachains-common/src/impls.rs

+				&remote_asset_location,
+			)
+			.unwrap_or(u32::default());
+		assert_eq!(asset_id, 42);


Previously I have tried to summarize your problem as the following:

if I'm hearing you correctly, you're saying that the GeneralIndex is used to specify which one of the many assets in statemine that you want to transfer to the destination chain via reserve transfers? In other words, your flow is exactly the same as InititateReserveWithdraw, but with the ability to specify an asset that is not KSM, correct?

But this unit test seems to indicate that you're doing something more -- you're trying to represent a remote asset containing a GeneralIndex locally as a GeneralIndex as well, e.g. a ../Parachain(1000)/GeneralIndex(42) gets converted and represented as ./GeneralIndex(42). Is this what you are trying to do?

If so, then this is absolutely not the right thing to do. Assets should always have ONE canonical MultiLocation to represent it, and as such, an asset located at ../Parachain(1000)/GeneralIndex(42) MUST be interpreted as a different asset from an asset located at ./GeneralIndex(42). If the intention is to mint a derivative asset of the one located at ../Parachain(1000)/GeneralIndex(42), then it is the AssetTransactor logic of your chain that needs to change, and not the XCM configuration, and definitely not the MultiLocation of the reserve asset.

Pondering a bit more into this, I think what you are trying to solve is the reanchoring issue, in which case, this is an operation that should have been done by the XCM sender, and not the recipient, i.e. when sending an asset containing a AssetId of ../Parachain(1000)/GeneralIndex(42) to ../Parachain(1000), the XCM executor should have reanchored this AssetId to ./GeneralIndex(42) on the sender's chain. Did this not happen when you're testing?

@KiChjang @apopiak I do not think we can achive that without hack in cumulus, let us put more details here:

transfer asset from statemine to our chain with TransferReserveAsset

since Local defined here original asset sent from statemine should be represented as ./GeneralIndex(42) and after the reanchor logic you mentioned above asset will be converted to ../Parachain(1000)/GeneralIndex(42) and in our chain we should add some adaptor logic to handle this

transfer asset back from our chain to statemine with InitiateReserveWithdraw

I agree with

Assets should always have ONE canonical MultiLocation to represent

so I think from the perspective of our chain statemine asset should always reprensented as ../Parachain(1000)/GeneralIndex(42) but after the reanchor logic here we can not convert it back to ./GeneralIndex(42) anyway so statemine will not understand without hack

A couple of observations:

What you interact on your local chain should always be ./GeneralIndex(42) and not ../Parachain(1000)/GeneralIndex(42), i.e. you interact with the derivative asset and never the reserve asset. Therefore when you do a reserve asset transfer, you will first execute a WithdrawAsset(GeneralIndex(42)) and you will get some GeneralIndex(42) stored in your chain's holding register.

The next step in the process is InitiateReserveWithdraw, which takes the derivative asset in the holding register and sends it back to Statemine. Before that happens though, what should happen on your location chain is that it should burn and convert the derivative asset (i.e. GeneralIndex(42)) to the reserve asset (i.e. ../Parachain(1000)/GeneralIndex(42)).

The receiver should never be responsible for converting between derivative asset IDs and reserve asset IDs. Doing so would mean that all derivative assets should "register" themselves on the reserve chain, which is absolutely not the direction we want to head towards.

I've looked into the code for the XCM executor now and it does seem like there's no way to burn and convert a derivative asset back to its reserve asset during the execution of InitiateReserveWithdraw, and I think that is the crux of the issue here.

@KiChjang

What you interact on your local chain should always be ./GeneralIndex(42) and not ../Parachain(1000)/GeneralIndex(42) i.e. you interact with the derivative asset and never the reserve asset.

No, I do think we should use reserve asset rather than derivative asset here. except for statemine we need to talk to other parachains like Acala and as you mentioned before only the GeneralIndex is too abstract. How to differ the same reprensentation ./GeneralIndex(42) in acala and statemine but with different meaning?

That could easily be solved if you prepend an ExchangeAsset instruction before you execute InitiateReserveWithdraw, and in ExchangeAsset, your custom XCM executor will convert your derivative asset back to the reserve asset.

@KiChjang I really do not know this primitive and will try, thanks for your suggestion

@KiChjang
seems ExchangeAsset you mentioned not implemented yet, is it in some feature branch? And I would like to know if there is any other solution or implementation other parachain team already solved as you mentioned?

It is purposefully not implemented; as I said, we do not provide any opinionated approaches as to how a chain should manage their bookkeeping of their reserve assets. Any chain that wants to execute ExchangeAsset will have to provide their own implementation. I'm not as familiar as to how other parachain teams in the ecosystem solves this problem, however, but I would think that any parachain that has onboarded to Kusama and has dealt with reserve assets would have come up with a solution dealing with it.

Understood. we can implement this to do some asset convert logic from sender side. But I would assume at last we still need InitiateReserveWithdraw primitive to send asset back to statemine,right? From what I've tested after the renchor logic in InitiateReserveWithdraw here asset will still be converted to ../Parachain(1000)/GeneralIndex(42) no matter what it previously is. And this format can not be recognized by statemine without hack which only knows ./GeneralIndex(42).Correct me if I'm wrong

Correct, the reanchoring logic is what we're in the middle of investigating as well. However, in the context of this PR, your changes will unfortunately not be the correct fix. We will provide an update in #827 as soon as we have any more details to share.

gavofyork · 2021-12-04T21:31:15Z

#827 (comment)

As I mention here, the primitives in the XCM builder expect that the derivative asset (on your chain, which represented ownership of the reserve assets on Statemint) should generally be represented in XCM as the same conceptual location as the reserve asset.

So, generally this will mean it is GeneralIndex(42) on Statemint and (Parent, Parachain(1000), GeneralIndex(42)) on your parachain.

Reanchoring should ensure that the former MultiLocation value (used on Statemint) gets changed into the latter MultiLocation value (used on your chain) and back again.

If you believe there is an issuer with the reanchoring, then I'll just need a failing test to demonstrate this. So far I have not seen one.

You will likely want to have an instance of the Assets pallet which represents all Statemint assets. You might also write your own Convert<MultiLocation, AssetId> to feed into the FungiblesAdapter which would effectively "namespace" the Statemint assets within your general assets pallet; to do this you'd need to ensure that your AssetId type is big enough to contain all Statemint AssetId values as well as any other AssetId values specific to your chain which you might want.

None of this should require alterations to Statemint.

gavofyork · 2021-12-04T21:54:26Z

Ok, I believe I figured it out. There is indeed a subtle issue with the reanchoring logic, specifically it gives a correct non-canonical destination value. Basically, it sends a MultiLocation to Statemint of (Parent, Statemint, 42) rather than just (42). From Statemint's point of view, they're equivalent, only the former includes a superfluous (Parent, Statemint) prefix.

I'll put in something to simplify it and stuff should work.

gavofyork · 2021-12-06T09:23:54Z

paritytech/polkadot#4470 should fix things

yrong · 2021-12-07T02:59:51Z

@gavofyork @KiChjang seems this fix will break current behavior how we bifrost(2001) communicate with acala(2000) and other parachains, breaking test cases as following:

#[test]
fn reanchor_1_works() {
  let mut id: MultiLocation = (Parent, Parachain(2000), GeneralKey(42.encode())).into();
  let ancestry = Parachain(2001).into();
  let target = (Parent, Parachain(2000)).into();
  let expected = (Parent, Parachain(2001), GeneralKey(42.encode())).into();
  id.reanchor(&target, &ancestry).unwrap();
  assert_eq!(id, expected);
}

maybe @xlc can add some comments here also

yrong · 2021-12-07T03:24:37Z

@KiChjang I also see this pr which seems we decide to add pallet index in between to represent asset in statemine, right? if so we may also revamp adaptor logic from our side accordingly.

gavofyork · 2021-12-07T10:49:15Z

@gavofyork @KiChjang seems this fix will break current behavior how we bifrost(2001) communicate with acala(2000) and other parachains, breaking test cases as following:
#[test]
fn reanchor_1_works() {
  let mut id: MultiLocation = (Parent, Parachain(2000), GeneralKey(42.encode())).into();
  let ancestry = Parachain(2001).into();
  let target = (Parent, Parachain(2000)).into();
  let expected = (Parent, Parachain(2001), GeneralKey(42.encode())).into();
  id.reanchor(&target, &ancestry).unwrap();
  assert_eq!(id, expected);
}
maybe @xlc can add some comments here also

Yes. This test is very much incorrect. The GeneralKey(42) exists on parachain #2000, not #2001. So expected should be just GeneralKey(42).

From parachain #2001's point of view (where we begin):

ancestry: Parachain(2001)
parachain Polkadot v0.9.32 #2000: (Parent, Parachain(2000))
parachain Bump serde from 1.0.150 to 1.0.151 #2001: Here
the asset: (Parent, Parachain(2000), GeneralKey(42))

From parachain #2000's point of view (where we reanchor to):
5. ancestry: Parachain(2000)
6. parachain #2000: Here
7. parachain #2001: (Parent, Parachain(2001))
8. the asset: GeneralKey(42)

So the reanchoring should take the ancestry of the location where we begin (1), the destination (relative to where we begin) (2) and the asset (also relatively to where we begin) (4).

From these three pieces of information, it is able to tell us how the destination would refer to the same asset (8).

gavofyork · 2021-12-07T10:51:38Z

Closing since this is definitely not the right fix. Please move any further discussion to paritytech/polkadot#4470 or #833.

yrong mentioned this pull request Nov 29, 2021

Support statemine asset transfer bifrost-io/bifrost#420

Merged

apopiak self-requested a review November 30, 2021 10:00

apopiak reviewed Dec 1, 2021

View reviewed changes

polkadot-parachains/westmint/src/lib.rs Outdated Show resolved Hide resolved

apopiak suggested changes Dec 1, 2021

View reviewed changes

yrong force-pushed the asset-transfer branch 2 times, most recently from 80f010b to 82eae51 Compare December 2, 2021 14:20

apopiak reviewed Dec 2, 2021

View reviewed changes

polkadot-parachains/parachains-common/src/impls.rs Outdated Show resolved Hide resolved

yrong force-pushed the asset-transfer branch from 82eae51 to 64552e1 Compare December 2, 2021 14:56

Support asset transfer from/to sibling chain

ff777bc

yrong force-pushed the asset-transfer branch from 64552e1 to ff777bc Compare December 2, 2021 15:16

yrong changed the title ~~support asset transfer from other parachain~~ support asset transfer from/to sibling chain Dec 2, 2021

apopiak approved these changes Dec 2, 2021

View reviewed changes

apopiak requested review from gavofyork, KiChjang and bkchr December 2, 2021 16:16

apopiak added A0-pleasereview labels Dec 2, 2021

apopiak added T6-XCM This PR/Issue is related to XCM. T7-system_parachains This PR/Issue is related to System Parachains. labels Dec 2, 2021

gavofyork reviewed Dec 2, 2021

View reviewed changes

gavofyork suggested changes Dec 2, 2021

View reviewed changes

gavofyork reviewed Dec 2, 2021

View reviewed changes

KiChjang reviewed Dec 3, 2021

View reviewed changes

apopiak self-requested a review December 3, 2021 10:29

bkchr removed their request for review December 3, 2021 13:33

xlc mentioned this pull request Dec 4, 2021

Xtokens seems incompatible with what statemint expects #827

Open

yrong mentioned this pull request Dec 6, 2021

support transfering multiple assets(now only one) in orml-xtokens open-web3-stack/open-runtime-module-library#663

Closed

yrong closed this Dec 7, 2021

yrong reopened this Dec 7, 2021

gavofyork closed this Dec 7, 2021

yrong mentioned this pull request Dec 9, 2021

Reanchor should return canonical location paritytech/polkadot#4470

Merged

1 task

support asset transfer from/to sibling chain #806

support asset transfer from/to sibling chain #806

Conversation

yrong commented Nov 29, 2021 • edited by apopiak Loading

yrong commented Nov 29, 2021 • edited Loading

KiChjang commented Nov 29, 2021

yrong commented Nov 30, 2021 • edited Loading

KiChjang commented Nov 30, 2021

yrong commented Nov 30, 2021 • edited Loading

KiChjang commented Nov 30, 2021 • edited Loading

apopiak commented Nov 30, 2021 • edited Loading

yrong commented Nov 30, 2021

apopiak commented Dec 1, 2021

apopiak left a comment • edited Loading

Choose a reason for hiding this comment

yrong commented Dec 2, 2021

apopiak commented Dec 2, 2021

apopiak commented Dec 2, 2021

gavofyork commented Dec 2, 2021

gavofyork Dec 2, 2021

Choose a reason for hiding this comment

gavofyork Dec 2, 2021

Choose a reason for hiding this comment

gavofyork left a comment

Choose a reason for hiding this comment

gavofyork Dec 2, 2021

Choose a reason for hiding this comment

KiChjang Dec 3, 2021 • edited Loading

Choose a reason for hiding this comment

KiChjang Dec 3, 2021 • edited Loading

Choose a reason for hiding this comment

yrong Dec 3, 2021 • edited Loading

Choose a reason for hiding this comment

KiChjang Dec 3, 2021 • edited Loading

Choose a reason for hiding this comment

yrong Dec 3, 2021 • edited Loading

Choose a reason for hiding this comment

yrong Dec 4, 2021

Choose a reason for hiding this comment

yrong Dec 4, 2021

Choose a reason for hiding this comment

KiChjang Dec 4, 2021 • edited Loading

Choose a reason for hiding this comment

yrong Dec 4, 2021 • edited Loading

Choose a reason for hiding this comment

KiChjang Dec 4, 2021

Choose a reason for hiding this comment

gavofyork commented Dec 4, 2021 • edited Loading

gavofyork commented Dec 4, 2021

gavofyork commented Dec 6, 2021

yrong commented Dec 7, 2021 • edited Loading

yrong commented Dec 7, 2021 • edited Loading

gavofyork commented Dec 7, 2021 • edited Loading

gavofyork commented Dec 7, 2021 • edited Loading

yrong commented Nov 29, 2021 •

edited by apopiak

Loading

yrong commented Nov 29, 2021 •

edited

Loading

yrong commented Nov 30, 2021 •

edited

Loading

yrong commented Nov 30, 2021 •

edited

Loading

KiChjang commented Nov 30, 2021 •

edited

Loading

apopiak commented Nov 30, 2021 •

edited

Loading

apopiak left a comment •

edited

Loading

KiChjang Dec 3, 2021 •

edited

Loading

KiChjang Dec 3, 2021 •

edited

Loading

yrong Dec 3, 2021 •

edited

Loading

KiChjang Dec 3, 2021 •

edited

Loading

yrong Dec 3, 2021 •

edited

Loading

KiChjang Dec 4, 2021 •

edited

Loading

yrong Dec 4, 2021 •

edited

Loading

gavofyork commented Dec 4, 2021 •

edited

Loading

yrong commented Dec 7, 2021 •

edited

Loading

yrong commented Dec 7, 2021 •

edited

Loading

gavofyork commented Dec 7, 2021 •

edited

Loading

gavofyork commented Dec 7, 2021 •

edited

Loading