Treasury Spend Detects Relative Locations of Native Asset #233

muharem · 2024-03-11T17:10:28Z

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

joepetrowski · 2024-03-11T19:12:29Z

relay/polkadot/src/impls.rs

+			V3 { location, asset_id } => (location.try_into()?, asset_id.try_into()?),
+			V4 { location, asset_id } => (location, asset_id),
+		};
+		if asset_id.0.contains_parents_only(1) &&


From the Relay Chain, wouldn't this be contains_parents_only(0)?

asset_id will be relative to the parachain, this is why it's only parent 1
when I tried (0, here) on Asset Hub, it was not recognised as native asset, only (1, here)

Ah, this is checking a message from AH, and it hasn't been reanchored.

Treasury pallet receives the locatable asset as

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

and it should know it's a native asset

updated the PR description to include the example

You should add it to this type's (or impl's) doc comment as well

joepetrowski

A few questions, mostly about origins/locations.

...ation-tests/emulated/tests/collectives/collectives-polkadot/src/tests/fellowship_treasury.rs

joepetrowski · 2024-03-13T15:55:28Z

...ation-tests/emulated/tests/collectives/collectives-polkadot/src/tests/fellowship_treasury.rs

+			as_origin: bx!(OriginCaller::system(RawOrigin::Signed(treasury_account))),
+			call: bx!(RuntimeCall::XcmPallet(pallet_xcm::Call::<Runtime>::teleport_assets {
+				dest: bx!(VersionedLocation::V4(asset_hub_location.clone())),
+				beneficiary: bx!(VersionedLocation::V4(treasury_location_on_asset_hub)),


Interesting that the beneficiary is actually the Location of the Relay's Treasury pallet and not the converted local Account ID.

Since the implementation Pay trait which used to execute the spend uses xcm TransferAsset instruction, we can use the pallet's location from which the xcm-executor will derive the account id.

joepetrowski · 2024-03-13T15:57:00Z

...ation-tests/emulated/tests/collectives/collectives-polkadot/src/tests/fellowship_treasury.rs

+		// Dispatched from Root to `despatch_as` `Signed(treasury_account)`.
+		assert_ok!(teleport_call.dispatch(root));


We allow Treasurer to send messages. Could we just do a referendum to teleport_assets on the Treasurer track?
https://github.com/polkadot-fellows/runtimes/blob/ba359a4/relay/polkadot/src/xcm_config.rs#L286

In the type SovereignAccountOf we will need to associate a Treasurer Origin with Treasury::account_id(). Not sure if this is right, I think pallet's location account id can be Treasury::account_id(). And also it would mean that Treasurer Origin can spend any amount from Treasury.

joepetrowski · 2024-03-13T15:58:38Z

...ation-tests/emulated/tests/collectives/collectives-polkadot/src/tests/fellowship_treasury.rs

+		let treasury_origin: RuntimeOrigin =
+			polkadot_runtime::governance::pallet_custom_origins::Origin::Treasurer.into();
+		let fellowship_treasury_location: Location =
+			Location::new(1, [Parachain(1001), PalletInstance(65)]);
+		let asset_hub_location: Location = [Parachain(1000)].into();
+		let native_asset_on_asset_hub = Location::parent();


treasury_origin is on the Relay Chain, but all the others are defined from the perspective of Asset Hub. Got a bit confused with this. Perhaps a comment to clarify?

asset_kind = {
location - or destination, from the perspective of Relay Chain,
asset_id - from the perspective of asset_kind.location
}
beneficiary - from the perspective of asset_kind.location

Add that as a comment in the code maybe?

We have all this documented in the functions' docs.

Co-authored-by: joe petrowski <25483142+joepetrowski@users.noreply.github.com>

integration-tests/emulated/tests/collectives/collectives-polkadot/src/lib.rs

ggwpez · 2024-03-18T12:07:17Z

relay/kusama/src/impls.rs

+		{
+			Ok(balance)
+		} else {
+			I::from_asset_balance(balance, asset_kind).map_err(|_| ())


This hinders debuggability. Can you please log the error?

In this particular case, upstream we do not really care about type of error and there is only one reason for error.
But for a general implementation I will add the log.

ggwpez · 2024-03-18T12:07:48Z

relay/polkadot/src/impls.rs

+		{
+			Ok(balance)
+		} else {
+			I::from_asset_balance(balance, asset_kind).map_err(|_| ())


system-parachains/collectives/collectives-polkadot/src/impls.rs

ggwpez · 2024-03-18T12:11:26Z

system-parachains/collectives/collectives-polkadot/src/impls.rs

+		{
+			Ok(balance)
+		} else {
+			I::from_asset_balance(balance, asset_kind).map_err(|_| ())


ggwpez

I dont understand enough of the code, so please consider further review before merging.

…achain-for-asset-rate

muharem · 2024-03-18T14:57:57Z

@acatangiu can you please check this PR

muharem · 2024-03-18T15:38:12Z

@bkchr can you please check this PR

system-parachains/collectives/collectives-polkadot/src/impls.rs

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

franciscoaguirre

The whole LocatableAsset thing seems very convoluted, it'd be good to simplify it in the future.
It looks like it works though

integration-tests/emulated/tests/assets/asset-hub-kusama/src/tests/treasury.rs

franciscoaguirre · 2024-03-18T16:12:14Z

integration-tests/emulated/tests/assets/asset-hub-kusama/src/tests/treasury.rs

+
+		let native_asset = Location::here();
+		let asset_hub_location: Location = [Parachain(1000)].into();
+		let treasury_location_on_asset_hub: Location = (Parent, PalletInstance(18)).into();


The name is confusing. Previous variables are relative to the relay, this one starts with Parent but says on_asset_hub, leading to believe that it should be relative to the relay as well.

Looks like you want the relay's treasury sovereign account. I'd probably name it relay_treasury_location and add a comment.

I try to avoid a prefix relay for the treasury. We have only one the Treasury, or the Polkadot/Kusama Treasury, which eventually will move from Relay chain. I rename it to treasury_location.

integration-tests/emulated/tests/assets/asset-hub-kusama/src/tests/treasury.rs

franciscoaguirre · 2024-03-18T16:51:33Z

...ation-tests/emulated/tests/collectives/collectives-polkadot/src/tests/fellowship_treasury.rs

+
+		let native_asset = Location::here();
+		let asset_hub_location: Location = [Parachain(1000)].into();
+		let treasury_location_on_asset_hub: Location = (Parent, PalletInstance(19)).into();


The pallet index is different, should this be called collectives_treasury_location?
Also, maybe add a constant for the magic number 19 or even get it from the pallet itself with PalletAccessInfo, that'd be great

it's different, because this is polkadot's treasury

franciscoaguirre · 2024-03-18T16:53:34Z

...ation-tests/emulated/tests/collectives/collectives-polkadot/src/tests/fellowship_treasury.rs

+		let treasury_origin: RuntimeOrigin =
+			polkadot_runtime::governance::pallet_custom_origins::Origin::Treasurer.into();
+		let fellowship_treasury_location: Location =
+			Location::new(1, [Parachain(1001), PalletInstance(65)]);


Yeah I definitely think we should try to access the actual pallet's index for these locations

I do not have that strong opinion about this. When I use literals, I want this test fail if for some reason pallet's index changes.

franciscoaguirre · 2024-03-18T16:54:03Z

...ation-tests/emulated/tests/collectives/collectives-polkadot/src/tests/fellowship_treasury.rs

+		let treasury_origin: RuntimeOrigin =
+			polkadot_runtime::governance::pallet_custom_origins::Origin::Treasurer.into();
+		let fellowship_treasury_location: Location =
+			Location::new(1, [Parachain(1001), PalletInstance(65)]);
+		let asset_hub_location: Location = [Parachain(1000)].into();
+		let native_asset_on_asset_hub = Location::parent();


Add that as a comment in the code maybe?

relay/polkadot/src/impls.rs

franciscoaguirre · 2024-03-18T16:57:17Z

relay/polkadot/src/impls.rs

+			V3 { location, asset_id } => (location.try_into()?, asset_id.try_into()?),
+			V4 { location, asset_id } => (location, asset_id),
+		};
+		if asset_id.0.contains_parents_only(1) &&


You should add it to this type's (or impl's) doc comment as well

muharem · 2024-03-19T11:46:15Z

The whole LocatableAsset thing seems very convoluted, it'd be good to simplify it in the future. It looks like it works though

Do you have an idea how? The origin for the current structure comes from this type - https://github.com/paritytech/polkadot-sdk/blob/430ad2f5610dc3d53b4708c651617e34affa97f7/polkadot/xcm/xcm-builder/src/pay.rs#L193

system-parachains/collectives/collectives-polkadot/src/fellowship/mod.rs

integration-tests/emulated/tests/assets/asset-hub-kusama/src/tests/treasury.rs

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

bkontur · 2024-03-20T11:11:10Z

relay/polkadot/src/impls.rs

+pub struct NativeOnSystemParachain<I>(PhantomData<I>);
+impl<I> ConversionFromAssetBalance<Balance, VersionedLocatableAsset, Balance>
+	for NativeOnSystemParachain<I>


Suggested change

pub struct NativeOnSystemParachain(PhantomData);

impl ConversionFromAssetBalance<Balance, VersionedLocatableAsset, Balance>

for NativeOnSystemParachain

pub struct NativeOnSystemParachainElseConvertWith(PhantomData);

impl ConversionFromAssetBalance<Balance, VersionedLocatableAsset, Balance>

for NativeOnSystemParachainElseConvertWith

I like my version more, I personally prefer short and laconic names with docs, instead long names.
But if you get one more vote for your rename suggestion, I rename

well, I don't mind longer names if they are self-describing :)
e.g. NativeOnSiblingParachain does not mention in the docs that it filters just system parachains, so I think at least NativeOnSystemParachain or NativeOnSiblingSystemParachain is better

well, I don't mind longer names if they are self-describing :) e.g. NativeOnSiblingParachain does not mention in the docs that it filters just system parachains, so I think at least NativeOnSystemParachain or NativeOnSiblingSystemParachain is better

@muharem to avoid any confusions (e.g. NativeOnSibling, can be ACA or whatever), why not to just rename all of NativeOnSiblingParachain and NativeOnSystemParachain to the final UnityOrOuterConversion temporary with several impls here? When we do polkadot-sdk bump with paritytech/polkadot-sdk#3659, it wil just "change imports"

bkontur · 2024-03-20T11:11:31Z

relay/kusama/src/impls.rs

+pub struct NativeOnSystemParachain<I>(PhantomData<I>);
+impl<I> ConversionFromAssetBalance<Balance, VersionedLocatableAsset, Balance>
+	for NativeOnSystemParachain<I>


Suggested change

pub struct NativeOnSystemParachain(PhantomData);

impl ConversionFromAssetBalance<Balance, VersionedLocatableAsset, Balance>

for NativeOnSystemParachain

pub struct NativeOnSystemParachainElseConvertWith(PhantomData);

impl ConversionFromAssetBalance<Balance, VersionedLocatableAsset, Balance>

for NativeOnSystemParachainElseConvertWith

bkontur · 2024-03-20T11:14:09Z

system-parachains/collectives/collectives-polkadot/src/impls.rs

+pub struct NativeOnSiblingParachain<I, SelfParaId>(PhantomData<(I, SelfParaId)>);
+impl<I, SelfParaId> ConversionFromAssetBalance<Balance, VersionedLocatableAsset, Balance>
+	for NativeOnSiblingParachain<I, SelfParaId>


Suggested change

pub struct NativeOnSiblingParachain<I, SelfParaId>(PhantomData<(I, SelfParaId)>);

impl<I, SelfParaId> ConversionFromAssetBalance<Balance, VersionedLocatableAsset, Balance>

for NativeOnSiblingParachain<I, SelfParaId>

pub struct NativeOnSystemParachainElseConvertWith<I, SelfParaId>(PhantomData<(I, SelfParaId)>);

impl<I, SelfParaId> ConversionFromAssetBalance<Balance, VersionedLocatableAsset, Balance>

for NativeOnSystemParachainElseConvertWith<I, SelfParaId>

bkontur · 2024-03-20T11:40:00Z

system-parachains/collectives/collectives-polkadot/src/impls.rs

+		if asset_id.0.contains_parents_only(1) &&
+			IsSiblingSystemParachain::<ParaId, SelfParaId>::contains(&location)


I think that the NativeOnSiblingParachain for collectives and NativeOnSystemParachain for relays are "almost" the same implementation and differ only with the different ::contains(&location) implementation for matching system parachains.

I'm also not sure about the same asset_id.0.contains_parents_only(1) in both implementations, but regardless of that, they are exactly the same. I would suggest creating just one generic implementation and placing it in the polkadot-sdk repository somewhere. As a temporary solution, with a TODO, it could be placed in the system-parachains-constants repository. Yes, it may be ugly, but it serves the purpose. Alternatively, you could copy the exact same generic implementation where needed and remove it later.

Suggested generic implementation:

pub struct NativeOnSystemParachainElseConvertWith<I, N, IsSystemParachain>(PhantomData<(I, N, IsSystemParachain)>); impl<I, N, IsSystemParachain> ConversionFromAssetBalance<Balance, VersionedLocatableAsset, Balance> for NativeOnSystemParachainElseConvertWith<I, N, IsSystemParachain> where I: ConversionFromAssetBalance<Balance, VersionedLocatableAsset, Balance>, IsNativeAsset: Contains<Location> // <-- or maybe Contains<AssetId>?, we have already matchers for Contains<Location> IsSystemParachain: Contains<Location>, { type Error = (); fn from_asset_balance( balance: Balance, asset_kind: VersionedLocatableAsset, ) -> Result<Balance, Self::Error> { use VersionedLocatableAsset::*; let (location, asset_id) = match asset_kind.clone() { V3 { location, asset_id } => (location.try_into()?, asset_id.try_into()?), V4 { location, asset_id } => (location, asset_id), }; if IsNativeAsset::contains(asset_id.0) && IsSystemParachain::contains(&location) { Ok(balance) } else { I::from_asset_balance(balance, asset_kind).map_err(|_| ()) } } #[cfg(feature = "runtime-benchmarks")] fn ensure_successful(asset_kind: VersionedLocatableAsset) { I::ensure_successful(asset_kind) } }

so with this one generic implementation, it can be used for both:

// for collectives type BalanceConverter = NativeOnSystemParachainElseConvertWith< AssetRate, Equals<xcm_config::DotLocation> IsSiblingSystemParachain<ParaId, ParachainInfo>, >; // for relays type BalanceConverter = NativeOnSystemParachainElseConvertWith< AssetRate, ( Equals<DotLocation> Equals<DotLocationWithParent1> // <- kind of OR matching ), IsChildSystemParachain::<ParaId>, >;

PR description referes to polkadot-sdk version - paritytech/polkadot-sdk#3659

PR description referes to polkadot-sdk version - paritytech/polkadot-sdk#3659

ahaaa, ok, I see, looks like it does exactly the same even more generic way, cool :)

…achain-for-asset-rate

bkontur

lgtm, as it will be changed later by paritytech/polkadot-sdk#3659

muharem · 2024-03-21T11:40:50Z

@ordian can you please check this PR

bkchr · 2024-03-24T14:19:54Z

/merge

bkchr · 2024-03-24T14:34:22Z

/merge

fellowship-merge-bot · 2024-03-24T14:41:35Z

Enabled auto-merge in Pull Request

Available commands

/merge: Enables auto-merge for Pull Request
/merge cancel: Cancels auto-merge for Pull Request
/merge help: Shows this menu

For more information see the documentation

Introduce teleport and whitelist call tests for Polkadot Collectives. Based on #233 - [x] Does not require a CHANGELOG entry --------- Co-authored-by: joe petrowski <25483142+joepetrowski@users.noreply.github.com> Co-authored-by: Branislav Kontur <bkontur@gmail.com>

muharem added 3 commits March 11, 2024 17:05

balance conversion implementation for native asset

fc8bcb0

fellowship treasury spend test

f64b6da

kusama tests

fa5136f

joepetrowski reviewed Mar 11, 2024

View reviewed changes

muharem changed the title ~~Treasury Spend determines relative locations of native asset~~ Treasury Spend Detects Relative Locations of Native Asset Mar 13, 2024

muharem requested a review from joepetrowski March 13, 2024 09:36

changelog

1cb4f6d

joepetrowski approved these changes Mar 13, 2024

View reviewed changes

muharem and others added 2 commits March 13, 2024 17:48

Apply suggestions from code review

81a0a86

Co-authored-by: joe petrowski <25483142+joepetrowski@users.noreply.github.com>

Merge branch 'main' into muharem-native-on-parachain-for-asset-rate

646db40

joepetrowski mentioned this pull request Mar 15, 2024

1.2.0 Release #140

Closed

19 tasks

Merge branch 'main' into muharem-native-on-parachain-for-asset-rate

f1e63db

ggwpez reviewed Mar 18, 2024

View reviewed changes

integration-tests/emulated/tests/collectives/collectives-polkadot/src/lib.rs Outdated Show resolved Hide resolved

ggwpez reviewed Mar 18, 2024

View reviewed changes

relay/polkadot/src/impls.rs

{

Ok(balance)

} else {

I::from_asset_balance(balance, asset_kind).map_err(|_| ())

Copy link

Member

ggwpez Mar 18, 2024

Choose a reason for hiding this comment

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

ditto

ggwpez reviewed Mar 18, 2024

View reviewed changes

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

ggwpez reviewed Mar 18, 2024

View reviewed changes

ggwpez approved these changes Mar 18, 2024

View reviewed changes

muharem added 2 commits March 18, 2024 15:44

review fixes

8094c44

Merge remote-tracking branch 'origin/main' into muharem-native-on-par…

e0cb671

…achain-for-asset-rate

bkontur reviewed Mar 18, 2024

View reviewed changes

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

Update system-parachains/collectives/collectives-polkadot/src/impls.rs

562a18d

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

franciscoaguirre approved these changes Mar 18, 2024

View reviewed changes

minor docs and naming fixes

239880a

bkontur reviewed Mar 19, 2024

View reviewed changes

muharem and others added 2 commits March 19, 2024 14:36

Apply suggestions from code review

a041dc8

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

Apply suggestions from code review

470a444

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

bkontur reviewed Mar 20, 2024

View reviewed changes

muharem added 3 commits March 20, 2024 14:36

Merge remote-tracking branch 'origin/main' into muharem-native-on-par…

a5c5bd7

…achain-for-asset-rate

TODOs

4e922b8

remove unused imports

f87c0e9

acatangiu approved these changes Mar 20, 2024

View reviewed changes

muharem mentioned this pull request Mar 20, 2024

Test: Fund and Spend Fellowship Treasury #227

Closed

empty commit

a97e959

bkontur approved these changes Mar 20, 2024

View reviewed changes

muharem added 2 commits March 21, 2024 12:41

Merge branch 'main' into muharem-native-on-parachain-for-asset-rate

5a515a4

fix docs

744591a

muharem mentioned this pull request Mar 21, 2024

Collectives tests: Teleports and Whitelist Call #251

Merged

1 task

bkchr approved these changes Mar 24, 2024

View reviewed changes

Merge branch 'main' into muharem-native-on-parachain-for-asset-rate

0781205

andresilva approved these changes Mar 24, 2024

View reviewed changes

fellowship-merge-bot bot enabled auto-merge (squash) March 24, 2024 14:41

fellowship-merge-bot bot merged commit 3ae3d29 into polkadot-fellows:main Mar 24, 2024
33 of 34 checks passed

		// Dispatched from Root to `despatch_as` `Signed(treasury_account)`.
		assert_ok!(teleport_call.dispatch(root));

		if asset_id.0.contains_parents_only(1) &&
		IsSiblingSystemParachain::<ParaId, SelfParaId>::contains(&location)

Treasury Spend Detects Relative Locations of Native Asset #233

Treasury Spend Detects Relative Locations of Native Asset #233

Conversation

muharem commented Mar 11, 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

joepetrowski 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

muharem Mar 13, 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

ggwpez left a comment

Choose a reason for hiding this comment

muharem commented Mar 18, 2024

muharem commented Mar 18, 2024

franciscoaguirre 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

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

muharem commented Mar 19, 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

bkontur left a comment

Choose a reason for hiding this comment

muharem commented Mar 21, 2024

bkchr commented Mar 24, 2024

bkchr commented Mar 24, 2024

fellowship-merge-bot bot commented Mar 24, 2024

muharem commented Mar 11, 2024 •

edited

Loading

muharem Mar 13, 2024 •

edited

Loading