FRAME: Unity Balance Conversion for Different IDs of Native Asset #3659

muharem · 2024-03-12T12:01:21Z

Introduce types to define 1:1 balance conversion for different relative asset ids/locations of native asset.

Examples:
native asset on Asset Hub presented as VersionedLocatableAsset type in the context of Relay Chain is

{
  `location`: (0, Parachain(1000)),
  `asset_id`: (1, Here),
}

and it's balance should be converted 1:1 by implementations of ConversionToAssetBalance trait.

…ce-conversion

bkontur

lgmt, just two nits and I didn't check integration tests, just runtimes and xcm stuff

polkadot/runtime/common/src/impls.rs

bkontur · 2024-03-20T13:47:05Z

polkadot/runtime/common/src/impls.rs

+/// Adapter for [`Contains`] trait to match [`VersionedLocatableAsset`] type converted to the latest
+/// version of itself where it's location matched by `L` and it's asset id by `A` parameter types.
+pub struct ContainsLatest<L, A>(PhantomData<(L, A)>);
+impl<L, A> Contains<VersionedLocatableAsset> for ContainsLatest<L, A>


I would move it to the xcm-builder, maybe here: https://github.com/paritytech/polkadot-sdk/blob/master/polkadot/xcm/xcm-builder/src/matches_location.rs ?

it's implemented over VersionedLocatableAsset type, which is part of common.

I see, I thought VersionedLocatableAsset was part of the XCM stuff, so maybe we could move everything related to VersionedLocatableAsset and ContainsLatest to one place in the XCM builder., e.g.:
https://github.com/paritytech/polkadot-sdk/blob/master/polkadot/xcm/xcm-builder/src/locatable_asset.rs.
(I think I saw some complaints about adding too much stuff to the polkadot-runtime-common.)

As I see, we already have LocatableAssetId in xcm-builder here: https://github.com/paritytech/polkadot-sdk/blob/master/polkadot/xcm/xcm-builder/src/pay.rs#L191-L198, which could possibly also go to the locatable_asset.rs

And maybe VersionedLocatableAsset could also wrap that LocatableAssetId, but I think this can be just from V5, e.g.:

