Improve Penpal runtime/emulated tests + AssetHub `TokenLocationAsNative` #3339

NachoPal · 2024-02-15T14:38:23Z

Before I continue working on this PR (apply changes to AssetHubWestend too), I would appreciate some feedback @acatangiu @joepetrowski @KiChjang @bkontur to see if what I am proposing is acceptable.

I am adding TokenLocationAsNative = (0, Here) in the AssetHub runtime to be recognised as local currency too, being in practice (0, Here) & (1, Here) equivalents. I need this change to make reserve_transfer_asset() / transfer_asset() work for a reserve transfer between Parachains through AssetHub. The reason is how these pallet_xcm methods determine the Remote Reserve Location, where the location is extracted from the asset location to be sent.

If I try to send from a Parachain the asset (1, Here), the Remote Reserve Location would be the Relay Chain, whereas if I try to send (1, Parachain(1000)), the Remote Reserve Location would be AsseHub, which is correct, but the received asset has been reanchored giving as result (0, Here) which is not recognised by AssetHub.

I know that in principle a runtime shouldn't be changed to make a pallet work, but I do not see anything wrong about accepting (0, Here) as local currency too (which actually is the local currency), but I might be overlooking something. The pallet is well designed, but it is not working for this special case where the local currency is recognised as (1, Here) instead of (0, Here).

In addition to the previous change, there are other modifications in the PR that remain valid regardless:

Improve Penpal's runtime to properly handle received assets. Before it was treating (1, Here) as local currency, updating its native currency balances, while it should be treated as a ForeignAsset. That wasn't a great example of a standard Parachain's behaviour, since none treats the System asset as local currency.
- Two ForeignAssets were registered in genesis:
  - (1, Here) - Relay Chain native asset
  - (1, Parachain(1000)) - AssetHub native asset
- Emulated integration tests were changed accordingly
Improve hygiene for include_penpal_create_foreign_asset_on_asset_hub macro.
Prevent integration tests crates imports from being re-exported, they were polluting the polkadot-sdk docs.
Includes: Fix DepositReserveAsset fees payment #3340

paritytech-cicd-pr · 2024-02-16T18:15:26Z

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: cargo-clippy
Logs: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/5242085

acatangiu

I am supportive of the penpal changes and all the emulated tests cleanup and improvements, but don't agree with the AssetHub config change without going through an RFC. That change needs to happen at the ecosystem level, it is not just an AH-local change (see in-line comments bellow).

acatangiu · 2024-02-19T08:04:23Z

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

