Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Xcm emulator nits #1649

Merged
merged 17 commits into from
Oct 11, 2023
Merged

Xcm emulator nits #1649

merged 17 commits into from
Oct 11, 2023

Conversation

bkontur
Copy link
Contributor

@bkontur bkontur commented Sep 20, 2023

Desription

Summary

This PR introduces several nits and tweaks to xcm emulator tests for system parachains.

Explanation

Deduplicate XcmPallet::send( with root origin code

  • Introduced send_transact_to_parachain which could be easily reuse for scenarios like governance call from relay chain to parachain.

Refactor send_transact_sudo_from_relay_to_system_para_works

  • Test covered just one use-case which was moved to the do_force_create_asset_from_relay_to_system_para, so now we can extend this test with more governance-like senarios.
  • Renamed to send_transact_as_superuser_from_relay_to_system_para_works.

Remove send_transact_native_from_relay_to_system_para_fails test

  • This test and/or description is kind of misleading, because system paras support Native from relay chain by RelayChainAsNative with correct xcm origin.
  • It tested only sending on relay chain which should go directly to the relay chain unit-tests (does not even need to be in xcm emulator level).

Future directions

Check restructure parachains integration tests issue and PR with more TODOs.

@bkontur bkontur added T9-cumulus This PR/Issue is related to cumulus. T14-system_parachains This PR/Issue is related to system parachains. labels Sep 20, 2023
@bkontur bkontur marked this pull request as ready for review September 25, 2023 10:52
@bkontur bkontur changed the title (draft) Xcm emulator nits Xcm emulator nits Sep 25, 2023
@bkontur bkontur requested a review from muharem October 8, 2023 08:00
use $crate::impls::{bx, Chain, RelayChain};

<Self as $crate::impls::TestExt>::execute_with(|| {
let root_origin = <Self as Chain>::RuntimeOrigin::root();
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we assuming that is going to be always root? Someone would like to test sending a Transact with different origin. I think we should either accept origin as an extra argument for send_transact_to_parachain or rename method to send_transact_to_parachain_as_sudo (better the first option)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, this is meant for governance-like calls from relay to para with RuntimeOrigin::root() and UnpaidExecution not to repeat the same code everywhere:

/// A root origin (as governance) sends `xcm::Transact` with encoded `call` to child parachain.

If we want it make it more general, then yes origin and also some "switch" for UnpaidExecution/BuyExecution wrapper should be added as a parameter. Then we should add it as a generic helper function for Chain trait.

I think, for me, send_transact_to_parachain_as_sudo means more like there is pallet_sudo which is not the case.

I am inclined to keep it as it is :).
Anyway, anybody else can still create whatever scenario with [<$chain Pallet>]>::XcmPallet::send.

@NachoPal wdyt?

Copy link
Contributor

@NachoPal NachoPal Oct 9, 2023

Choose a reason for hiding this comment

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

Didn't realise about the hardcoded UnpaidExcution. Are there cases where an UnpaidExecution is going to work with a different origin than Superuser? If not then I do not see it necessary to have it as function argument.

Sorry, I sometimes use sudo and root interchangeably. If it is meant to be for governance-like calls (root) then we can do:

pub fn send_transact_to_parachain_as_root(
					recipient: $crate::impls::ParaId,
					call: $crate::impls::DoubleEncoded<()>
				)

What I am trying to avoid is to impl a method that is only meant to work for root, UnpaidExecution and Origin::Superuser with a so generic name - send_transact_to_parachain - (even if it is properly documented)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, I renamed to send_unpaid_transact_to_parachain_as_root, please, check here: c30119c
I also was able to reuse do_force_create_asset_from_relay_as_root for force_create_and_mint_asset, which looks also much clear now

@bkontur bkontur requested a review from NachoPal October 9, 2023 10:15
Copy link
Contributor

@NachoPal NachoPal left a comment

Choose a reason for hiding this comment

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

Just small nit to keep consistent with naming

@bkontur bkontur requested a review from NachoPal October 9, 2023 18:43
@bkontur bkontur enabled auto-merge (squash) October 10, 2023 13:07
Copy link
Contributor

@seadanda seadanda left a comment

Choose a reason for hiding this comment

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

LGTM

@bkontur bkontur merged commit cfb2925 into master Oct 11, 2023
12 checks passed
@bkontur bkontur deleted the bko-xcm-emulator-nits branch October 11, 2023 08:32
ordian added a commit that referenced this pull request Oct 12, 2023
* master: (33 commits)
  ci: set CI_IMAGE back to (now updated) .ci-unified (#1854)
  ci: bump ci image to rust 1.73.0 (#1830)
  Refactor Identity to benchmark v2 (#1838)
  PVF worker: bump landlock, update ABI docs (#1850)
  Xcm emulator nits (#1649)
  Fixes path issue in derive-impl (#1823)
  upgrade to macro_magic 0.4.3 (#1832)
  Use safe math when pruning statuses (#1835)
  remote-ext: fix state download stall on slow connections and reduce memory usage (#1295)
  Update testnet bootnode dns name (#1712)
  [FRAME] Warn on unchecked weight witness (#1818)
  [xcm] Use `Weight::MAX` for `reserve_asset_deposited`, `receive_teleported_asset` benchmarks (#1726)
  Update bridges subtree (#1803)
  Check for parent of first ready block being on chain (#1812)
  Make CheckNonce refuse transactions signed by accounts with no providers (#1578)
  Fix Asset Hub collator crashing when starting from genesis (#1788)
  Mixnet integration (#1346)
  [xcm-emulator] Decouple the `AccountId` type from `AccountId32` (#1458)
  Treasury spends various asset kinds (#1333)
  chore: bump zombienter version (#1806)
  ...
serban300 pushed a commit to serban300/polkadot-sdk that referenced this pull request Mar 26, 2024
serban300 pushed a commit to serban300/polkadot-sdk that referenced this pull request Mar 27, 2024
serban300 pushed a commit to serban300/polkadot-sdk that referenced this pull request Apr 8, 2024
serban300 pushed a commit to serban300/polkadot-sdk that referenced this pull request Apr 8, 2024
serban300 pushed a commit to serban300/polkadot-sdk that referenced this pull request Apr 8, 2024
serban300 pushed a commit to serban300/polkadot-sdk that referenced this pull request Apr 8, 2024
serban300 pushed a commit to serban300/polkadot-sdk that referenced this pull request Apr 8, 2024
serban300 pushed a commit to serban300/polkadot-sdk that referenced this pull request Apr 9, 2024
serban300 pushed a commit to serban300/polkadot-sdk that referenced this pull request Apr 9, 2024
serban300 pushed a commit to serban300/polkadot-sdk that referenced this pull request Apr 9, 2024
serban300 pushed a commit to serban300/polkadot-sdk that referenced this pull request Apr 9, 2024
serban300 pushed a commit to serban300/polkadot-sdk that referenced this pull request Apr 9, 2024
serban300 pushed a commit to serban300/polkadot-sdk that referenced this pull request Apr 9, 2024
serban300 pushed a commit to serban300/polkadot-sdk that referenced this pull request Apr 10, 2024
serban300 pushed a commit to serban300/polkadot-sdk that referenced this pull request Apr 10, 2024
bkchr pushed a commit that referenced this pull request Apr 10, 2024
* remove callbacks

* clippy

* fmt
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T9-cumulus This PR/Issue is related to cumulus. T14-system_parachains This PR/Issue is related to system parachains.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants