Upgrade Polkadot and Kusama to XCM v5#753
Conversation
| // See the License for the specific language governing permissions and | ||
| // limitations under the License. | ||
|
|
||
| pub mod migration { |
There was a problem hiding this comment.
This module is generic, and can be reused for Polkadot and Kusama AssetHub. I couldn't mind a common place to put it in this repo though, so I copied the module for now.
There was a problem hiding this comment.
you could create a system-parachains-common crate under runtimes/system-parachains/common - i've seen code duplicated before because of lack of common dir/crate
# Conflicts: # CHANGELOG.md
|
@yrong @alistair-singh @vgeddes please review. :) |
|
Yea I tried to fix it here. Hope CI goes green now paritytech/polkadot-sdk#8980 after my MR. |
| /// storage. This migration doesn't actually alter storage, it only verifies that: | ||
| /// 1. XCM V4 encoded locations can be decoded as V5 | ||
| /// 2. Location encoding remains the same between V4 and V5 | ||
| pub struct VerifyXcmV4ToV5Compatibility<T, I = crate::ForeignAssetsInstance>( |
There was a problem hiding this comment.
Why are we adding tests to production code?
There was a problem hiding this comment.
Probably discussable, because most code is hidden behind try-runtime, but I don't see any need for adding this to the repo. You have tested it and now we can remove it? Better would be a proper test.
There was a problem hiding this comment.
@bkchr I added sample data tests here: https://github.com/polkadot-fellows/runtimes/pull/753/files#diff-d2b25217b924763d262a2a61b7c4a291cce0854d8a28fb425d35c30f462f0731R176 The try-runtime checks are nice because it is directly against storage. I still think this is helpful to ensure a successful migration, but if you don't think it is useful I guess I could remove it?
There was a problem hiding this comment.
I still think this is helpful to ensure a successful migration
But you already have ensured this? And outside of CI we are not executing these checks right now.
|
/merge |
|
Enabled Available commands
For more information see the documentation |
fellowship-merge-bot
Bot
merged commit 356837c
into
polkadot-fellows:main
|
Please no futher big changes to Asset Hub polkadot or relay while we are preparing runtimes for the Asset Hub Migration, thanks. |
|
@ggwpez do you mean this change needs to wait for the Asset Hub migration, or it can be done before the Asset Hub migration but no further big changes? I am asking because we are aiming to get Snowbridge V2 live on Polkadot in August, and we cannot do so without this change. |
I think this PR is totally fine and can be released - it doesn’t affect migrated data, since we’re not changing any encoding here. My guess is that Oliver had a lot of conflicts and code changes (v4 → v5) while rebasing the AHM dev-migration branch, right? Or is there another reason?
The only thing, @franciscoaguirre @anaelleltd do we need to notify - UIs/dapps/... about this change? How did we handle v3->v4 change, #472? Maybe, we don't need to? |
No notification needed this time. Here is the feedback from Cisco who did extra checks with PAPI team: |
8 participants
Upgrades Kusama and Polkadot AssetHub and BridgeHub to use XCM V5. Adds a no-migration to check that changing ForeignAssets and AssetConversion pallets to XCM V5 do not wreck storage.