From e6f3106d894277deba043a83e91565de24263a1b Mon Sep 17 00:00:00 2001 From: Sergej Sakac <73715684+Szegoo@users.noreply.github.com> Date: Wed, 17 Apr 2024 11:25:33 +0200 Subject: [PATCH] XCM coretime region transfers (#3455) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This PR introduces changes enabling the transfer of coretime regions via XCM. TL;DR: There are two primary issues that are resolved in this PR: 1. The `mint` and `burn` functions were not implemented for coretime regions. These operations are essential for moving assets to and from the XCM holding register. 2. The transfer of non-fungible assets through XCM was previously disallowed. This was due to incorrectly benchmarking non-fungible asset transfers via XCM, which led to assigning it a weight of `Weight::Max`, effectively preventing its execution. ### `mint_into` and `burn` implementation This PR addresses the issue with cross-chain transferring regions back to the Coretime chain. Remote reserve transfers are performed by withdrawing and depositing the asset to and from the holding registry. This requires the asset to support burning and minting functionality. This PR adds burning and minting; however, they work a bit differently than usual so that the associated region record is not lost when burning. Instead of removing all the data, burning will set the owner of the region to `None`, and when minting it back, it will set it to an actual value. So, when cross-chain transferring, withdrawing into the registry will remove the region from its original owner, and when depositing it from the registry, it will set its owner to another account This was originally implemented in this PR: #3455, however we decided to move all of it to this single PR (https://github.com/paritytech/polkadot-sdk/pull/3455#discussion_r1547324892) ### Fixes made in this PR - Update the `XcmReserveTransferFilter` on coretime chain since it is meant as a reserve chain for coretime regions. - Update the XCM benchmark to use `AssetTransactor` instead of assuming `pallet-balances` for fungible transfers. - Update the XCM benchmark to properly measure weight consumption for nonfungible reserve asset transfers. ATM reserve transfers via the extrinsic do not work since the weight for it is set to `Weight::max()`. Closes: https://github.com/paritytech/polkadot-sdk/issues/865 --------- Co-authored-by: Branislav Kontur Co-authored-by: Francisco Aguirre Co-authored-by: Dónal Murray --- Cargo.lock | 1 + .../coretime/coretime-rococo/src/lib.rs | 57 +++++++--- .../coretime-rococo/src/xcm_config.rs | 2 +- .../coretime/coretime-westend/src/lib.rs | 59 ++++++++--- .../coretime-westend/src/xcm_config.rs | 2 +- polkadot/xcm/pallet-xcm/src/benchmarking.rs | 100 +++++++++++------- prdoc/pr_3455.prdoc | 28 +++++ substrate/frame/broker/Cargo.toml | 2 + substrate/frame/broker/src/benchmarking.rs | 4 +- .../frame/broker/src/dispatchable_impls.rs | 11 +- substrate/frame/broker/src/lib.rs | 11 +- substrate/frame/broker/src/migration.rs | 87 +++++++++++++++ .../frame/broker/src/nonfungible_impl.rs | 57 ++++++++-- substrate/frame/broker/src/tests.rs | 33 ++++-- substrate/frame/broker/src/types.rs | 2 +- substrate/frame/broker/src/utility_impls.rs | 6 +- 16 files changed, 368 insertions(+), 94 deletions(-) create mode 100644 prdoc/pr_3455.prdoc create mode 100644 substrate/frame/broker/src/migration.rs diff --git a/Cargo.lock b/Cargo.lock index 4ee0dfdccff6..584c37dc9c3a 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -9971,6 +9971,7 @@ dependencies = [ "frame-benchmarking", "frame-support", "frame-system", + "log", "parity-scale-codec", "pretty_assertions", "scale-info", diff --git a/cumulus/parachains/runtimes/coretime/coretime-rococo/src/lib.rs b/cumulus/parachains/runtimes/coretime/coretime-rococo/src/lib.rs index 9959c15b1110..ad065ee34774 100644 --- a/cumulus/parachains/runtimes/coretime/coretime-rococo/src/lib.rs +++ b/cumulus/parachains/runtimes/coretime/coretime-rococo/src/lib.rs @@ -109,6 +109,7 @@ pub type UncheckedExtrinsic = /// Migrations to apply on runtime upgrade. pub type Migrations = ( cumulus_pallet_xcmp_queue::migration::v4::MigrationToV4, + pallet_broker::migration::MigrateV0ToV1, // permanent pallet_xcm::migration::MigrateToLatestXcmVersion, ); @@ -719,11 +720,20 @@ impl_runtime_apis! { use pallet_xcm::benchmarking::Pallet as PalletXcmExtrinsicsBenchmark; impl pallet_xcm::benchmarking::Config for Runtime { - type DeliveryHelper = cumulus_primitives_utility::ToParentDeliveryHelper< - xcm_config::XcmConfig, - ExistentialDepositAsset, - xcm_config::PriceForParentDelivery, - >; + type DeliveryHelper = ( + cumulus_primitives_utility::ToParentDeliveryHelper< + xcm_config::XcmConfig, + ExistentialDepositAsset, + xcm_config::PriceForParentDelivery, + >, + polkadot_runtime_common::xcm_sender::ToParachainDeliveryHelper< + xcm_config::XcmConfig, + ExistentialDepositAsset, + PriceForSiblingParachainDelivery, + RandomParaId, + ParachainSystem, + > + ); fn reachable_dest() -> Option { Some(Parent.into()) @@ -741,8 +751,21 @@ impl_runtime_apis! { } fn reserve_transferable_asset_and_dest() -> Option<(Asset, Location)> { - // Reserve transfers are disabled - None + // Coretime chain can reserve transfer regions to some random parachain. + + // Properties of a mock region: + let core = 0; + let begin = 0; + let end = 42; + + let region_id = pallet_broker::Pallet::::issue(core, begin, end, None, None); + Some(( + Asset { + fun: NonFungible(Index(region_id.into())), + id: AssetId(xcm_config::BrokerPalletLocation::get()) + }, + ParentThen(Parachain(RandomParaId::get().into()).into()).into(), + )) } fn get_asset() -> Asset { @@ -758,15 +781,25 @@ impl_runtime_apis! { RocRelayLocation::get(), ExistentialDeposit::get() ).into()); + pub const RandomParaId: ParaId = ParaId::new(43211234); } impl pallet_xcm_benchmarks::Config for Runtime { type XcmConfig = xcm_config::XcmConfig; - type DeliveryHelper = cumulus_primitives_utility::ToParentDeliveryHelper< - xcm_config::XcmConfig, - ExistentialDepositAsset, - xcm_config::PriceForParentDelivery, - >; + type DeliveryHelper = ( + cumulus_primitives_utility::ToParentDeliveryHelper< + xcm_config::XcmConfig, + ExistentialDepositAsset, + xcm_config::PriceForParentDelivery, + >, + polkadot_runtime_common::xcm_sender::ToParachainDeliveryHelper< + xcm_config::XcmConfig, + ExistentialDepositAsset, + PriceForSiblingParachainDelivery, + RandomParaId, + ParachainSystem, + > + ); type AccountIdConverter = xcm_config::LocationToAccountId; fn valid_destination() -> Result { Ok(RocRelayLocation::get()) diff --git a/cumulus/parachains/runtimes/coretime/coretime-rococo/src/xcm_config.rs b/cumulus/parachains/runtimes/coretime/coretime-rococo/src/xcm_config.rs index 7580ab33b8d6..7eab53b8a7cf 100644 --- a/cumulus/parachains/runtimes/coretime/coretime-rococo/src/xcm_config.rs +++ b/cumulus/parachains/runtimes/coretime/coretime-rococo/src/xcm_config.rs @@ -293,7 +293,7 @@ impl pallet_xcm::Config for Runtime { type XcmExecuteFilter = Everything; type XcmExecutor = XcmExecutor; type XcmTeleportFilter = Everything; - type XcmReserveTransferFilter = Nothing; // This parachain is not meant as a reserve location. + type XcmReserveTransferFilter = Everything; type Weigher = WeightInfoBounds< crate::weights::xcm::CoretimeRococoXcmWeight, RuntimeCall, diff --git a/cumulus/parachains/runtimes/coretime/coretime-westend/src/lib.rs b/cumulus/parachains/runtimes/coretime/coretime-westend/src/lib.rs index 1ee8c3bc0ec3..0f0742268618 100644 --- a/cumulus/parachains/runtimes/coretime/coretime-westend/src/lib.rs +++ b/cumulus/parachains/runtimes/coretime/coretime-westend/src/lib.rs @@ -109,6 +109,7 @@ pub type UncheckedExtrinsic = /// Migrations to apply on runtime upgrade. pub type Migrations = ( cumulus_pallet_xcmp_queue::migration::v4::MigrationToV4, + pallet_broker::migration::MigrateV0ToV1, // permanent pallet_xcm::migration::MigrateToLatestXcmVersion, ); @@ -218,6 +219,7 @@ impl pallet_authorship::Config for Runtime { parameter_types! { pub const ExistentialDeposit: Balance = EXISTENTIAL_DEPOSIT; + pub const RandomParaId: ParaId = ParaId::new(43211234); } impl pallet_balances::Config for Runtime { @@ -710,11 +712,20 @@ impl_runtime_apis! { use pallet_xcm::benchmarking::Pallet as PalletXcmExtrinsicsBenchmark; impl pallet_xcm::benchmarking::Config for Runtime { - type DeliveryHelper = cumulus_primitives_utility::ToParentDeliveryHelper< - xcm_config::XcmConfig, - ExistentialDepositAsset, - xcm_config::PriceForParentDelivery, - >; + type DeliveryHelper = ( + cumulus_primitives_utility::ToParentDeliveryHelper< + xcm_config::XcmConfig, + ExistentialDepositAsset, + xcm_config::PriceForParentDelivery, + >, + polkadot_runtime_common::xcm_sender::ToParachainDeliveryHelper< + xcm_config::XcmConfig, + ExistentialDepositAsset, + PriceForSiblingParachainDelivery, + RandomParaId, + ParachainSystem, + > + ); fn reachable_dest() -> Option { Some(Parent.into()) @@ -732,8 +743,21 @@ impl_runtime_apis! { } fn reserve_transferable_asset_and_dest() -> Option<(Asset, Location)> { - // Reserve transfers are disabled - None + // Coretime chain can reserve transfer regions to some random parachain. + + // Properties of a mock region: + let core = 0; + let begin = 0; + let end = 42; + + let region_id = pallet_broker::Pallet::::issue(core, begin, end, None, None); + Some(( + Asset { + fun: NonFungible(Index(region_id.into())), + id: AssetId(xcm_config::BrokerPalletLocation::get()) + }, + ParentThen(Parachain(RandomParaId::get().into()).into()).into(), + )) } fn get_asset() -> Asset { @@ -753,11 +777,22 @@ impl_runtime_apis! { impl pallet_xcm_benchmarks::Config for Runtime { type XcmConfig = xcm_config::XcmConfig; - type DeliveryHelper = cumulus_primitives_utility::ToParentDeliveryHelper< - xcm_config::XcmConfig, - ExistentialDepositAsset, - xcm_config::PriceForParentDelivery, - >; + + type DeliveryHelper = ( + cumulus_primitives_utility::ToParentDeliveryHelper< + xcm_config::XcmConfig, + ExistentialDepositAsset, + xcm_config::PriceForParentDelivery, + >, + polkadot_runtime_common::xcm_sender::ToParachainDeliveryHelper< + xcm_config::XcmConfig, + ExistentialDepositAsset, + PriceForSiblingParachainDelivery, + RandomParaId, + ParachainSystem, + > + ); + type AccountIdConverter = xcm_config::LocationToAccountId; fn valid_destination() -> Result { Ok(TokenRelayLocation::get()) diff --git a/cumulus/parachains/runtimes/coretime/coretime-westend/src/xcm_config.rs b/cumulus/parachains/runtimes/coretime/coretime-westend/src/xcm_config.rs index 346bdfa4d8c9..e1452ec63f20 100644 --- a/cumulus/parachains/runtimes/coretime/coretime-westend/src/xcm_config.rs +++ b/cumulus/parachains/runtimes/coretime/coretime-westend/src/xcm_config.rs @@ -300,7 +300,7 @@ impl pallet_xcm::Config for Runtime { type XcmExecuteFilter = Everything; type XcmExecutor = XcmExecutor; type XcmTeleportFilter = Everything; - type XcmReserveTransferFilter = Nothing; // This parachain is not meant as a reserve location. + type XcmReserveTransferFilter = Everything; type Weigher = WeightInfoBounds< crate::weights::xcm::CoretimeWestendXcmWeight, RuntimeCall, diff --git a/polkadot/xcm/pallet-xcm/src/benchmarking.rs b/polkadot/xcm/pallet-xcm/src/benchmarking.rs index e2903d592dc1..5d2e0f7b96f9 100644 --- a/polkadot/xcm/pallet-xcm/src/benchmarking.rs +++ b/polkadot/xcm/pallet-xcm/src/benchmarking.rs @@ -18,10 +18,7 @@ use super::*; use bounded_collections::{ConstU32, WeakBoundedVec}; use codec::Encode; use frame_benchmarking::{benchmarks, whitelisted_caller, BenchmarkError, BenchmarkResult}; -use frame_support::{ - traits::fungible::{Inspect, Mutate}, - weights::Weight, -}; +use frame_support::{assert_ok, weights::Weight}; use frame_system::RawOrigin; use sp_std::prelude::*; use xcm::{latest::prelude::*, v2}; @@ -90,11 +87,6 @@ pub trait Config: crate::Config { } benchmarks! { - where_clause { - where - T: pallet_balances::Config, - ::Balance: From + Into, - } send { let send_origin = T::SendXcmOrigin::try_successful_origin().map_err(|_| BenchmarkError::Weightless)?; @@ -129,11 +121,7 @@ benchmarks! { BenchmarkError::Override(BenchmarkResult::from_weight(Weight::MAX)), )?; - let transferred_amount = match &asset.fun { - Fungible(amount) => *amount, - _ => return Err(BenchmarkError::Stop("Benchmark asset not fungible")), - }.into(); - let assets: Assets = asset.into(); + let assets: Assets = asset.clone().into(); let caller: T::AccountId = whitelisted_caller(); let send_origin = RawOrigin::Signed(caller.clone()); @@ -150,13 +138,26 @@ benchmarks! { FeeReason::ChargeFees, ); - // Actual balance (e.g. `ensure_successful_delivery` could drip delivery fees, ...) - let balance = as Inspect<_>>::balance(&caller); - // Add transferred_amount to origin - as Mutate<_>>::mint_into(&caller, transferred_amount)?; - // verify initial balance - let balance = balance + transferred_amount; - assert_eq!( as Inspect<_>>::balance(&caller), balance); + match &asset.fun { + Fungible(amount) => { + // Add transferred_amount to origin + ::AssetTransactor::deposit_asset( + &Asset { fun: Fungible(*amount), id: asset.id }, + &origin_location, + None, + ).map_err(|error| { + log::error!("Fungible asset couldn't be deposited, error: {:?}", error); + BenchmarkError::Override(BenchmarkResult::from_weight(Weight::MAX)) + })?; + }, + NonFungible(instance) => { + ::AssetTransactor::deposit_asset(&asset, &origin_location, None) + .map_err(|error| { + log::error!("Nonfungible asset couldn't be deposited, error: {:?}", error); + BenchmarkError::Override(BenchmarkResult::from_weight(Weight::MAX)) + })?; + } + }; let recipient = [0u8; 32]; let versioned_dest: VersionedLocation = destination.into(); @@ -164,21 +165,13 @@ benchmarks! { AccountId32 { network: None, id: recipient.into() }.into(); let versioned_assets: VersionedAssets = assets.into(); }: _>(send_origin.into(), Box::new(versioned_dest), Box::new(versioned_beneficiary), Box::new(versioned_assets), 0) - verify { - // verify balance after transfer, decreased by transferred amount (+ maybe XCM delivery fees) - assert!( as Inspect<_>>::balance(&caller) <= balance - transferred_amount); - } reserve_transfer_assets { let (asset, destination) = T::reserve_transferable_asset_and_dest().ok_or( BenchmarkError::Override(BenchmarkResult::from_weight(Weight::MAX)), )?; - let transferred_amount = match &asset.fun { - Fungible(amount) => *amount, - _ => return Err(BenchmarkError::Stop("Benchmark asset not fungible")), - }.into(); - let assets: Assets = asset.into(); + let assets: Assets = asset.clone().into(); let caller: T::AccountId = whitelisted_caller(); let send_origin = RawOrigin::Signed(caller.clone()); @@ -195,23 +188,50 @@ benchmarks! { FeeReason::ChargeFees, ); - // Actual balance (e.g. `ensure_successful_delivery` could drip delivery fees, ...) - let balance = as Inspect<_>>::balance(&caller); - // Add transferred_amount to origin - as Mutate<_>>::mint_into(&caller, transferred_amount)?; - // verify initial balance - let balance = balance + transferred_amount; - assert_eq!( as Inspect<_>>::balance(&caller), balance); + match &asset.fun { + Fungible(amount) => { + // Add transferred_amount to origin + ::AssetTransactor::deposit_asset( + &Asset { fun: Fungible(*amount), id: asset.id.clone() }, + &origin_location, + None, + ).map_err(|error| { + log::error!("Fungible asset couldn't be deposited, error: {:?}", error); + BenchmarkError::Override(BenchmarkResult::from_weight(Weight::MAX)) + })?; + }, + NonFungible(instance) => { + ::AssetTransactor::deposit_asset(&asset, &origin_location, None) + .map_err(|error| { + log::error!("Nonfungible asset couldn't be deposited, error: {:?}", error); + BenchmarkError::Override(BenchmarkResult::from_weight(Weight::MAX)) + })?; + } + }; let recipient = [0u8; 32]; - let versioned_dest: VersionedLocation = destination.into(); + let versioned_dest: VersionedLocation = destination.clone().into(); let versioned_beneficiary: VersionedLocation = AccountId32 { network: None, id: recipient.into() }.into(); let versioned_assets: VersionedAssets = assets.into(); }: _>(send_origin.into(), Box::new(versioned_dest), Box::new(versioned_beneficiary), Box::new(versioned_assets), 0) verify { - // verify balance after transfer, decreased by transferred amount (+ maybe XCM delivery fees) - assert!( as Inspect<_>>::balance(&caller) <= balance - transferred_amount); + match &asset.fun { + Fungible(amount) => { + assert_ok!(::AssetTransactor::withdraw_asset( + &Asset { fun: Fungible(*amount), id: asset.id }, + &destination, + None, + )); + }, + NonFungible(instance) => { + assert_ok!(::AssetTransactor::withdraw_asset( + &asset, + &destination, + None, + )); + } + }; } transfer_assets { diff --git a/prdoc/pr_3455.prdoc b/prdoc/pr_3455.prdoc new file mode 100644 index 000000000000..c16ac2244862 --- /dev/null +++ b/prdoc/pr_3455.prdoc @@ -0,0 +1,28 @@ +# Schema: Polkadot SDK PRDoc Schema (prdoc) v1.0.0 +# See doc at https://raw.githubusercontent.com/paritytech/polkadot-sdk/master/prdoc/schema_user.json + +title: Region reserve transfers fix + +doc: + - audience: Runtime User + description: | + This PR introduces changes enabling the transfer of coretime regions via XCM. + There are two primary issues that are resolved in this PR: + 1. The mint and burn functions were not implemented for coretime regions. These operations + are essential for moving assets to and from the XCM holding register. + 2. The transfer of non-fungible assets through XCM was previously disallowed. This was due + to incorrectly benchmarking non-fungible asset transfers via XCM, which led to assigning + it a weight of Weight::Max, effectively preventing its execution. + +migrations: + db: [] + runtime: + - reference: pallet-broker + description: | + The region owner is optional. + +crates: + - name: pallet-broker + - name: pallet-xcm + - name: coretime-rococo-runtime + - name: coretime-westend-runtime diff --git a/substrate/frame/broker/Cargo.toml b/substrate/frame/broker/Cargo.toml index f36b94c3cec5..ce8d41530451 100644 --- a/substrate/frame/broker/Cargo.toml +++ b/substrate/frame/broker/Cargo.toml @@ -15,6 +15,7 @@ workspace = true targets = ["x86_64-unknown-linux-gnu"] [dependencies] +log = { workspace = true } codec = { package = "parity-scale-codec", version = "3.0.0", default-features = false, features = ["derive"] } scale-info = { version = "2.11.1", default-features = false, features = ["derive"] } bitvec = { version = "1.0.0", default-features = false } @@ -40,6 +41,7 @@ std = [ "frame-benchmarking?/std", "frame-support/std", "frame-system/std", + "log/std", "scale-info/std", "sp-api/std", "sp-arithmetic/std", diff --git a/substrate/frame/broker/src/benchmarking.rs b/substrate/frame/broker/src/benchmarking.rs index 98ac074ca917..1fc1c3a101ab 100644 --- a/substrate/frame/broker/src/benchmarking.rs +++ b/substrate/frame/broker/src/benchmarking.rs @@ -313,8 +313,8 @@ mod benches { assert_last_event::( Event::Transferred { region_id: region, - old_owner: caller, - owner: recipient, + old_owner: Some(caller), + owner: Some(recipient), duration: 3u32.into(), } .into(), diff --git a/substrate/frame/broker/src/dispatchable_impls.rs b/substrate/frame/broker/src/dispatchable_impls.rs index b43911b6bc12..cb7393cc9e32 100644 --- a/substrate/frame/broker/src/dispatchable_impls.rs +++ b/substrate/frame/broker/src/dispatchable_impls.rs @@ -120,7 +120,8 @@ impl Pallet { sale.sellout_price = Some(price); } SaleInfo::::put(&sale); - let id = Self::issue(core, sale.region_begin, sale.region_end, who.clone(), Some(price)); + let id = + Self::issue(core, sale.region_begin, sale.region_end, Some(who.clone()), Some(price)); let duration = sale.region_end.saturating_sub(sale.region_begin); Self::deposit_event(Event::Purchased { who, region_id: id, price, duration }); Ok(id) @@ -178,11 +179,11 @@ impl Pallet { let mut region = Regions::::get(®ion_id).ok_or(Error::::UnknownRegion)?; if let Some(check_owner) = maybe_check_owner { - ensure!(check_owner == region.owner, Error::::NotOwner); + ensure!(Some(check_owner) == region.owner, Error::::NotOwner); } let old_owner = region.owner; - region.owner = new_owner; + region.owner = Some(new_owner); Regions::::insert(®ion_id, ®ion); let duration = region.end.saturating_sub(region_id.begin); Self::deposit_event(Event::Transferred { @@ -203,7 +204,7 @@ impl Pallet { let mut region = Regions::::get(®ion_id).ok_or(Error::::UnknownRegion)?; if let Some(check_owner) = maybe_check_owner { - ensure!(check_owner == region.owner, Error::::NotOwner); + ensure!(Some(check_owner) == region.owner, Error::::NotOwner); } let pivot = region_id.begin.saturating_add(pivot_offset); ensure!(pivot < region.end, Error::::PivotTooLate); @@ -227,7 +228,7 @@ impl Pallet { let region = Regions::::get(®ion_id).ok_or(Error::::UnknownRegion)?; if let Some(check_owner) = maybe_check_owner { - ensure!(check_owner == region.owner, Error::::NotOwner); + ensure!(Some(check_owner) == region.owner, Error::::NotOwner); } ensure!((pivot & !region_id.mask).is_void(), Error::::ExteriorPivot); diff --git a/substrate/frame/broker/src/lib.rs b/substrate/frame/broker/src/lib.rs index 330491eb2087..1ef9e59f0186 100644 --- a/substrate/frame/broker/src/lib.rs +++ b/substrate/frame/broker/src/lib.rs @@ -36,6 +36,7 @@ mod tick_impls; mod types; mod utility_impls; +pub mod migration; pub mod runtime_api; pub mod weights; @@ -46,6 +47,9 @@ pub use core_mask::*; pub use coretime_interface::*; pub use types::*; +/// The log target for this pallet. +const LOG_TARGET: &str = "runtime::broker"; + #[frame_support::pallet] pub mod pallet { use super::*; @@ -61,7 +65,10 @@ pub mod pallet { use sp_runtime::traits::{Convert, ConvertBack}; use sp_std::vec::Vec; + const STORAGE_VERSION: StorageVersion = StorageVersion::new(1); + #[pallet::pallet] + #[pallet::storage_version(STORAGE_VERSION)] pub struct Pallet(_); #[pallet::config] @@ -216,9 +223,9 @@ pub mod pallet { /// The duration of the Region. duration: Timeslice, /// The old owner of the Region. - old_owner: T::AccountId, + old_owner: Option, /// The new owner of the Region. - owner: T::AccountId, + owner: Option, }, /// A Region has been split into two non-overlapping Regions. Partitioned { diff --git a/substrate/frame/broker/src/migration.rs b/substrate/frame/broker/src/migration.rs new file mode 100644 index 000000000000..95aa28250a62 --- /dev/null +++ b/substrate/frame/broker/src/migration.rs @@ -0,0 +1,87 @@ +// This file is part of Substrate. + +// Copyright (C) Parity Technologies (UK) Ltd. +// SPDX-License-Identifier: Apache-2.0 + +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +use super::*; +use crate::types::RegionRecord; +use codec::{Decode, Encode}; +use core::marker::PhantomData; +use frame_support::traits::{Get, UncheckedOnRuntimeUpgrade}; +use sp_runtime::Saturating; + +#[cfg(feature = "try-runtime")] +use frame_support::ensure; +#[cfg(feature = "try-runtime")] +use sp_std::vec::Vec; + +mod v1 { + use super::*; + + /// V0 region record. + #[derive(Encode, Decode)] + struct RegionRecordV0 { + /// The end of the Region. + pub end: Timeslice, + /// The owner of the Region. + pub owner: AccountId, + /// The amount paid to Polkadot for this Region, or `None` if renewal is not allowed. + pub paid: Option, + } + + pub struct MigrateToV1Impl(PhantomData); + + impl UncheckedOnRuntimeUpgrade for MigrateToV1Impl { + fn on_runtime_upgrade() -> frame_support::weights::Weight { + let mut count: u64 = 0; + + >::translate::>, _>(|_, v0| { + count.saturating_inc(); + Some(RegionRecord { end: v0.end, owner: Some(v0.owner), paid: v0.paid }) + }); + + log::info!( + target: LOG_TARGET, + "Storage migration v1 for pallet-broker finished.", + ); + + // calculate and return migration weights + T::DbWeight::get().reads_writes(count as u64 + 1, count as u64 + 1) + } + + #[cfg(feature = "try-runtime")] + fn pre_upgrade() -> Result, sp_runtime::TryRuntimeError> { + Ok((Regions::::iter_keys().count() as u32).encode()) + } + + #[cfg(feature = "try-runtime")] + fn post_upgrade(state: Vec) -> Result<(), sp_runtime::TryRuntimeError> { + let old_count = u32::decode(&mut &state[..]).expect("Known good"); + let new_count = Regions::::iter_values().count() as u32; + + ensure!(old_count == new_count, "Regions count should not change"); + Ok(()) + } + } +} + +/// Migrate the pallet storage from `0` to `1`. +pub type MigrateV0ToV1 = frame_support::migrations::VersionedMigration< + 0, + 1, + v1::MigrateToV1Impl, + Pallet, + ::DbWeight, +>; diff --git a/substrate/frame/broker/src/nonfungible_impl.rs b/substrate/frame/broker/src/nonfungible_impl.rs index b2e88bf09a0e..80dcc175df53 100644 --- a/substrate/frame/broker/src/nonfungible_impl.rs +++ b/substrate/frame/broker/src/nonfungible_impl.rs @@ -25,12 +25,13 @@ use sp_std::vec::Vec; impl Inspect for Pallet { type ItemId = u128; - fn owner(index: &Self::ItemId) -> Option { - Regions::::get(RegionId::from(*index)).map(|r| r.owner) + fn owner(item: &Self::ItemId) -> Option { + let record = Regions::::get(RegionId::from(*item))?; + record.owner } - fn attribute(index: &Self::ItemId, key: &[u8]) -> Option> { - let id = RegionId::from(*index); + fn attribute(item: &Self::ItemId, key: &[u8]) -> Option> { + let id = RegionId::from(*item); let item = Regions::::get(id)?; match key { b"begin" => Some(id.begin.encode()), @@ -46,11 +47,49 @@ impl Inspect for Pallet { } impl Transfer for Pallet { - fn transfer(index: &Self::ItemId, dest: &T::AccountId) -> DispatchResult { - Self::do_transfer((*index).into(), None, dest.clone()).map_err(Into::into) + fn transfer(item: &Self::ItemId, dest: &T::AccountId) -> DispatchResult { + Self::do_transfer((*item).into(), None, dest.clone()).map_err(Into::into) } } -// We don't allow any of the mutate operations, so the default implementation is used, which will -// return `TokenError::Unsupported` in case any of the operations is called. -impl Mutate for Pallet {} +/// We don't really support burning and minting. +/// +/// We only need this to allow the region to be reserve transferable. +/// +/// For reserve transfers that are not 'local', the asset must first be withdrawn to the holding +/// register and then deposited into the designated account. This process necessitates that the +/// asset is capable of being 'burned' and 'minted'. +/// +/// Since each region is associated with specific record data, we will not actually burn the asset. +/// If we did, we wouldn't know what record to assign to the newly minted region. Therefore, instead +/// of burning, we set the asset's owner to `None`. In essence, 'burning' a region involves setting +/// its owner to `None`, whereas 'minting' the region assigns its owner to an actual account. This +/// way we never lose track of the associated record data. +impl Mutate for Pallet { + /// Deposit a region into an account. + fn mint_into(item: &Self::ItemId, who: &T::AccountId) -> DispatchResult { + let region_id: RegionId = (*item).into(); + let record = Regions::::get(®ion_id).ok_or(Error::::UnknownRegion)?; + + // 'Minting' can only occur if the asset has previously been burned (i.e. moved to the + // holding register) + ensure!(record.owner.is_none(), Error::::NotAllowed); + Self::issue(region_id.core, region_id.begin, record.end, Some(who.clone()), record.paid); + + Ok(()) + } + + /// Withdraw a region from account. + fn burn(item: &Self::ItemId, maybe_check_owner: Option<&T::AccountId>) -> DispatchResult { + let region_id: RegionId = (*item).into(); + let mut record = Regions::::get(®ion_id).ok_or(Error::::UnknownRegion)?; + if let Some(owner) = maybe_check_owner { + ensure!(Some(owner.clone()) == record.owner, Error::::NotOwner); + } + + record.owner = None; + Regions::::insert(region_id, record); + + Ok(()) + } +} diff --git a/substrate/frame/broker/src/tests.rs b/substrate/frame/broker/src/tests.rs index 8ec0c6d158b3..c573b6c55a20 100644 --- a/substrate/frame/broker/src/tests.rs +++ b/substrate/frame/broker/src/tests.rs @@ -200,14 +200,35 @@ fn transfer_works() { } #[test] -fn mutate_operations_unsupported_for_regions() { - TestExt::new().execute_with(|| { +fn mutate_operations_work() { + TestExt::new().endow(1, 1000).execute_with(|| { let region_id = RegionId { begin: 0, core: 0, mask: CoreMask::complete() }; assert_noop!( >::mint_into(®ion_id.into(), &2), - TokenError::Unsupported + Error::::UnknownRegion + ); + + assert_ok!(Broker::do_start_sales(100, 1)); + advance_to(2); + let region_id = Broker::do_purchase(1, u64::max_value()).unwrap(); + assert_noop!( + >::mint_into(®ion_id.into(), &2), + Error::::NotAllowed + ); + + assert_noop!( + >::burn(®ion_id.into(), Some(&2)), + Error::::NotOwner ); - assert_noop!(>::burn(®ion_id.into(), None), TokenError::Unsupported); + // 'withdraw' the region from user 1: + assert_ok!(>::burn(®ion_id.into(), Some(&1))); + assert_eq!(Regions::::get(region_id).unwrap().owner, None); + + // `mint_into` works after burning: + assert_ok!(>::mint_into(®ion_id.into(), &2)); + assert_eq!(Regions::::get(region_id).unwrap().owner, Some(2)); + + // Unsupported operations: assert_noop!( >::set_attribute(®ion_id.into(), &[], &[]), TokenError::Unsupported @@ -285,7 +306,7 @@ fn nft_metadata_works() { assert_eq!(attribute::(region, b"begin"), 4); assert_eq!(attribute::(region, b"length"), 3); assert_eq!(attribute::(region, b"end"), 7); - assert_eq!(attribute::(region, b"owner"), 1); + assert_eq!(attribute::>(region, b"owner"), Some(1)); assert_eq!(attribute::(region, b"part"), 0xfffff_fffff_fffff_fffff.into()); assert_eq!(attribute::(region, b"core"), 0); assert_eq!(attribute::>(region, b"paid"), Some(100)); @@ -297,7 +318,7 @@ fn nft_metadata_works() { assert_eq!(attribute::(region, b"begin"), 6); assert_eq!(attribute::(region, b"length"), 1); assert_eq!(attribute::(region, b"end"), 7); - assert_eq!(attribute::(region, b"owner"), 42); + assert_eq!(attribute::>(region, b"owner"), Some(42)); assert_eq!(attribute::(region, b"part"), 0x00000_fffff_fffff_00000.into()); assert_eq!(attribute::(region, b"core"), 0); assert_eq!(attribute::>(region, b"paid"), None); diff --git a/substrate/frame/broker/src/types.rs b/substrate/frame/broker/src/types.rs index e8119d29ef5d..f2cae9a41ad4 100644 --- a/substrate/frame/broker/src/types.rs +++ b/substrate/frame/broker/src/types.rs @@ -84,7 +84,7 @@ pub struct RegionRecord { /// The end of the Region. pub end: Timeslice, /// The owner of the Region. - pub owner: AccountId, + pub owner: Option, /// The amount paid to Polkadot for this Region, or `None` if renewal is not allowed. pub paid: Option, } diff --git a/substrate/frame/broker/src/utility_impls.rs b/substrate/frame/broker/src/utility_impls.rs index 3dba5be5b398..4163817a8b58 100644 --- a/substrate/frame/broker/src/utility_impls.rs +++ b/substrate/frame/broker/src/utility_impls.rs @@ -72,11 +72,11 @@ impl Pallet { Ok(()) } - pub(crate) fn issue( + pub fn issue( core: CoreIndex, begin: Timeslice, end: Timeslice, - owner: T::AccountId, + owner: Option, paid: Option>, ) -> RegionId { let id = RegionId { begin, core, mask: CoreMask::complete() }; @@ -94,7 +94,7 @@ impl Pallet { let region = Regions::::get(®ion_id).ok_or(Error::::UnknownRegion)?; if let Some(check_owner) = maybe_check_owner { - ensure!(check_owner == region.owner, Error::::NotOwner); + ensure!(Some(check_owner) == region.owner, Error::::NotOwner); } Regions::::remove(®ion_id);