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.

Sign up for GitHub

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

Jump to bottom

[XCMv5]Remove XCM testnet NetworkIds #5390

Merged
merged 20 commits into from
Nov 1, 2024
Merged

[XCMv5]Remove XCM testnet NetworkIds #5390

merged 20 commits into from
Nov 1, 2024

Conversation

programskillforverification
Copy link
Contributor

Context

Close #5241, for more detail, please refer to RFC0108

Changes

  • Remove Rococo and Westend from NetworkId
  • Add Rococo Genesis Hash and Westend Genesis Hash

acatangiu
acatangiu reviewed Aug 19, 2024
View reviewed changes
Copy link
Contributor

@acatangiu acatangiu left a comment

Choose a reason for hiding this comment

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

You should not change the definition for existing XCM v3 and v4 versions, but instead, remove the test networks ids in the new upcoming XCM v5 version.

There is a PR for XCMv5: #4826

You should also change this PR's base branch to https://github.com/paritytech/polkadot-sdk/tree/xcm-v5 instead of master, to get your changes added to the xcm-v5 branch.

polkadot/xcm/src/v3/junction.rs Outdated Show resolved Hide resolved
polkadot/xcm/src/v4/junction.rs Outdated Show resolved Hide resolved
@programskillforverification
Copy link
Contributor Author

You should not change the definition for existing XCM v3 and v4 versions, but instead, remove the test networks ids in the new upcoming XCM v5 version.

There is a PR for XCMv5: #4826

You should also change this PR's base branch to https://github.com/paritytech/polkadot-sdk/tree/xcm-v5 instead of master, to get your changes added to the xcm-v5 branch.

Oh, I am sorry that I don't know this PR. I will change it.

