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

XCM coretime region transfers #3455

Merged
merged 46 commits into from
Apr 17, 2024
Merged

XCM coretime region transfers #3455

Show file tree
Hide file tree
Changes from 15 commits
Commits
Show all changes
46 commits
Select commit Hold shift + click to select a range
5581f42
Fix XCM benchmarks
Szegoo Feb 23, 2024
89b8829
nonfungible implementation
Szegoo Feb 23, 2024
55844cf
progress
Szegoo Feb 23, 2024
c398930
works
Szegoo Feb 23, 2024
9200399
prdoc
Szegoo Feb 23, 2024
7f9eb12
Merge branch 'master' into xcm-bench
Szegoo Feb 23, 2024
99a447e
Merge branch 'master' into xcm-bench
Szegoo Feb 27, 2024
507eb47
conflict readjustements
Szegoo Feb 27, 2024
4b0ee5d
fix
Szegoo Feb 27, 2024
eb7a61b
..
Szegoo Feb 27, 2024
e1f99dd
Region reserve transfers
Szegoo Mar 2, 2024
01ffbe0
tests & migration
Szegoo Mar 4, 2024
6307dd3
fix benchmarks
Szegoo Mar 4, 2024
c9c2504
try runtmie
Szegoo Mar 4, 2024
2ebe29d
fix
Szegoo Mar 4, 2024
beee619
Merge branch 'master' into xcm-bench
bkontur Mar 5, 2024
5448324
fix
Szegoo Mar 7, 2024
911c8c2
Update cumulus/parachains/runtimes/coretime/coretime-rococo/src/lib.rs
Szegoo Mar 7, 2024
30a9133
Update cumulus/parachains/runtimes/coretime/coretime-rococo/src/lib.rs
Szegoo Mar 7, 2024
8ec636b
fix
Szegoo Mar 7, 2024
7809eba
check
Szegoo Mar 7, 2024
663dc0b
...
Szegoo Mar 8, 2024
799a1d9
remove duplicate
Szegoo Mar 11, 2024
52f7d4b
Update polkadot/xcm/pallet-xcm/src/benchmarking.rs
Szegoo Mar 11, 2024
5b5f8de
log error
Szegoo Mar 11, 2024
b414856
improve docs & tests
Szegoo Mar 14, 2024
fd70daa
prdoc
Szegoo Mar 14, 2024
d8eb54b
Update cumulus/parachains/runtimes/coretime/coretime-rococo/src/lib.rs
Szegoo Mar 27, 2024
fdd21e4
Update cumulus/parachains/runtimes/coretime/coretime-westend/src/lib.rs
Szegoo Mar 27, 2024
128b665
Merge branch 'region-reserve-transfers' into xcm-bench
Szegoo Mar 29, 2024
9b2575b
region reserve transfer
Szegoo Mar 29, 2024
09789f9
fix delivery helper
Szegoo Mar 30, 2024
afc3193
apply same fixes in westend
Szegoo Mar 30, 2024
c75b486
update prdoc
Szegoo Apr 2, 2024
3c714a5
move RegionRecordV0 to v1
Szegoo Apr 2, 2024
962b589
Update prdoc/pr_3455.prdoc
Szegoo Apr 5, 2024
94fc92f
Update prdoc/pr_3455.prdoc
Szegoo Apr 5, 2024
883ded4
Update substrate/frame/broker/src/nonfungible_impl.rs
Szegoo Apr 5, 2024
0f510c8
revert contracts-rococo xcm config
Szegoo Apr 5, 2024
e6cc759
update prdoc
Szegoo Apr 5, 2024
73e9c7f
bump version
Szegoo Apr 5, 2024
a04285e
Merge branch 'master' into xcm-bench
Szegoo Apr 8, 2024
e312286
Update substrate/frame/broker/src/lib.rs
Szegoo Apr 8, 2024
948aafa
Merge branch 'master' into xcm-bench
Szegoo Apr 16, 2024
1374042
add missing migration in rococo & westend
Szegoo Apr 16, 2024
8d1d338
fmt
Szegoo Apr 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion cumulus/parachains/runtimes/contracts/contracts-rococo/src/xcm_config.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -227,7 +227,7 @@ impl pallet_xcm::Config for Runtime {
type XcmExecuteFilter = Nothing;
type XcmExecutor = XcmExecutor<XcmConfig>;
type XcmTeleportFilter = Everything;
type XcmReserveTransferFilter = Everything;
type XcmReserveTransferFilter = Nothing;
Szegoo marked this conversation as resolved.
Show resolved Hide resolved
type Weigher = FixedWeightBounds<UnitWeightCost, RuntimeCall, MaxInstructions>;
type UniversalLocation = UniversalLocation;
type RuntimeOrigin = RuntimeOrigin;
Expand Down
31 changes: 24 additions & 7 deletions cumulus/parachains/runtimes/coretime/coretime-rococo/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -740,8 +740,15 @@ 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.
let raw_region_id = 42;
Some((
Asset {
fun: NonFungible(Index(raw_region_id)),
id: AssetId(Location::new(0, [PalletInstance(50)]))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This will still fail since we don't support minting regions, resulting in setting the weight to the max value.

I added 'region minting' here: #3553, however, that would still disallow this.

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 Any ideas on what we might do in this regard?

Copy link
Contributor

Choose a reason for hiding this comment

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

cc @seadanda

Copy link
Contributor

Choose a reason for hiding this comment

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

@Szegoo is this still an issue? I think you need to create a region with raw_region_id=42 first, so maybe some broker pallet magic, e.g.:

        let caller: T::AccountId = whitelisted_caller();
		T::Currency::set_balance(
			&caller.clone(),
			T::Currency::minimum_balance().saturating_add(20u32.into()),
		);

        /// TODO: maybe some more magic e.g. `setup_and_start_sale` in the pallet_broker's benchmarks
		let region = Broker::<T>::do_purchase(caller.clone(), 10u32.into())?;

or alternatively, simply inserting some data could do the trick:

let raw_region_id = 42;
let region = RegionRecord<AccountId, Balance> {
	end: ...,
	owner: ...,
	paid: ...,
};
Regions::<T>::insert(&raw_region_id, &region);

Copy link
Contributor Author

@Szegoo Szegoo Mar 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Inside pallet-xcm when running the benchmark for reserve transfers it will attempt to deposit the region to an account before transferring it.

This requires the mint_into to be implemented for regions.

A possible solution could be to do something like this:

fn mint_into(item: &Self::ItemId, who: &T::AccountId) -> DispatchResult {

	let region_id: RegionId = (*item).into();

	#[cfg(not(features = "runtime-benchmarks"))]
	// Code from #3553:
	{
		let record = Regions::<T>::get(&region_id).ok_or(Error::<T>::UnknownRegion)?;

		// 'Minting' can only occur if the asset has previously been burned(i.e. moved to the
		// holding registrar)
		ensure!(record.owner.is_none(), Error::<T>::NotAllowed);
		Self::issue(region_id.core, region_id.begin, record.end, who.clone(), record.paid);
	}
	#[cfg(features = "runtime-benchmarks")]
	{
   		// If the region record exists, use that data; if not, we assume the intention was to 
		// mint the region, so we simply provide some dummy data.
		let record = Regions::<T>::get(&region_id).unwrap_or_default(dummy_data);
		Self::issue(region_id.core, region_id.begin, end, who.clone(), None);
	}

	Ok(())
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this will still fail:

  1. call Broker::create_region_for_benchmark <- creates the region we will be reserve transferring
  2. AssetTransactor::deposit_asset will fail since based on the code from Fix: Region reserve transfers #3553 we can't mint an already existing region: https://github.com/paritytech/polkadot-sdk/pull/3553/files#diff-40b92c4632f430a76d4f2c6a366582cbd2e4898fb24cd1a106a20c39872ea4d1R76

I think this is clean solution. I would not change or ignore MAX for failed AssetTransactor::deposit_asset(.

Yeah, I agree, that isn't a good solution.

Maybe branching based on the feature flag might not actually be a bad solution

Copy link
Contributor

@bkontur bkontur Mar 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

What exactly will fail?

  1. call Broker::create_region_for_benchmark will create region without owner (I see, that in #3553 is changed to Option):
let region = RegionRecord<AccountId, Balance> {
			end: ...,
			owner: None,
			paid: ...,
		};
Regions::<T>::insert(&raw_region_id, &region);

  1. AssetTransactor::deposit_asset will call mint_into and that ensure!(record.owner.is_none(), Error::<T>::NotAllowed); will pass, because we set owner: None, so mint_into sets an owner with origin_location as AccountId, so it exactly does what we need.

  2. pallet_xcm::reserve_transfer_assets executes TransferAsset XCM instruction which should end up with calling pallet_broker::do_transfer, which just does some owner change.

so far so good, unless I am missing anything.

Maybe, I would suggest to add here: #3553 a xcm-emulator tests or xcm unit-tests for the coretime-rococo/westend to ensure that pallet_xcm::reserve_transfer_assets really works, because you need to probably handle some Fungible fees besides NonFungible(regionId), because we need to handle/set BuyExecution on the destination, but that is not a problem, I could help, just tell me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

call Broker::create_region_for_benchmark will create region without owner

Ah, ok, I get it now. Yes, this is a good solution. I was assuming the region would be created with an owner.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I merged #3553 into this branch to test if it works, and it did, I managed to run the benchmark and generate weights :)

@bkontur Should we first merge #3553, or have both of these changes in this single PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

@Szegoo very cool, I see we don't even need create_region_for_benchmark and Pallet::issue` works, much better :)

@bkontur Should we first merge #3553, or have both of these changes in this single PR?

I would go just with one PR

Szegoo marked this conversation as resolved.
Show resolved Hide resolved
},
ParentThen(Parachain(RandomParaId::get().into()).into()).into(),
))
}

fn get_asset() -> Asset {
Expand All @@ -757,15 +764,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<Location, BenchmarkError> {
Ok(RocRelayLocation::get())
Expand Down
2 changes: 1 addition & 1 deletion cumulus/parachains/runtimes/coretime/coretime-rococo/src/xcm_config.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -286,7 +286,7 @@ impl pallet_xcm::Config for Runtime {
type XcmExecuteFilter = Nothing;
type XcmExecutor = XcmExecutor<XcmConfig>;
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>,
RuntimeCall,
Expand Down
31 changes: 24 additions & 7 deletions cumulus/parachains/runtimes/coretime/coretime-westend/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -222,6 +222,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 {
Expand Down Expand Up @@ -709,11 +710,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<Location> {
Some(Parent.into())
Expand All @@ -731,8 +741,15 @@ 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.
Szegoo marked this conversation as resolved.
Show resolved Hide resolved
let raw_region_id = 42;
Some((
Asset {
fun: NonFungible(Index(raw_region_id)),
id: AssetId(Location::new(0, [PalletInstance(50)]))
Szegoo marked this conversation as resolved.
Show resolved Hide resolved
},
ParentThen(Parachain(RandomParaId::get().into()).into()).into(),
))
}

fn get_asset() -> Asset {
Expand Down
2 changes: 1 addition & 1 deletion cumulus/parachains/runtimes/coretime/coretime-westend/src/xcm_config.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -298,7 +298,7 @@ impl pallet_xcm::Config for Runtime {
type XcmExecuteFilter = Nothing;
type XcmExecutor = XcmExecutor<XcmConfig>;
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>,
RuntimeCall,
Expand Down
88 changes: 48 additions & 40 deletions polkadot/xcm/pallet-xcm/src/benchmarking.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,7 @@
use super::*;
use bounded_collections::{ConstU32, WeakBoundedVec};
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};
Expand Down Expand Up @@ -89,11 +86,6 @@ pub trait Config: crate::Config {
}

benchmarks! {
where_clause {
where
T: pallet_balances::Config,
<T as pallet_balances::Config>::Balance: From<u128> + Into<u128>,
}
bkontur marked this conversation as resolved.
Show resolved Hide resolved
send {
let send_origin =
T::SendXcmOrigin::try_successful_origin().map_err(|_| BenchmarkError::Weightless)?;
Expand All @@ -113,11 +105,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());
Expand All @@ -134,35 +122,34 @@ benchmarks! {
FeeReason::ChargeFees,
);

// Actual balance (e.g. `ensure_successful_delivery` could drip delivery fees, ...)
let balance = <pallet_balances::Pallet<T> as Inspect<_>>::balance(&caller);
// Add transferred_amount to origin
<pallet_balances::Pallet<T> as Mutate<_>>::mint_into(&caller, transferred_amount)?;
// verify initial balance
let balance = balance + transferred_amount;
assert_eq!(<pallet_balances::Pallet<T> as Inspect<_>>::balance(&caller), balance);
match &asset.fun {
Fungible(amount) => {
// Add transferred_amount to origin
<T::XcmExecutor as XcmAssetTransfers>::AssetTransactor::deposit_asset(
&Asset { fun: Fungible(*amount), id: asset.id },
&origin_location,
None,
).map_err(|_| BenchmarkError::Override(BenchmarkResult::from_weight(Weight::MAX)))?;
Szegoo marked this conversation as resolved.
Show resolved Hide resolved
},
NonFungible(instance) => {
<T::XcmExecutor as XcmAssetTransfers>::AssetTransactor::deposit_asset(&asset, &origin_location, None)
.map_err(|_| BenchmarkError::Override(BenchmarkResult::from_weight(Weight::MAX)))?;
Szegoo marked this conversation as resolved.
Show resolved Hide resolved
}
};

let recipient = [0u8; 32];
let versioned_dest: VersionedLocation = destination.into();
let versioned_beneficiary: VersionedLocation =
AccountId32 { network: None, id: recipient.into() }.into();
let versioned_assets: VersionedAssets = assets.into();
}: _<RuntimeOrigin<T>>(send_origin.into(), Box::new(versioned_dest), Box::new(versioned_beneficiary), Box::new(versioned_assets), 0)
seadanda marked this conversation as resolved.
Show resolved Hide resolved
verify {
// verify balance after transfer, decreased by transferred amount (+ maybe XCM delivery fees)
assert!(<pallet_balances::Pallet<T> as Inspect<_>>::balance(&caller) <= balance - transferred_amount);
}
bkontur marked this conversation as resolved.
Show resolved Hide resolved

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());
Expand All @@ -179,23 +166,44 @@ benchmarks! {
FeeReason::ChargeFees,
);

// Actual balance (e.g. `ensure_successful_delivery` could drip delivery fees, ...)
let balance = <pallet_balances::Pallet<T> as Inspect<_>>::balance(&caller);
// Add transferred_amount to origin
<pallet_balances::Pallet<T> as Mutate<_>>::mint_into(&caller, transferred_amount)?;
// verify initial balance
let balance = balance + transferred_amount;
assert_eq!(<pallet_balances::Pallet<T> as Inspect<_>>::balance(&caller), balance);
match &asset.fun {
Fungible(amount) => {
// Add transferred_amount to origin
<T::XcmExecutor as XcmAssetTransfers>::AssetTransactor::deposit_asset(
&Asset { fun: Fungible(*amount), id: asset.id.clone() },
&origin_location,
None,
).map_err(|_| BenchmarkError::Override(BenchmarkResult::from_weight(Weight::MAX)))?;
},
NonFungible(instance) => {
<T::XcmExecutor as XcmAssetTransfers>::AssetTransactor::deposit_asset(&asset, &origin_location, None)
.map_err(|_| 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();
}: _<RuntimeOrigin<T>>(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!(<pallet_balances::Pallet<T> as Inspect<_>>::balance(&caller) <= balance - transferred_amount);
match &asset.fun {
Fungible(amount) => {
assert_ok!(<T::XcmExecutor as XcmAssetTransfers>::AssetTransactor::withdraw_asset(
&Asset { fun: Fungible(*amount), id: asset.id },
&destination,
None,
));
},
NonFungible(instance) => {
assert_ok!(<T::XcmExecutor as XcmAssetTransfers>::AssetTransactor::withdraw_asset(
&asset,
&destination,
None,
));
}
};
}

transfer_assets {
Expand Down
18 changes: 18 additions & 0 deletions prdoc/pr_3455.prdoc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
# 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: pallet-xcm benchmarking fixes

doc:
- audience: Runtime Dev
description: |
With this PR, the benchmarks for pallet-xcm no longer assume the use of
`pallet-balances` for fungible implementations. Furthermore, with this PR
non-fungible teleport and reserve transfers are properly benchmarked.

crates:
- name: pallet-xcm
- name: contracts-rococo-runtime
- name: coretime-rococo-runtime
- name: coretime-westend-runtime
- name: coretime-westend-runtime
Szegoo marked this conversation as resolved.
Show resolved Hide resolved
Loading