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

Ensure outbound XCMs are decodable with limits + add EnsureDecodableXcm router (for testing purposes) #4186

Merged
merged 8 commits into from
Apr 23, 2024
Merged

Ensure outbound XCMs are decodable with limits + add EnsureDecodableXcm router (for testing purposes) #4186

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
9331a6c
Add `EnsureDecodableXcm` router (for testing purposes)
bkontur Apr 18, 2024
5f53e19
Use `EnsureDecodableXcm` for `polkadot-test-runtime`
bkontur Apr 18, 2024
feaf731
Update polkadot/xcm/xcm-builder/src/routing.rs
bkontur Apr 18, 2024
5080212
Fix import
bkontur Apr 18, 2024
f7e94f4
Fixed XCM benchmarks to generate holding register data
bkontur Apr 18, 2024
fd213ca
Merge branch 'master' into bko-xcm-codec-tests
bkontur Apr 23, 2024
d08c440
Removed `ensure_encode_decode` and replaced by `VersionedXcm::validat…
bkontur Apr 23, 2024
198bef1
Merge branch 'master' into bko-xcm-codec-tests
bkontur Apr 23, 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
8 changes: 0 additions & 8 deletions Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

10 changes: 0 additions & 10 deletions polkadot/runtime/test-runtime/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,14 +11,10 @@ license.workspace = true
workspace = true

[dependencies]
bitvec = { version = "1.0.0", default-features = false, features = ["alloc"] }
parity-scale-codec = { version = "3.6.1", default-features = false, features = ["derive"] }
log = { workspace = true }
rustc-hex = { version = "2.1.0", default-features = false }
scale-info = { version = "2.11.1", default-features = false, features = ["derive"] }
serde = { workspace = true }
serde_derive = { optional = true, workspace = true }
smallvec = "1.8.0"

authority-discovery-primitives = { package = "sp-authority-discovery", path = "../../../substrate/primitives/authority-discovery", default-features = false }
babe-primitives = { package = "sp-consensus-babe", path = "../../../substrate/primitives/consensus/babe", default-features = false }
Expand Down Expand Up @@ -63,7 +59,6 @@ pallet-vesting = { path = "../../../substrate/frame/vesting", default-features =
runtime-common = { package = "polkadot-runtime-common", path = "../common", default-features = false }
primitives = { package = "polkadot-primitives", path = "../../primitives", default-features = false }
pallet-xcm = { path = "../../xcm/pallet-xcm", default-features = false }
polkadot-parachain-primitives = { path = "../../parachain", default-features = false }
polkadot-runtime-parachains = { path = "../parachains", default-features = false }
xcm-builder = { package = "staging-xcm-builder", path = "../../xcm/xcm-builder", default-features = false }
xcm-executor = { package = "staging-xcm-executor", path = "../../xcm/xcm-executor", default-features = false }
Expand Down Expand Up @@ -92,7 +87,6 @@ std = [
"authority-discovery-primitives/std",
"babe-primitives/std",
"beefy-primitives/std",
"bitvec/std",
"block-builder-api/std",
"frame-election-provider-support/std",
"frame-executive/std",
Expand All @@ -118,14 +112,11 @@ std = [
"pallet-vesting/std",
"pallet-xcm/std",
"parity-scale-codec/std",
"polkadot-parachain-primitives/std",
"polkadot-runtime-parachains/std",
"primitives/std",
"runtime-common/std",
"rustc-hex/std",
"scale-info/std",
"serde/std",
"serde_derive",
"sp-api/std",
"sp-core/std",
"sp-genesis-builder/std",
Expand Down Expand Up @@ -157,7 +148,6 @@ runtime-benchmarks = [
"pallet-timestamp/runtime-benchmarks",
"pallet-vesting/runtime-benchmarks",
"pallet-xcm/runtime-benchmarks",
"polkadot-parachain-primitives/runtime-benchmarks",
"polkadot-runtime-parachains/runtime-benchmarks",
"primitives/runtime-benchmarks",
"runtime-common/runtime-benchmarks",
Expand Down
6 changes: 0 additions & 6 deletions polkadot/runtime/test-runtime/constants/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,18 +14,12 @@ smallvec = "1.8.0"

frame-support = { path = "../../../../substrate/frame/support", default-features = false }
primitives = { package = "polkadot-primitives", path = "../../../primitives", default-features = false }
runtime-common = { package = "polkadot-runtime-common", path = "../../common", default-features = false }
sp-runtime = { path = "../../../../substrate/primitives/runtime", default-features = false }
sp-weights = { path = "../../../../substrate/primitives/weights", default-features = false }
sp-core = { path = "../../../../substrate/primitives/core", default-features = false }

[features]
default = ["std"]
std = [
"frame-support/std",
"primitives/std",
"runtime-common/std",
"sp-core/std",
"sp-runtime/std",
"sp-weights/std",
]
10 changes: 6 additions & 4 deletions polkadot/runtime/test-runtime/src/xcm_config.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,8 @@ use polkadot_runtime_parachains::FeeTracker;
use runtime_common::xcm_sender::{ChildParachainRouter, PriceForMessageDelivery};
use xcm::latest::prelude::*;
use xcm_builder::{
AllowUnpaidExecutionFrom, EnsureXcmOrigin, FixedWeightBounds, FrameTransactionalProcessor,
SignedAccountId32AsNative, SignedToAccountId32, WithUniqueTopic,
AllowUnpaidExecutionFrom, EnsureDecodableXcm, EnsureXcmOrigin, FixedWeightBounds,
FrameTransactionalProcessor, SignedAccountId32AsNative, SignedToAccountId32, WithUniqueTopic,
};
use xcm_executor::{
traits::{TransactAsset, WeightTrader},
Expand Down Expand Up @@ -82,8 +82,10 @@ pub type PriceForChildParachainDelivery = TestDeliveryPrice<FeeAssetId, super::D
/// The XCM router. When we want to send an XCM message, we use this type. It amalgamates all of our
/// individual routers.
pub type XcmRouter = WithUniqueTopic<
// Only one router so far - use DMP to communicate with child parachains.
ChildParachainRouter<super::Runtime, super::Xcm, PriceForChildParachainDelivery>,
EnsureDecodableXcm<
// Only one router so far - use DMP to communicate with child parachains.
ChildParachainRouter<super::Runtime, super::Xcm, PriceForChildParachainDelivery>,
>,
>;

pub type Barrier = AllowUnpaidExecutionFrom<Everything>;
Expand Down
26 changes: 19 additions & 7 deletions polkadot/xcm/pallet-xcm-benchmarks/src/fungible/benchmarking.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,8 +146,6 @@ benchmarks_instance_pallet! {

initiate_reserve_withdraw {
let (sender_account, sender_location) = account_and_location::<T>(1);
let holding = T::worst_case_holding(1);
let assets_filter = AssetFilter::Definite(holding.clone().into_inner().into_iter().take(MAX_ITEMS_IN_ASSETS).collect::<Vec<_>>().into());
let reserve = T::valid_destination().map_err(|_| BenchmarkError::Skip)?;

let (expected_fees_mode, expected_assets_in_holding) = T::DeliveryHelper::ensure_successful_delivery(
Expand All @@ -157,15 +155,29 @@ benchmarks_instance_pallet! {
);
let sender_account_balance_before = T::TransactAsset::balance(&sender_account);

// generate holding and add possible required fees
Copy link
Contributor

Choose a reason for hiding this comment

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

Where do you add the possible required fee?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

right bellow - required fees = expected_assets_in_holding

            for a in expected_assets_in_holding.into_inner() {
				holding.push(a);
			}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

there was just executor.set_holding(expected_assets_in_holding) before, which could possibly remove previously executor.set_holding(holding.clone()

let holding = if let Some(expected_assets_in_holding) = expected_assets_in_holding {
let mut holding = T::worst_case_holding(1 + expected_assets_in_holding.len() as u32);
for a in expected_assets_in_holding.into_inner() {
holding.push(a);
}
holding
} else {
T::worst_case_holding(1)
};

let mut executor = new_executor::<T>(sender_location);
executor.set_holding(holding.into());
executor.set_holding(holding.clone().into());
if let Some(expected_fees_mode) = expected_fees_mode {
executor.set_fees_mode(expected_fees_mode);
}
if let Some(expected_assets_in_holding) = expected_assets_in_holding {
executor.set_holding(expected_assets_in_holding.into());
}
let instruction = Instruction::InitiateReserveWithdraw { assets: assets_filter, reserve, xcm: Xcm(vec![]) };

let instruction = Instruction::InitiateReserveWithdraw {
// Worst case is looking through all holdings for every asset explicitly - respecting the limit `MAX_ITEMS_IN_ASSETS`.
assets: Definite(holding.into_inner().into_iter().take(MAX_ITEMS_IN_ASSETS).collect::<Vec<_>>().into()),
reserve,
xcm: Xcm(vec![])
};
let xcm = Xcm(vec![instruction]);
}: {
executor.bench_process(xcm)?;
Expand Down
13 changes: 7 additions & 6 deletions polkadot/xcm/pallet-xcm-benchmarks/src/fungible/mock.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@

//! A mock runtime for XCM benchmarking.

use crate::{fungible as xcm_balances_benchmark, mock::*};
use crate::{fungible as xcm_balances_benchmark, generate_holding_assets, mock::*};
use frame_benchmarking::BenchmarkError;
use frame_support::{
derive_impl, parameter_types,
Expand All @@ -26,7 +26,9 @@ use frame_support::{
use sp_core::H256;
use sp_runtime::traits::{BlakeTwo256, IdentityLookup};
use xcm::latest::prelude::*;
use xcm_builder::{AllowUnpaidExecutionFrom, FrameTransactionalProcessor, MintLocation};
use xcm_builder::{
AllowUnpaidExecutionFrom, EnsureDecodableXcm, FrameTransactionalProcessor, MintLocation,
};

type Block = frame_system::mocking::MockBlock<Test>;

Expand Down Expand Up @@ -121,7 +123,7 @@ parameter_types! {
pub struct XcmConfig;
impl xcm_executor::Config for XcmConfig {
type RuntimeCall = RuntimeCall;
type XcmSender = DevNull;
type XcmSender = EnsureDecodableXcm<DevNull>;
type AssetTransactor = AssetTransactor;
type OriginConverter = ();
type IsReserve = TrustedReserves;
Expand Down Expand Up @@ -160,9 +162,8 @@ impl crate::Config for Test {
Ok(valid_destination)
}
fn worst_case_holding(depositable_count: u32) -> Assets {
crate::mock_worst_case_holding(
depositable_count,
<XcmConfig as xcm_executor::Config>::MaxAssetsIntoHolding::get(),
generate_holding_assets(
<XcmConfig as xcm_executor::Config>::MaxAssetsIntoHolding::get() - depositable_count,
)
}
}
Expand Down
36 changes: 24 additions & 12 deletions polkadot/xcm/pallet-xcm-benchmarks/src/generic/benchmarking.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,9 @@ use crate::{account_and_location, new_executor, EnsureDelivery, XcmCallOf};
use codec::Encode;
use frame_benchmarking::{benchmarks, BenchmarkError};
use frame_support::{dispatch::GetDispatchInfo, traits::fungible::Inspect};
use sp_std::vec;
use sp_std::{prelude::*, vec};
use xcm::{
latest::{prelude::*, MaxDispatchErrorLen, MaybeErrorCode, Weight},
latest::{prelude::*, MaxDispatchErrorLen, MaybeErrorCode, Weight, MAX_ITEMS_IN_ASSETS},
DoubleEncoded,
};
use xcm_executor::{
Expand All @@ -32,7 +32,6 @@ use xcm_executor::{
benchmarks! {
report_holding {
let (sender_account, sender_location) = account_and_location::<T>(1);
let holding = T::worst_case_holding(0);
let destination = T::valid_destination().map_err(|_| BenchmarkError::Skip)?;

let (expected_fees_mode, expected_assets_in_holding) = T::DeliveryHelper::ensure_successful_delivery(
Expand All @@ -42,23 +41,31 @@ benchmarks! {
);
let sender_account_balance_before = T::TransactAsset::balance(&sender_account);

// generate holding and add possible required fees
let holding = if let Some(expected_assets_in_holding) = expected_assets_in_holding {
let mut holding = T::worst_case_holding(expected_assets_in_holding.len() as u32);
for a in expected_assets_in_holding.into_inner() {
holding.push(a);
}
holding
} else {
T::worst_case_holding(0)
};

let mut executor = new_executor::<T>(sender_location);
executor.set_holding(holding.clone().into());
if let Some(expected_fees_mode) = expected_fees_mode {
executor.set_fees_mode(expected_fees_mode);
}
if let Some(expected_assets_in_holding) = expected_assets_in_holding {
executor.set_holding(expected_assets_in_holding.into());
}

let instruction = Instruction::<XcmCallOf<T>>::ReportHolding {
response_info: QueryResponseInfo {
destination,
query_id: Default::default(),
max_weight: Weight::MAX,
},
// Worst case is looking through all holdings for every asset explicitly.
assets: Definite(holding),
// Worst case is looking through all holdings for every asset explicitly - respecting the limit `MAX_ITEMS_IN_ASSETS`.
assets: Definite(holding.into_inner().into_iter().take(MAX_ITEMS_IN_ASSETS).collect::<Vec<_>>().into()),
};

let xcm = Xcm(vec![instruction]);
Expand Down Expand Up @@ -612,14 +619,19 @@ benchmarks! {
let sender_account = T::AccountIdConverter::convert_location(&owner).unwrap();
let sender_account_balance_before = T::TransactAsset::balance(&sender_account);

// generate holding and add possible required fees
let mut holding: Assets = asset.clone().into();
if let Some(expected_assets_in_holding) = expected_assets_in_holding {
for a in expected_assets_in_holding.into_inner() {
holding.push(a);
}
};

let mut executor = new_executor::<T>(owner);
executor.set_holding(asset.clone().into());
executor.set_holding(holding.into());
if let Some(expected_fees_mode) = expected_fees_mode {
executor.set_fees_mode(expected_fees_mode);
}
if let Some(expected_assets_in_holding) = expected_assets_in_holding {
executor.set_holding(expected_assets_in_holding.into());
}

let instruction = Instruction::LockAsset { asset, unlocker };
let xcm = Xcm(vec![instruction]);
Expand Down
10 changes: 5 additions & 5 deletions polkadot/xcm/pallet-xcm-benchmarks/src/generic/mock.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,8 @@ use xcm_builder::{
AssetsInHolding, TestAssetExchanger, TestAssetLocker, TestAssetTrap,
TestSubscriptionService, TestUniversalAliases,
},
AliasForeignAccountId32, AllowUnpaidExecutionFrom, FrameTransactionalProcessor,
AliasForeignAccountId32, AllowUnpaidExecutionFrom, EnsureDecodableXcm,
FrameTransactionalProcessor,
};
use xcm_executor::traits::ConvertOrigin;

Expand Down Expand Up @@ -110,7 +111,7 @@ type Aliasers = AliasForeignAccountId32<OnlyParachains>;
pub struct XcmConfig;
impl xcm_executor::Config for XcmConfig {
type RuntimeCall = RuntimeCall;
type XcmSender = DevNull;
type XcmSender = EnsureDecodableXcm<DevNull>;
type AssetTransactor = NoAssetTransactor;
type OriginConverter = AlwaysSignedByDefault<RuntimeOrigin>;
type IsReserve = AllAssetLocationsPass;
Expand Down Expand Up @@ -161,9 +162,8 @@ impl crate::Config for Test {
Ok(valid_destination)
}
fn worst_case_holding(depositable_count: u32) -> Assets {
crate::mock_worst_case_holding(
depositable_count,
<XcmConfig as xcm_executor::Config>::MaxAssetsIntoHolding::get(),
generate_holding_assets(
<XcmConfig as xcm_executor::Config>::MaxAssetsIntoHolding::get() - depositable_count,
)
}
}
Expand Down
13 changes: 9 additions & 4 deletions polkadot/xcm/pallet-xcm-benchmarks/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,8 @@ pub trait Config: frame_system::Config {
fn valid_destination() -> Result<Location, BenchmarkError>;

/// Worst case scenario for a holding account in this runtime.
/// - `depositable_count` specifies the count of assets we plan to add to the holding on top of
/// those generated by the `worst_case_holding` implementation.
Copy link
Contributor

Choose a reason for hiding this comment

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

😄

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 other suggestion?

fn worst_case_holding(depositable_count: u32) -> Assets;
}

Expand All @@ -64,19 +66,22 @@ pub type AssetTransactorOf<T> = <<T as Config>::XcmConfig as XcmConfig>::AssetTr
/// The call type of executor's config. Should eventually resolve to the same overarching call type.
pub type XcmCallOf<T> = <<T as Config>::XcmConfig as XcmConfig>::RuntimeCall;

pub fn mock_worst_case_holding(depositable_count: u32, max_assets: u32) -> Assets {
pub fn generate_holding_assets(max_assets: u32) -> Assets {
let fungibles_amount: u128 = 100;
let holding_fungibles = max_assets / 2 - depositable_count;
let holding_non_fungibles = holding_fungibles;
let holding_fungibles = max_assets / 2;
let holding_non_fungibles = max_assets - holding_fungibles - 1; // -1 because of adding `Here` asset
// add count of `holding_fungibles`
(0..holding_fungibles)
.map(|i| {
Asset {
id: AssetId(GeneralIndex(i as u128).into()),
fun: Fungible(fungibles_amount * i as u128),
fun: Fungible(fungibles_amount * (i + 1) as u128), // non-zero amount
}
.into()
})
// add one more `Here` asset
.chain(core::iter::once(Asset { id: AssetId(Here.into()), fun: Fungible(u128::MAX) }))
// add count of `holding_non_fungibles`
.chain((0..holding_non_fungibles).map(|i| Asset {
id: AssetId(GeneralIndex(i as u128).into()),
fun: NonFungible(asset_instance_from(i)),
Expand Down
12 changes: 7 additions & 5 deletions polkadot/xcm/pallet-xcm/src/mock.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,10 +33,11 @@ use xcm::prelude::*;
use xcm_builder::{
AccountId32Aliases, AllowKnownQueryResponses, AllowSubscriptionsFrom,
AllowTopLevelPaidExecutionFrom, Case, ChildParachainAsNative, ChildParachainConvertsVia,
ChildSystemParachainAsSuperuser, DescribeAllTerminal, FixedRateOfFungible, FixedWeightBounds,
FrameTransactionalProcessor, FungibleAdapter, FungiblesAdapter, HashedDescription, IsConcrete,
MatchedConvertedConcreteId, NoChecking, SignedAccountId32AsNative, SignedToAccountId32,
SovereignSignedViaLocation, TakeWeightCredit, XcmFeeManagerFromComponents, XcmFeeToAccount,
ChildSystemParachainAsSuperuser, DescribeAllTerminal, EnsureDecodableXcm, FixedRateOfFungible,
FixedWeightBounds, FrameTransactionalProcessor, FungibleAdapter, FungiblesAdapter,
HashedDescription, IsConcrete, MatchedConvertedConcreteId, NoChecking,
SignedAccountId32AsNative, SignedToAccountId32, SovereignSignedViaLocation, TakeWeightCredit,
XcmFeeManagerFromComponents, XcmFeeToAccount,
};
use xcm_executor::{
traits::{Identity, JustTry},
Expand Down Expand Up @@ -488,7 +489,8 @@ pub type Barrier = (
AllowSubscriptionsFrom<Everything>,
);

pub type XcmRouter = (TestPaidForPara3000SendXcm, TestSendXcmErrX8, TestSendXcm);
pub type XcmRouter =
EnsureDecodableXcm<(TestPaidForPara3000SendXcm, TestSendXcmErrX8, TestSendXcm)>;

pub struct XcmConfig;
impl xcm_executor::Config for XcmConfig {
Expand Down
Loading
Loading