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

Fixes for gav-xcm-v3 #1835

Merged
merged 8 commits into from
Nov 29, 2022
Merged

Fixes for gav-xcm-v3 #1835

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
23f0bcd
Fix for FungiblesAdapter - trait changes: Contains -> AssetChecking
bkontur Nov 8, 2022
a6dd610
Fix for missing weight for `fn unpaid_execution()`
bkontur Nov 8, 2022
288ba7b
Merge remote-tracking branch 'origin/gav-xcm-v3' into bko-gav-xcm-v3
bkontur Nov 19, 2022
8c08657
Merge remote-tracking branch 'origin/gav-xcm-v3' into bko-gav-xcm-v3
bkontur Nov 19, 2022
ccc49d1
Used NonLocalMint for all NonZeroIssuance
bkontur Nov 20, 2022
499316b
Merge remote-tracking branch 'origin/gav-xcm-v3' into bko-gav-xcm-v3
bkontur Nov 22, 2022
fa1cddb
Fix
bkontur Nov 22, 2022
d9e7673
Merge remote-tracking branch 'origin/gav-xcm-v3' into bko-gav-xcm-v3
bkontur Nov 22, 2022
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
1 change: 1 addition & 0 deletions Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions parachains/common/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ sp-std = { git = "https://github.com/paritytech/substrate", branch = "master", d
# Polkadot
polkadot-primitives = { git = "https://github.com/paritytech/polkadot", default-features = false, branch = "master" }
xcm = { git = "https://github.com/paritytech/polkadot", default-features = false, branch = "master" }
xcm-builder = { git = "https://github.com/paritytech/polkadot", default-features = false, branch = "master" }
xcm-executor = { git = "https://github.com/paritytech/polkadot", default-features = false, branch = "master" }

# Cumulus
Expand Down
14 changes: 9 additions & 5 deletions parachains/common/src/impls.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,12 +18,13 @@

use frame_support::traits::{
fungibles::{self, Balanced, CreditOf},
Contains, ContainsPair, Currency, Get, Imbalance, OnUnbalanced,
ContainsPair, Currency, Get, Imbalance, OnUnbalanced,
};
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_builder::{AssetChecking, MintLocation};

/// 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 @@ -87,13 +88,16 @@ where

/// Allow checking in assets that have issuance > 0.
pub struct NonZeroIssuance<AccountId, Assets>(PhantomData<(AccountId, Assets)>);
impl<AccountId, Assets> Contains<<Assets as fungibles::Inspect<AccountId>>::AssetId>
for NonZeroIssuance<AccountId, Assets>
impl<AccountId, Assets> AssetChecking<Assets::AssetId> for NonZeroIssuance<AccountId, Assets>
where
Assets: fungibles::Inspect<AccountId>,
{
fn contains(id: &<Assets as fungibles::Inspect<AccountId>>::AssetId) -> bool {
!Assets::total_issuance(*id).is_zero()
fn asset_checking(id: &Assets::AssetId) -> Option<MintLocation> {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@joepetrowski
I am not sure about this change, could you please check it?

there is a change for FungiblesAdapter: Contains -> AssetChecking:

actual master - CheckAsset: Contains<Assets::AssetId>,
https://github.com/paritytech/polkadot/blob/master/xcm/xcm-builder/src/fungibles_adapter.rs#L265

new xcm v3 - CheckAsset: AssetChecking<Assets::AssetId>,
https://github.com/paritytech/polkadot/blob/gav-xcm-v3/xcm/xcm-builder/src/fungibles_adapter.rs#L298

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure of the motivation to change this interface, but from reviewing its introduction, especially the docs on MintLocation, I don't think the logic is correct, that if an asset has some issuance then its MintLocation should be Local.

We could say that the MintLocation of bridged/para assets is NonLocal, however other locations within the network still trust system paras to the extent that they trust Polkadot governance, so would see the asset on Statemint as "equally local".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@joepetrowski @KiChjang
here is also some more description: paritytech/polkadot#4097 search for "Reverse Checking" with future issue: https://github.com/paritytech/polkadot/issues/6125

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think NonZeroIssuance should be changed at all. What looks to me is that it's meant to be used in conjunction with LocalMint, NonLocalMint and DualMint, rather than have AssetChecking directly implemented on it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@joepetrowski
ok, next iteration :), I put everywhere NonLocalMint,

Lets say:

  • we are configuring Statemint here, where we have pallet_assets
  • we have Moonbeam parachain with GLMR

What is the basic use case for Moonbeam to use Statemint?
"Somebody" from Moonbeam should create asset for GLMR at first?
Then MoonbeamUser can transfer GLMR to some StatemintUserAccount? Then some StatemintUserAccount can transfer to other accounts or move something back to the Moonbeam (but not more then he receives), so this means NonLocalMint?

What would be example for LocalMint from the Statemint perspective?

You wrote here before:

We could say that the MintLocation of bridged/para assets is NonLocal

e.g.: How could I resolve/check that in the code? I am checking structs: AssetDetails / AssetAccount / AssetMetadata

or similar: implementation for DualMint, how could we differ/tell that some Asset should be LocalMint or NonLocalMint?

Copy link
Contributor

Choose a reason for hiding this comment

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

"Somebody" from Moonbeam should create asset for GLMR at first?

The CreateOrigin should ensure that only Moonbeam itself, i.e. via their Root origin (however it is accessed) could create GLMR. So yeah, they'd need some referendum to send a message to Statemint registering GLMR.

Then MoonbeamUser can transfer GLMR to some StatemintUserAccount?

Right, so they should have (probably already do) some dispatchable function that will construct an XCM to send to Statemint. Something like fn reserve_transfer_asset(GLMR, Statemint, my_address) { ... }, and inside it would put together the [BuyExecution, ReserveAssetDeposited, ...] etc. instructions. For UX they'd probably want to WithdrawAsset from the Moonbeam sovereign account and charge a slightly higher tx fee on the Moonbeam side to cover it. Then their treasury can top up the sovereign account DOT every now and then.

Then some StatemintUserAccount can transfer to other accounts or move something back to the Moonbeam (but not more then he receives), so this means NonLocalMint?

We are not dealing with teleports, so I'm not sure how much this MintLocation applies to e.g. GLMR. Since the para origin corresponding to Moonbeam conforms to the CreateOrigin = ForeignCreators, "this chain" (being Moonbeam) can mint the asset on Statemint. But, as I read these docs (full enum below), the key part for me is: "We ensure that...

  • ...no more of the asset returns back to the chain than has been sent out." => This is nonsense, because in order for assets to be "sent out" they will have had to be created first.
  • ...no more of the asset is sent out from the chain than has been previously received." => This makes sense, because the GLMR started on Moonbeam, and Statemint has received it, and an issuance of 0 would indicate that it's all been sent out, and it should be further unable to withdraw any more.

Based on that, I would call all parachain token NonLocalMint. An example of LocalMint would be DOT and the assets created under the first (existing) instance of the Assets pallet, e.g. USDT. So, as to how to resolve it in code, I would say:

  • Any assets under pallet_balances || pallet_assets::Instance1 are Local and any assets under pallet_assets::Instance2 are NonLocal. @KiChjang wdyt?
/// The location which is allowed to mint a particular asset.
#[derive(Copy, Clone, Eq, PartialEq)]
pub enum MintLocation {
	/// This chain is allowed to mint the asset. When we track teleports of the asset we ensure that
	/// no more of the asset returns back to the chain than has been sent out.
	Local,
	/// This chain is not allowed to mint the asset. When we track teleports of the asset we ensure
	/// that no more of the asset is sent out from the chain than has been previously received.
	NonLocal,
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@joepetrowski
ok, Joe, thank you, so I will change it to something like this:

LocalMint<parachains_common::impls::NonZeroIssuance<AccountId, AssetsPlusBalanceContainsImpl>>

and later for westmint/bridge-hub stuff to:

DualMint<
       parachains_common::impls::NonZeroIssuance<AccountId, TrustBackedAssetsPlusBalanceContainsImpl>,
       parachains_common::impls::NonZeroIssuance<AccountId, ForeignAssets>
>

Copy link
Contributor

Choose a reason for hiding this comment

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

@joepetrowski If instance 1 of the assets pallet refers to the one created using integer IDs and instance 2 of the assets pallet represents those created with MultiLocation IDs, then this makes sense. Although I do have to echo your sentiment about how this may not be very relevant, as it is only used for teleports. I don't really see Statemint/e allowing for teleports between community parachains anytime soon; the most obvious use cases are however between system parachains teleporting DOT/KSM to each other.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@joepetrowski
so, I pushed there: LocalMint<NonZeroIssuance<AccountId, Assets>>,

or do we want to fix this just for DOT/KSM (which means allow relay chain currency)? in this case, how is identified "DOT/KSM" as AssetId?

Copy link
Contributor

Choose a reason for hiding this comment

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

It would just be { parents: 1, interior: Here }, which should correspond to the Balances pallet, not an AssetId.

if Assets::total_issuance(*id).is_zero() {
None
} else {
Some(MintLocation::Local)
}
}
}

Expand Down
3 changes: 3 additions & 0 deletions parachains/runtimes/assets/statemine/src/weights/xcm/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -241,4 +241,7 @@ impl<Call> XcmWeightInfo<Call> for StatemineXcmWeight<Call> {
// XCM Executor does not currently support alias origin operations
Weight::MAX.ref_time()
}
fn unpaid_execution(_: &WeightLimit, _: &Option<MultiLocation>) -> XCMWeight {
XcmGeneric::<Runtime>::unpaid_execution().ref_time()
}
}
1 change: 1 addition & 0 deletions parachains/runtimes/assets/statemine/src/weights/xcm/pallet_xcm_benchmarks_generic.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,4 +185,5 @@ impl<T: frame_system::Config> WeightInfo<T> {
pub(crate) fn set_fees_mode() -> Weight {
Weight::from_ref_time(6_426_000 as u64)
}
pub(crate) fn unpaid_execution() -> Weight { Weight::from_ref_time(3_111_000 as u64) }
}
3 changes: 3 additions & 0 deletions parachains/runtimes/assets/statemint/src/weights/xcm/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -241,4 +241,7 @@ impl<Call> XcmWeightInfo<Call> for StatemintXcmWeight<Call> {
// XCM Executor does not currently support alias origin operations
Weight::MAX.ref_time()
}
fn unpaid_execution(_: &WeightLimit, _: &Option<MultiLocation>) -> XCMWeight {
XcmGeneric::<Runtime>::unpaid_execution().ref_time()
}
}
1 change: 1 addition & 0 deletions parachains/runtimes/assets/statemint/src/weights/xcm/pallet_xcm_benchmarks_generic.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,4 +185,5 @@ impl<T: frame_system::Config> WeightInfo<T> {
pub(crate) fn set_fees_mode() -> Weight {
Weight::from_ref_time(6_336_000 as u64)
}
pub(crate) fn unpaid_execution() -> Weight { Weight::from_ref_time(3_111_000 as u64) }
}
3 changes: 3 additions & 0 deletions parachains/runtimes/assets/westmint/src/weights/xcm/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -241,4 +241,7 @@ impl<Call> XcmWeightInfo<Call> for WestmintXcmWeight<Call> {
// XCM Executor does not currently support alias origin operations
Weight::MAX.ref_time()
}
fn unpaid_execution(_: &WeightLimit, _: &Option<MultiLocation>) -> XCMWeight {
XcmGeneric::<Runtime>::unpaid_execution().ref_time()
}
}
1 change: 1 addition & 0 deletions parachains/runtimes/assets/westmint/src/weights/xcm/pallet_xcm_benchmarks_generic.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,4 +185,5 @@ impl<T: frame_system::Config> WeightInfo<T> {
pub(crate) fn set_fees_mode() -> Weight {
Weight::from_ref_time(6_392_000 as u64)
}
pub(crate) fn unpaid_execution() -> Weight { Weight::from_ref_time(3_111_000 as u64) }
}
21 changes: 12 additions & 9 deletions parachains/runtimes/testing/penpal/src/xcm_config.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ use frame_support::{
log, match_types, parameter_types,
traits::{
fungibles::{self, Balanced, CreditOf},
ConstU32, Contains, ContainsPair, Everything, Get, Nothing,
ConstU32, ContainsPair, Everything, Get, Nothing,
},
};
use pallet_asset_tx_payment::HandleCredit;
Expand All @@ -43,10 +43,10 @@ use xcm::latest::{prelude::*, Weight as XCMWeight};
use xcm_builder::{
AccountId32Aliases, AllowKnownQueryResponses, AllowSubscriptionsFrom,
AllowTopLevelPaidExecutionFrom, AllowUnpaidExecutionFrom, AsPrefixedGeneralIndex,
ConvertedConcreteId, CurrencyAdapter, EnsureXcmOrigin, FixedWeightBounds, FungiblesAdapter,
IsConcrete, NativeAsset, ParentIsPreset, RelayChainAsNative, SiblingParachainAsNative,
SiblingParachainConvertsVia, SignedAccountId32AsNative, SignedToAccountId32,
SovereignSignedViaLocation, TakeWeightCredit, UsingComponents,
AssetChecking, ConvertedConcreteId, CurrencyAdapter, EnsureXcmOrigin, FixedWeightBounds,
FungiblesAdapter, IsConcrete, MintLocation, NativeAsset, ParentIsPreset, RelayChainAsNative,
SiblingParachainAsNative, SiblingParachainConvertsVia, SignedAccountId32AsNative,
SignedToAccountId32, SovereignSignedViaLocation, TakeWeightCredit, UsingComponents,
};
use xcm_executor::{
traits::{JustTry, ShouldExecute},
Expand Down Expand Up @@ -244,13 +244,16 @@ impl<T: Get<MultiLocation>> ContainsPair<MultiAsset, MultiLocation> for AssetsFr

/// Allow checking in assets that have issuance > 0.
pub struct NonZeroIssuance<AccountId, Assets>(PhantomData<(AccountId, Assets)>);
impl<AccountId, Assets> Contains<<Assets as fungibles::Inspect<AccountId>>::AssetId>
for NonZeroIssuance<AccountId, Assets>
impl<AccountId, Assets> AssetChecking<Assets::AssetId> for NonZeroIssuance<AccountId, Assets>
where
Assets: fungibles::Inspect<AccountId>,
{
fn contains(id: &<Assets as fungibles::Inspect<AccountId>>::AssetId) -> bool {
!Assets::total_issuance(*id).is_zero()
fn asset_checking(id: &Assets::AssetId) -> Option<MintLocation> {
if Assets::total_issuance(*id).is_zero() {
None
} else {
Some(MintLocation::Local)
}
}
}

Expand Down