Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

support asset transfer from/to sibling chain #806

Closed
wants to merge 1 commit into from
Closed

support asset transfer from/to sibling chain #806

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
102 changes: 99 additions & 3 deletions polkadot-parachains/parachains-common/src/impls.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,10 @@ use frame_support::traits::{
};
use pallet_asset_tx_payment::HandleCredit;
use sp_runtime::traits::Zero;
use sp_std::marker::PhantomData;
use xcm::latest::{AssetId, Fungibility::Fungible, MultiAsset, MultiLocation};
use xcm_executor::traits::FilterAssetLocation;
use sp_std::{borrow::Borrow, marker::PhantomData, result};
use xcm::latest::{AssetId, Fungibility::Fungible, Junction, MultiAsset, MultiLocation};

use xcm_executor::traits::{Convert, FilterAssetLocation};

/// Type alias to conveniently refer to the `Currency::NegativeImbalance` associated type.
pub type NegativeImbalance<T> = <pallet_balances::Pallet<T> as Currency<
Expand Down Expand Up @@ -113,6 +114,44 @@ impl<T: Get<MultiLocation>> FilterAssetLocation for AssetsFrom<T> {
}
}

/// Converter struct with two main usage scenarios:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Documentation needs improvement. This is incomprehensible at present.

/// 1. Transfer asset from local to other para-chain
/// - 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<
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

LocalPrefix,
RemotePrefix,
AssetId,
ConvertAssetId,
>(PhantomData<(LocalPrefix, RemotePrefix, AssetId, ConvertAssetId)>);
impl<
LocalPrefix: Get<MultiLocation>,
RemotePrefix: Get<MultiLocation>,
AssetId: Clone,
ConvertAssetId: Convert<u128, AssetId>,
> Convert<MultiLocation, AssetId>
for AsPrefixedGeneralIndexFromLocalOrRemote<LocalPrefix, RemotePrefix, AssetId, ConvertAssetId>
{
fn convert_ref(id: impl Borrow<MultiLocation>) -> result::Result<AssetId, ()> {
let id = id.borrow();

match id.match_and_split(&LocalPrefix::get()) {
Some(Junction::GeneralIndex(id)) => ConvertAssetId::convert_ref(id),
_ => match id.match_and_split(&RemotePrefix::get()) {
Some(Junction::GeneralIndex(id)) => ConvertAssetId::convert_ref(id),
_ => Err(()),
},
}
}
fn reverse_ref(what: impl Borrow<AssetId>) -> result::Result<MultiLocation, ()> {
let mut location = LocalPrefix::get();
let id = ConvertAssetId::reverse_ref(what)?;
location.push_interior(Junction::GeneralIndex(id)).map_err(|_| ())?;
Ok(location)
}
}

#[cfg(test)]
mod tests {
use super::*;
Expand All @@ -130,7 +169,9 @@ mod tests {
traits::{BlakeTwo256, IdentityLookup},
Perbill,
};

use xcm::prelude::*;
use xcm_executor::traits::JustTry;

type UncheckedExtrinsic = frame_system::mocking::MockUncheckedExtrinsic<Test>;
type Block = frame_system::mocking::MockBlock<Test>;
Expand Down Expand Up @@ -281,4 +322,59 @@ mod tests {
"AssetsFrom should allow assets from any of its interior locations"
);
}

#[test]
fn assets_from_convert_correctly() {
parameter_types! {
pub const Local: MultiLocation = Here.into();
pub Remote: MultiLocation = MultiLocation::new(1, X1(Parachain(1000)));
}
let local_asset_location = Local::get().pushed_with_interior(GeneralIndex(42)).unwrap();
let remote_asset_location = Remote::get().pushed_with_interior(GeneralIndex(42)).unwrap();
let mut asset_id =
AsPrefixedGeneralIndexFromLocalOrRemote::<Local, Remote, u32, JustTry>::convert_ref(
&local_asset_location,
)
.unwrap_or(u32::default());
assert_eq!(asset_id, 42);
asset_id =
AsPrefixedGeneralIndexFromLocalOrRemote::<Local, Remote, u32, JustTry>::convert_ref(
&remote_asset_location,
)
.unwrap_or(u32::default());
assert_eq!(asset_id, 42);
Copy link
Contributor

@KiChjang KiChjang Dec 3, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@KiChjang KiChjang Dec 3, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

@yrong yrong Dec 3, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

Copy link
Contributor

@KiChjang KiChjang Dec 3, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple of observations:

  1. 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.
  2. 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)).
  3. 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.

Copy link
Author

@yrong yrong Dec 3, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Copy link
Contributor

@KiChjang KiChjang Dec 4, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

@yrong yrong Dec 4, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

#[test]
fn assets_from_convert_error_with_different_parent() {
parameter_types! {
pub const Local: MultiLocation = Here.into();
pub Remote: MultiLocation = MultiLocation::new(2, Junctions::Here);
}
let remote_asset_location = MultiLocation::new(1, X1(GeneralIndex(42)));
let asset_id =
AsPrefixedGeneralIndexFromLocalOrRemote::<Local, Remote, u32, JustTry>::convert_ref(
&remote_asset_location,
)
.unwrap_or(u32::default());
assert_eq!(asset_id, u32::default())
}