@@ -114,18 +110,20 @@ fn para_dest_assertions(t: RelayToSystemParaTest) {

 fn penpal_to_ah_foreign_assets_sender_assertions(t: ParaToSystemParaTest) {
 	type RuntimeEvent = <PenpalA as Chain>::RuntimeEvent;
-	PenpalA::assert_xcm_pallet_attempted_complete(None);


why remove this assertion?

acatangiu · 2024-02-19T08:35:18Z

...hains/integration-tests/emulated/tests/assets/asset-hub-rococo/src/tests/reserve_transfer.rs

+	let sender = PenpalASender::get();
+	let amount_to_send: Balance = ASSET_HUB_ROCOCO_ED * 10000;
+	let asset_owner = penpal_asset_owner();
+	let assets: Assets = ((Parent, Parachain(ASSETHUB_PARA_ID)), amount_to_send).into();


What is this asset?

IIUC this PR makes this sort of a "symlink" to RelayNative. Aka you propose that all parachains be able to interchangeably say (1, Parachain(ASSETHUB_PARA_ID)) and (1, Here) while still referring to DOT/KSM.

I don't see anything wrong with this on the surface, but it is a big conceptual change that the whole ecosystem needs to be aware of (and align to). In my opinion this definitely needs a Polkadot RFC before implementing/merging.

acatangiu · 2024-02-19T08:45:31Z

cumulus/parachains/runtimes/assets/asset-hub-rococo/src/xcm_config.rs

@@ -117,7 +120,7 @@ pub type FungibleTransactor = FungibleAdapter<
 	// Use this currency:
 	Balances,
 	// Use this currency when it is a fungible asset matching the given location or name:
-	IsConcrete<TokenLocation>,
+	(IsConcrete<TokenLocation>, IsConcrete<TokenLocationAsNative>),


Every parachain in the ecosystem needs to do an equivalent change to their runtime, If we make this equivalence in AssetHub where RelayLocation and AssetHubLocation are both describing DOT/KSM asset.

Otherwise one parachain could align early with our change, then send DOT to another parachain, and send it through AH by referring to the DOT as (1, Parachain(AssetHubId)). AH will happily handle the reserve transfer and send it to the final/destination parachain, but that chain doesn't (yet) recognize (1, Parachain(AssetHubId)) as being the same asset as (1, Here).

That's why this equivalence needs to be done at the protocol/ecosystem level through https://github.com/polkadot-fellows/RFCs

acatangiu · 2024-02-20T16:37:19Z

I would split this PR in two:

all the tests improvements, including the penpal change to have its own native asset in pallet_balances while moving ROC/WND to pallet_assets (includes most of the changes in current PR, non-controversial)
the (more controversial) change to treat "asset with id == location of AssetHub" as equivalent to "asset with id == location of Relay chain" (ROC/WND/KSM/DOT) + the dedicated test for reserve transfers of DOT through AH reserve

NachoPal · 2024-02-27T11:03:30Z

all the tests improvements, including the penpal change to have its own native asset in pallet_balances while moving ROC/WND to pallet_assets (includes most of the changes in current PR, non-controversial)

I was going to do this but I actually can not. The reason is that if the system asset in AssetHub is recognised as (1, Here), what Penpal receives is also (1, Here), not (1, Parachain(1000)). The problem arises from needing a runtime (Penpal) that recognises two reserves for the same asset. To make it work, I could modify Penpal to have a single foreign asset (1, Here) for both reserves (Relay and AssetHub).

NachoPal · 2024-03-01T14:03:42Z

@acatangiu Opened a new PR without the controversial changes in: #3543

Issues addressed in this PR: - Improve *Penpal* runtime: - Properly handled received assets. Previously, it treated `(1, Here)` as the local native currency, whereas it should be treated as a `ForeignAsset`. This wasn't a great example of standard Parachain behaviour, as no Parachain treats the system asset as the local currency. - Remove `AllowExplicitUnpaidExecutionFrom` the system. Again, this wasn't a great example of standard Parachain behaviour. - Move duplicated `ForeignAssetFeeAsExistentialDepositMultiplierFeeCharger` to `assets_common` crate. - Improve emulated tests: - Update *Penpal* tests to new runtime. - To simplify tests, register the reserve transferred, teleported, and system assets in *Penpal* and *AssetHub* genesis. This saves us from having to create the assets repeatedly for each test - Add missing test case: `reserve_transfer_assets_from_para_to_system_para`. - Cleanup. - Prevent integration tests crates imports from being re-exported, as they were polluting the `polkadot-sdk` docs. There is still a test case missing for reserve transfers: - Reserve transfer of system asset from *Parachain* to *Parachain* trough *AssetHub*. - This is not yet possible with `pallet-xcm` due to the reasons explained in #3339 --------- Co-authored-by: command-bot <>

Issues addressed in this PR: - Improve *Penpal* runtime: - Properly handled received assets. Previously, it treated `(1, Here)` as the local native currency, whereas it should be treated as a `ForeignAsset`. This wasn't a great example of standard Parachain behaviour, as no Parachain treats the system asset as the local currency. - Remove `AllowExplicitUnpaidExecutionFrom` the system. Again, this wasn't a great example of standard Parachain behaviour. - Move duplicated `ForeignAssetFeeAsExistentialDepositMultiplierFeeCharger` to `assets_common` crate. - Improve emulated tests: - Update *Penpal* tests to new runtime. - To simplify tests, register the reserve transferred, teleported, and system assets in *Penpal* and *AssetHub* genesis. This saves us from having to create the assets repeatedly for each test - Add missing test case: `reserve_transfer_assets_from_para_to_system_para`. - Cleanup. - Prevent integration tests crates imports from being re-exported, as they were polluting the `polkadot-sdk` docs. There is still a test case missing for reserve transfers: - Reserve transfer of system asset from *Parachain* to *Parachain* trough *AssetHub*. - This is not yet possible with `pallet-xcm` due to the reasons explained in paritytech#3339 --------- Co-authored-by: command-bot <>

improve penpal + assethub local asset change

5c21fd6

NachoPal mentioned this pull request Feb 16, 2024

Fix DepositReserveAsset fees payment #3340

Merged

merge master

79792af

acatangiu reviewed Feb 19, 2024

View reviewed changes

muharem self-requested a review February 27, 2024 13:11

NachoPal mentioned this pull request Mar 1, 2024

Improve Penpal runtime + emulated tests #3543

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve Penpal runtime/emulated tests + AssetHub `TokenLocationAsNative` #3339

Improve Penpal runtime/emulated tests + AssetHub `TokenLocationAsNative` #3339

NachoPal commented Feb 15, 2024 •

edited

Loading

paritytech-cicd-pr commented Feb 16, 2024

acatangiu left a comment

acatangiu Feb 19, 2024

acatangiu Feb 19, 2024 •

edited

Loading

acatangiu Feb 19, 2024

acatangiu commented Feb 20, 2024 •

edited

Loading

NachoPal commented Feb 27, 2024

NachoPal commented Mar 1, 2024

Improve Penpal runtime/emulated tests + AssetHub TokenLocationAsNative #3339

Are you sure you want to change the base?

Improve Penpal runtime/emulated tests + AssetHub TokenLocationAsNative #3339

Conversation

NachoPal commented Feb 15, 2024 • edited Loading

paritytech-cicd-pr commented Feb 16, 2024

acatangiu left a comment

Choose a reason for hiding this comment

acatangiu Feb 19, 2024

Choose a reason for hiding this comment

acatangiu Feb 19, 2024 • edited Loading

Choose a reason for hiding this comment

acatangiu Feb 19, 2024

Choose a reason for hiding this comment

acatangiu commented Feb 20, 2024 • edited Loading

NachoPal commented Feb 27, 2024

NachoPal commented Mar 1, 2024

Improve Penpal runtime/emulated tests + AssetHub `TokenLocationAsNative` #3339

Improve Penpal runtime/emulated tests + AssetHub `TokenLocationAsNative` #3339

NachoPal commented Feb 15, 2024 •

edited

Loading

acatangiu Feb 19, 2024 •

edited

Loading

acatangiu commented Feb 20, 2024 •

edited

Loading