From 4a70b2cffb23db148a9af50cfbf13c4655dbaf7b Mon Sep 17 00:00:00 2001
From: Francisco Aguirre <franciscoaguirreperez@gmail.com>
Date: Thu, 10 Oct 2024 10:46:49 +0200
Subject: [PATCH] Remove redundant XCMs from dry run's forwarded xcms (#5913)
# Description
This PR addresses
https://github.com/paritytech/polkadot-sdk/issues/5878.
After dry running an xcm on asset hub, we had redundant xcms showing up
in the `forwarded_xcms` field of the dry run effects returned.
These were caused by two things:
- The `UpwardMessageSender` router always added an element even if there
were no messages.
- The two routers on asset hub westend related to bridging (to rococo
and sepolia) getting the message from their queues when their queues is
actually the same xcmp queue that was already contemplated.
In order to fix this, we check for no messages in UMP and clear the
implementation of `InspectMessageQueues` for these bridging routers.
Keep in mind that the bridged message is still sent, as normal via the
xcmp-queue to Bridge Hub.
To keep on dry-running the journey of the message, the next hop to
dry-run is Bridge Hub.
That'll be tackled in a different PR.
Added a test in `bridge-hub-westend-integration-tests` and
`bridge-hub-rococo-integration-tests` that show that dry-running a
transfer across the bridge from asset hub results in one and only one
message sent to bridge hub.
## TODO
- [x] Functionality
- [x] Test
---------
Co-authored-by: command-bot <>
---
Cargo.lock | 2 +
.../modules/xcm-bridge-hub-router/src/lib.rs | 35 +++-----------
cumulus/pallets/parachain-system/src/lib.rs | 6 ++-
.../emulated/common/src/macros.rs | 48 +++++++++++++++++++
.../bridges/bridge-hub-rococo/Cargo.toml | 1 +
.../bridges/bridge-hub-rococo/src/lib.rs | 4 +-
.../src/tests/asset_transfers.rs | 9 ++++
.../bridges/bridge-hub-westend/Cargo.toml | 1 +
.../bridges/bridge-hub-westend/src/lib.rs | 4 +-
.../src/tests/asset_transfers.rs | 9 ++++
.../xcm/xcm-builder/src/universal_exports.rs | 10 ++--
prdoc/pr_5913.prdoc | 20 ++++++++
12 files changed, 111 insertions(+), 38 deletions(-)
create mode 100644 prdoc/pr_5913.prdoc
diff --git a/Cargo.lock b/Cargo.lock
index fa8f486e01f..ca71985b69f 100644
--- a/Cargo.lock
+++ b/Cargo.lock
@@ -2228,6 +2228,7 @@ dependencies = [
"staging-xcm",
"staging-xcm-executor",
"testnet-parachains-constants",
+ "xcm-runtime-apis",
]
[[package]]
@@ -2421,6 +2422,7 @@ dependencies = [
"staging-xcm",
"staging-xcm-executor",
"testnet-parachains-constants",
+ "xcm-runtime-apis",
]
[[package]]
diff --git a/bridges/modules/xcm-bridge-hub-router/src/lib.rs b/bridges/modules/xcm-bridge-hub-router/src/lib.rs
index 7ba524e95b1..fe8f5a2efdf 100644
--- a/bridges/modules/xcm-bridge-hub-router/src/lib.rs
+++ b/bridges/modules/xcm-bridge-hub-router/src/lib.rs
@@ -99,7 +99,7 @@ pub mod pallet {
type DestinationVersion: GetVersion;
/// Actual message sender (`HRMP` or `DMP`) to the sibling bridge hub location.
- type ToBridgeHubSender: SendXcm + InspectMessageQueues;
+ type ToBridgeHubSender: SendXcm;
/// Local XCM channel manager.
type LocalXcmChannelManager: XcmChannelStatusProvider;
@@ -408,12 +408,12 @@ impl<T: Config<I>, I: 'static> SendXcm for Pallet<T, I> {
}
impl<T: Config<I>, I: 'static> InspectMessageQueues for Pallet<T, I> {
- fn clear_messages() {
- ViaBridgeHubExporter::<T, I>::clear_messages()
- }
+ fn clear_messages() {}
+ /// This router needs to implement `InspectMessageQueues` but doesn't have to
+ /// return any messages, since it just reuses the `XcmpQueue` router.
fn get_messages() -> Vec<(VersionedLocation, Vec<VersionedXcm<()>>)> {
- ViaBridgeHubExporter::<T, I>::get_messages()
+ Vec::new()
}
}
@@ -646,34 +646,13 @@ mod tests {
}
#[test]
- fn get_messages_works() {
+ fn get_messages_does_not_return_anything() {
run_test(|| {
assert_ok!(send_xcm::<XcmBridgeHubRouter>(
(Parent, Parent, GlobalConsensus(BridgedNetworkId::get()), Parachain(1000)).into(),
vec![ClearOrigin].into()
));
- assert_eq!(
- XcmBridgeHubRouter::get_messages(),
- vec![(
- VersionedLocation::V4((Parent, Parachain(1002)).into()),
- vec![VersionedXcm::V4(
- Xcm::builder()
- .withdraw_asset((Parent, 1_002_000))
- .buy_execution((Parent, 1_002_000), Unlimited)
- .set_appendix(
- Xcm::builder_unsafe()
- .deposit_asset(AllCounted(1), (Parent, Parachain(1000)))
- .build()
- )
- .export_message(
- Kusama,
- Parachain(1000),
- Xcm::builder_unsafe().clear_origin().build()
- )
- .build()
- )],
- ),],
- );
+ assert_eq!(XcmBridgeHubRouter::get_messages(), vec![]);
});
}
}
diff --git a/cumulus/pallets/parachain-system/src/lib.rs b/cumulus/pallets/parachain-system/src/lib.rs
index a4b505b98e5..98989a852b8 100644
--- a/cumulus/pallets/parachain-system/src/lib.rs
+++ b/cumulus/pallets/parachain-system/src/lib.rs
@@ -1627,7 +1627,11 @@ impl<T: Config> InspectMessageQueues for Pallet<T> {
.map(|encoded_message| VersionedXcm::<()>::decode(&mut &encoded_message[..]).unwrap())
.collect();
- vec![(VersionedLocation::V4(Parent.into()), messages)]
+ if messages.is_empty() {
+ vec![]
+ } else {
+ vec![(VersionedLocation::from(Location::parent()), messages)]
+ }
}
}
diff --git a/cumulus/parachains/integration-tests/emulated/common/src/macros.rs b/cumulus/parachains/integration-tests/emulated/common/src/macros.rs
index 578bca84ce5..68926b04bfe 100644
--- a/cumulus/parachains/integration-tests/emulated/common/src/macros.rs
+++ b/cumulus/parachains/integration-tests/emulated/common/src/macros.rs
@@ -403,3 +403,51 @@ macro_rules! test_chain_can_claim_assets {
}
};
}
+
+#[macro_export]
+macro_rules! test_dry_run_transfer_across_pk_bridge {
+ ( $sender_asset_hub:ty, $sender_bridge_hub:ty, $destination:expr ) => {
+ $crate::macros::paste::paste! {
+ use frame_support::{dispatch::RawOrigin, traits::fungible};
+ use sp_runtime::AccountId32;
+ use xcm::prelude::*;
+ use xcm_runtime_apis::dry_run::runtime_decl_for_dry_run_api::DryRunApiV1;
+
+ let who = AccountId32::new([1u8; 32]);
+ let transfer_amount = 10_000_000_000_000u128;
+ let initial_balance = transfer_amount * 10;
+
+ // Bridge setup.
+ $sender_asset_hub::force_xcm_version($destination, XCM_VERSION);
+ open_bridge_between_asset_hub_rococo_and_asset_hub_westend();
+
+ <$sender_asset_hub as TestExt>::execute_with(|| {
+ type Runtime = <$sender_asset_hub as Chain>::Runtime;
+ type RuntimeCall = <$sender_asset_hub as Chain>::RuntimeCall;
+ type OriginCaller = <$sender_asset_hub as Chain>::OriginCaller;
+ type Balances = <$sender_asset_hub as [<$sender_asset_hub Pallet>]>::Balances;
+
+ // Give some initial funds.
+ <Balances as fungible::Mutate<_>>::set_balance(&who, initial_balance);
+
+ let call = RuntimeCall::PolkadotXcm(pallet_xcm::Call::transfer_assets {
+ dest: Box::new(VersionedLocation::from($destination)),
+ beneficiary: Box::new(VersionedLocation::from(Junction::AccountId32 {
+ id: who.clone().into(),
+ network: None,
+ })),
+ assets: Box::new(VersionedAssets::from(vec![
+ (Parent, transfer_amount).into(),
+ ])),
+ fee_asset_item: 0,
+ weight_limit: Unlimited,
+ });
+ let result = Runtime::dry_run_call(OriginCaller::system(RawOrigin::Signed(who)), call).unwrap();
+ // We assert the dry run succeeds and sends only one message to the local bridge hub.
+ assert!(result.execution_result.is_ok());
+ assert_eq!(result.forwarded_xcms.len(), 1);
+ assert_eq!(result.forwarded_xcms[0].0, VersionedLocation::from(Location::new(1, [Parachain($sender_bridge_hub::para_id().into())])));
+ });
+ }
+ };
+}
diff --git a/cumulus/parachains/integration-tests/emulated/tests/bridges/bridge-hub-rococo/Cargo.toml b/cumulus/parachains/integration-tests/emulated/tests/bridges/bridge-hub-rococo/Cargo.toml
index 86ace7d564e..9f6fe78a33e 100644
--- a/cumulus/parachains/integration-tests/emulated/tests/bridges/bridge-hub-rococo/Cargo.toml
+++ b/cumulus/parachains/integration-tests/emulated/tests/bridges/bridge-hub-rococo/Cargo.toml
@@ -28,6 +28,7 @@ sp-runtime = { workspace = true }
xcm = { workspace = true }
pallet-xcm = { workspace = true }
xcm-executor = { workspace = true }
+xcm-runtime-apis = { workspace = true }
# Bridges
pallet-bridge-messages = { workspace = true }
diff --git a/cumulus/parachains/integration-tests/emulated/tests/bridges/bridge-hub-rococo/src/lib.rs b/cumulus/parachains/integration-tests/emulated/tests/bridges/bridge-hub-rococo/src/lib.rs
index e83b2807678..a542de16de5 100644
--- a/cumulus/parachains/integration-tests/emulated/tests/bridges/bridge-hub-rococo/src/lib.rs
+++ b/cumulus/parachains/integration-tests/emulated/tests/bridges/bridge-hub-rococo/src/lib.rs
@@ -33,8 +33,8 @@ mod imports {
pub use emulated_integration_tests_common::{
accounts::ALICE,
impls::Inspect,
- test_parachain_is_trusted_teleporter, test_parachain_is_trusted_teleporter_for_relay,
- test_relay_is_trusted_teleporter,
+ test_dry_run_transfer_across_pk_bridge, test_parachain_is_trusted_teleporter,
+ test_parachain_is_trusted_teleporter_for_relay, test_relay_is_trusted_teleporter,
xcm_emulator::{
assert_expected_events, bx, Chain, Parachain as Para, RelayChain as Relay, TestExt,
},
diff --git a/cumulus/parachains/integration-tests/emulated/tests/bridges/bridge-hub-rococo/src/tests/asset_transfers.rs b/cumulus/parachains/integration-tests/emulated/tests/bridges/bridge-hub-rococo/src/tests/asset_transfers.rs
index 11930798da4..f1e2a6dcca6 100644
--- a/cumulus/parachains/integration-tests/emulated/tests/bridges/bridge-hub-rococo/src/tests/asset_transfers.rs
+++ b/cumulus/parachains/integration-tests/emulated/tests/bridges/bridge-hub-rococo/src/tests/asset_transfers.rs
@@ -534,3 +534,12 @@ fn send_back_wnds_from_penpal_rococo_through_asset_hub_rococo_to_asset_hub_weste
assert!(receiver_wnds_after > receiver_wnds_before);
assert!(receiver_wnds_after <= receiver_wnds_before + amount);
}
+
+#[test]
+fn dry_run_transfer_to_westend_sends_xcm_to_bridge_hub() {
+ test_dry_run_transfer_across_pk_bridge!(
+ AssetHubRococo,
+ BridgeHubRococo,
+ asset_hub_westend_location()
+ );
+}
diff --git a/cumulus/parachains/integration-tests/emulated/tests/bridges/bridge-hub-westend/Cargo.toml b/cumulus/parachains/integration-tests/emulated/tests/bridges/bridge-hub-westend/Cargo.toml
index 44121cbfdaf..b87f25ac0f0 100644
--- a/cumulus/parachains/integration-tests/emulated/tests/bridges/bridge-hub-westend/Cargo.toml
+++ b/cumulus/parachains/integration-tests/emulated/tests/bridges/bridge-hub-westend/Cargo.toml
@@ -29,6 +29,7 @@ sp-runtime = { workspace = true }
xcm = { workspace = true }
pallet-xcm = { workspace = true }
xcm-executor = { workspace = true }
+xcm-runtime-apis = { workspace = true }
# Bridges
pallet-bridge-messages = { workspace = true }
diff --git a/cumulus/parachains/integration-tests/emulated/tests/bridges/bridge-hub-westend/src/lib.rs b/cumulus/parachains/integration-tests/emulated/tests/bridges/bridge-hub-westend/src/lib.rs
index d9b92cb11e9..9228700c019 100644
--- a/cumulus/parachains/integration-tests/emulated/tests/bridges/bridge-hub-westend/src/lib.rs
+++ b/cumulus/parachains/integration-tests/emulated/tests/bridges/bridge-hub-westend/src/lib.rs
@@ -32,8 +32,8 @@ mod imports {
pub use emulated_integration_tests_common::{
accounts::ALICE,
impls::Inspect,
- test_parachain_is_trusted_teleporter, test_parachain_is_trusted_teleporter_for_relay,
- test_relay_is_trusted_teleporter,
+ test_dry_run_transfer_across_pk_bridge, test_parachain_is_trusted_teleporter,
+ test_parachain_is_trusted_teleporter_for_relay, test_relay_is_trusted_teleporter,
xcm_emulator::{
assert_expected_events, bx, Chain, Parachain as Para, RelayChain as Relay, TestExt,
},
diff --git a/cumulus/parachains/integration-tests/emulated/tests/bridges/bridge-hub-westend/src/tests/asset_transfers.rs b/cumulus/parachains/integration-tests/emulated/tests/bridges/bridge-hub-westend/src/tests/asset_transfers.rs
index 6492c520234..7def637a66e 100644
--- a/cumulus/parachains/integration-tests/emulated/tests/bridges/bridge-hub-westend/src/tests/asset_transfers.rs
+++ b/cumulus/parachains/integration-tests/emulated/tests/bridges/bridge-hub-westend/src/tests/asset_transfers.rs
@@ -554,3 +554,12 @@ fn send_back_rocs_from_penpal_westend_through_asset_hub_westend_to_asset_hub_roc
assert!(receiver_rocs_after > receiver_rocs_before);
assert!(receiver_rocs_after <= receiver_rocs_before + amount);
}
+
+#[test]
+fn dry_run_transfer_to_rococo_sends_xcm_to_bridge_hub() {
+ test_dry_run_transfer_across_pk_bridge!(
+ AssetHubWestend,
+ BridgeHubWestend,
+ asset_hub_rococo_location()
+ );
+}
diff --git a/polkadot/xcm/xcm-builder/src/universal_exports.rs b/polkadot/xcm/xcm-builder/src/universal_exports.rs
index 30e0b7c72b0..5c754f01ec0 100644
--- a/polkadot/xcm/xcm-builder/src/universal_exports.rs
+++ b/polkadot/xcm/xcm-builder/src/universal_exports.rs
@@ -337,15 +337,15 @@ impl<Bridges: ExporterFor, Router: SendXcm, UniversalLocation: Get<InteriorLocat
}
}
-impl<Bridges, Router: InspectMessageQueues, UniversalLocation> InspectMessageQueues
+impl<Bridges, Router, UniversalLocation> InspectMessageQueues
for SovereignPaidRemoteExporter<Bridges, Router, UniversalLocation>
{
- fn clear_messages() {
- Router::clear_messages()
- }
+ fn clear_messages() {}
+ /// This router needs to implement `InspectMessageQueues` but doesn't have to
+ /// return any messages, since it just reuses the `XcmpQueue` router.
fn get_messages() -> Vec<(VersionedLocation, Vec<VersionedXcm<()>>)> {
- Router::get_messages()
+ Vec::new()
}
}
diff --git a/prdoc/pr_5913.prdoc b/prdoc/pr_5913.prdoc
new file mode 100644
index 00000000000..f50cd722c71
--- /dev/null
+++ b/prdoc/pr_5913.prdoc
@@ -0,0 +1,20 @@
+# Schema: Polkadot SDK PRDoc Schema (prdoc) v1.0.0
+# See doc at https://raw.githubusercontent.com/paritytech/polkadot-sdk/master/prdoc/schema_user.json
+
+title: Remove redundant XCMs from dry run's forwarded xcms
+
+doc:
+ - audience: Runtime User
+ description: |
+ The DryRunApi was returning the same message repeated multiple times in the
+ `forwarded_xcms` field. This is no longer the case.
+
+crates:
+ - name: pallet-xcm-bridge-hub-router
+ bump: patch
+ - name: cumulus-pallet-parachain-system
+ bump: patch
+ - name: staging-xcm-builder
+ bump: patch
+ - name: emulated-integration-tests-common
+ bump: minor
--
GitLab