From faa431a5b1e7b20bba58bed3bc0570d6c9ac0eb6 Mon Sep 17 00:00:00 2001 From: ron Date: Wed, 15 Jan 2025 10:42:39 +0800 Subject: [PATCH 01/35] Fix for DenyThenTry --- polkadot/xcm/xcm-builder/src/barriers.rs | 47 ++++++- polkadot/xcm/xcm-builder/src/lib.rs | 6 +- .../xcm/xcm-builder/src/tests/barriers.rs | 125 ++++++++++++++++++ polkadot/xcm/xcm-executor/src/traits/mod.rs | 2 +- .../xcm-executor/src/traits/should_execute.rs | 62 +++++++++ 5 files changed, 232 insertions(+), 10 deletions(-) diff --git a/polkadot/xcm/xcm-builder/src/barriers.rs b/polkadot/xcm/xcm-builder/src/barriers.rs index adba9a3ef79f..6c6cd95f7484 100644 --- a/polkadot/xcm/xcm-builder/src/barriers.rs +++ b/polkadot/xcm/xcm-builder/src/barriers.rs @@ -24,7 +24,9 @@ use frame_support::{ }; use polkadot_parachain_primitives::primitives::IsSystem; use xcm::prelude::*; -use xcm_executor::traits::{CheckSuspension, OnResponse, Properties, ShouldExecute}; +use xcm_executor::traits::{ + CheckSuspension, OnResponse, Properties, ShouldExecute, ShouldNotExecute, +}; /// Execution barrier that just takes `max_weight` from `properties.weight_credit`. /// @@ -444,12 +446,12 @@ impl ShouldExecute for AllowHrmpNotificationsFromRelayChain { /// If it passes the Deny, and matches one of the Allow cases then it is let through. pub struct DenyThenTry(PhantomData, PhantomData) where - Deny: ShouldExecute, + Deny: ShouldNotExecute, Allow: ShouldExecute; impl ShouldExecute for DenyThenTry where - Deny: ShouldExecute, + Deny: ShouldNotExecute, Allow: ShouldExecute, { fn should_execute( @@ -458,15 +460,15 @@ where max_weight: Weight, properties: &mut Properties, ) -> Result<(), ProcessMessageError> { - Deny::should_execute(origin, message, max_weight, properties)?; + Deny::should_not_execute(origin, message, max_weight, properties)?; Allow::should_execute(origin, message, max_weight, properties) } } // See issue pub struct DenyReserveTransferToRelayChain; -impl ShouldExecute for DenyReserveTransferToRelayChain { - fn should_execute( +impl ShouldNotExecute for DenyReserveTransferToRelayChain { + fn should_not_execute( origin: &Location, message: &mut [Instruction], _max_weight: Weight, @@ -504,3 +506,36 @@ impl ShouldExecute for DenyReserveTransferToRelayChain { Ok(()) } } + +// See https://github.com/paritytech/polkadot-sdk/pull/6838 +pub struct DenyFirstExportMessageFrom( + PhantomData<(FromOrigin, ToGlobalConsensus)>, +); + +impl ShouldNotExecute + for DenyFirstExportMessageFrom +where + FromOrigin: Contains, + ToGlobalConsensus: Contains, +{ + fn should_not_execute( + origin: &Location, + message: &mut [Instruction], + _max_weight: Weight, + _properties: &mut Properties, + ) -> Result<(), ProcessMessageError> { + message.matcher().match_next_inst_while( + |_| true, + |inst| match inst { + ExportMessage { network, .. } => + if ToGlobalConsensus::contains(network) && FromOrigin::contains(origin) { + return Err(ProcessMessageError::Unsupported) + } else { + Ok(ControlFlow::Continue(())) + }, + _ => Ok(ControlFlow::Continue(())), + }, + )?; + Ok(()) + } +} diff --git a/polkadot/xcm/xcm-builder/src/lib.rs b/polkadot/xcm/xcm-builder/src/lib.rs index e23412a97ebc..0c01345941d8 100644 --- a/polkadot/xcm/xcm-builder/src/lib.rs +++ b/polkadot/xcm/xcm-builder/src/lib.rs @@ -42,9 +42,9 @@ mod barriers; pub use barriers::{ AllowExplicitUnpaidExecutionFrom, AllowHrmpNotificationsFromRelayChain, AllowKnownQueryResponses, AllowSubscriptionsFrom, AllowTopLevelPaidExecutionFrom, - AllowUnpaidExecutionFrom, DenyReserveTransferToRelayChain, DenyThenTry, IsChildSystemParachain, - IsParentsOnly, IsSiblingSystemParachain, RespectSuspension, TakeWeightCredit, - TrailingSetTopicAsId, WithComputedOrigin, + AllowUnpaidExecutionFrom, DenyFirstExportMessageFrom, DenyReserveTransferToRelayChain, + DenyThenTry, IsChildSystemParachain, IsParentsOnly, IsSiblingSystemParachain, + RespectSuspension, TakeWeightCredit, TrailingSetTopicAsId, WithComputedOrigin, }; mod controller; diff --git a/polkadot/xcm/xcm-builder/src/tests/barriers.rs b/polkadot/xcm/xcm-builder/src/tests/barriers.rs index d8805274d3a5..b857986f01b2 100644 --- a/polkadot/xcm/xcm-builder/src/tests/barriers.rs +++ b/polkadot/xcm/xcm-builder/src/tests/barriers.rs @@ -14,6 +14,10 @@ // You should have received a copy of the GNU General Public License // along with Polkadot. If not, see . +use frame_support::{ + assert_ok, + traits::{Equals, Everything, EverythingBut}, +}; use xcm_executor::traits::Properties; use super::*; @@ -532,3 +536,124 @@ fn allow_hrmp_notifications_from_relay_chain_should_work() { Ok(()), ); } + +parameter_types! { + pub AssetHubLocation: Location = Location::new(1, Parachain(1000)); + pub ParachainLocation: Location = Location::new(1, Parachain(2000)); + pub EthereumNetwork: NetworkId = Ethereum { chain_id: 1 }; +} + +#[test] +fn deny_reserver_transfer_to_relaychain_should_work() { + pub type Barrier = DenyThenTry< + (DenyReserveTransferToRelayChain, DenyFirstExportMessageFrom), + TakeWeightCredit, + >; + + let mut xcm: Vec> = vec![DepositReserveAsset { + assets: AssetFilter::try_from(Wild(All)).unwrap(), + dest: Location { parents: 1, interior: Here }, + xcm: Default::default(), + }]; + + let result = Barrier::should_execute( + &Here.into(), + &mut xcm, + Weight::zero(), + &mut Properties { weight_credit: Weight::zero(), message_id: None }, + ); + + assert_err!(result, ProcessMessageError::Unsupported); +} + +#[test] +fn deny_export_message_from_source_other_than_asset_hub_should_work() { + pub type Barrier = DenyThenTry< + ( + DenyReserveTransferToRelayChain, + DenyFirstExportMessageFrom< + EverythingBut>, + Equals, + >, + ), + TakeWeightCredit, + >; + + let mut xcm: Vec> = vec![ + AliasOrigin(Here.into()), + ExportMessage { + network: EthereumNetwork::get(), + destination: Here, + xcm: Default::default(), + }, + ]; + + let result = Barrier::should_execute( + &ParachainLocation::get(), + &mut xcm, + Weight::zero(), + &mut Properties { weight_credit: Weight::zero(), message_id: None }, + ); + + assert_err!(result, ProcessMessageError::Unsupported); +} + +#[test] +fn allow_export_message_from_asset_hub_should_work() { + pub type Barrier = DenyThenTry< + ( + DenyReserveTransferToRelayChain, + DenyFirstExportMessageFrom< + EverythingBut>, + Equals, + >, + ), + TakeWeightCredit, + >; + + let mut xcm: Vec> = vec![ + AliasOrigin(Here.into()), + ExportMessage { + network: EthereumNetwork::get(), + destination: Here, + xcm: Default::default(), + }, + ]; + + let result = Barrier::should_execute( + &AssetHubLocation::get(), + &mut xcm, + Weight::zero(), + &mut Properties { weight_credit: Weight::zero(), message_id: None }, + ); + + assert_ok!(result); +} + +#[test] +fn allow_export_message_from_source_other_than_asset_hub_if_destination_other_than_ethereum() { + pub type Barrier = DenyThenTry< + ( + DenyReserveTransferToRelayChain, + DenyFirstExportMessageFrom< + EverythingBut>, + Equals, + >, + ), + TakeWeightCredit, + >; + + let mut xcm: Vec> = vec![ + AliasOrigin(Here.into()), + ExportMessage { network: Polkadot, destination: Here, xcm: Default::default() }, + ]; + + let result = Barrier::should_execute( + &ParachainLocation::get(), + &mut xcm, + Weight::zero(), + &mut Properties { weight_credit: Weight::zero(), message_id: None }, + ); + + assert_ok!(result); +} diff --git a/polkadot/xcm/xcm-executor/src/traits/mod.rs b/polkadot/xcm/xcm-executor/src/traits/mod.rs index feb2922bcdff..69d302bda4eb 100644 --- a/polkadot/xcm/xcm-executor/src/traits/mod.rs +++ b/polkadot/xcm/xcm-executor/src/traits/mod.rs @@ -42,7 +42,7 @@ pub use on_response::{OnResponse, QueryHandler, QueryResponseStatus, VersionChan mod process_transaction; pub use process_transaction::ProcessTransaction; mod should_execute; -pub use should_execute::{CheckSuspension, Properties, ShouldExecute}; +pub use should_execute::{CheckSuspension, Properties, ShouldExecute, ShouldNotExecute}; mod transact_asset; pub use transact_asset::TransactAsset; mod hrmp; diff --git a/polkadot/xcm/xcm-executor/src/traits/should_execute.rs b/polkadot/xcm/xcm-executor/src/traits/should_execute.rs index ec9ef70cc817..a2370dbbf1fc 100644 --- a/polkadot/xcm/xcm-executor/src/traits/should_execute.rs +++ b/polkadot/xcm/xcm-executor/src/traits/should_execute.rs @@ -127,3 +127,65 @@ impl CheckSuspension for Tuple { false } } + +/// Trait to determine whether the execution engine should not execute a given XCM. +/// +/// Can be amalgamated into a tuple to have multiple trials. If any of the tuple elements returns +/// `Err(())`, the execution stops. Else, `Ok(_)` is returned if all elements accept the message. +pub trait ShouldNotExecute { + /// Returns `Ok(())` if the given `message` may be executed. + /// + /// - `origin`: The origin (sender) of the message. + /// - `instructions`: The message itself. + /// - `max_weight`: The (possibly over-) estimation of the weight of execution of the message. + /// - `properties`: Various pre-established properties of the message which may be mutated by + /// this API. + fn should_not_execute( + origin: &Location, + instructions: &mut [Instruction], + max_weight: Weight, + properties: &mut Properties, + ) -> Result<(), ProcessMessageError>; +} + +#[impl_trait_for_tuples::impl_for_tuples(30)] +impl ShouldNotExecute for Tuple { + fn should_not_execute( + origin: &Location, + instructions: &mut [Instruction], + max_weight: Weight, + properties: &mut Properties, + ) -> Result<(), ProcessMessageError> { + for_tuples!( #( + let barrier = core::any::type_name::(); + match Tuple::should_not_execute(origin, instructions, max_weight, properties) { + Ok(()) => { + tracing::trace!( + target: "xcm::should_execute", + ?origin, + ?instructions, + ?max_weight, + ?properties, + %barrier, + "pass barrier", + ); + }, + Err(error) => { + tracing::trace!( + target: "xcm::should_execute", + ?origin, + ?instructions, + ?max_weight, + ?properties, + ?error, + %barrier, + "did not pass barrier", + ); + return Err(error); + }, + } + )* ); + + Ok(()) + } +} From d0748cf1fa53afaec1c9b47079722fee3e11e4fb Mon Sep 17 00:00:00 2001 From: Branislav Kontur Date: Wed, 15 Jan 2025 10:45:26 +0100 Subject: [PATCH 02/35] Apply suggestions from code review --- polkadot/xcm/xcm-executor/src/traits/should_execute.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/polkadot/xcm/xcm-executor/src/traits/should_execute.rs b/polkadot/xcm/xcm-executor/src/traits/should_execute.rs index a2370dbbf1fc..8629bbad7511 100644 --- a/polkadot/xcm/xcm-executor/src/traits/should_execute.rs +++ b/polkadot/xcm/xcm-executor/src/traits/should_execute.rs @@ -161,7 +161,7 @@ impl ShouldNotExecute for Tuple { match Tuple::should_not_execute(origin, instructions, max_weight, properties) { Ok(()) => { tracing::trace!( - target: "xcm::should_execute", + target: "xcm::should_not_execute", ?origin, ?instructions, ?max_weight, @@ -172,7 +172,7 @@ impl ShouldNotExecute for Tuple { }, Err(error) => { tracing::trace!( - target: "xcm::should_execute", + target: "xcm::should_not_execute", ?origin, ?instructions, ?max_weight, From bf669bb24eb069f3ae5cfe9b7e9f75ac9d7850e3 Mon Sep 17 00:00:00 2001 From: ron Date: Wed, 15 Jan 2025 18:22:51 +0800 Subject: [PATCH 03/35] Rename to DenyExportMessageFrom with comments --- polkadot/xcm/xcm-builder/src/barriers.rs | 8 +++++--- polkadot/xcm/xcm-builder/src/lib.rs | 6 +++--- polkadot/xcm/xcm-builder/src/tests/barriers.rs | 17 ++++------------- 3 files changed, 12 insertions(+), 19 deletions(-) diff --git a/polkadot/xcm/xcm-builder/src/barriers.rs b/polkadot/xcm/xcm-builder/src/barriers.rs index 6c6cd95f7484..858413422893 100644 --- a/polkadot/xcm/xcm-builder/src/barriers.rs +++ b/polkadot/xcm/xcm-builder/src/barriers.rs @@ -507,13 +507,15 @@ impl ShouldNotExecute for DenyReserveTransferToRelayChain { } } -// See https://github.com/paritytech/polkadot-sdk/pull/6838 -pub struct DenyFirstExportMessageFrom( +/// Deny execution if the message contains instruction `ExportMessage` with +/// a. origin is contained in `FromOrigin` (i.e.`FromOrigin::Contains(origin)`) +/// b. network is contained in `ToGlobalConsensus`, (i.e. `ToGlobalConsensus::contains(network)`) +pub struct DenyExportMessageFrom( PhantomData<(FromOrigin, ToGlobalConsensus)>, ); impl ShouldNotExecute - for DenyFirstExportMessageFrom + for DenyExportMessageFrom where FromOrigin: Contains, ToGlobalConsensus: Contains, diff --git a/polkadot/xcm/xcm-builder/src/lib.rs b/polkadot/xcm/xcm-builder/src/lib.rs index 0c01345941d8..96733719dc24 100644 --- a/polkadot/xcm/xcm-builder/src/lib.rs +++ b/polkadot/xcm/xcm-builder/src/lib.rs @@ -42,9 +42,9 @@ mod barriers; pub use barriers::{ AllowExplicitUnpaidExecutionFrom, AllowHrmpNotificationsFromRelayChain, AllowKnownQueryResponses, AllowSubscriptionsFrom, AllowTopLevelPaidExecutionFrom, - AllowUnpaidExecutionFrom, DenyFirstExportMessageFrom, DenyReserveTransferToRelayChain, - DenyThenTry, IsChildSystemParachain, IsParentsOnly, IsSiblingSystemParachain, - RespectSuspension, TakeWeightCredit, TrailingSetTopicAsId, WithComputedOrigin, + AllowUnpaidExecutionFrom, DenyExportMessageFrom, DenyReserveTransferToRelayChain, DenyThenTry, + IsChildSystemParachain, IsParentsOnly, IsSiblingSystemParachain, RespectSuspension, + TakeWeightCredit, TrailingSetTopicAsId, WithComputedOrigin, }; mod controller; diff --git a/polkadot/xcm/xcm-builder/src/tests/barriers.rs b/polkadot/xcm/xcm-builder/src/tests/barriers.rs index b857986f01b2..027448baa6c5 100644 --- a/polkadot/xcm/xcm-builder/src/tests/barriers.rs +++ b/polkadot/xcm/xcm-builder/src/tests/barriers.rs @@ -546,7 +546,7 @@ parameter_types! { #[test] fn deny_reserver_transfer_to_relaychain_should_work() { pub type Barrier = DenyThenTry< - (DenyReserveTransferToRelayChain, DenyFirstExportMessageFrom), + (DenyReserveTransferToRelayChain, DenyExportMessageFrom), TakeWeightCredit, >; @@ -571,10 +571,7 @@ fn deny_export_message_from_source_other_than_asset_hub_should_work() { pub type Barrier = DenyThenTry< ( DenyReserveTransferToRelayChain, - DenyFirstExportMessageFrom< - EverythingBut>, - Equals, - >, + DenyExportMessageFrom>, Equals>, ), TakeWeightCredit, >; @@ -603,10 +600,7 @@ fn allow_export_message_from_asset_hub_should_work() { pub type Barrier = DenyThenTry< ( DenyReserveTransferToRelayChain, - DenyFirstExportMessageFrom< - EverythingBut>, - Equals, - >, + DenyExportMessageFrom>, Equals>, ), TakeWeightCredit, >; @@ -635,10 +629,7 @@ fn allow_export_message_from_source_other_than_asset_hub_if_destination_other_th pub type Barrier = DenyThenTry< ( DenyReserveTransferToRelayChain, - DenyFirstExportMessageFrom< - EverythingBut>, - Equals, - >, + DenyExportMessageFrom>, Equals>, ), TakeWeightCredit, >; From 716ccdc8d53edbcf0a7947caa766111e21f7faca Mon Sep 17 00:00:00 2001 From: ron Date: Wed, 15 Jan 2025 20:06:24 +0800 Subject: [PATCH 04/35] Move DenyExportMessageFrom to bridge-hub-common --- Cargo.lock | 2 + .../runtimes/bridge-hubs/common/Cargo.toml | 6 + .../bridge-hubs/common/src/barriers.rs | 57 +++++++ .../runtimes/bridge-hubs/common/src/lib.rs | 2 + .../bridge-hubs/common/tests/tests.rs | 140 ++++++++++++++++++ polkadot/xcm/xcm-builder/src/barriers.rs | 35 ----- polkadot/xcm/xcm-builder/src/lib.rs | 6 +- .../xcm/xcm-builder/src/tests/barriers.rs | 116 --------------- 8 files changed, 210 insertions(+), 154 deletions(-) create mode 100644 cumulus/parachains/runtimes/bridge-hubs/common/src/barriers.rs create mode 100644 cumulus/parachains/runtimes/bridge-hubs/common/tests/tests.rs diff --git a/Cargo.lock b/Cargo.lock index 7725db743c41..d258a0f3ae35 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2584,6 +2584,8 @@ dependencies = [ "sp-runtime 31.0.1", "sp-std 14.0.0", "staging-xcm 7.0.0", + "staging-xcm-builder 7.0.0", + "staging-xcm-executor 7.0.0", ] [[package]] diff --git a/cumulus/parachains/runtimes/bridge-hubs/common/Cargo.toml b/cumulus/parachains/runtimes/bridge-hubs/common/Cargo.toml index 2fbb96d75163..9c5c7c513909 100644 --- a/cumulus/parachains/runtimes/bridge-hubs/common/Cargo.toml +++ b/cumulus/parachains/runtimes/bridge-hubs/common/Cargo.toml @@ -19,6 +19,8 @@ sp-core = { workspace = true } sp-runtime = { workspace = true } sp-std = { workspace = true } xcm = { workspace = true } +xcm-builder = { workspace = true } +xcm-executor = { workspace = true } [features] default = ["std"] @@ -32,6 +34,8 @@ std = [ "sp-core/std", "sp-runtime/std", "sp-std/std", + "xcm-builder/std", + "xcm-executor/std", "xcm/std", ] @@ -41,5 +45,7 @@ runtime-benchmarks = [ "pallet-message-queue/runtime-benchmarks", "snowbridge-core/runtime-benchmarks", "sp-runtime/runtime-benchmarks", + "xcm-builder/runtime-benchmarks", + "xcm-executor/runtime-benchmarks", "xcm/runtime-benchmarks", ] diff --git a/cumulus/parachains/runtimes/bridge-hubs/common/src/barriers.rs b/cumulus/parachains/runtimes/bridge-hubs/common/src/barriers.rs new file mode 100644 index 000000000000..5fdf850420a0 --- /dev/null +++ b/cumulus/parachains/runtimes/bridge-hubs/common/src/barriers.rs @@ -0,0 +1,57 @@ +// 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 core::{marker::PhantomData, ops::ControlFlow}; +use cumulus_primitives_core::Weight; +use frame_support::traits::{Contains, ProcessMessageError}; +use xcm::prelude::{ExportMessage, Instruction, Location, NetworkId}; + +use xcm_builder::{CreateMatcher, MatchXcm}; +use xcm_executor::traits::{Properties, ShouldNotExecute}; + +/// Deny execution if the message contains instruction `ExportMessage` with +/// a. origin is contained in `FromOrigin` (i.e.`FromOrigin::Contains(origin)`) +/// b. network is contained in `ToGlobalConsensus`, (i.e. `ToGlobalConsensus::contains(network)`) +pub struct DenyExportMessageFrom( + PhantomData<(FromOrigin, ToGlobalConsensus)>, +); + +impl ShouldNotExecute + for DenyExportMessageFrom +where + FromOrigin: Contains, + ToGlobalConsensus: Contains, +{ + fn should_not_execute( + origin: &Location, + message: &mut [Instruction], + _max_weight: Weight, + _properties: &mut Properties, + ) -> Result<(), ProcessMessageError> { + message.matcher().match_next_inst_while( + |_| true, + |inst| match inst { + ExportMessage { network, .. } => + if ToGlobalConsensus::contains(network) && FromOrigin::contains(origin) { + return Err(ProcessMessageError::Unsupported) + } else { + Ok(ControlFlow::Continue(())) + }, + _ => Ok(ControlFlow::Continue(())), + }, + )?; + Ok(()) + } +} diff --git a/cumulus/parachains/runtimes/bridge-hubs/common/src/lib.rs b/cumulus/parachains/runtimes/bridge-hubs/common/src/lib.rs index b806b8cdb22d..dbd87bea2142 100644 --- a/cumulus/parachains/runtimes/bridge-hubs/common/src/lib.rs +++ b/cumulus/parachains/runtimes/bridge-hubs/common/src/lib.rs @@ -14,9 +14,11 @@ // limitations under the License. #![cfg_attr(not(feature = "std"), no_std)] +pub mod barriers; pub mod digest_item; pub mod message_queue; pub mod xcm_version; +pub use barriers::DenyExportMessageFrom; pub use digest_item::CustomDigestItem; pub use message_queue::{AggregateMessageOrigin, BridgeHubMessageRouter}; diff --git a/cumulus/parachains/runtimes/bridge-hubs/common/tests/tests.rs b/cumulus/parachains/runtimes/bridge-hubs/common/tests/tests.rs new file mode 100644 index 000000000000..a5d70d5741eb --- /dev/null +++ b/cumulus/parachains/runtimes/bridge-hubs/common/tests/tests.rs @@ -0,0 +1,140 @@ +// Copyright (C) Parity Technologies (UK) Ltd. +// This file is part of Cumulus. + +// Cumulus is free software: you can redistribute it and/or modify +// it under the terms of the GNU General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. + +// Cumulus is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. + +// You should have received a copy of the GNU General Public License +// along with Cumulus. If not, see . + +#![cfg(test)] +use bridge_hub_common::DenyExportMessageFrom; +use frame_support::{ + assert_err, assert_ok, parameter_types, + traits::{Equals, Everything, EverythingBut, ProcessMessageError}, +}; +use xcm::prelude::{ + AliasOrigin, All, AssetFilter, DepositReserveAsset, Ethereum, ExportMessage, Here, Instruction, + Location, NetworkId, NetworkId::Polkadot, Parachain, Weight, Wild, +}; +use xcm_builder::{DenyReserveTransferToRelayChain, DenyThenTry, TakeWeightCredit}; +use xcm_executor::traits::{Properties, ShouldExecute}; + +parameter_types! { + pub AssetHubLocation: Location = Location::new(1, Parachain(1000)); + pub ParachainLocation: Location = Location::new(1, Parachain(2000)); + pub EthereumNetwork: NetworkId = Ethereum { chain_id: 1 }; +} + +#[test] +fn deny_export_message_from_source_other_than_asset_hub_should_work() { + pub type Barrier = DenyThenTry< + ( + DenyReserveTransferToRelayChain, + DenyExportMessageFrom>, Equals>, + ), + TakeWeightCredit, + >; + + let mut xcm: Vec> = vec![ + AliasOrigin(Here.into()), + ExportMessage { + network: EthereumNetwork::get(), + destination: Here, + xcm: Default::default(), + }, + ]; + + let result = Barrier::should_execute( + &ParachainLocation::get(), + &mut xcm, + Weight::zero(), + &mut Properties { weight_credit: Weight::zero(), message_id: None }, + ); + + assert_err!(result, ProcessMessageError::Unsupported); +} + +#[test] +fn allow_export_message_from_asset_hub_should_work() { + pub type Barrier = DenyThenTry< + ( + DenyReserveTransferToRelayChain, + DenyExportMessageFrom>, Equals>, + ), + TakeWeightCredit, + >; + + let mut xcm: Vec> = vec![ + AliasOrigin(Here.into()), + ExportMessage { + network: EthereumNetwork::get(), + destination: Here, + xcm: Default::default(), + }, + ]; + + let result = Barrier::should_execute( + &AssetHubLocation::get(), + &mut xcm, + Weight::zero(), + &mut Properties { weight_credit: Weight::zero(), message_id: None }, + ); + + assert_ok!(result); +} + +#[test] +fn allow_export_message_from_source_other_than_asset_hub_if_destination_other_than_ethereum() { + pub type Barrier = DenyThenTry< + ( + DenyReserveTransferToRelayChain, + DenyExportMessageFrom>, Equals>, + ), + TakeWeightCredit, + >; + + let mut xcm: Vec> = vec![ + AliasOrigin(Here.into()), + ExportMessage { network: Polkadot, destination: Here, xcm: Default::default() }, + ]; + + let result = Barrier::should_execute( + &ParachainLocation::get(), + &mut xcm, + Weight::zero(), + &mut Properties { weight_credit: Weight::zero(), message_id: None }, + ); + + assert_ok!(result); +} + +#[test] +fn deny_reserver_transfer_to_relaychain_does_not_break() { + pub type Barrier = DenyThenTry< + (DenyReserveTransferToRelayChain, DenyExportMessageFrom), + TakeWeightCredit, + >; + + let mut xcm: Vec> = vec![DepositReserveAsset { + assets: AssetFilter::try_from(Wild(All)).unwrap(), + dest: Location { parents: 1, interior: Here }, + xcm: Default::default(), + }]; + + let result = Barrier::should_execute( + &Here.into(), + &mut xcm, + Weight::zero(), + &mut Properties { weight_credit: Weight::zero(), message_id: None }, + ); + + assert_err!(result, ProcessMessageError::Unsupported); +} diff --git a/polkadot/xcm/xcm-builder/src/barriers.rs b/polkadot/xcm/xcm-builder/src/barriers.rs index 858413422893..029e40673163 100644 --- a/polkadot/xcm/xcm-builder/src/barriers.rs +++ b/polkadot/xcm/xcm-builder/src/barriers.rs @@ -506,38 +506,3 @@ impl ShouldNotExecute for DenyReserveTransferToRelayChain { Ok(()) } } - -/// Deny execution if the message contains instruction `ExportMessage` with -/// a. origin is contained in `FromOrigin` (i.e.`FromOrigin::Contains(origin)`) -/// b. network is contained in `ToGlobalConsensus`, (i.e. `ToGlobalConsensus::contains(network)`) -pub struct DenyExportMessageFrom( - PhantomData<(FromOrigin, ToGlobalConsensus)>, -); - -impl ShouldNotExecute - for DenyExportMessageFrom -where - FromOrigin: Contains, - ToGlobalConsensus: Contains, -{ - fn should_not_execute( - origin: &Location, - message: &mut [Instruction], - _max_weight: Weight, - _properties: &mut Properties, - ) -> Result<(), ProcessMessageError> { - message.matcher().match_next_inst_while( - |_| true, - |inst| match inst { - ExportMessage { network, .. } => - if ToGlobalConsensus::contains(network) && FromOrigin::contains(origin) { - return Err(ProcessMessageError::Unsupported) - } else { - Ok(ControlFlow::Continue(())) - }, - _ => Ok(ControlFlow::Continue(())), - }, - )?; - Ok(()) - } -} diff --git a/polkadot/xcm/xcm-builder/src/lib.rs b/polkadot/xcm/xcm-builder/src/lib.rs index 96733719dc24..e23412a97ebc 100644 --- a/polkadot/xcm/xcm-builder/src/lib.rs +++ b/polkadot/xcm/xcm-builder/src/lib.rs @@ -42,9 +42,9 @@ mod barriers; pub use barriers::{ AllowExplicitUnpaidExecutionFrom, AllowHrmpNotificationsFromRelayChain, AllowKnownQueryResponses, AllowSubscriptionsFrom, AllowTopLevelPaidExecutionFrom, - AllowUnpaidExecutionFrom, DenyExportMessageFrom, DenyReserveTransferToRelayChain, DenyThenTry, - IsChildSystemParachain, IsParentsOnly, IsSiblingSystemParachain, RespectSuspension, - TakeWeightCredit, TrailingSetTopicAsId, WithComputedOrigin, + AllowUnpaidExecutionFrom, DenyReserveTransferToRelayChain, DenyThenTry, IsChildSystemParachain, + IsParentsOnly, IsSiblingSystemParachain, RespectSuspension, TakeWeightCredit, + TrailingSetTopicAsId, WithComputedOrigin, }; mod controller; diff --git a/polkadot/xcm/xcm-builder/src/tests/barriers.rs b/polkadot/xcm/xcm-builder/src/tests/barriers.rs index 027448baa6c5..d8805274d3a5 100644 --- a/polkadot/xcm/xcm-builder/src/tests/barriers.rs +++ b/polkadot/xcm/xcm-builder/src/tests/barriers.rs @@ -14,10 +14,6 @@ // You should have received a copy of the GNU General Public License // along with Polkadot. If not, see . -use frame_support::{ - assert_ok, - traits::{Equals, Everything, EverythingBut}, -}; use xcm_executor::traits::Properties; use super::*; @@ -536,115 +532,3 @@ fn allow_hrmp_notifications_from_relay_chain_should_work() { Ok(()), ); } - -parameter_types! { - pub AssetHubLocation: Location = Location::new(1, Parachain(1000)); - pub ParachainLocation: Location = Location::new(1, Parachain(2000)); - pub EthereumNetwork: NetworkId = Ethereum { chain_id: 1 }; -} - -#[test] -fn deny_reserver_transfer_to_relaychain_should_work() { - pub type Barrier = DenyThenTry< - (DenyReserveTransferToRelayChain, DenyExportMessageFrom), - TakeWeightCredit, - >; - - let mut xcm: Vec> = vec![DepositReserveAsset { - assets: AssetFilter::try_from(Wild(All)).unwrap(), - dest: Location { parents: 1, interior: Here }, - xcm: Default::default(), - }]; - - let result = Barrier::should_execute( - &Here.into(), - &mut xcm, - Weight::zero(), - &mut Properties { weight_credit: Weight::zero(), message_id: None }, - ); - - assert_err!(result, ProcessMessageError::Unsupported); -} - -#[test] -fn deny_export_message_from_source_other_than_asset_hub_should_work() { - pub type Barrier = DenyThenTry< - ( - DenyReserveTransferToRelayChain, - DenyExportMessageFrom>, Equals>, - ), - TakeWeightCredit, - >; - - let mut xcm: Vec> = vec![ - AliasOrigin(Here.into()), - ExportMessage { - network: EthereumNetwork::get(), - destination: Here, - xcm: Default::default(), - }, - ]; - - let result = Barrier::should_execute( - &ParachainLocation::get(), - &mut xcm, - Weight::zero(), - &mut Properties { weight_credit: Weight::zero(), message_id: None }, - ); - - assert_err!(result, ProcessMessageError::Unsupported); -} - -#[test] -fn allow_export_message_from_asset_hub_should_work() { - pub type Barrier = DenyThenTry< - ( - DenyReserveTransferToRelayChain, - DenyExportMessageFrom>, Equals>, - ), - TakeWeightCredit, - >; - - let mut xcm: Vec> = vec![ - AliasOrigin(Here.into()), - ExportMessage { - network: EthereumNetwork::get(), - destination: Here, - xcm: Default::default(), - }, - ]; - - let result = Barrier::should_execute( - &AssetHubLocation::get(), - &mut xcm, - Weight::zero(), - &mut Properties { weight_credit: Weight::zero(), message_id: None }, - ); - - assert_ok!(result); -} - -#[test] -fn allow_export_message_from_source_other_than_asset_hub_if_destination_other_than_ethereum() { - pub type Barrier = DenyThenTry< - ( - DenyReserveTransferToRelayChain, - DenyExportMessageFrom>, Equals>, - ), - TakeWeightCredit, - >; - - let mut xcm: Vec> = vec![ - AliasOrigin(Here.into()), - ExportMessage { network: Polkadot, destination: Here, xcm: Default::default() }, - ]; - - let result = Barrier::should_execute( - &ParachainLocation::get(), - &mut xcm, - Weight::zero(), - &mut Properties { weight_credit: Weight::zero(), message_id: None }, - ); - - assert_ok!(result); -} From e10587b09a24c6537bb2ca17f8d602c253711e4b Mon Sep 17 00:00:00 2001 From: ron Date: Wed, 15 Jan 2025 20:14:54 +0800 Subject: [PATCH 05/35] Update comments --- polkadot/xcm/xcm-executor/src/traits/should_execute.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/polkadot/xcm/xcm-executor/src/traits/should_execute.rs b/polkadot/xcm/xcm-executor/src/traits/should_execute.rs index 8629bbad7511..0e2f6c20b748 100644 --- a/polkadot/xcm/xcm-executor/src/traits/should_execute.rs +++ b/polkadot/xcm/xcm-executor/src/traits/should_execute.rs @@ -133,7 +133,8 @@ impl CheckSuspension for Tuple { /// Can be amalgamated into a tuple to have multiple trials. If any of the tuple elements returns /// `Err(())`, the execution stops. Else, `Ok(_)` is returned if all elements accept the message. pub trait ShouldNotExecute { - /// Returns `Ok(())` if the given `message` may be executed. + /// Returns `Ok(())` means there is no reason not to execute the message + /// while Err(e) indicates there is a reason not to execute. /// /// - `origin`: The origin (sender) of the message. /// - `instructions`: The message itself. From d83dadb455264e39c4b5d6f94dd9fd3f758caad0 Mon Sep 17 00:00:00 2001 From: ron Date: Wed, 15 Jan 2025 20:36:53 +0800 Subject: [PATCH 06/35] Add the prdoc --- prdoc/pr_7169.prdoc | 14 ++++++++++++++ 1 file changed, 14 insertions(+) create mode 100644 prdoc/pr_7169.prdoc diff --git a/prdoc/pr_7169.prdoc b/prdoc/pr_7169.prdoc new file mode 100644 index 000000000000..8d7078a2e7e4 --- /dev/null +++ b/prdoc/pr_7169.prdoc @@ -0,0 +1,14 @@ +title: 'xcm: fix DenyThenTry when work with multiple Deny tuples' +doc: +- audience: Runtime User + description: |- + This PR changes the behavior of DenyThenTry to fix #7148 + If any of the tuple elements returns `Err(())`, the execution stops. + Else, `Ok(_)` is returned if all elements accept the message. +crates: +- name: staging-xcm-executor + bump: patch +- name: staging-xcm-builder + bump: patch +- name: bridge-hub-common + bump: patch From ac810f714c97102c28067fbb7b034d2f2a78cbcd Mon Sep 17 00:00:00 2001 From: ron Date: Wed, 15 Jan 2025 23:57:46 +0800 Subject: [PATCH 07/35] Refactor DenyExecution to be more explicit --- .../bridge-hubs/common/src/barriers.rs | 16 ++++++---- polkadot/xcm/xcm-builder/src/barriers.rs | 30 ++++++++--------- polkadot/xcm/xcm-executor/src/traits/mod.rs | 2 +- .../xcm-executor/src/traits/should_execute.rs | 32 +++++++++---------- 4 files changed, 41 insertions(+), 39 deletions(-) diff --git a/cumulus/parachains/runtimes/bridge-hubs/common/src/barriers.rs b/cumulus/parachains/runtimes/bridge-hubs/common/src/barriers.rs index 5fdf850420a0..ab72de92d484 100644 --- a/cumulus/parachains/runtimes/bridge-hubs/common/src/barriers.rs +++ b/cumulus/parachains/runtimes/bridge-hubs/common/src/barriers.rs @@ -19,7 +19,7 @@ use frame_support::traits::{Contains, ProcessMessageError}; use xcm::prelude::{ExportMessage, Instruction, Location, NetworkId}; use xcm_builder::{CreateMatcher, MatchXcm}; -use xcm_executor::traits::{Properties, ShouldNotExecute}; +use xcm_executor::traits::{DenyExecution, Properties}; /// Deny execution if the message contains instruction `ExportMessage` with /// a. origin is contained in `FromOrigin` (i.e.`FromOrigin::Contains(origin)`) @@ -28,19 +28,19 @@ pub struct DenyExportMessageFrom( PhantomData<(FromOrigin, ToGlobalConsensus)>, ); -impl ShouldNotExecute +impl DenyExecution for DenyExportMessageFrom where FromOrigin: Contains, ToGlobalConsensus: Contains, { - fn should_not_execute( + fn deny_execution( origin: &Location, message: &mut [Instruction], _max_weight: Weight, _properties: &mut Properties, - ) -> Result<(), ProcessMessageError> { - message.matcher().match_next_inst_while( + ) -> Option { + match message.matcher().match_next_inst_while( |_| true, |inst| match inst { ExportMessage { network, .. } => @@ -51,7 +51,9 @@ where }, _ => Ok(ControlFlow::Continue(())), }, - )?; - Ok(()) + ) { + Ok(_) => None, + Err(error) => Some(error), + } } } diff --git a/polkadot/xcm/xcm-builder/src/barriers.rs b/polkadot/xcm/xcm-builder/src/barriers.rs index 029e40673163..584021c8d9c0 100644 --- a/polkadot/xcm/xcm-builder/src/barriers.rs +++ b/polkadot/xcm/xcm-builder/src/barriers.rs @@ -24,9 +24,7 @@ use frame_support::{ }; use polkadot_parachain_primitives::primitives::IsSystem; use xcm::prelude::*; -use xcm_executor::traits::{ - CheckSuspension, OnResponse, Properties, ShouldExecute, ShouldNotExecute, -}; +use xcm_executor::traits::{CheckSuspension, DenyExecution, OnResponse, Properties, ShouldExecute}; /// Execution barrier that just takes `max_weight` from `properties.weight_credit`. /// @@ -446,12 +444,12 @@ impl ShouldExecute for AllowHrmpNotificationsFromRelayChain { /// If it passes the Deny, and matches one of the Allow cases then it is let through. pub struct DenyThenTry(PhantomData, PhantomData) where - Deny: ShouldNotExecute, + Deny: DenyExecution, Allow: ShouldExecute; impl ShouldExecute for DenyThenTry where - Deny: ShouldNotExecute, + Deny: DenyExecution, Allow: ShouldExecute, { fn should_execute( @@ -460,21 +458,23 @@ where max_weight: Weight, properties: &mut Properties, ) -> Result<(), ProcessMessageError> { - Deny::should_not_execute(origin, message, max_weight, properties)?; - Allow::should_execute(origin, message, max_weight, properties) + match Deny::deny_execution(origin, message, max_weight, properties) { + Some(err) => Err(err), + None => Allow::should_execute(origin, message, max_weight, properties), + } } } // See issue pub struct DenyReserveTransferToRelayChain; -impl ShouldNotExecute for DenyReserveTransferToRelayChain { - fn should_not_execute( +impl DenyExecution for DenyReserveTransferToRelayChain { + fn deny_execution( origin: &Location, message: &mut [Instruction], _max_weight: Weight, _properties: &mut Properties, - ) -> Result<(), ProcessMessageError> { - message.matcher().match_next_inst_while( + ) -> Option { + match message.matcher().match_next_inst_while( |_| true, |inst| match inst { InitiateReserveWithdraw { @@ -500,9 +500,9 @@ impl ShouldNotExecute for DenyReserveTransferToRelayChain { _ => Ok(ControlFlow::Continue(())), }, - )?; - - // Permit everything else - Ok(()) + ) { + Ok(_) => None, + Err(err) => Some(err), + } } } diff --git a/polkadot/xcm/xcm-executor/src/traits/mod.rs b/polkadot/xcm/xcm-executor/src/traits/mod.rs index 69d302bda4eb..fe73477f516f 100644 --- a/polkadot/xcm/xcm-executor/src/traits/mod.rs +++ b/polkadot/xcm/xcm-executor/src/traits/mod.rs @@ -42,7 +42,7 @@ pub use on_response::{OnResponse, QueryHandler, QueryResponseStatus, VersionChan mod process_transaction; pub use process_transaction::ProcessTransaction; mod should_execute; -pub use should_execute::{CheckSuspension, Properties, ShouldExecute, ShouldNotExecute}; +pub use should_execute::{CheckSuspension, DenyExecution, Properties, ShouldExecute}; mod transact_asset; pub use transact_asset::TransactAsset; mod hrmp; diff --git a/polkadot/xcm/xcm-executor/src/traits/should_execute.rs b/polkadot/xcm/xcm-executor/src/traits/should_execute.rs index 0e2f6c20b748..34790eebde43 100644 --- a/polkadot/xcm/xcm-executor/src/traits/should_execute.rs +++ b/polkadot/xcm/xcm-executor/src/traits/should_execute.rs @@ -132,7 +132,7 @@ impl CheckSuspension for Tuple { /// /// Can be amalgamated into a tuple to have multiple trials. If any of the tuple elements returns /// `Err(())`, the execution stops. Else, `Ok(_)` is returned if all elements accept the message. -pub trait ShouldNotExecute { +pub trait DenyExecution { /// Returns `Ok(())` means there is no reason not to execute the message /// while Err(e) indicates there is a reason not to execute. /// @@ -141,52 +141,52 @@ pub trait ShouldNotExecute { /// - `max_weight`: The (possibly over-) estimation of the weight of execution of the message. /// - `properties`: Various pre-established properties of the message which may be mutated by /// this API. - fn should_not_execute( + fn deny_execution( origin: &Location, instructions: &mut [Instruction], max_weight: Weight, properties: &mut Properties, - ) -> Result<(), ProcessMessageError>; + ) -> Option; } #[impl_trait_for_tuples::impl_for_tuples(30)] -impl ShouldNotExecute for Tuple { - fn should_not_execute( +impl DenyExecution for Tuple { + fn deny_execution( origin: &Location, instructions: &mut [Instruction], max_weight: Weight, properties: &mut Properties, - ) -> Result<(), ProcessMessageError> { + ) -> Option { for_tuples!( #( let barrier = core::any::type_name::(); - match Tuple::should_not_execute(origin, instructions, max_weight, properties) { - Ok(()) => { + match Tuple::deny_execution(origin, instructions, max_weight, properties) { + Some(error) => { tracing::trace!( - target: "xcm::should_not_execute", + target: "xcm::should_execute", ?origin, ?instructions, ?max_weight, ?properties, + ?error, %barrier, - "pass barrier", + "did not pass barrier", ); + return Some(error); }, - Err(error) => { + None => { tracing::trace!( - target: "xcm::should_not_execute", + target: "xcm::should_execute", ?origin, ?instructions, ?max_weight, ?properties, - ?error, %barrier, - "did not pass barrier", + "pass barrier", ); - return Err(error); }, } )* ); - Ok(()) + None } } From 8f0af5bab79f87ab4538fde38fdad7a68aaf9940 Mon Sep 17 00:00:00 2001 From: Ron Date: Wed, 15 Jan 2025 23:59:03 +0800 Subject: [PATCH 08/35] Update prdoc/pr_7169.prdoc Co-authored-by: Francisco Aguirre --- prdoc/pr_7169.prdoc | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/prdoc/pr_7169.prdoc b/prdoc/pr_7169.prdoc index 8d7078a2e7e4..f78dbfd8d2cd 100644 --- a/prdoc/pr_7169.prdoc +++ b/prdoc/pr_7169.prdoc @@ -7,8 +7,8 @@ doc: Else, `Ok(_)` is returned if all elements accept the message. crates: - name: staging-xcm-executor - bump: patch + bump: minor - name: staging-xcm-builder - bump: patch + bump: minor - name: bridge-hub-common - bump: patch + bump: minor From 2b496e52446cefde67ba28f7136c1008c1aa41c0 Mon Sep 17 00:00:00 2001 From: Branislav Kontur Date: Thu, 16 Jan 2025 10:14:49 +0100 Subject: [PATCH 09/35] Update polkadot/xcm/xcm-executor/src/traits/should_execute.rs --- polkadot/xcm/xcm-executor/src/traits/should_execute.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/polkadot/xcm/xcm-executor/src/traits/should_execute.rs b/polkadot/xcm/xcm-executor/src/traits/should_execute.rs index 34790eebde43..4f2f6202877e 100644 --- a/polkadot/xcm/xcm-executor/src/traits/should_execute.rs +++ b/polkadot/xcm/xcm-executor/src/traits/should_execute.rs @@ -161,7 +161,7 @@ impl DenyExecution for Tuple { let barrier = core::any::type_name::(); match Tuple::deny_execution(origin, instructions, max_weight, properties) { Some(error) => { - tracing::trace!( + tracing::error!( target: "xcm::should_execute", ?origin, ?instructions, From 81ec7f1161fd0651df3ecfe470e600d7ab4741ac Mon Sep 17 00:00:00 2001 From: Branislav Kontur Date: Thu, 16 Jan 2025 10:15:09 +0100 Subject: [PATCH 10/35] Update polkadot/xcm/xcm-executor/src/traits/should_execute.rs --- polkadot/xcm/xcm-executor/src/traits/should_execute.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/polkadot/xcm/xcm-executor/src/traits/should_execute.rs b/polkadot/xcm/xcm-executor/src/traits/should_execute.rs index 4f2f6202877e..2e3330c229f1 100644 --- a/polkadot/xcm/xcm-executor/src/traits/should_execute.rs +++ b/polkadot/xcm/xcm-executor/src/traits/should_execute.rs @@ -133,8 +133,8 @@ impl CheckSuspension for Tuple { /// Can be amalgamated into a tuple to have multiple trials. If any of the tuple elements returns /// `Err(())`, the execution stops. Else, `Ok(_)` is returned if all elements accept the message. pub trait DenyExecution { - /// Returns `Ok(())` means there is no reason not to execute the message - /// while Err(e) indicates there is a reason not to execute. + /// Returns `None` if there is no reason to deny execution, + /// while `Some(ProcessMessageError)` indicates there is a reason to deny execution. /// /// - `origin`: The origin (sender) of the message. /// - `instructions`: The message itself. From 17c49729b8d9c6127f3dfb1a37c8b7b0b4d3a2b7 Mon Sep 17 00:00:00 2001 From: ron Date: Thu, 16 Jan 2025 23:28:58 +0800 Subject: [PATCH 11/35] Improve tests --- .../bridge-hubs/common/tests/tests.rs | 152 ++++++------------ .../xcm/xcm-builder/src/tests/barriers.rs | 61 +++++++ polkadot/xcm/xcm-builder/src/tests/mock.rs | 130 ++++++++++++++- 3 files changed, 233 insertions(+), 110 deletions(-) diff --git a/cumulus/parachains/runtimes/bridge-hubs/common/tests/tests.rs b/cumulus/parachains/runtimes/bridge-hubs/common/tests/tests.rs index a5d70d5741eb..ea940d58b347 100644 --- a/cumulus/parachains/runtimes/bridge-hubs/common/tests/tests.rs +++ b/cumulus/parachains/runtimes/bridge-hubs/common/tests/tests.rs @@ -17,124 +17,66 @@ #![cfg(test)] use bridge_hub_common::DenyExportMessageFrom; use frame_support::{ - assert_err, assert_ok, parameter_types, - traits::{Equals, Everything, EverythingBut, ProcessMessageError}, + parameter_types, + traits::{Equals, EverythingBut, ProcessMessageError::Unsupported}, }; use xcm::prelude::{ - AliasOrigin, All, AssetFilter, DepositReserveAsset, Ethereum, ExportMessage, Here, Instruction, - Location, NetworkId, NetworkId::Polkadot, Parachain, Weight, Wild, + AliasOrigin, ByGenesis, ExportMessage, Here, Instruction, Location, NetworkId, Parachain, + Weight, }; -use xcm_builder::{DenyReserveTransferToRelayChain, DenyThenTry, TakeWeightCredit}; -use xcm_executor::traits::{Properties, ShouldExecute}; +use xcm_executor::traits::{DenyExecution, Properties}; parameter_types! { - pub AssetHubLocation: Location = Location::new(1, Parachain(1000)); - pub ParachainLocation: Location = Location::new(1, Parachain(2000)); - pub EthereumNetwork: NetworkId = Ethereum { chain_id: 1 }; + pub Source1: Location = Location::new(1, Parachain(1)); + pub Source2: Location = Location::new(1, Parachain(2)); + pub Remote1: NetworkId = ByGenesis([1;32]); + pub Remote2: NetworkId = ByGenesis([2;32]); } #[test] -fn deny_export_message_from_source_other_than_asset_hub_should_work() { - pub type Barrier = DenyThenTry< - ( - DenyReserveTransferToRelayChain, - DenyExportMessageFrom>, Equals>, - ), - TakeWeightCredit, - >; - - let mut xcm: Vec> = vec![ - AliasOrigin(Here.into()), - ExportMessage { - network: EthereumNetwork::get(), - destination: Here, - xcm: Default::default(), - }, - ]; - - let result = Barrier::should_execute( - &ParachainLocation::get(), - &mut xcm, - Weight::zero(), - &mut Properties { weight_credit: Weight::zero(), message_id: None }, +fn test_deny_export_message_from() { + pub type Denied = DenyExportMessageFrom>, Equals>; + + let assert_deny_execution = |mut xcm: Vec>, origin, expected_result| { + assert_eq!( + Denied::deny_execution( + &origin, + &mut xcm, + Weight::zero(), + &mut Properties { weight_credit: Weight::zero(), message_id: None } + ), + expected_result + ); + }; + + // No ExportMessage should pass + assert_deny_execution(vec![AliasOrigin(Here.into())], Source1::get(), None); + + // Export message from source1 and remote1 should pass + assert_deny_execution( + vec![ExportMessage { network: Remote1::get(), destination: Here, xcm: Default::default() }], + Source1::get(), + None, ); - assert_err!(result, ProcessMessageError::Unsupported); -} - -#[test] -fn allow_export_message_from_asset_hub_should_work() { - pub type Barrier = DenyThenTry< - ( - DenyReserveTransferToRelayChain, - DenyExportMessageFrom>, Equals>, - ), - TakeWeightCredit, - >; - - let mut xcm: Vec> = vec![ - AliasOrigin(Here.into()), - ExportMessage { - network: EthereumNetwork::get(), - destination: Here, - xcm: Default::default(), - }, - ]; - - let result = Barrier::should_execute( - &AssetHubLocation::get(), - &mut xcm, - Weight::zero(), - &mut Properties { weight_credit: Weight::zero(), message_id: None }, + // Export message from source1 and remote2 should pass + assert_deny_execution( + vec![ExportMessage { network: Remote2::get(), destination: Here, xcm: Default::default() }], + Source1::get(), + None, ); - assert_ok!(result); -} - -#[test] -fn allow_export_message_from_source_other_than_asset_hub_if_destination_other_than_ethereum() { - pub type Barrier = DenyThenTry< - ( - DenyReserveTransferToRelayChain, - DenyExportMessageFrom>, Equals>, - ), - TakeWeightCredit, - >; - - let mut xcm: Vec> = vec![ - AliasOrigin(Here.into()), - ExportMessage { network: Polkadot, destination: Here, xcm: Default::default() }, - ]; - - let result = Barrier::should_execute( - &ParachainLocation::get(), - &mut xcm, - Weight::zero(), - &mut Properties { weight_credit: Weight::zero(), message_id: None }, + // Export message from source2 and remote2 should pass + assert_deny_execution( + vec![ExportMessage { network: Remote2::get(), destination: Here, xcm: Default::default() }], + Source2::get(), + None, ); - assert_ok!(result); -} - -#[test] -fn deny_reserver_transfer_to_relaychain_does_not_break() { - pub type Barrier = DenyThenTry< - (DenyReserveTransferToRelayChain, DenyExportMessageFrom), - TakeWeightCredit, - >; - - let mut xcm: Vec> = vec![DepositReserveAsset { - assets: AssetFilter::try_from(Wild(All)).unwrap(), - dest: Location { parents: 1, interior: Here }, - xcm: Default::default(), - }]; - - let result = Barrier::should_execute( - &Here.into(), - &mut xcm, - Weight::zero(), - &mut Properties { weight_credit: Weight::zero(), message_id: None }, + // Export message from source2 and remote1 should be banned + assert_deny_execution( + vec![ExportMessage { network: Remote1::get(), destination: Here, xcm: Default::default() }], + Source2::get(), + Some(Unsupported), ); - - assert_err!(result, ProcessMessageError::Unsupported); } diff --git a/polkadot/xcm/xcm-builder/src/tests/barriers.rs b/polkadot/xcm/xcm-builder/src/tests/barriers.rs index d8805274d3a5..9b469b2dd500 100644 --- a/polkadot/xcm/xcm-builder/src/tests/barriers.rs +++ b/polkadot/xcm/xcm-builder/src/tests/barriers.rs @@ -532,3 +532,64 @@ fn allow_hrmp_notifications_from_relay_chain_should_work() { Ok(()), ); } + +#[test] +fn deny_then_try_works() { + // closure for (xcm, origin) testing with `DenyThenTry` + let assert_should_execute = |mut xcm: Vec>, origin, expected_result| { + pub type Barrier = DenyThenTry< + ( + DenyClearTransactStatusAsYield, + DenyClearOriginFromHereAsBadFormat, + DenyUnsubscribeVersionAsStackLimitReached, + ), + (AllowSingleClearErrorOrYield, AllowClearTopicFromHere), + >; + assert_eq!( + Barrier::should_execute( + &origin, + &mut xcm, + Weight::from_parts(10, 10), + &mut props(Weight::zero()), + ), + expected_result + ); + }; + + // Deny cases: + // trigger DenyClearTransactStatusAsYield + assert_should_execute( + vec![ClearTransactStatus], + Parachain(1).into_location(), + Err(ProcessMessageError::Yield), + ); + // DenyClearTransactStatusAsYield wins against AllowClearErrorOrYield + assert_should_execute( + vec![ClearError, ClearTransactStatus], + Parachain(1).into_location(), + Err(ProcessMessageError::Yield), + ); + // trigger DenyClearOriginFromHereAsBadFormat + assert_should_execute( + vec![ClearOrigin], + Here.into_location(), + Err(ProcessMessageError::BadFormat), + ); + // trigger DenyUnsubscribeVersionAsStackLimitReached + assert_should_execute( + vec![UnsubscribeVersion], + Here.into_location(), + Err(ProcessMessageError::StackLimitReached), + ); + + // ok + assert_should_execute(vec![ClearError], Parachain(1).into_location(), Ok(())); + assert_should_execute(vec![ClearTopic], Here.into(), Ok(())); + + // deny because none of the allow items match + assert_should_execute( + vec![ClearError, ClearTopic], + Parachain(1).into_location(), + Err(ProcessMessageError::Unsupported), + ); +} diff --git a/polkadot/xcm/xcm-builder/src/tests/mock.rs b/polkadot/xcm/xcm-builder/src/tests/mock.rs index bec7b253977b..0cdf0cfec707 100644 --- a/polkadot/xcm/xcm-builder/src/tests/mock.rs +++ b/polkadot/xcm/xcm-builder/src/tests/mock.rs @@ -19,7 +19,7 @@ use crate::{ barriers::{AllowSubscriptionsFrom, RespectSuspension, TrailingSetTopicAsId}, test_utils::*, - EnsureDecodableXcm, + CreateMatcher, EnsureDecodableXcm, MatchXcm, }; pub use crate::{ AliasForeignAccountId32, AllowExplicitUnpaidExecutionFrom, AllowKnownQueryResponses, @@ -31,8 +31,9 @@ pub use codec::{Decode, Encode}; pub use core::{ cell::{Cell, RefCell}, fmt::Debug, + ops::ControlFlow, }; -use frame_support::traits::{ContainsPair, Everything}; +use frame_support::traits::{ContainsPair, Everything, ProcessMessageError}; pub use frame_support::{ dispatch::{DispatchInfo, DispatchResultWithPostInfo, GetDispatchInfo, PostDispatchInfo}, ensure, parameter_types, @@ -40,11 +41,11 @@ pub use frame_support::{ traits::{Contains, Get, IsInVec}, }; pub use xcm::latest::{prelude::*, QueryId, Weight}; -use xcm_executor::traits::{Properties, QueryHandler, QueryResponseStatus}; pub use xcm_executor::{ traits::{ - AssetExchange, AssetLock, CheckSuspension, ConvertOrigin, Enact, ExportXcm, FeeManager, - FeeReason, LockError, OnResponse, TransactAsset, + AssetExchange, AssetLock, CheckSuspension, ConvertOrigin, DenyExecution, Enact, ExportXcm, + FeeManager, FeeReason, LockError, OnResponse, Properties, QueryHandler, + QueryResponseStatus, ShouldExecute, TransactAsset, }, AssetsInHolding, Config, }; @@ -772,3 +773,122 @@ pub fn fungible_multi_asset(location: Location, amount: u128) -> Asset { pub fn fake_message_hash(message: &Xcm) -> XcmHash { message.using_encoded(sp_io::hashing::blake2_256) } + +/// A dummy `DenyExecution` impl which returns `ProcessMessageError::Yield` when XCM contains +/// `ClearTransactStatus` +pub struct DenyClearTransactStatusAsYield; +impl DenyExecution for DenyClearTransactStatusAsYield { + fn deny_execution( + _origin: &Location, + instructions: &mut [Instruction], + _max_weight: Weight, + _properties: &mut Properties, + ) -> Option { + match instructions.matcher().match_next_inst_while( + |_| true, + |inst| match inst { + ClearTransactStatus { .. } => Err(ProcessMessageError::Yield), + _ => Ok(ControlFlow::Continue(())), + }, + ) { + Ok(_) => None, + Err(err) => Some(err), + } + } +} + +/// A dummy `DenyExecution` impl which returns `ProcessMessageError::BadFormat` when XCM contains +/// `ClearOrigin` with origin location from `Here` +pub struct DenyClearOriginFromHereAsBadFormat; +impl DenyExecution for DenyClearOriginFromHereAsBadFormat { + fn deny_execution( + origin: &Location, + instructions: &mut [Instruction], + _max_weight: Weight, + _properties: &mut Properties, + ) -> Option { + match instructions.matcher().match_next_inst_while( + |_| true, + |inst| match inst { + ClearOrigin { .. } => + if origin.clone() == Here.into_location() { + Err(ProcessMessageError::BadFormat) + } else { + Ok(ControlFlow::Continue(())) + }, + _ => Ok(ControlFlow::Continue(())), + }, + ) { + Ok(_) => None, + Err(err) => Some(err), + } + } +} + +/// A dummy `DenyExecution` impl which returns `ProcessMessageError::StackLimitReached` when XCM +/// contains a single `UnsubscribeVersion` +pub struct DenyUnsubscribeVersionAsStackLimitReached; +impl DenyExecution for DenyUnsubscribeVersionAsStackLimitReached { + fn deny_execution( + _origin: &Location, + instructions: &mut [Instruction], + _max_weight: Weight, + _properties: &mut Properties, + ) -> Option { + if instructions.len() != 1 { + return None; + } + match instructions.get(0).unwrap() { + UnsubscribeVersion { .. } => Some(ProcessMessageError::StackLimitReached), + _ => None, + } + } +} + +/// A dummy `ShouldExecute` impl which returns `Ok(())` when XCM contains a single `ClearError`, +/// else return `ProcessMessageError::Yield` +pub struct AllowSingleClearErrorOrYield; +impl ShouldExecute for AllowSingleClearErrorOrYield { + fn should_execute( + _origin: &Location, + instructions: &mut [Instruction], + _max_weight: Weight, + _properties: &mut Properties, + ) -> Result<(), ProcessMessageError> { + instructions + .matcher() + .assert_remaining_insts(1)? + .match_next_inst(|inst| match inst { + ClearError { .. } => Ok(()), + _ => Err(ProcessMessageError::Yield), + })?; + Ok(()) + } +} + +/// A dummy `ShouldExecute` impl which returns `Ok(())` when XCM contains `ClearTopic` and origin +/// from `Here`, else return `ProcessMessageError::Unsupported` +pub struct AllowClearTopicFromHere; +impl ShouldExecute for AllowClearTopicFromHere { + fn should_execute( + origin: &Location, + instructions: &mut [Instruction], + _max_weight: Weight, + _properties: &mut Properties, + ) -> Result<(), ProcessMessageError> { + ensure!(origin.clone() == Here.into_location(), ProcessMessageError::Unsupported); + let mut found = false; + instructions.matcher().match_next_inst_while( + |_| true, + |inst| match inst { + ClearTopic { .. } => { + found = true; + Ok(ControlFlow::Break(())) + }, + _ => Ok(ControlFlow::Continue(())), + }, + )?; + ensure!(found, ProcessMessageError::Unsupported); + Ok(()) + } +} From 58d79909011fb0f6173542ff779d210835338e49 Mon Sep 17 00:00:00 2001 From: ron Date: Thu, 16 Jan 2025 23:52:18 +0800 Subject: [PATCH 12/35] Test for DenyReserveTransferToRelayChain --- .../xcm/xcm-builder/src/tests/barriers.rs | 48 +++++++++++++++++++ 1 file changed, 48 insertions(+) diff --git a/polkadot/xcm/xcm-builder/src/tests/barriers.rs b/polkadot/xcm/xcm-builder/src/tests/barriers.rs index 9b469b2dd500..8abe86d77960 100644 --- a/polkadot/xcm/xcm-builder/src/tests/barriers.rs +++ b/polkadot/xcm/xcm-builder/src/tests/barriers.rs @@ -14,6 +14,7 @@ // You should have received a copy of the GNU General Public License // along with Polkadot. If not, see . +use frame_support::traits::ProcessMessageError::Unsupported; use xcm_executor::traits::Properties; use super::*; @@ -593,3 +594,50 @@ fn deny_then_try_works() { Err(ProcessMessageError::Unsupported), ); } + +#[test] +fn deny_reserver_transfer_to_relaychain_should_work() { + let assert_deny_execution = |mut xcm: Vec>, origin, expected_result| { + assert_eq!( + DenyReserveTransferToRelayChain::deny_execution( + &origin, + &mut xcm, + Weight::from_parts(10, 10), + &mut props(Weight::zero()), + ), + expected_result + ); + }; + // deny DepositReserveAsset to RelayChain + assert_deny_execution( + vec![DepositReserveAsset { + assets: Wild(All), + dest: Location::parent(), + xcm: vec![].into(), + }], + Here.into_location(), + Some(Unsupported), + ); + // deny InitiateReserveWithdraw to RelayChain + assert_deny_execution( + vec![InitiateReserveWithdraw { + assets: Wild(All), + reserve: Location::parent(), + xcm: vec![].into(), + }], + Here.into_location(), + Some(Unsupported), + ); + // deny TransferReserveAsset to RelayChain + assert_deny_execution( + vec![TransferReserveAsset { + assets: vec![].into(), + dest: Location::parent(), + xcm: vec![].into(), + }], + Here.into_location(), + Some(Unsupported), + ); + // others will pass + assert_deny_execution(vec![ClearOrigin], Here.into_location(), None); +} From 462c049ba86a159a190360ebc8a6b6160f73415b Mon Sep 17 00:00:00 2001 From: Branislav Kontur Date: Fri, 17 Jan 2025 11:03:09 +0100 Subject: [PATCH 13/35] Apply suggestions from code review --- .../bridge-hubs/common/tests/tests.rs | 23 ++++++++++--------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/cumulus/parachains/runtimes/bridge-hubs/common/tests/tests.rs b/cumulus/parachains/runtimes/bridge-hubs/common/tests/tests.rs index ea940d58b347..490a8c806d7c 100644 --- a/cumulus/parachains/runtimes/bridge-hubs/common/tests/tests.rs +++ b/cumulus/parachains/runtimes/bridge-hubs/common/tests/tests.rs @@ -26,15 +26,16 @@ use xcm::prelude::{ }; use xcm_executor::traits::{DenyExecution, Properties}; -parameter_types! { - pub Source1: Location = Location::new(1, Parachain(1)); - pub Source2: Location = Location::new(1, Parachain(2)); - pub Remote1: NetworkId = ByGenesis([1;32]); - pub Remote2: NetworkId = ByGenesis([2;32]); -} #[test] fn test_deny_export_message_from() { + parameter_types! { + pub Source1: Location = Location::new(1, Parachain(1)); + pub Source2: Location = Location::new(1, Parachain(2)); + pub Remote1: NetworkId = ByGenesis([1;32]); + pub Remote2: NetworkId = ByGenesis([2;32]); + } + pub type Denied = DenyExportMessageFrom>, Equals>; let assert_deny_execution = |mut xcm: Vec>, origin, expected_result| { @@ -49,31 +50,31 @@ fn test_deny_export_message_from() { ); }; - // No ExportMessage should pass + // No `ExportMessage` should pass assert_deny_execution(vec![AliasOrigin(Here.into())], Source1::get(), None); - // Export message from source1 and remote1 should pass + // `ExportMessage` from source1 and remote1 should pass assert_deny_execution( vec![ExportMessage { network: Remote1::get(), destination: Here, xcm: Default::default() }], Source1::get(), None, ); - // Export message from source1 and remote2 should pass + // `ExportMessage` from source1 and remote2 should pass assert_deny_execution( vec![ExportMessage { network: Remote2::get(), destination: Here, xcm: Default::default() }], Source1::get(), None, ); - // Export message from source2 and remote2 should pass + // `ExportMessage` from source2 and remote2 should pass assert_deny_execution( vec![ExportMessage { network: Remote2::get(), destination: Here, xcm: Default::default() }], Source2::get(), None, ); - // Export message from source2 and remote1 should be banned + // `ExportMessage` from source2 and remote1 should be banned assert_deny_execution( vec![ExportMessage { network: Remote1::get(), destination: Here, xcm: Default::default() }], Source2::get(), From 6e578bf21e23d9e57d2ad3060716e6a1b28c1585 Mon Sep 17 00:00:00 2001 From: Branislav Kontur Date: Fri, 17 Jan 2025 11:08:56 +0100 Subject: [PATCH 14/35] Update polkadot/xcm/xcm-builder/src/tests/barriers.rs --- polkadot/xcm/xcm-builder/src/tests/barriers.rs | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/polkadot/xcm/xcm-builder/src/tests/barriers.rs b/polkadot/xcm/xcm-builder/src/tests/barriers.rs index 8abe86d77960..1fb5d1a069c7 100644 --- a/polkadot/xcm/xcm-builder/src/tests/barriers.rs +++ b/polkadot/xcm/xcm-builder/src/tests/barriers.rs @@ -583,16 +583,17 @@ fn deny_then_try_works() { Err(ProcessMessageError::StackLimitReached), ); - // ok - assert_should_execute(vec![ClearError], Parachain(1).into_location(), Ok(())); - assert_should_execute(vec![ClearTopic], Here.into(), Ok(())); - // deny because none of the allow items match assert_should_execute( vec![ClearError, ClearTopic], Parachain(1).into_location(), Err(ProcessMessageError::Unsupported), ); + + // ok + assert_should_execute(vec![ClearError], Parachain(1).into_location(), Ok(())); + assert_should_execute(vec![ClearTopic], Here.into(), Ok(())); + assert_should_execute(vec![ClearError, ClearTopic], Here.into_location(), Ok(())); } #[test] From e4acb39869be8d3b388f37fb727ba12c150b1145 Mon Sep 17 00:00:00 2001 From: command-bot <> Date: Fri, 17 Jan 2025 10:23:00 +0000 Subject: [PATCH 15/35] Update from bkontur running command 'fmt' --- cumulus/parachains/runtimes/bridge-hubs/common/tests/tests.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/cumulus/parachains/runtimes/bridge-hubs/common/tests/tests.rs b/cumulus/parachains/runtimes/bridge-hubs/common/tests/tests.rs index 490a8c806d7c..99ad61ab8e9d 100644 --- a/cumulus/parachains/runtimes/bridge-hubs/common/tests/tests.rs +++ b/cumulus/parachains/runtimes/bridge-hubs/common/tests/tests.rs @@ -26,7 +26,6 @@ use xcm::prelude::{ }; use xcm_executor::traits::{DenyExecution, Properties}; - #[test] fn test_deny_export_message_from() { parameter_types! { From 8b949639e928519661f1b082ceb53d284239a6fe Mon Sep 17 00:00:00 2001 From: ron Date: Fri, 17 Jan 2025 18:37:21 +0800 Subject: [PATCH 16/35] Move dummy impls to tests --- .../xcm/xcm-builder/src/tests/barriers.rs | 117 ++++++++++++++++ polkadot/xcm/xcm-builder/src/tests/mock.rs | 125 +----------------- 2 files changed, 120 insertions(+), 122 deletions(-) diff --git a/polkadot/xcm/xcm-builder/src/tests/barriers.rs b/polkadot/xcm/xcm-builder/src/tests/barriers.rs index 1fb5d1a069c7..4faf15076cdf 100644 --- a/polkadot/xcm/xcm-builder/src/tests/barriers.rs +++ b/polkadot/xcm/xcm-builder/src/tests/barriers.rs @@ -536,6 +536,123 @@ fn allow_hrmp_notifications_from_relay_chain_should_work() { #[test] fn deny_then_try_works() { + /// A dummy `DenyExecution` impl which returns `ProcessMessageError::Yield` when XCM contains + /// `ClearTransactStatus` + struct DenyClearTransactStatusAsYield; + impl DenyExecution for DenyClearTransactStatusAsYield { + fn deny_execution( + _origin: &Location, + instructions: &mut [Instruction], + _max_weight: Weight, + _properties: &mut Properties, + ) -> Option { + match instructions.matcher().match_next_inst_while( + |_| true, + |inst| match inst { + ClearTransactStatus { .. } => Err(ProcessMessageError::Yield), + _ => Ok(ControlFlow::Continue(())), + }, + ) { + Ok(_) => None, + Err(err) => Some(err), + } + } + } + + /// A dummy `DenyExecution` impl which returns `ProcessMessageError::BadFormat` when XCM + /// contains `ClearOrigin` with origin location from `Here` + struct DenyClearOriginFromHereAsBadFormat; + impl DenyExecution for DenyClearOriginFromHereAsBadFormat { + fn deny_execution( + origin: &Location, + instructions: &mut [Instruction], + _max_weight: Weight, + _properties: &mut Properties, + ) -> Option { + match instructions.matcher().match_next_inst_while( + |_| true, + |inst| match inst { + ClearOrigin { .. } => + if origin.clone() == Here.into_location() { + Err(ProcessMessageError::BadFormat) + } else { + Ok(ControlFlow::Continue(())) + }, + _ => Ok(ControlFlow::Continue(())), + }, + ) { + Ok(_) => None, + Err(err) => Some(err), + } + } + } + + /// A dummy `DenyExecution` impl which returns `ProcessMessageError::StackLimitReached` when XCM + /// contains a single `UnsubscribeVersion` + struct DenyUnsubscribeVersionAsStackLimitReached; + impl DenyExecution for DenyUnsubscribeVersionAsStackLimitReached { + fn deny_execution( + _origin: &Location, + instructions: &mut [Instruction], + _max_weight: Weight, + _properties: &mut Properties, + ) -> Option { + if instructions.len() != 1 { + return None; + } + match instructions.get(0).unwrap() { + UnsubscribeVersion { .. } => Some(ProcessMessageError::StackLimitReached), + _ => None, + } + } + } + + /// A dummy `ShouldExecute` impl which returns `Ok(())` when XCM contains a single `ClearError`, + /// else return `ProcessMessageError::Yield` + struct AllowSingleClearErrorOrYield; + impl ShouldExecute for AllowSingleClearErrorOrYield { + fn should_execute( + _origin: &Location, + instructions: &mut [Instruction], + _max_weight: Weight, + _properties: &mut Properties, + ) -> Result<(), ProcessMessageError> { + instructions.matcher().assert_remaining_insts(1)?.match_next_inst( + |inst| match inst { + ClearError { .. } => Ok(()), + _ => Err(ProcessMessageError::Yield), + }, + )?; + Ok(()) + } + } + + /// A dummy `ShouldExecute` impl which returns `Ok(())` when XCM contains `ClearTopic` and + /// origin from `Here`, else return `ProcessMessageError::Unsupported` + struct AllowClearTopicFromHere; + impl ShouldExecute for AllowClearTopicFromHere { + fn should_execute( + origin: &Location, + instructions: &mut [Instruction], + _max_weight: Weight, + _properties: &mut Properties, + ) -> Result<(), ProcessMessageError> { + ensure!(origin.clone() == Here.into_location(), ProcessMessageError::Unsupported); + let mut found = false; + instructions.matcher().match_next_inst_while( + |_| true, + |inst| match inst { + ClearTopic { .. } => { + found = true; + Ok(ControlFlow::Break(())) + }, + _ => Ok(ControlFlow::Continue(())), + }, + )?; + ensure!(found, ProcessMessageError::Unsupported); + Ok(()) + } + } // closure for (xcm, origin) testing with `DenyThenTry` let assert_should_execute = |mut xcm: Vec>, origin, expected_result| { pub type Barrier = DenyThenTry< diff --git a/polkadot/xcm/xcm-builder/src/tests/mock.rs b/polkadot/xcm/xcm-builder/src/tests/mock.rs index 0cdf0cfec707..127888104a4a 100644 --- a/polkadot/xcm/xcm-builder/src/tests/mock.rs +++ b/polkadot/xcm/xcm-builder/src/tests/mock.rs @@ -19,7 +19,7 @@ use crate::{ barriers::{AllowSubscriptionsFrom, RespectSuspension, TrailingSetTopicAsId}, test_utils::*, - CreateMatcher, EnsureDecodableXcm, MatchXcm, + EnsureDecodableXcm, }; pub use crate::{ AliasForeignAccountId32, AllowExplicitUnpaidExecutionFrom, AllowKnownQueryResponses, @@ -33,7 +33,7 @@ pub use core::{ fmt::Debug, ops::ControlFlow, }; -use frame_support::traits::{ContainsPair, Everything, ProcessMessageError}; +use frame_support::traits::{ContainsPair, Everything}; pub use frame_support::{ dispatch::{DispatchInfo, DispatchResultWithPostInfo, GetDispatchInfo, PostDispatchInfo}, ensure, parameter_types, @@ -45,7 +45,7 @@ pub use xcm_executor::{ traits::{ AssetExchange, AssetLock, CheckSuspension, ConvertOrigin, DenyExecution, Enact, ExportXcm, FeeManager, FeeReason, LockError, OnResponse, Properties, QueryHandler, - QueryResponseStatus, ShouldExecute, TransactAsset, + QueryResponseStatus, TransactAsset, }, AssetsInHolding, Config, }; @@ -773,122 +773,3 @@ pub fn fungible_multi_asset(location: Location, amount: u128) -> Asset { pub fn fake_message_hash(message: &Xcm) -> XcmHash { message.using_encoded(sp_io::hashing::blake2_256) } - -/// A dummy `DenyExecution` impl which returns `ProcessMessageError::Yield` when XCM contains -/// `ClearTransactStatus` -pub struct DenyClearTransactStatusAsYield; -impl DenyExecution for DenyClearTransactStatusAsYield { - fn deny_execution( - _origin: &Location, - instructions: &mut [Instruction], - _max_weight: Weight, - _properties: &mut Properties, - ) -> Option { - match instructions.matcher().match_next_inst_while( - |_| true, - |inst| match inst { - ClearTransactStatus { .. } => Err(ProcessMessageError::Yield), - _ => Ok(ControlFlow::Continue(())), - }, - ) { - Ok(_) => None, - Err(err) => Some(err), - } - } -} - -/// A dummy `DenyExecution` impl which returns `ProcessMessageError::BadFormat` when XCM contains -/// `ClearOrigin` with origin location from `Here` -pub struct DenyClearOriginFromHereAsBadFormat; -impl DenyExecution for DenyClearOriginFromHereAsBadFormat { - fn deny_execution( - origin: &Location, - instructions: &mut [Instruction], - _max_weight: Weight, - _properties: &mut Properties, - ) -> Option { - match instructions.matcher().match_next_inst_while( - |_| true, - |inst| match inst { - ClearOrigin { .. } => - if origin.clone() == Here.into_location() { - Err(ProcessMessageError::BadFormat) - } else { - Ok(ControlFlow::Continue(())) - }, - _ => Ok(ControlFlow::Continue(())), - }, - ) { - Ok(_) => None, - Err(err) => Some(err), - } - } -} - -/// A dummy `DenyExecution` impl which returns `ProcessMessageError::StackLimitReached` when XCM -/// contains a single `UnsubscribeVersion` -pub struct DenyUnsubscribeVersionAsStackLimitReached; -impl DenyExecution for DenyUnsubscribeVersionAsStackLimitReached { - fn deny_execution( - _origin: &Location, - instructions: &mut [Instruction], - _max_weight: Weight, - _properties: &mut Properties, - ) -> Option { - if instructions.len() != 1 { - return None; - } - match instructions.get(0).unwrap() { - UnsubscribeVersion { .. } => Some(ProcessMessageError::StackLimitReached), - _ => None, - } - } -} - -/// A dummy `ShouldExecute` impl which returns `Ok(())` when XCM contains a single `ClearError`, -/// else return `ProcessMessageError::Yield` -pub struct AllowSingleClearErrorOrYield; -impl ShouldExecute for AllowSingleClearErrorOrYield { - fn should_execute( - _origin: &Location, - instructions: &mut [Instruction], - _max_weight: Weight, - _properties: &mut Properties, - ) -> Result<(), ProcessMessageError> { - instructions - .matcher() - .assert_remaining_insts(1)? - .match_next_inst(|inst| match inst { - ClearError { .. } => Ok(()), - _ => Err(ProcessMessageError::Yield), - })?; - Ok(()) - } -} - -/// A dummy `ShouldExecute` impl which returns `Ok(())` when XCM contains `ClearTopic` and origin -/// from `Here`, else return `ProcessMessageError::Unsupported` -pub struct AllowClearTopicFromHere; -impl ShouldExecute for AllowClearTopicFromHere { - fn should_execute( - origin: &Location, - instructions: &mut [Instruction], - _max_weight: Weight, - _properties: &mut Properties, - ) -> Result<(), ProcessMessageError> { - ensure!(origin.clone() == Here.into_location(), ProcessMessageError::Unsupported); - let mut found = false; - instructions.matcher().match_next_inst_while( - |_| true, - |inst| match inst { - ClearTopic { .. } => { - found = true; - Ok(ControlFlow::Break(())) - }, - _ => Ok(ControlFlow::Continue(())), - }, - )?; - ensure!(found, ProcessMessageError::Unsupported); - Ok(()) - } -} From b2f733819b33cd596c9769a752e3df96a2ec5ffe Mon Sep 17 00:00:00 2001 From: ron Date: Fri, 17 Jan 2025 18:43:46 +0800 Subject: [PATCH 17/35] More tests --- polkadot/xcm/xcm-builder/src/tests/barriers.rs | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/polkadot/xcm/xcm-builder/src/tests/barriers.rs b/polkadot/xcm/xcm-builder/src/tests/barriers.rs index 4faf15076cdf..ed87b0e11e99 100644 --- a/polkadot/xcm/xcm-builder/src/tests/barriers.rs +++ b/polkadot/xcm/xcm-builder/src/tests/barriers.rs @@ -756,6 +756,16 @@ fn deny_reserver_transfer_to_relaychain_should_work() { Here.into_location(), Some(Unsupported), ); - // others will pass + // accept DepositReserveAsset to destination other than RelayChain + assert_deny_execution( + vec![DepositReserveAsset { + assets: Wild(All), + dest: Here.into_location(), + xcm: vec![].into(), + }], + Here.into_location(), + None, + ); + // others instructions should pass assert_deny_execution(vec![ClearOrigin], Here.into_location(), None); } From a78449ebf96728d2243242743de4e2256ee4a564 Mon Sep 17 00:00:00 2001 From: ron Date: Fri, 17 Jan 2025 18:55:51 +0800 Subject: [PATCH 18/35] Fix comments --- polkadot/xcm/xcm-executor/src/traits/should_execute.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/polkadot/xcm/xcm-executor/src/traits/should_execute.rs b/polkadot/xcm/xcm-executor/src/traits/should_execute.rs index 2e3330c229f1..df40896d586d 100644 --- a/polkadot/xcm/xcm-executor/src/traits/should_execute.rs +++ b/polkadot/xcm/xcm-executor/src/traits/should_execute.rs @@ -131,7 +131,8 @@ impl CheckSuspension for Tuple { /// Trait to determine whether the execution engine should not execute a given XCM. /// /// Can be amalgamated into a tuple to have multiple trials. If any of the tuple elements returns -/// `Err(())`, the execution stops. Else, `Ok(_)` is returned if all elements accept the message. +/// `Some(ProcessMessageError)`, the execution stops. Else, `None` is returned if all elements +/// accept the message. pub trait DenyExecution { /// Returns `None` if there is no reason to deny execution, /// while `Some(ProcessMessageError)` indicates there is a reason to deny execution. From ddba5dc7dbfb622614b56d9794afb13aac6e15da Mon Sep 17 00:00:00 2001 From: Branislav Kontur Date: Fri, 17 Jan 2025 11:57:38 +0100 Subject: [PATCH 19/35] Apply suggestions from code review --- polkadot/xcm/xcm-builder/src/tests/barriers.rs | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/polkadot/xcm/xcm-builder/src/tests/barriers.rs b/polkadot/xcm/xcm-builder/src/tests/barriers.rs index ed87b0e11e99..fc94074a9f86 100644 --- a/polkadot/xcm/xcm-builder/src/tests/barriers.rs +++ b/polkadot/xcm/xcm-builder/src/tests/barriers.rs @@ -14,7 +14,6 @@ // You should have received a copy of the GNU General Public License // along with Polkadot. If not, see . -use frame_support::traits::ProcessMessageError::Unsupported; use xcm_executor::traits::Properties; use super::*; @@ -734,7 +733,7 @@ fn deny_reserver_transfer_to_relaychain_should_work() { xcm: vec![].into(), }], Here.into_location(), - Some(Unsupported), + Some(ProcessMessageError::Unsupported), ); // deny InitiateReserveWithdraw to RelayChain assert_deny_execution( @@ -744,7 +743,7 @@ fn deny_reserver_transfer_to_relaychain_should_work() { xcm: vec![].into(), }], Here.into_location(), - Some(Unsupported), + Some(ProcessMessageError::Unsupported), ); // deny TransferReserveAsset to RelayChain assert_deny_execution( @@ -754,7 +753,7 @@ fn deny_reserver_transfer_to_relaychain_should_work() { xcm: vec![].into(), }], Here.into_location(), - Some(Unsupported), + Some(ProcessMessageError::Unsupported), ); // accept DepositReserveAsset to destination other than RelayChain assert_deny_execution( From 66814c9e1619f850bb8c90be35829d04a174dbf0 Mon Sep 17 00:00:00 2001 From: Branislav Kontur Date: Tue, 21 Jan 2025 13:54:26 +0100 Subject: [PATCH 20/35] Update polkadot/xcm/xcm-executor/src/traits/should_execute.rs Co-authored-by: Clara van Staden --- polkadot/xcm/xcm-executor/src/traits/should_execute.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/polkadot/xcm/xcm-executor/src/traits/should_execute.rs b/polkadot/xcm/xcm-executor/src/traits/should_execute.rs index df40896d586d..bb7807008d71 100644 --- a/polkadot/xcm/xcm-executor/src/traits/should_execute.rs +++ b/polkadot/xcm/xcm-executor/src/traits/should_execute.rs @@ -130,7 +130,7 @@ impl CheckSuspension for Tuple { /// Trait to determine whether the execution engine should not execute a given XCM. /// -/// Can be amalgamated into a tuple to have multiple trials. If any of the tuple elements returns +/// Can be amalgamated into a tuple to have multiple traits. If any of the tuple elements returns /// `Some(ProcessMessageError)`, the execution stops. Else, `None` is returned if all elements /// accept the message. pub trait DenyExecution { From 02912a6e6c3421c23436626c79898e2d2ca27c3e Mon Sep 17 00:00:00 2001 From: Branislav Kontur Date: Tue, 21 Jan 2025 13:54:47 +0100 Subject: [PATCH 21/35] Update cumulus/parachains/runtimes/bridge-hubs/common/tests/tests.rs Co-authored-by: Clara van Staden --- cumulus/parachains/runtimes/bridge-hubs/common/tests/tests.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cumulus/parachains/runtimes/bridge-hubs/common/tests/tests.rs b/cumulus/parachains/runtimes/bridge-hubs/common/tests/tests.rs index 99ad61ab8e9d..bdc6dd389be9 100644 --- a/cumulus/parachains/runtimes/bridge-hubs/common/tests/tests.rs +++ b/cumulus/parachains/runtimes/bridge-hubs/common/tests/tests.rs @@ -49,7 +49,7 @@ fn test_deny_export_message_from() { ); }; - // No `ExportMessage` should pass + // A message without an `ExportMessage` should pass assert_deny_execution(vec![AliasOrigin(Here.into())], Source1::get(), None); // `ExportMessage` from source1 and remote1 should pass From 85b06c2b04e3ecf8bc2a8c015d2d973c237f147d Mon Sep 17 00:00:00 2001 From: ron Date: Wed, 22 Jan 2025 00:38:32 +0800 Subject: [PATCH 22/35] Add comments --- cumulus/parachains/runtimes/bridge-hubs/common/tests/tests.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/cumulus/parachains/runtimes/bridge-hubs/common/tests/tests.rs b/cumulus/parachains/runtimes/bridge-hubs/common/tests/tests.rs index bdc6dd389be9..4f26a6d35a15 100644 --- a/cumulus/parachains/runtimes/bridge-hubs/common/tests/tests.rs +++ b/cumulus/parachains/runtimes/bridge-hubs/common/tests/tests.rs @@ -35,6 +35,9 @@ fn test_deny_export_message_from() { pub Remote2: NetworkId = ByGenesis([2;32]); } + // Deny ExportMessage when both of the conditions met: + // 1: source != Source1 + // 2: network == Remote1 pub type Denied = DenyExportMessageFrom>, Equals>; let assert_deny_execution = |mut xcm: Vec>, origin, expected_result| { From a842c4de3670e85599dd977e0094426cf92b4871 Mon Sep 17 00:00:00 2001 From: ron Date: Wed, 22 Jan 2025 23:23:51 +0800 Subject: [PATCH 23/35] Fix clippy --- cumulus/parachains/runtimes/bridge-hubs/common/src/barriers.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cumulus/parachains/runtimes/bridge-hubs/common/src/barriers.rs b/cumulus/parachains/runtimes/bridge-hubs/common/src/barriers.rs index ab72de92d484..d9c5162c82c7 100644 --- a/cumulus/parachains/runtimes/bridge-hubs/common/src/barriers.rs +++ b/cumulus/parachains/runtimes/bridge-hubs/common/src/barriers.rs @@ -45,7 +45,7 @@ where |inst| match inst { ExportMessage { network, .. } => if ToGlobalConsensus::contains(network) && FromOrigin::contains(origin) { - return Err(ProcessMessageError::Unsupported) + Err(ProcessMessageError::Unsupported) } else { Ok(ControlFlow::Continue(())) }, From 551afc11cbc07dfc6547ff3440af6cdb0ff6496d Mon Sep 17 00:00:00 2001 From: Ron Date: Wed, 22 Jan 2025 23:26:12 +0800 Subject: [PATCH 24/35] Update polkadot/xcm/xcm-executor/src/traits/should_execute.rs Co-authored-by: Adrian Catangiu --- polkadot/xcm/xcm-executor/src/traits/should_execute.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/polkadot/xcm/xcm-executor/src/traits/should_execute.rs b/polkadot/xcm/xcm-executor/src/traits/should_execute.rs index bb7807008d71..007ec2f4b0e8 100644 --- a/polkadot/xcm/xcm-executor/src/traits/should_execute.rs +++ b/polkadot/xcm/xcm-executor/src/traits/should_execute.rs @@ -150,7 +150,7 @@ pub trait DenyExecution { ) -> Option; } -#[impl_trait_for_tuples::impl_for_tuples(30)] +#[impl_trait_for_tuples::impl_for_tuples(10)] impl DenyExecution for Tuple { fn deny_execution( origin: &Location, From cd33044fe6fa5cf82458f37d723c57e04efed12f Mon Sep 17 00:00:00 2001 From: Ron Date: Wed, 22 Jan 2025 23:30:11 +0800 Subject: [PATCH 25/35] Update polkadot/xcm/xcm-executor/src/traits/should_execute.rs Co-authored-by: Adrian Catangiu --- polkadot/xcm/xcm-executor/src/traits/should_execute.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/polkadot/xcm/xcm-executor/src/traits/should_execute.rs b/polkadot/xcm/xcm-executor/src/traits/should_execute.rs index 007ec2f4b0e8..d04f6a3700dc 100644 --- a/polkadot/xcm/xcm-executor/src/traits/should_execute.rs +++ b/polkadot/xcm/xcm-executor/src/traits/should_execute.rs @@ -163,7 +163,7 @@ impl DenyExecution for Tuple { match Tuple::deny_execution(origin, instructions, max_weight, properties) { Some(error) => { tracing::error!( - target: "xcm::should_execute", + target: "xcm::deny_execution", ?origin, ?instructions, ?max_weight, From 639177b30a05d5279247fae55572d8d1dd1f7e2c Mon Sep 17 00:00:00 2001 From: Ron Date: Wed, 22 Jan 2025 23:36:38 +0800 Subject: [PATCH 26/35] Update polkadot/xcm/xcm-executor/src/traits/should_execute.rs Co-authored-by: Adrian Catangiu --- polkadot/xcm/xcm-executor/src/traits/should_execute.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/polkadot/xcm/xcm-executor/src/traits/should_execute.rs b/polkadot/xcm/xcm-executor/src/traits/should_execute.rs index d04f6a3700dc..f5cad20d1f94 100644 --- a/polkadot/xcm/xcm-executor/src/traits/should_execute.rs +++ b/polkadot/xcm/xcm-executor/src/traits/should_execute.rs @@ -176,7 +176,7 @@ impl DenyExecution for Tuple { }, None => { tracing::trace!( - target: "xcm::should_execute", + target: "xcm::deny_execution", ?origin, ?instructions, ?max_weight, From 7564e23a36f364c9f3a7493c49ff69b48130415e Mon Sep 17 00:00:00 2001 From: Ron Date: Wed, 22 Jan 2025 23:39:40 +0800 Subject: [PATCH 27/35] Update cumulus/parachains/runtimes/bridge-hubs/common/src/barriers.rs Co-authored-by: Adrian Catangiu --- .../parachains/runtimes/bridge-hubs/common/src/barriers.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/cumulus/parachains/runtimes/bridge-hubs/common/src/barriers.rs b/cumulus/parachains/runtimes/bridge-hubs/common/src/barriers.rs index ab72de92d484..f884e057313a 100644 --- a/cumulus/parachains/runtimes/bridge-hubs/common/src/barriers.rs +++ b/cumulus/parachains/runtimes/bridge-hubs/common/src/barriers.rs @@ -40,6 +40,10 @@ where _max_weight: Weight, _properties: &mut Properties, ) -> Option { + // This barrier only cares about messages with `origin` matching `FromOrigin`. + if !FromOrigin::contains(origin) { + return None; + } match message.matcher().match_next_inst_while( |_| true, |inst| match inst { From d9c2f0e19b6d4882bf7673bd530d92f04611a4f7 Mon Sep 17 00:00:00 2001 From: Ron Date: Wed, 22 Jan 2025 23:39:53 +0800 Subject: [PATCH 28/35] Update cumulus/parachains/runtimes/bridge-hubs/common/src/barriers.rs Co-authored-by: Adrian Catangiu --- .../runtimes/bridge-hubs/common/src/barriers.rs | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/cumulus/parachains/runtimes/bridge-hubs/common/src/barriers.rs b/cumulus/parachains/runtimes/bridge-hubs/common/src/barriers.rs index f884e057313a..7db80ba962ab 100644 --- a/cumulus/parachains/runtimes/bridge-hubs/common/src/barriers.rs +++ b/cumulus/parachains/runtimes/bridge-hubs/common/src/barriers.rs @@ -47,12 +47,9 @@ where match message.matcher().match_next_inst_while( |_| true, |inst| match inst { - ExportMessage { network, .. } => - if ToGlobalConsensus::contains(network) && FromOrigin::contains(origin) { - return Err(ProcessMessageError::Unsupported) - } else { - Ok(ControlFlow::Continue(())) - }, + ExportMessage { network, .. } if ToGlobalConsensus::contains(network) => { + return Err(ProcessMessageError::Unsupported) + }, _ => Ok(ControlFlow::Continue(())), }, ) { From b39f07e1bcf8b95ea56578ae4a0cdd8fc5870bac Mon Sep 17 00:00:00 2001 From: Ron Date: Sat, 25 Jan 2025 11:58:51 +0800 Subject: [PATCH 29/35] Update polkadot/xcm/xcm-builder/src/tests/barriers.rs Co-authored-by: Francisco Aguirre --- polkadot/xcm/xcm-builder/src/tests/barriers.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/polkadot/xcm/xcm-builder/src/tests/barriers.rs b/polkadot/xcm/xcm-builder/src/tests/barriers.rs index fc94074a9f86..84d8b9e408c3 100644 --- a/polkadot/xcm/xcm-builder/src/tests/barriers.rs +++ b/polkadot/xcm/xcm-builder/src/tests/barriers.rs @@ -680,7 +680,7 @@ fn deny_then_try_works() { Parachain(1).into_location(), Err(ProcessMessageError::Yield), ); - // DenyClearTransactStatusAsYield wins against AllowClearErrorOrYield + // DenyClearTransactStatusAsYield wins against AllowSingleClearErrorOrYield assert_should_execute( vec![ClearError, ClearTransactStatus], Parachain(1).into_location(), From 110d42e7255bc63687305fa24989f5df479f6506 Mon Sep 17 00:00:00 2001 From: Ron Date: Sat, 25 Jan 2025 11:59:14 +0800 Subject: [PATCH 30/35] Update polkadot/xcm/xcm-builder/src/tests/barriers.rs Co-authored-by: Francisco Aguirre --- polkadot/xcm/xcm-builder/src/tests/barriers.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/polkadot/xcm/xcm-builder/src/tests/barriers.rs b/polkadot/xcm/xcm-builder/src/tests/barriers.rs index 84d8b9e408c3..ee4a7d2c6d12 100644 --- a/polkadot/xcm/xcm-builder/src/tests/barriers.rs +++ b/polkadot/xcm/xcm-builder/src/tests/barriers.rs @@ -713,7 +713,7 @@ fn deny_then_try_works() { } #[test] -fn deny_reserver_transfer_to_relaychain_should_work() { +fn deny_reserve_transfer_to_relaychain_should_work() { let assert_deny_execution = |mut xcm: Vec>, origin, expected_result| { assert_eq!( DenyReserveTransferToRelayChain::deny_execution( From 8699575103cae1deb2d31cd714cfd301c0899223 Mon Sep 17 00:00:00 2001 From: Ron Date: Sat, 25 Jan 2025 11:59:25 +0800 Subject: [PATCH 31/35] Update cumulus/parachains/runtimes/bridge-hubs/common/src/barriers.rs Co-authored-by: Francisco Aguirre --- cumulus/parachains/runtimes/bridge-hubs/common/src/barriers.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cumulus/parachains/runtimes/bridge-hubs/common/src/barriers.rs b/cumulus/parachains/runtimes/bridge-hubs/common/src/barriers.rs index 607a37ae57a3..5d66a502de39 100644 --- a/cumulus/parachains/runtimes/bridge-hubs/common/src/barriers.rs +++ b/cumulus/parachains/runtimes/bridge-hubs/common/src/barriers.rs @@ -48,7 +48,7 @@ where |_| true, |inst| match inst { ExportMessage { network, .. } if ToGlobalConsensus::contains(network) => - return Err(ProcessMessageError::Unsupported), + Err(ProcessMessageError::Unsupported), _ => Ok(ControlFlow::Continue(())), }, ) { From 6b65c8146514fcf35ac58d82b0893fa605c632f4 Mon Sep 17 00:00:00 2001 From: ron Date: Sat, 25 Jan 2025 14:06:19 +0800 Subject: [PATCH 32/35] Change return to Result<(), ProcessMessageError> --- .../bridge-hubs/common/src/barriers.rs | 14 +++---- polkadot/xcm/xcm-builder/src/barriers.rs | 16 +++----- .../xcm/xcm-builder/src/tests/barriers.rs | 38 +++++++++---------- .../xcm-executor/src/traits/should_execute.rs | 12 +++--- 4 files changed, 35 insertions(+), 45 deletions(-) diff --git a/cumulus/parachains/runtimes/bridge-hubs/common/src/barriers.rs b/cumulus/parachains/runtimes/bridge-hubs/common/src/barriers.rs index 5d66a502de39..6b5dee3de384 100644 --- a/cumulus/parachains/runtimes/bridge-hubs/common/src/barriers.rs +++ b/cumulus/parachains/runtimes/bridge-hubs/common/src/barriers.rs @@ -39,21 +39,19 @@ where message: &mut [Instruction], _max_weight: Weight, _properties: &mut Properties, - ) -> Option { + ) -> Result<(), ProcessMessageError> { // This barrier only cares about messages with `origin` matching `FromOrigin`. if !FromOrigin::contains(origin) { - return None; + return Ok(()) } - match message.matcher().match_next_inst_while( + message.matcher().match_next_inst_while( |_| true, |inst| match inst { ExportMessage { network, .. } if ToGlobalConsensus::contains(network) => - Err(ProcessMessageError::Unsupported), + Err(ProcessMessageError::Unsupported), _ => Ok(ControlFlow::Continue(())), }, - ) { - Ok(_) => None, - Err(error) => Some(error), - } + )?; + Ok(()) } } diff --git a/polkadot/xcm/xcm-builder/src/barriers.rs b/polkadot/xcm/xcm-builder/src/barriers.rs index 1ab907b0bf03..9f9d3c232122 100644 --- a/polkadot/xcm/xcm-builder/src/barriers.rs +++ b/polkadot/xcm/xcm-builder/src/barriers.rs @@ -458,10 +458,8 @@ where max_weight: Weight, properties: &mut Properties, ) -> Result<(), ProcessMessageError> { - match Deny::deny_execution(origin, message, max_weight, properties) { - Some(err) => Err(err), - None => Allow::should_execute(origin, message, max_weight, properties), - } + Deny::deny_execution(origin, message, max_weight, properties)?; + Allow::should_execute(origin, message, max_weight, properties) } } @@ -473,8 +471,8 @@ impl DenyExecution for DenyReserveTransferToRelayChain { message: &mut [Instruction], _max_weight: Weight, _properties: &mut Properties, - ) -> Option { - match message.matcher().match_next_inst_while( + ) -> Result<(), ProcessMessageError> { + message.matcher().match_next_inst_while( |_| true, |inst| match inst { InitiateReserveWithdraw { @@ -500,9 +498,7 @@ impl DenyExecution for DenyReserveTransferToRelayChain { _ => Ok(ControlFlow::Continue(())), }, - ) { - Ok(_) => None, - Err(err) => Some(err), - } + )?; + Ok(()) } } diff --git a/polkadot/xcm/xcm-builder/src/tests/barriers.rs b/polkadot/xcm/xcm-builder/src/tests/barriers.rs index ee4a7d2c6d12..2fb8e8ed0363 100644 --- a/polkadot/xcm/xcm-builder/src/tests/barriers.rs +++ b/polkadot/xcm/xcm-builder/src/tests/barriers.rs @@ -544,17 +544,15 @@ fn deny_then_try_works() { instructions: &mut [Instruction], _max_weight: Weight, _properties: &mut Properties, - ) -> Option { - match instructions.matcher().match_next_inst_while( + ) -> Result<(), ProcessMessageError> { + instructions.matcher().match_next_inst_while( |_| true, |inst| match inst { ClearTransactStatus { .. } => Err(ProcessMessageError::Yield), _ => Ok(ControlFlow::Continue(())), }, - ) { - Ok(_) => None, - Err(err) => Some(err), - } + )?; + Ok(()) } } @@ -567,8 +565,8 @@ fn deny_then_try_works() { instructions: &mut [Instruction], _max_weight: Weight, _properties: &mut Properties, - ) -> Option { - match instructions.matcher().match_next_inst_while( + ) -> Result<(), ProcessMessageError> { + instructions.matcher().match_next_inst_while( |_| true, |inst| match inst { ClearOrigin { .. } => @@ -579,10 +577,8 @@ fn deny_then_try_works() { }, _ => Ok(ControlFlow::Continue(())), }, - ) { - Ok(_) => None, - Err(err) => Some(err), - } + )?; + Ok(()) } } @@ -595,13 +591,13 @@ fn deny_then_try_works() { instructions: &mut [Instruction], _max_weight: Weight, _properties: &mut Properties, - ) -> Option { + ) -> Result<(), ProcessMessageError> { if instructions.len() != 1 { - return None; + return Ok(()) } match instructions.get(0).unwrap() { - UnsubscribeVersion { .. } => Some(ProcessMessageError::StackLimitReached), - _ => None, + UnsubscribeVersion { .. } => Err(ProcessMessageError::StackLimitReached), + _ => Ok(()), } } } @@ -733,7 +729,7 @@ fn deny_reserve_transfer_to_relaychain_should_work() { xcm: vec![].into(), }], Here.into_location(), - Some(ProcessMessageError::Unsupported), + Err(ProcessMessageError::Unsupported), ); // deny InitiateReserveWithdraw to RelayChain assert_deny_execution( @@ -743,7 +739,7 @@ fn deny_reserve_transfer_to_relaychain_should_work() { xcm: vec![].into(), }], Here.into_location(), - Some(ProcessMessageError::Unsupported), + Err(ProcessMessageError::Unsupported), ); // deny TransferReserveAsset to RelayChain assert_deny_execution( @@ -753,7 +749,7 @@ fn deny_reserve_transfer_to_relaychain_should_work() { xcm: vec![].into(), }], Here.into_location(), - Some(ProcessMessageError::Unsupported), + Err(ProcessMessageError::Unsupported), ); // accept DepositReserveAsset to destination other than RelayChain assert_deny_execution( @@ -763,8 +759,8 @@ fn deny_reserve_transfer_to_relaychain_should_work() { xcm: vec![].into(), }], Here.into_location(), - None, + Ok(()), ); // others instructions should pass - assert_deny_execution(vec![ClearOrigin], Here.into_location(), None); + assert_deny_execution(vec![ClearOrigin], Here.into_location(), Ok(())); } diff --git a/polkadot/xcm/xcm-executor/src/traits/should_execute.rs b/polkadot/xcm/xcm-executor/src/traits/should_execute.rs index f5cad20d1f94..7ee093040431 100644 --- a/polkadot/xcm/xcm-executor/src/traits/should_execute.rs +++ b/polkadot/xcm/xcm-executor/src/traits/should_execute.rs @@ -147,7 +147,7 @@ pub trait DenyExecution { instructions: &mut [Instruction], max_weight: Weight, properties: &mut Properties, - ) -> Option; + ) -> Result<(), ProcessMessageError>; } #[impl_trait_for_tuples::impl_for_tuples(10)] @@ -157,11 +157,11 @@ impl DenyExecution for Tuple { instructions: &mut [Instruction], max_weight: Weight, properties: &mut Properties, - ) -> Option { + ) -> Result<(), ProcessMessageError> { for_tuples!( #( let barrier = core::any::type_name::(); match Tuple::deny_execution(origin, instructions, max_weight, properties) { - Some(error) => { + Err(error) => { tracing::error!( target: "xcm::deny_execution", ?origin, @@ -172,9 +172,9 @@ impl DenyExecution for Tuple { %barrier, "did not pass barrier", ); - return Some(error); + return Err(error); }, - None => { + Ok(()) => { tracing::trace!( target: "xcm::deny_execution", ?origin, @@ -188,6 +188,6 @@ impl DenyExecution for Tuple { } )* ); - None + Ok(()) } } From 03ff441c474cfd821b0bca7a0777217a9eae9970 Mon Sep 17 00:00:00 2001 From: ron Date: Sat, 25 Jan 2025 16:50:09 +0800 Subject: [PATCH 33/35] Fix tests --- .../runtimes/bridge-hubs/common/tests/tests.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/cumulus/parachains/runtimes/bridge-hubs/common/tests/tests.rs b/cumulus/parachains/runtimes/bridge-hubs/common/tests/tests.rs index 4f26a6d35a15..84c135728d5d 100644 --- a/cumulus/parachains/runtimes/bridge-hubs/common/tests/tests.rs +++ b/cumulus/parachains/runtimes/bridge-hubs/common/tests/tests.rs @@ -53,33 +53,33 @@ fn test_deny_export_message_from() { }; // A message without an `ExportMessage` should pass - assert_deny_execution(vec![AliasOrigin(Here.into())], Source1::get(), None); + assert_deny_execution(vec![AliasOrigin(Here.into())], Source1::get(), Ok(())); // `ExportMessage` from source1 and remote1 should pass assert_deny_execution( vec![ExportMessage { network: Remote1::get(), destination: Here, xcm: Default::default() }], Source1::get(), - None, + Ok(()), ); // `ExportMessage` from source1 and remote2 should pass assert_deny_execution( vec![ExportMessage { network: Remote2::get(), destination: Here, xcm: Default::default() }], Source1::get(), - None, + Ok(()), ); // `ExportMessage` from source2 and remote2 should pass assert_deny_execution( vec![ExportMessage { network: Remote2::get(), destination: Here, xcm: Default::default() }], Source2::get(), - None, + Ok(()), ); // `ExportMessage` from source2 and remote1 should be banned assert_deny_execution( vec![ExportMessage { network: Remote1::get(), destination: Here, xcm: Default::default() }], Source2::get(), - Some(Unsupported), + Err(Unsupported), ); } From c2719f99f867f068ee07b70175a9630d375b8735 Mon Sep 17 00:00:00 2001 From: Branislav Kontur Date: Mon, 27 Jan 2025 11:37:50 +0100 Subject: [PATCH 34/35] Update polkadot/xcm/xcm-executor/src/traits/should_execute.rs --- polkadot/xcm/xcm-executor/src/traits/should_execute.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/polkadot/xcm/xcm-executor/src/traits/should_execute.rs b/polkadot/xcm/xcm-executor/src/traits/should_execute.rs index 7ee093040431..0c536b21daf4 100644 --- a/polkadot/xcm/xcm-executor/src/traits/should_execute.rs +++ b/polkadot/xcm/xcm-executor/src/traits/should_execute.rs @@ -134,8 +134,8 @@ impl CheckSuspension for Tuple { /// `Some(ProcessMessageError)`, the execution stops. Else, `None` is returned if all elements /// accept the message. pub trait DenyExecution { - /// Returns `None` if there is no reason to deny execution, - /// while `Some(ProcessMessageError)` indicates there is a reason to deny execution. + /// Returns `Ok(())` if there is no reason to deny execution, + /// while `Err(ProcessMessageError)` indicates there is a reason to deny execution. /// /// - `origin`: The origin (sender) of the message. /// - `instructions`: The message itself. From 54dadec6d1a528c5ab1b9f20617f7d3ebc1c5fdc Mon Sep 17 00:00:00 2001 From: Adrian Catangiu Date: Mon, 27 Jan 2025 18:23:38 +0200 Subject: [PATCH 35/35] Update polkadot/xcm/xcm-executor/src/traits/should_execute.rs --- polkadot/xcm/xcm-executor/src/traits/should_execute.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/polkadot/xcm/xcm-executor/src/traits/should_execute.rs b/polkadot/xcm/xcm-executor/src/traits/should_execute.rs index 0c536b21daf4..48a562a1648d 100644 --- a/polkadot/xcm/xcm-executor/src/traits/should_execute.rs +++ b/polkadot/xcm/xcm-executor/src/traits/should_execute.rs @@ -131,7 +131,7 @@ impl CheckSuspension for Tuple { /// Trait to determine whether the execution engine should not execute a given XCM. /// /// Can be amalgamated into a tuple to have multiple traits. If any of the tuple elements returns -/// `Some(ProcessMessageError)`, the execution stops. Else, `None` is returned if all elements +/// `Err(ProcessMessageError)`, the execution stops. Else, `Ok(())` is returned if all elements /// accept the message. pub trait DenyExecution { /// Returns `Ok(())` if there is no reason to deny execution,