#[test]
fn assets_from_convert_error_with_different_interiors() {
parameter_types! {
pub const Local: MultiLocation = Here.into();
pub Remote: MultiLocation = MultiLocation::new(1, X1(Parachain(1000)));
}
let mut remote_asset_location =
Remote::get().pushed_with_interior(PalletInstance(10)).unwrap();
remote_asset_location =
remote_asset_location.pushed_with_interior(GeneralIndex(42)).unwrap();
let asset_id =
AsPrefixedGeneralIndexFromLocalOrRemote::<Local, Remote, u32, JustTry>::convert_ref(
&remote_asset_location,
)
.unwrap_or(u32::default());
assert_eq!(asset_id, u32::default())
}
}
14 changes: 8 additions & 6 deletions polkadot-parachains/statemine/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,17 +65,18 @@ use parachains_common::{
pub use sp_runtime::BuildStorage;

// Polkadot imports
use crate::common::impls::AsPrefixedGeneralIndexFromLocalOrRemote;
use pallet_xcm::{EnsureXcm, IsMajorityOfBody, XcmPassthrough};
use polkadot_parachain::primitives::Sibling;
use polkadot_runtime_common::{BlockHashCount, RocksDbWeight, SlowAdjustingFeeUpdate};
use xcm::latest::prelude::*;
use xcm_builder::{
AccountId32Aliases, AllowKnownQueryResponses, AllowSubscriptionsFrom,
AllowTopLevelPaidExecutionFrom, AllowUnpaidExecutionFrom, AsPrefixedGeneralIndex,
ConvertedConcreteAssetId, CurrencyAdapter, EnsureXcmOrigin, FixedWeightBounds,
FungiblesAdapter, IsConcrete, LocationInverter, NativeAsset, ParentAsSuperuser,
ParentIsDefault, RelayChainAsNative, SiblingParachainAsNative, SiblingParachainConvertsVia,
SignedAccountId32AsNative, SovereignSignedViaLocation, TakeWeightCredit, UsingComponents,
AllowTopLevelPaidExecutionFrom, AllowUnpaidExecutionFrom, ConvertedConcreteAssetId,
CurrencyAdapter, EnsureXcmOrigin, FixedWeightBounds, FungiblesAdapter, IsConcrete,
LocationInverter, NativeAsset, ParentAsSuperuser, ParentIsDefault, RelayChainAsNative,
SiblingParachainAsNative, SiblingParachainConvertsVia, SignedAccountId32AsNative,
SovereignSignedViaLocation, TakeWeightCredit, UsingComponents,
};
use xcm_executor::{traits::JustTry, Config, XcmExecutor};

Expand Down Expand Up @@ -433,6 +434,7 @@ parameter_types! {
pub Ancestry: MultiLocation = Parachain(ParachainInfo::parachain_id().into()).into();
pub const Local: MultiLocation = Here.into();
pub CheckingAccount: AccountId = PolkadotXcm::check_account();
pub Remote: MultiLocation = MultiLocation::new(1, X1(Parachain(ParachainInfo::parachain_id().into())));
}

/// Type for specifying how a `MultiLocation` can be converted into an `AccountId`. This is used
Expand Down Expand Up @@ -469,7 +471,7 @@ pub type FungiblesTransactor = FungiblesAdapter<
ConvertedConcreteAssetId<
AssetId,
Balance,
AsPrefixedGeneralIndex<Local, AssetId, JustTry>,
AsPrefixedGeneralIndexFromLocalOrRemote<Local, Remote, AssetId, JustTry>,
JustTry,
>,
// Convert an XCM MultiLocation into a local account id:
Expand Down
14 changes: 8 additions & 6 deletions polkadot-parachains/statemint/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,17 +65,18 @@ use parachains_common::{
pub use sp_runtime::BuildStorage;

// Polkadot imports
use crate::common::impls::AsPrefixedGeneralIndexFromLocalOrRemote;
use pallet_xcm::{EnsureXcm, IsMajorityOfBody, XcmPassthrough};
use polkadot_parachain::primitives::Sibling;
use polkadot_runtime_common::{BlockHashCount, RocksDbWeight, SlowAdjustingFeeUpdate};
use xcm::latest::prelude::*;
use xcm_builder::{
AccountId32Aliases, AllowKnownQueryResponses, AllowSubscriptionsFrom,
AllowTopLevelPaidExecutionFrom, AllowUnpaidExecutionFrom, AsPrefixedGeneralIndex,
ConvertedConcreteAssetId, CurrencyAdapter, EnsureXcmOrigin, FixedWeightBounds,
FungiblesAdapter, IsConcrete, LocationInverter, NativeAsset, ParentAsSuperuser,
ParentIsDefault, RelayChainAsNative, SiblingParachainAsNative, SiblingParachainConvertsVia,
SignedAccountId32AsNative, SovereignSignedViaLocation, TakeWeightCredit, UsingComponents,
AllowTopLevelPaidExecutionFrom, AllowUnpaidExecutionFrom, ConvertedConcreteAssetId,
CurrencyAdapter, EnsureXcmOrigin, FixedWeightBounds, FungiblesAdapter, IsConcrete,
LocationInverter, NativeAsset, ParentAsSuperuser, ParentIsDefault, RelayChainAsNative,
SiblingParachainAsNative, SiblingParachainConvertsVia, SignedAccountId32AsNative,
SovereignSignedViaLocation, TakeWeightCredit, UsingComponents,
};
use xcm_executor::{traits::JustTry, Config, XcmExecutor};

Expand Down Expand Up @@ -445,6 +446,7 @@ parameter_types! {
pub Ancestry: MultiLocation = Parachain(ParachainInfo::parachain_id().into()).into();
pub const Local: MultiLocation = Here.into();
pub CheckingAccount: AccountId = PolkadotXcm::check_account();
pub Remote: MultiLocation = MultiLocation::new(1, X1(Parachain(ParachainInfo::parachain_id().into())));
}

/// Type for specifying how a `MultiLocation` can be converted into an `AccountId`. This is used
Expand Down Expand Up @@ -481,7 +483,7 @@ pub type FungiblesTransactor = FungiblesAdapter<
ConvertedConcreteAssetId<
AssetId,
Balance,
AsPrefixedGeneralIndex<Local, AssetId, JustTry>,
AsPrefixedGeneralIndexFromLocalOrRemote<Local, Remote, AssetId, JustTry>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

JustTry,
>,
// Convert an XCM MultiLocation into a local account id:
Expand Down
16 changes: 9 additions & 7 deletions polkadot-parachains/westmint/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,18 +65,18 @@ use parachains_common::{
pub use sp_runtime::BuildStorage;

// Polkadot imports
use crate::common::impls::AsPrefixedGeneralIndexFromLocalOrRemote;
use pallet_xcm::XcmPassthrough;
use polkadot_parachain::primitives::Sibling;
use polkadot_runtime_common::{BlockHashCount, RocksDbWeight, SlowAdjustingFeeUpdate};
use xcm::latest::prelude::*;
use xcm_builder::{
AccountId32Aliases, AllowKnownQueryResponses, AllowSubscriptionsFrom,
AllowTopLevelPaidExecutionFrom, AllowUnpaidExecutionFrom, AsPrefixedGeneralIndex,
ConvertedConcreteAssetId, CurrencyAdapter, EnsureXcmOrigin, FixedWeightBounds,
FungiblesAdapter, IsConcrete, LocationInverter, NativeAsset, ParentAsSuperuser,
ParentIsDefault, RelayChainAsNative, SiblingParachainAsNative, SiblingParachainConvertsVia,
SignedAccountId32AsNative, SignedToAccountId32, SovereignSignedViaLocation, TakeWeightCredit,
UsingComponents,
AllowTopLevelPaidExecutionFrom, AllowUnpaidExecutionFrom, ConvertedConcreteAssetId,
CurrencyAdapter, EnsureXcmOrigin, FixedWeightBounds, FungiblesAdapter, IsConcrete,
LocationInverter, NativeAsset, ParentAsSuperuser, ParentIsDefault, RelayChainAsNative,
SiblingParachainAsNative, SiblingParachainConvertsVia, SignedAccountId32AsNative,
SignedToAccountId32, SovereignSignedViaLocation, TakeWeightCredit, UsingComponents,
};
use xcm_executor::{traits::JustTry, Config, XcmExecutor};

Expand Down Expand Up @@ -428,6 +428,7 @@ parameter_types! {
pub Ancestry: MultiLocation = Parachain(ParachainInfo::parachain_id().into()).into();
pub const Local: MultiLocation = Here.into();
pub CheckingAccount: AccountId = PolkadotXcm::check_account();
pub Remote: MultiLocation = MultiLocation::new(1, X1(Parachain(ParachainInfo::parachain_id().into())));
}

/// Type for specifying how a `MultiLocation` can be converted into an `AccountId`. This is used
Expand Down Expand Up @@ -464,7 +465,7 @@ pub type FungiblesTransactor = FungiblesAdapter<
ConvertedConcreteAssetId<
AssetId,
Balance,
AsPrefixedGeneralIndex<Local, AssetId, JustTry>,
AsPrefixedGeneralIndexFromLocalOrRemote<Local, Remote, AssetId, JustTry>,
JustTry,
>,
// Convert an XCM MultiLocation into a local account id:
Expand All @@ -477,6 +478,7 @@ pub type FungiblesTransactor = FungiblesAdapter<
// The account to use for tracking teleports.
CheckingAccount,
>;

/// Means for transacting assets on this chain.
pub type AssetTransactors = (CurrencyTransactor, FungiblesTransactor);

Expand Down