@programskillforverification programskillforverification changed the base branch from master to xcm-v5 August 19, 2024 15:24
@programskillforverification
initial modifications
cd33e83
@nazar-pc @programskillforverification
Replace unnecessary &mut self with &self in `BlockImport::import_…
2ce6458 
…block()` (paritytech#5339)

There was no need for it to be `&mut self` since block import can happen
concurrently for different blocks and in many cases it was `&mut Arc<dyn
BlockImport>` anyway 🤷‍♂️

Similar in nature to
paritytech#4844
@bkchr @shawntabrizi @programskillforverification
binary-merkle-tree: Do not spam test output (paritytech#5376)
e83e8b3 
The CI isn't happy with the amount of output:
https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/7035621/raw

---------

Co-authored-by: Shawn Tabrizi <shawntabrizi@gmail.com>
@programskillforverification
replace all networkids in cumulus and polkadot
856a3fe
@programskillforverification
remove networkis in xcm-v5
4f16f3c
@programskillforverification programskillforverification marked this pull request as ready for review August 20, 2024 11:35
@paritytech-review-bot paritytech-review-bot bot requested a review from a team August 20, 2024 11:35
acatangiu
acatangiu reviewed Aug 20, 2024
View reviewed changes
Copy link
Contributor

@acatangiu acatangiu left a comment

Choose a reason for hiding this comment

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

The XCM changes look good 👍

You need to clean up the current PR against xcm-v5 as it also contains unrelated changes coming from other master PRs.

@franciscoaguirre @bkontur besides the foreign assets ids, do we have any other affected storage items?

cumulus/client/consensus/common/src/lib.rs Outdated Show resolved Hide resolved
cumulus/parachains/runtimes/assets/asset-hub-rococo/tests/tests.rs
Comment on lines +1110 to +1113
let foreign_asset_id_location = Location::new(
2,
[Junction::GlobalConsensus(NetworkId::ByGenesis(WESTEND_GENESIS_HASH))],
);
Copy link
Contributor

Choose a reason for hiding this comment

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

Will also need a migration for the bridged assets registered as foreign assets on Rococo and Westend Asset Hubs.

Their IDs are changing now, storage items need migrating.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will also need a migration for the bridged assets registered as foreign assets on Rococo and Westend Asset Hubs.

Their IDs are changing now, storage items need migrating.

Migrates storage to v5?

Copy link
Contributor

Choose a reason for hiding this comment

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

cc @franciscoaguirre since he looked at same migration for v3->v4

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@franciscoaguirre pls provide more info or ref about migration

Copy link
Contributor

Choose a reason for hiding this comment

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

This will need multi-block migrations, which we don't have the setup for now. I need to work on getting that setup done anyway. When I'm done I'll update here.

Copy link
Contributor

Choose a reason for hiding this comment

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

We're going to merge this as is before the migration. We'll tackle the migration later. Created an issue to track it: #6319

cumulus/parachains/runtimes/bridge-hubs/bridge-hub-rococo/src/lib.rs Outdated Show resolved Hide resolved
cumulus/parachains/runtimes/bridge-hubs/bridge-hub-westend/src/lib.rs Outdated Show resolved Hide resolved
cumulus/parachains/runtimes/testing/penpal/src/xcm_config.rs Outdated Show resolved Hide resolved
prdoc/pr_5339.prdoc Outdated Show resolved Hide resolved
prdoc/pr_5376.prdoc Outdated Show resolved Hide resolved
substrate/utils/binary-merkle-tree/src/lib.rs Outdated Show resolved Hide resolved
@programskillforverification programskillforverification changed the title Remove XCM testnet NetworkIds [XCMv5]Remove XCM testnet NetworkIds Aug 20, 2024
@paritytech-review-bot paritytech-review-bot bot requested a review from a team August 22, 2024 03:32
@programskillforverification @acatangiu
Update cumulus/parachains/runtimes/bridge-hubs/bridge-hub-rococo/src/…
736f2ec 
…lib.rs

Co-authored-by: Adrian Catangiu <adrian@parity.io>
@paritytech-review-bot paritytech-review-bot bot requested a review from a team August 22, 2024 12:34
@paritytech-cicd-pr
Copy link

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: test-linux-stable 1/3
Logs: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/7273019

@acatangiu acatangiu mentioned this pull request Oct 22, 2024
Merged
10 tasks
acatangiu
acatangiu approved these changes Oct 31, 2024
View reviewed changes
Copy link
Contributor

@acatangiu acatangiu left a comment

Choose a reason for hiding this comment

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

Looks good, we can migrate the storage on testnets in a separate PR

@franciscoaguirre
Copy link
Contributor

Agreed, just fixing the conflicts

@franciscoaguirre
Copy link
Contributor

@yrong I'm getting an error here "expected primitive_types::H160, found H160" all over the place in bridges/snowbridge/primitives/core/src/outbound.rs. Any idea why?

@franciscoaguirre franciscoaguirre added the T6-XCM This PR/Issue is related to XCM. label Oct 31, 2024
@franciscoaguirre
Copy link
Contributor

@yrong I'm getting an error here "expected primitive_types::H160, found H160" all over the place in bridges/snowbridge/primitives/core/src/outbound.rs. Any idea why?

Turns out the branch had a weird Cargo.lock. I replaced it with the upstream one and that error was gone

@franciscoaguirre franciscoaguirre merged commit e21773a into paritytech:xcm-v5 Nov 1, 2024
74 of 109 checks passed
github-merge-queue bot pushed a commit that referenced this pull request Nov 6, 2024
@franciscoaguirre @acatangiu
@x3c41a @bkontur @programskillforverification @nazar-pc @bkchr @shawntabrizi @actions-user @serban300
XCM v5 (#4826)
85521e8 
# Context

This PR aims to introduce XCMv5, for now it's in progress and will be
updated over time.
This branch will serve as a milestone branch for merging in all features
we want to add to XCM, roughly outlined
[here](polkadot-fellows/xcm-format#60).
More features could be added.

## TODO
- [x] Migrate foreign assets from v3 to v4
- [x] Setup v5 skeleton
- [x] Remove XCMv2
- [x] #5390
- [x] #5585
- [x] #5420
- [x] #5876
- [x] #5971
- [x] #6148
- [x] #6228

Fixes #3434 
Fixes #4190
Fixes #5209
Fixes #5241
Fixes #4284

---------

Signed-off-by: Adrian Catangiu <adrian@parity.io>
Co-authored-by: Adrian Catangiu <adrian@parity.io>
Co-authored-by: Andrii <ndk@parity.io>
Co-authored-by: Branislav Kontur <bkontur@gmail.com>
Co-authored-by: Joseph Zhao <65984904+programskillforverification@users.noreply.github.com>
Co-authored-by: Nazar Mokrynskyi <nazar@mokrynskyi.com>
Co-authored-by: Bastian Köcher <git@kchr.de>
Co-authored-by: Shawn Tabrizi <shawntabrizi@gmail.com>
Co-authored-by: command-bot <>
Co-authored-by: GitHub Action <action@github.com>
Co-authored-by: Serban Iorga <serban@parity.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@acatangiu acatangiu acatangiu approved these changes

@franciscoaguirre franciscoaguirre franciscoaguirre approved these changes

@sorpaas sorpaas Awaiting requested review from sorpaas

Assignees
No one assigned
Labels
T6-XCM This PR/Issue is related to XCM.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[XCM]: Remove NetworkIds for testnets
6 participants
@programskillforverification @paritytech-cicd-pr @franciscoaguirre @acatangiu @nazar-pc @bkchr