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

[XCMv5]Remove XCM testnet NetworkIds #5390

Open
wants to merge 15 commits into
base: xcm-v5
Choose a base branch
from

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

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
…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
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>
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
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.

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
…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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[XCM]: Remove NetworkIds for testnets
6 participants