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

Improve Penpal runtime + emulated tests #3543

Merged
merged 23 commits into from
Mar 14, 2024
Merged

Improve Penpal runtime + emulated tests #3543

merged 23 commits into from
Mar 14, 2024

Conversation

NachoPal
Copy link
Contributor

@NachoPal NachoPal commented Mar 1, 2024

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:

@NachoPal NachoPal added T14-system_parachains This PR/Issue is related to system parachains. R0-silent Changes should not be mentioned in any release notes labels Mar 1, 2024
@NachoPal
Copy link
Contributor Author

NachoPal commented Mar 1, 2024

bot fmt

@command-bot
Copy link

command-bot bot commented Mar 1, 2024

@NachoPal https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/5410423 was started for your command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh". Check out https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/pipelines?page=1&scope=all&username=group_605_bot to know what else is being executed currently.

Comment bot cancel 11-31935f3e-e51f-4120-86ce-e2e26f9b9740 to cancel this command or bot cancel to cancel all commands in this pull request.

@command-bot
Copy link

command-bot bot commented Mar 1, 2024

@NachoPal Command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh" has finished. Result: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/5410423 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/5410423/artifacts/download.

@NachoPal
Copy link
Contributor Author

NachoPal commented Mar 4, 2024

bot fmt

@command-bot
Copy link

command-bot bot commented Mar 4, 2024

@NachoPal https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/5426126 was started for your command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh". Check out https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/pipelines?page=1&scope=all&username=group_605_bot to know what else is being executed currently.

Comment bot cancel 1-2e3f115a-f527-4a3b-8cea-9f232734be87 to cancel this command or bot cancel to cancel all commands in this pull request.

@command-bot
Copy link

command-bot bot commented Mar 4, 2024

@NachoPal Command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh" has finished. Result: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/5426126 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/5426126/artifacts/download.

@NachoPal NachoPal requested a review from bkontur March 4, 2024 15:35
@NachoPal NachoPal enabled auto-merge March 5, 2024 09:14
@NachoPal
Copy link
Contributor Author

bot fmt

@command-bot
Copy link

command-bot bot commented Mar 11, 2024

@NachoPal https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/5494268 was started for your command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh". Check out https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/pipelines?page=1&scope=all&username=group_605_bot to know what else is being executed currently.

Comment bot cancel 1-b857f1c0-b790-425a-a6eb-0e91452ff520 to cancel this command or bot cancel to cancel all commands in this pull request.

@command-bot
Copy link

command-bot bot commented Mar 11, 2024

@NachoPal Command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh" has finished. Result: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/5494268 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/5494268/artifacts/download.

@NachoPal NachoPal requested a review from bkontur March 12, 2024 09:41
@NachoPal
Copy link
Contributor Author

@bkontur Addressed the comments, please take a look.

// This asset is added to AH as ForeignAsset and teleported between Penpal and AH
pub const TELEPORTABLE_ASSET_ID: u32 = 2;

pub const PENPAL_ID: u32 = 2000;
Copy link
Contributor

Choose a reason for hiding this comment

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

we have also: https://github.com/paritytech/polkadot-sdk/pull/3543/files#diff-528b20b59bd1a15fbb063551a37e733ffa9b1e61c2d6ac30039e8d15271f5122R29-R30

pub const PARA_ID_A: u32 = 2000;
pub const PARA_ID_B: u32 = 2001;

so shouldn't be PenpalTeleportableAssetLocation and PenpalSiblingSovereigAccount specific for A and/or B?
E.g.:
PenpalATeleportableAssetLocation
PenpalBTeleportableAssetLocation

Copy link
Contributor

@bkontur bkontur left a comment

Choose a reason for hiding this comment

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

@NachoPal NachoPal added this pull request to the merge queue Mar 14, 2024
Merged via the queue into master with commit cfc4050 Mar 14, 2024
135 of 136 checks passed
@NachoPal NachoPal deleted the nacho/improve-penpal branch March 14, 2024 10:51
ordian added a commit that referenced this pull request Mar 16, 2024
* master: (65 commits)
  collator protocol changes for elastic scaling (validator side) (#3302)
  Contracts use polkavm workspace deps (#3715)
  Add elastic scaling support in ParaInherent BenchBuilder  (#3690)
  Removes `as [disambiguation_path]` from `derive_impl` usage (#3652)
  fix(paseo-spec): New Paseo Bootnodes (#3674)
  Improve Penpal runtime + emulated tests (#3543)
  Staking ledger bonding fixes (#3639)
  DescribeAllTerminal for HashedDescription (#3349)
  Increase timeout for assertions (#3680)
  Add subsystems regression tests to CI (#3527)
  Always print connectivity report (#3677)
  Revert "FRAME: Create `TransactionExtension` as a replacement for `SignedExtension` (#2280)" (#3665)
  authority-discovery: Add log for debugging DHT authority records (#3668)
  Construct Runtime v2  (#1378)
  Support for `keyring` in runtimes (#2044)
  Add api-name in `cannot query the runtime API version` warning (#3653)
  Add a PolkaVM-based executor (#3458)
  Adds default config for assets pallet (#3637)
  Bump handlebars from 4.3.7 to 5.1.0 (#3248)
  [Collator Selection] Fix weight refund for `set_candidacy_bond` (#3643)
  ...
dharjeezy pushed a commit to dharjeezy/polkadot-sdk that referenced this pull request Mar 24, 2024
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 <>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
R0-silent Changes should not be mentioned in any release notes 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