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.
# Conflicts: # CHANGELOG.md
|
@yrong @alistair-singh @vgeddes please review. :) |
|
|
||
| // Test Asset storage items | ||
| for (asset_id, _asset_details) in | ||
| pallet_assets::Asset::<T, crate::ForeignAssetsInstance>::iter() |
There was a problem hiding this comment.
I'm not sure if using TryFrom::try_from isn't too late.
I'm thinking: if someone reads the storage directly (e.g., some DApps or wallets - which unfortunately they probably do), will it still produce the same result?
Maybe we could do some direct storage decode/encode comparison, for example, something like the very pseudocode below:
// Define storage alias with xcm::v4 for pallet_assets::Asset
#[frame_support::storage_alias]
pub type AssetAsV4<T: Config<I>, I: 'static = ()> = StorageMap<
pallet_assets::Pallet<T, I>,
Blake2_128Concat,
xcm::v4::Location, // <- use storage alias with xcm::v4
AssetDetails<T::Balance, T::AccountId, DepositBalanceOf<T, I>>,
>;
// Define storage alias with xcm::v5 for pallet_assets::Asset
#[frame_support::storage_alias]
pub type AssetAsV5<T: Config<I>, I: 'static = ()> = StorageMap<
pallet_assets::Pallet<T, I>,
Blake2_128Concat,
xcm::v5::Location, // <- use storage alias with xcm::v5
AssetDetails<T::Balance, T::AccountId, DepositBalanceOf<T, I>>,
>;
// Compare that the both storage aliases decode/encode to the same keys/values
assert_eq!(
AssetAsV4::iter_keys(),
AssetAsV5::iter_keys()
);
There was a problem hiding this comment.
There was a problem hiding this comment.
You can wrap with submodules for example
There was a problem hiding this comment.
Nice, I changed the migration the way you suggested.
| { | ||
| let v5_location: xcm::v5::Location = asset_id.clone().into(); | ||
| let v4_location: xcm::v4::Location = | ||
| xcm::v4::Location::try_from(v5_location.clone()).unwrap(); |
There was a problem hiding this comment.
And another question is, do we want to break runtime upgrade here?
From the docs:
/// Similar to [`Hooks::on_initialize`], any code in this block is mandatory and MUST execute.
/// It is strongly recommended to dry-run the execution of these hooks using
/// [try-runtime-cli](https://github.com/paritytech/try-runtime-cli) to ensure they will not
/// produce and overweight block which can brick your chain. Use with care!
I would better not use unwrap and assert_eq here.
Maybe this comparision/check could be done just with pre_uprade / post_upgrade offchain check.
And we don't need to even implement fn on_runtime_upgrade here, because it has default impl:
fn on_runtime_upgrade() -> Weight {
Weight::from_parts(20, 0)
}
There was a problem hiding this comment.
Good idea! Done.
| @@ -64,26 +64,26 @@ use xcm_executor::{traits::ConvertLocation, XcmExecutor}; | |||
| parameter_types! { | |||
| pub const RootLocation: Location = Location::here(); | |||
| pub const DotLocation: Location = Location::parent(); | |||
| pub const DotLocationV4: xcm::v4::Location = xcm::v4::Location::parent(); | |||
| pub const DotLocationV4: xcm::v5::Location = xcm::v5::Location::parent(); | |||
There was a problem hiding this comment.
Should the name be DotLocationV5?
| pub PoolAssetsPalletLocationV5: xcm::v5::Location = | ||
| xcm::v5::Junction::PalletInstance(<PoolAssets as PalletInfoAccess>::index() as u8).into(); | ||
| pub PoolAssetsPalletLocationV4: xcm::v5::Location = | ||
| xcm::v5::Junction::PalletInstance(<PoolAssets as PalletInfoAccess>::index() as u8).into(); |
| parameter_types! { | ||
| pub EthereumNetworkXcmV4: xcm::v4::NetworkId = xcm::v4::NetworkId::Ethereum { chain_id: 1 }; | ||
| pub WethLocationXcmV4: xcm::v4::Location = xcm::v4::Location::new(2, [xcm::v4::Junction::GlobalConsensus(EthereumNetworkXcmV4::get()), xcm::v4::Junction::AccountKey20 { network: None, key: WETH }]); | ||
| pub EthLocationXcmV4: xcm::v4::Location = xcm::v4::Location::new(2, [xcm::v4::Junction::GlobalConsensus(EthereumNetworkXcmV4::get())]); | ||
| pub EthereumNetworkXcmV5: xcm::v5::NetworkId = xcm::v5::NetworkId::Ethereum { chain_id: 1 }; | ||
| pub WethLocationXcmV5: xcm::v5::Location = xcm::v5::Location::new(2, [xcm::v5::Junction::GlobalConsensus(EthereumNetworkXcmV5::get()), xcm::v5::Junction::AccountKey20 { network: None, key: WETH }]); | ||
| pub EthLocationXcmV5: xcm::v5::Location = xcm::v5::Location::new(2, [xcm::v5::Junction::GlobalConsensus(EthereumNetworkXcmV5::get())]); | ||
| } |
There was a problem hiding this comment.
I'm a bit curious - seems we explicitly specify the XCM version everywhere in the codebase, why not just import it globally, something like:
use xcm::opaque::v5::{*}
...
pub EthereumNetwork = NetworkId::Ethereum { chain_id: 1 };
pub WethLocation = Location::new(2, [Junction::GlobalConsensus(EthereumNetwork::get()),Junction::AccountKey20 { network: None, key: WETH }]);
pub EthLocation = Location::new(2, [Junction::GlobalConsensus(EthereumNetwork::get())]);
There was a problem hiding this comment.
yes, we should remove all these versioned names and just use xcm::latest::*
this ^ was always the plan but because we didn't add the right migrations at the pallet-level, we ended up doing migrations at the runtime level and having to differentiate between versions at the runtime level
my plan is that by the time XCMv6 comes out, we have pallet-level migrations and runtimes can always use xcm::latest::* - so let's work in that direction starting now
There was a problem hiding this comment.
Cool, I changed everything to xcm::latest::*.
| @@ -64,26 +64,26 @@ use xcm_executor::{traits::ConvertLocation, XcmExecutor}; | |||
| parameter_types! { | |||
| pub const RootLocation: Location = Location::here(); | |||
| pub const DotLocation: Location = Location::parent(); | |||
| pub const DotLocationV4: xcm::v4::Location = xcm::v4::Location::parent(); | |||
| pub const DotLocationV4: xcm::v5::Location = xcm::v5::Location::parent(); | |||
There was a problem hiding this comment.
same thing as pub const DotLocation: Location = Location::parent(); above
| pub const DotLocationV4: xcm::v5::Location = xcm::v5::Location::parent(); |
# Conflicts: # CHANGELOG.md
|
@bkontur @acatangiu @yrong I have made all the requested changes, please review again. |
# Conflicts: # system-parachains/asset-hubs/asset-hub-kusama/src/lib.rs # system-parachains/asset-hubs/asset-hub-polkadot/src/lib.rs
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.