pub enum VersionedLocatableAsset { ... #[codec(index = 4)] - V4 { location: xcm::v4::Location, asset_id: xcm::v4::AssetId }, + V4(LocatableAssetId), }

I wont wrap use LocatableAssetId directly for the last version from VersionedLocatableAsset. It will be confusion, different version will look different.

For now, I will keep everything as it is, specially wont change it with this PR.
Every time someone see these types, ( VersionedLocatableAsset and LocatableAssetId), they suggest to remove it and use only Location, refactor it somehow, etc. Since we should be more conservative with xcm-builder, I would not move it for now.

cc: @franciscoaguirre

Since we should be more conservative with xcm-builder...

Where is this coming from? Can you elaborate more?

Every time someone see these types, ( VersionedLocatableAsset and LocatableAssetId), they suggest to remove it and use only Location, refactor it somehow, etc.

Well, perhaps it's worth considering refactoring it then.

I think keeping VersionedLocatableAsset outside of xcm / xcm-builder, where we currently have all Versioned* stuff and also LocatableAssetId, seems kind of strange. As far as I can see, it is used by relay runtimes and collectives runtime, so from this perspective, polkadot-runtime-common is not the best place (xcm-builder seems better):

[package] name = "polkadot-runtime-common" description = "Pallets and constants used in Relay Chain networks."

VersionedLocatableAsset contains v3 and v4. How do you propose keeping this up-to-date? Do you expect that when we add xcm::v5, someone will also update this? If so, there's no way to know from the xcm module's point of view (considering that one day we might move xcm out of polkadot-sdk) that some versioned stuff is used elsewhere. If not, for example, when we add xcm::v6, we remove xcm:v3. Is that okay? I think the check-migrations jobs should detect this and fail.

Another thing to consider, I see it is used for pallet_collective as the key for ConversionRateToNative or pallet_treasury as AssetKind. I think it's a similar situation to what's discussed here: #1130. And if I recall correctly, the last time we spoke with Joe about pallet_assets and Location as AssetId, everything pointed to the solution of using xcm::latest::prelude::Location as AssetId (with migrations) and VersionedLocation as AssetIdParameter (which is used for extrinsics).

I think VersionedLocatableAsset presents a very similar situation. Perhaps it's worth considering splitting AssetKind into AssetKind / AssetKindParameter. For example, this could help avoid using Box for extrinsic parameters like asset_kind: Box<T::AssetKind>, though I'm not sure if this is good or bad, it can be left for the runtime to decide. My point is that ideally, we should have one method/pattern for storing XCM-related data.

I will check that draft PR, and I am also looking for a solution where that PayOverXcm can also detect that the asset is located on local chan, but beneficiary is on Asset Hub. We have this use case when treasury wants to fund it's own account on Asset Hub. I will initiate a discussion around it, and open an RFC.
@franciscoaguirre please approve this PR if you happy with it.

I only want to understand the purpose of this change. Is it because DOT was being passed on to the asset rate converter? Or is it to allow things like USDT to be used 1:1 when they come from system parachains?

Hmm, with sovereign accounts it definitely becomes harder. My draft PR was of the idea that we could just use the beneficiary location to know to which chain to send it to. It wouldn't work when we want to send it somewhere different than the location to fund a sovereign account.

We might need then a location for the chain and another for the beneficiary, like on the send extrinsic in pallet-xcm. There's no need to name it locatable asset though, just a regular asset, a regular destination, and a regular beneficiary.

I only want to understand the purpose of this change. Is it because DOT was being passed on to the asset rate converter? Or is it to allow things like USDT to be used 1:1 when they come from system parachains?

The asset rate does not know there are different locations of native asset (as in the example in the PR description). And will require a rate to be set for it. With there types we make asset conversion understand different locations of native asset, and do not require a rate to be set for it.

I guess that's one of the problems of marking the difference between the same asset on different chains

Co-authored-by: Branislav Kontur <bkontur@gmail.com>

Treasury Spend detects the relative locations of native asset to compare origin's spend limits. When a spend is approved for a native asset located on Asset Hub, the AssetRate pallet instance does not recognize the location (parent: 1, here) as a native asset. Therefore, it requires the asset rate to be set in advance to the rate of `1`. To address this issue in this PR, we introduce new types to wrap the `AssetRate` implementation of balance conversion. These types match the native asset's relative locations and result in a 1:1 balance conversion in such cases. In addition we provide integration tests to cover these cases. Examples: Treasury pallet on Relay Chain accepts the spend of the native asset on Asset Hub with it's ID presented as `VersionedLocatableAsset` type ``` { `location`: (0, Parachain(1000)), `asset_id`: (1, Here), } ``` and it's balance should be converted 1:1 by treasury pallet's `BalanceConverter` type. Note: I plan to introduce a similar but configurable implementation in the polkadot-sdk and utilize it here. However, to get this change in the next release, I open this PR first. Frame version: paritytech/polkadot-sdk#3659 --------- Co-authored-by: joe petrowski <25483142+joepetrowski@users.noreply.github.com> Co-authored-by: Branislav Kontur <bkontur@gmail.com> Co-authored-by: Bastian Köcher <git@kchr.de>

…ce-conversion

github-actions · 2024-04-15T15:29:26Z

Review required! Latest push from author must always be reviewed

franciscoaguirre · 2024-04-15T14:53:41Z

...us/parachains/integration-tests/emulated/tests/assets/asset-hub-rococo/src/tests/treasury.rs

+				location: asset_hub_location.clone(),
+				asset_id: native_asset.into(),


It's weird how the location is relative to the relay chain but the asset id is relative to asset hub. Is this correct?

yes, this is correct. DOT is parent on asset hub

franciscoaguirre · 2024-04-15T14:54:45Z

...us/parachains/integration-tests/emulated/tests/assets/asset-hub-rococo/src/tests/treasury.rs

+		assert_expected_events!(
+			AssetHubRococo,
+			vec![
+				RuntimeEvent::Balances(pallet_balances::Event::Transfer { .. }) => {},


You could specify the beneficiary here, so we know the transfer is working

this assert is broken, I cannot really match beneficiary here. but we are fine here, I check the balance above

franciscoaguirre · 2024-04-15T15:01:26Z

...us/parachains/integration-tests/emulated/tests/assets/asset-hub-rococo/src/tests/treasury.rs

+		assert_expected_events!(
+			Rococo,
+			vec![
+				RuntimeEvent::Treasury(pallet_treasury::Event::Paid { .. }) => {},


Probably the beneficiary of this can be asserted as well

It does not work right now with this macro

franciscoaguirre · 2024-04-15T15:25:47Z

...ration-tests/emulated/tests/collectives/collectives-westend/src/tests/fellowship_treasury.rs

+		assert_expected_events!(
+			Westend,
+			vec![
+				RuntimeEvent::XcmPallet(pallet_xcm::Event::Sent { .. }) => {},


Could assert the destination

It does not work right now with this macro

franciscoaguirre · 2024-04-15T15:27:33Z

...ration-tests/emulated/tests/collectives/collectives-westend/src/tests/fellowship_treasury.rs

+		let fellowship_treasury_location: Location =
+			Location::new(1, [Parachain(1001), PalletInstance(65)]);


This is the sovereign account in Asset Hub of the fellowship treasury?

franciscoaguirre · 2024-04-15T15:31:25Z

polkadot/xcm/xcm-builder/src/barriers.rs

+/// Matches if the given location is a system-level sibling parachain.
+pub struct IsSiblingSystemParachain<ParaId, SelfParaId>(PhantomData<(ParaId, SelfParaId)>);
+impl<ParaId: IsSystem + From<u32> + Eq, SelfParaId: Get<ParaId>> Contains<Location>
+	for IsSiblingSystemParachain<ParaId, SelfParaId>
+{
+	fn contains(l: &Location) -> bool {
+		matches!(
+			l.unpack(),
+			(1, [Junction::Parachain(id)])
+				if SelfParaId::get() != ParaId::from(*id) && ParaId::from(*id).is_system(),
+		)
+	}
+}
+
+/// Matches if the given location contains only the specified amount of parents and no interior
+/// junctions.
+pub struct IsParentsOnly<Count>(PhantomData<Count>);
+impl<Count: Get<u8>> Contains<Location> for IsParentsOnly<Count> {
+	fn contains(t: &Location) -> bool {
+		t.contains_parents_only(Count::get())
+	}
+}
+


This doesn't look like it should go in barriers.rs

its consistent to what we have in this file, IsChildSystemParachain just above for example

franciscoaguirre · 2024-04-15T15:37:46Z

substrate/frame/support/src/traits/tokens/misc.rs

+/// Implements [`ConversionFromAssetBalance`], allowing for a 1:1 balance conversion of the asset
+/// when it meets the conditions specified by `C`. If the conditions are not met, the conversion is
+/// delegated to `O`.
+pub struct UnityOrOuterConversion<C, O>(core::marker::PhantomData<(C, O)>);


Suggested change

pub struct UnityOrOuterConversion<C, O>(core::marker::PhantomData<(C, O)>);

pub struct OneToOneIfElse<C, O>(core::marker::PhantomData<(C, O)>);

or

Suggested change

pub struct UnityOrOuterConversion<C, O>(core::marker::PhantomData<(C, O)>);

pub struct OneToOneOrOuterConversion<C, O>(core::marker::PhantomData<(C, O)>);

I think it expresses what you want better

we have type above with a name - UnityAssetBalanceConversion, which also means 1:1. its not wrong and I would keep it consistent.

franciscoaguirre · 2024-04-15T15:40:23Z

prdoc/pr_3659.prdoc

+doc:
+  - audience: Runtime Dev
+    description: |
+      Introduce types to define 1:1 balance conversion for different relative asset ids/locations 


What is the purpose of this change? Should probably mention here to the users what has now changed in how the system is used.

not sure why you mean, this does not changes anything for users, only introduces few new types.
it does changes configuration for westend and rococo chains, but it changes it a way it is expected to work

polkadot/runtime/common/src/impls.rs

bkontur

one nit, I suggest renaming ContainsLatest to the ContainsAssetFrom

unity balance conversion

8acac24

muharem added the T1-FRAME This PR/Issue is related to core FRAME, the framework. label Mar 12, 2024

muharem requested review from a team as code owners March 12, 2024 12:01

muharem mentioned this pull request Mar 12, 2024

Treasury Spend Detects Relative Locations of Native Asset polkadot-fellows/runtimes#233

Merged

muharem added 5 commits March 12, 2024 13:10

fix

f7217a5

tests

870c2ec

prdoc

5d4bdb2

parachain is not sibling for itself

023d778

Merge remote-tracking branch 'origin/master' into muharem-unity-balan…

e311e50

…ce-conversion

bkontur approved these changes Mar 20, 2024

View reviewed changes

muharem and others added 2 commits March 20, 2024 15:32

remove unused imports

9d59fa3

Update polkadot/runtime/common/src/impls.rs

67ce378

Co-authored-by: Branislav Kontur <bkontur@gmail.com>

Merge remote-tracking branch 'origin/master' into muharem-unity-balan…

6569e30

…ce-conversion

github-actions bot requested a review from bkontur April 15, 2024 15:29

franciscoaguirre reviewed Apr 15, 2024

View reviewed changes

fix test

190e224

franciscoaguirre approved these changes Apr 16, 2024

View reviewed changes

bkontur reviewed Apr 16, 2024

View reviewed changes

polkadot/runtime/common/src/impls.rs Outdated Show resolved Hide resolved

bkontur approved these changes Apr 16, 2024

View reviewed changes

muharem added 2 commits April 16, 2024 17:11

contains parts and contains pair

d3fcd1c

fix

d294d38

muharem added this pull request to the merge queue Apr 16, 2024

Merged via the queue into master with commit 6f3d890 Apr 16, 2024
133 of 136 checks passed

muharem deleted the muharem-unity-balance-conversion branch April 16, 2024 16:38

This was referenced Jun 5, 2024

Update polkadot-sdk from v1.7.0 to v1.11.0 moondance-labs/tanssi#573

Closed

Update polkadot-sdk from v1.10.0 to v1.11.0 moondance-labs/tanssi#577

Closed

This was referenced Jun 5, 2024

Update polkadot-sdk from v1.7.0 to v1.11.0 moondance-labs/tanssi#578

Closed

Update polkadot-sdk from v1.10.0 to v1.11.0 moondance-labs/tanssi#579

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FRAME: Unity Balance Conversion for Different IDs of Native Asset #3659

FRAME: Unity Balance Conversion for Different IDs of Native Asset #3659

muharem commented Mar 12, 2024 •

edited

Loading

bkontur left a comment

bkontur Mar 20, 2024

muharem Mar 20, 2024

bkontur Mar 20, 2024

muharem Mar 25, 2024 •

edited

Loading

bkontur Mar 26, 2024

muharem Apr 15, 2024

franciscoaguirre Apr 15, 2024

franciscoaguirre Apr 15, 2024

muharem Apr 15, 2024

franciscoaguirre Apr 16, 2024

github-actions bot commented Apr 15, 2024

franciscoaguirre Apr 15, 2024

muharem Apr 15, 2024

franciscoaguirre Apr 15, 2024

muharem Apr 15, 2024

franciscoaguirre Apr 15, 2024

muharem Apr 15, 2024

franciscoaguirre Apr 15, 2024

muharem Apr 15, 2024

franciscoaguirre Apr 15, 2024

muharem Apr 15, 2024

franciscoaguirre Apr 15, 2024

muharem Apr 15, 2024

franciscoaguirre Apr 15, 2024

muharem Apr 15, 2024

franciscoaguirre Apr 15, 2024

muharem Apr 15, 2024

bkontur left a comment

		location: asset_hub_location.clone(),
		asset_id: native_asset.into(),

		let fellowship_treasury_location: Location =
		Location::new(1, [Parachain(1001), PalletInstance(65)]);

	pub struct UnityOrOuterConversion<C, O>(core::marker::PhantomData<(C, O)>);
	pub struct OneToOneIfElse<C, O>(core::marker::PhantomData<(C, O)>);

	pub struct UnityOrOuterConversion<C, O>(core::marker::PhantomData<(C, O)>);
	pub struct OneToOneOrOuterConversion<C, O>(core::marker::PhantomData<(C, O)>);

FRAME: Unity Balance Conversion for Different IDs of Native Asset #3659

FRAME: Unity Balance Conversion for Different IDs of Native Asset #3659

Conversation

muharem commented Mar 12, 2024 • edited Loading

bkontur left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

muharem Mar 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Apr 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bkontur left a comment

Choose a reason for hiding this comment

muharem commented Mar 12, 2024 •

edited

Loading

muharem Mar 25, 2024 •

edited

Loading