From bbaa1046fdba5b596a89a7fadd557fcc0aab0c4d Mon Sep 17 00:00:00 2001 From: Branislav Kontur Date: Thu, 26 Oct 2023 15:36:05 +0200 Subject: [PATCH] Removed TODO from test-case for hard-coded delivery fee estimation (#2042) Co-authored-by: Adrian Catangiu --- .../assets/asset-hub-rococo/tests/tests.rs | 38 +-- .../runtimes/assets/test-utils/src/lib.rs | 44 ++++ .../test-utils/src/test_cases_over_bridge.rs | 222 ++++++++++-------- 3 files changed, 164 insertions(+), 140 deletions(-) diff --git a/cumulus/parachains/runtimes/assets/asset-hub-rococo/tests/tests.rs b/cumulus/parachains/runtimes/assets/asset-hub-rococo/tests/tests.rs index a763382f90541..b93315cc39d87 100644 --- a/cumulus/parachains/runtimes/assets/asset-hub-rococo/tests/tests.rs +++ b/cumulus/parachains/runtimes/assets/asset-hub-rococo/tests/tests.rs @@ -683,6 +683,7 @@ mod asset_hub_rococo_tests { bridging_to_asset_hub_wococo, WeightLimit::Unlimited, Some(xcm_config::bridging::XcmBridgeHubRouterFeeAssetId::get()), + Some(xcm_config::TreasuryAccount::get().unwrap()), ) } @@ -717,29 +718,11 @@ mod asset_hub_rococo_tests { Runtime, AllPalletsWithoutSystem, XcmConfig, - ParachainSystem, - XcmpQueue, LocationToAccountId, ToWococoXcmRouterInstance, >( collator_session_keys(), - ExistentialDeposit::get(), - AccountId::from(ALICE), - Box::new(|runtime_event_encoded: Vec| { - match RuntimeEvent::decode(&mut &runtime_event_encoded[..]) { - Ok(RuntimeEvent::PolkadotXcm(event)) => Some(event), - _ => None, - } - }), - Box::new(|runtime_event_encoded: Vec| { - match RuntimeEvent::decode(&mut &runtime_event_encoded[..]) { - Ok(RuntimeEvent::XcmpQueue(event)) => Some(event), - _ => None, - } - }), bridging_to_asset_hub_wococo, - WeightLimit::Unlimited, - Some(xcm_config::bridging::XcmBridgeHubRouterFeeAssetId::get()), || { sp_std::vec![ UnpaidExecution { weight_limit: Unlimited, check_origin: None }, @@ -888,6 +871,7 @@ mod asset_hub_wococo_tests { with_wococo_flavor_bridging_to_asset_hub_rococo, WeightLimit::Unlimited, Some(xcm_config::bridging::XcmBridgeHubRouterFeeAssetId::get()), + Some(xcm_config::TreasuryAccount::get().unwrap()), ) } @@ -922,29 +906,11 @@ mod asset_hub_wococo_tests { Runtime, AllPalletsWithoutSystem, XcmConfig, - ParachainSystem, - XcmpQueue, LocationToAccountId, ToRococoXcmRouterInstance, >( collator_session_keys(), - ExistentialDeposit::get(), - AccountId::from(ALICE), - Box::new(|runtime_event_encoded: Vec| { - match RuntimeEvent::decode(&mut &runtime_event_encoded[..]) { - Ok(RuntimeEvent::PolkadotXcm(event)) => Some(event), - _ => None, - } - }), - Box::new(|runtime_event_encoded: Vec| { - match RuntimeEvent::decode(&mut &runtime_event_encoded[..]) { - Ok(RuntimeEvent::XcmpQueue(event)) => Some(event), - _ => None, - } - }), with_wococo_flavor_bridging_to_asset_hub_rococo, - WeightLimit::Unlimited, - Some(xcm_config::bridging::XcmBridgeHubRouterFeeAssetId::get()), || { sp_std::vec![ UnpaidExecution { weight_limit: Unlimited, check_origin: None }, diff --git a/cumulus/parachains/runtimes/assets/test-utils/src/lib.rs b/cumulus/parachains/runtimes/assets/test-utils/src/lib.rs index e0f05fa7b0a4c..471b1f09b567e 100644 --- a/cumulus/parachains/runtimes/assets/test-utils/src/lib.rs +++ b/cumulus/parachains/runtimes/assets/test-utils/src/lib.rs @@ -19,4 +19,48 @@ pub mod test_cases; pub mod test_cases_over_bridge; pub mod xcm_helpers; + +use frame_support::traits::ProcessMessageError; pub use parachains_runtimes_test_utils::*; +use std::fmt::Debug; + +use xcm::latest::prelude::*; +use xcm_builder::{CreateMatcher, MatchXcm}; + +/// Helper function to verify `xcm` contains all relevant instructions expected on destination +/// chain as part of a reserve-asset-transfer. +pub(crate) fn assert_matches_reserve_asset_deposited_instructions( + xcm: &mut Xcm, + expected_reserve_assets_deposited: &MultiAssets, + expected_beneficiary: &MultiLocation, +) { + let _ = xcm + .0 + .matcher() + .skip_inst_while(|inst| !matches!(inst, ReserveAssetDeposited(..))) + .expect("no instruction ReserveAssetDeposited?") + .match_next_inst(|instr| match instr { + ReserveAssetDeposited(reserve_assets) => { + assert_eq!(reserve_assets, expected_reserve_assets_deposited); + Ok(()) + }, + _ => Err(ProcessMessageError::BadFormat), + }) + .expect("expected instruction ReserveAssetDeposited") + .match_next_inst(|instr| match instr { + ClearOrigin => Ok(()), + _ => Err(ProcessMessageError::BadFormat), + }) + .expect("expected instruction ClearOrigin") + .match_next_inst(|instr| match instr { + BuyExecution { .. } => Ok(()), + _ => Err(ProcessMessageError::BadFormat), + }) + .expect("expected instruction BuyExecution") + .match_next_inst(|instr| match instr { + DepositAsset { assets: _, beneficiary } if beneficiary == expected_beneficiary => + Ok(()), + _ => Err(ProcessMessageError::BadFormat), + }) + .expect("expected instruction DepositAsset"); +} diff --git a/cumulus/parachains/runtimes/assets/test-utils/src/test_cases_over_bridge.rs b/cumulus/parachains/runtimes/assets/test-utils/src/test_cases_over_bridge.rs index 8cc5f81f49729..6c8ac8c6452b3 100644 --- a/cumulus/parachains/runtimes/assets/test-utils/src/test_cases_over_bridge.rs +++ b/cumulus/parachains/runtimes/assets/test-utils/src/test_cases_over_bridge.rs @@ -16,13 +16,12 @@ //! Module contains predefined test-case scenarios for `Runtime` with various assets transferred //! over a bridge. +use crate::assert_matches_reserve_asset_deposited_instructions; use codec::Encode; use cumulus_primitives_core::XcmpMessageSource; use frame_support::{ assert_ok, - traits::{ - fungible::Mutate, Currency, OnFinalize, OnInitialize, OriginTrait, ProcessMessageError, - }, + traits::{Currency, Get, OnFinalize, OnInitialize, OriginTrait, ProcessMessageError}, }; use frame_system::pallet_prelude::BlockNumberFor; use parachains_common::{AccountId, Balance}; @@ -30,10 +29,13 @@ use parachains_runtimes_test_utils::{ mock_open_hrmp_channel, AccountIdOf, BalanceOf, CollatorSessionKeys, ExtBuilder, RuntimeHelper, ValidatorIdOf, XcmReceivedFrom, }; -use sp_runtime::traits::StaticLookup; +use sp_runtime::{traits::StaticLookup, Saturating}; use xcm::{latest::prelude::*, VersionedMultiAssets}; use xcm_builder::{CreateMatcher, MatchXcm}; -use xcm_executor::{traits::ConvertLocation, XcmExecutor}; +use xcm_executor::{ + traits::{ConvertLocation, TransactAsset}, + XcmExecutor, +}; pub struct TestBridgingConfig { pub bridged_network: NetworkId, @@ -61,6 +63,7 @@ pub fn limited_reserve_transfer_assets_for_native_asset_works< prepare_configuration: fn() -> TestBridgingConfig, weight_limit: WeightLimit, maybe_paid_export_message: Option, + delivery_fees_account: Option>, ) where Runtime: frame_system::Config + pallet_balances::Config @@ -151,6 +154,11 @@ pub fn limited_reserve_transfer_assets_for_native_asset_works< existential_deposit ); + let delivery_fees_account_balance_before = delivery_fees_account + .as_ref() + .map(|dfa| >::free_balance(dfa)) + .unwrap_or(0.into()); + // local native asset (pallet_balances) let asset_to_transfer = MultiAsset { fun: Fungible(balance_to_transfer.into()), @@ -166,22 +174,76 @@ pub fn limited_reserve_transfer_assets_for_native_asset_works< }), }; - // Make sure sender has enough funds for paying delivery fees - // TODO: Get this fee via weighing the corresponding message - let delivery_fees = 1324039894; - >::mint_into(&alice_account, delivery_fees.into()) + let assets_to_transfer = MultiAssets::from(asset_to_transfer); + let mut expected_assets = assets_to_transfer.clone(); + let context = XcmConfig::UniversalLocation::get(); + expected_assets + .reanchor(&target_location_from_different_consensus, context) .unwrap(); + let expected_beneficiary = target_destination_account; + + // Make sure sender has enough funds for paying delivery fees + let handling_delivery_fees = { + // Probable XCM with `ReserveAssetDeposited`. + let mut expected_reserve_asset_deposited_message = Xcm(vec![ + ReserveAssetDeposited(MultiAssets::from(expected_assets.clone())), + ClearOrigin, + BuyExecution { + fees: MultiAsset { + id: Concrete(Default::default()), + fun: Fungible(balance_to_transfer), + }, + weight_limit: Unlimited, + }, + DepositAsset { assets: Wild(AllCounted(1)), beneficiary: expected_beneficiary }, + SetTopic([ + 220, 188, 144, 32, 213, 83, 111, 175, 44, 210, 111, 19, 90, 165, 191, 112, + 140, 247, 192, 124, 42, 17, 153, 141, 114, 34, 189, 20, 83, 69, 237, 173, + ]), + ]); + assert_matches_reserve_asset_deposited_instructions( + &mut expected_reserve_asset_deposited_message, + &expected_assets, + &expected_beneficiary, + ); + + // Call `SendXcm::validate` to get delivery fees. + let (_, delivery_fees): (_, MultiAssets) = XcmConfig::XcmSender::validate( + &mut Some(target_location_from_different_consensus), + &mut Some(expected_reserve_asset_deposited_message), + ) + .expect("validate passes"); + // Drip delivery fee to Alice account. + let mut delivery_fees_added = false; + for delivery_fee in delivery_fees.inner() { + assert_ok!(::deposit_asset( + &delivery_fee, + &MultiLocation { + parents: 0, + interior: X1(AccountId32 { + network: None, + id: alice_account.clone().into(), + }), + }, + None, + )); + delivery_fees_added = true; + } + delivery_fees_added + }; + // do pallet_xcm call reserve transfer assert_ok!(>::limited_reserve_transfer_assets( RuntimeHelper::::origin_of(alice_account.clone()), Box::new(target_location_from_different_consensus.into_versioned()), Box::new(target_destination_account.into_versioned()), - Box::new(VersionedMultiAssets::from(MultiAssets::from(asset_to_transfer))), + Box::new(VersionedMultiAssets::from(assets_to_transfer)), 0, weight_limit, )); + // check events // check pallet_xcm attempted RuntimeHelper::::assert_pallet_xcm_event_outcome( &unwrap_pallet_xcm_event, @@ -190,20 +252,6 @@ pub fn limited_reserve_transfer_assets_for_native_asset_works< }, ); - // check alice account decreased by balance_to_transfer - // TODO:check-parameter: change and assert in tests when (https://github.com/paritytech/polkadot-sdk/pull/1234) merged - assert_eq!( - >::free_balance(&alice_account), - alice_account_init_balance - balance_to_transfer.into() - ); - - // check reserve account - // check reserve account increased by balance_to_transfer - assert_eq!( - >::free_balance(&reserve_account), - existential_deposit + balance_to_transfer.into() - ); - // check that xcm was sent let xcm_sent_message_hash = >::events() .into_iter() @@ -219,7 +267,6 @@ pub fn limited_reserve_transfer_assets_for_native_asset_works< local_bridge_hub_para_id.into(), ) .unwrap(); - assert_eq!( xcm_sent_message_hash, Some(xcm_sent.using_encoded(sp_io::hashing::blake2_256)) @@ -268,12 +315,41 @@ pub fn limited_reserve_transfer_assets_for_native_asset_works< .split_global() .expect("split works"); assert_eq!(destination, &target_location_junctions_without_global_consensus); - assert_matches_pallet_xcm_reserve_transfer_assets_instructions(inner_xcm); + assert_matches_reserve_asset_deposited_instructions( + inner_xcm, + &expected_assets, + &expected_beneficiary, + ); Ok(()) }, _ => Err(ProcessMessageError::BadFormat), }) .expect("contains ExportMessage"); + + // check alice account decreased by balance_to_transfer + assert_eq!( + >::free_balance(&alice_account), + alice_account_init_balance + .saturating_sub(existential_deposit) + .saturating_sub(balance_to_transfer.into()) + ); + + // check reserve account increased by balance_to_transfer + assert_eq!( + >::free_balance(&reserve_account), + existential_deposit + balance_to_transfer.into() + ); + + // check dedicated account increased by delivery fees (if configured) + if handling_delivery_fees { + if let Some(delivery_fees_account) = delivery_fees_account { + let delivery_fees_account_balance_after = + >::free_balance(&delivery_fees_account); + assert!( + delivery_fees_account_balance_after > delivery_fees_account_balance_before + ); + } + } }) } @@ -405,15 +481,21 @@ pub fn receive_reserve_asset_deposited_from_different_consensus_works< 0.into() ); + let expected_assets = MultiAssets::from(vec![MultiAsset { + id: Concrete(foreign_asset_id_multilocation), + fun: Fungible(transfered_foreign_asset_id_amount), + }]); + let expected_beneficiary = MultiLocation { + parents: 0, + interior: X1(AccountId32 { network: None, id: target_account.clone().into() }), + }; + // Call received XCM execution let xcm = Xcm(vec![ DescendOrigin(bridge_instance), UniversalOrigin(universal_origin), DescendOrigin(descend_origin), - ReserveAssetDeposited(MultiAssets::from(vec![MultiAsset { - id: Concrete(foreign_asset_id_multilocation), - fun: Fungible(transfered_foreign_asset_id_amount), - }])), + ReserveAssetDeposited(expected_assets.clone()), ClearOrigin, BuyExecution { fees: MultiAsset { @@ -422,22 +504,17 @@ pub fn receive_reserve_asset_deposited_from_different_consensus_works< }, weight_limit: Unlimited, }, - DepositAsset { - assets: Wild(AllCounted(1)), - beneficiary: MultiLocation { - parents: 0, - interior: X1(AccountId32 { - network: None, - id: target_account.clone().into(), - }), - }, - }, + DepositAsset { assets: Wild(AllCounted(1)), beneficiary: expected_beneficiary }, SetTopic([ 220, 188, 144, 32, 213, 83, 111, 175, 44, 210, 111, 19, 90, 165, 191, 112, 140, 247, 192, 124, 42, 17, 153, 141, 114, 34, 189, 20, 83, 69, 237, 173, ]), ]); - assert_matches_pallet_xcm_reserve_transfer_assets_instructions(&mut xcm.clone()); + assert_matches_reserve_asset_deposited_instructions( + &mut xcm.clone(), + &expected_assets, + &expected_beneficiary, + ); let hash = xcm.using_encoded(sp_io::hashing::blake2_256); @@ -498,55 +575,15 @@ pub fn receive_reserve_asset_deposited_from_different_consensus_works< }) } -fn assert_matches_pallet_xcm_reserve_transfer_assets_instructions( - xcm: &mut Xcm, -) { - let _ = xcm - .0 - .matcher() - .skip_inst_while(|inst| !matches!(inst, ReserveAssetDeposited(..))) - .expect("no instruction ReserveAssetDeposited?") - .match_next_inst(|instr| match instr { - ReserveAssetDeposited(..) => Ok(()), - _ => Err(ProcessMessageError::BadFormat), - }) - .expect("expected instruction ReserveAssetDeposited") - .match_next_inst(|instr| match instr { - ClearOrigin => Ok(()), - _ => Err(ProcessMessageError::BadFormat), - }) - .expect("expected instruction ClearOrigin") - .match_next_inst(|instr| match instr { - BuyExecution { .. } => Ok(()), - _ => Err(ProcessMessageError::BadFormat), - }) - .expect("expected instruction BuyExecution") - .match_next_inst(|instr| match instr { - DepositAsset { .. } => Ok(()), - _ => Err(ProcessMessageError::BadFormat), - }) - .expect("expected instruction DepositAsset"); -} - pub fn report_bridge_status_from_xcm_bridge_router_works< Runtime, AllPalletsWithoutSystem, XcmConfig, - HrmpChannelOpener, - HrmpChannelSource, LocationToAccountId, XcmBridgeHubRouterInstance, >( collator_session_keys: CollatorSessionKeys, - existential_deposit: BalanceOf, - alice_account: AccountIdOf, - unwrap_pallet_xcm_event: Box) -> Option>>, - unwrap_xcmp_queue_event: Box< - dyn Fn(Vec) -> Option>, - >, prepare_configuration: fn() -> TestBridgingConfig, - weight_limit: WeightLimit, - maybe_paid_export_message: Option, congested_message: fn() -> Xcm, uncongested_message: fn() -> Xcm, ) where @@ -572,10 +609,6 @@ pub fn report_bridge_status_from_xcm_bridge_router_works< <::Lookup as StaticLookup>::Source: From<::AccountId>, ::AccountId: From, - HrmpChannelOpener: frame_support::inherent::ProvideInherent< - Call = cumulus_pallet_parachain_system::Call, - >, - HrmpChannelSource: XcmpMessageSource, XcmBridgeHubRouterInstance: 'static, { ExtBuilder::::default() @@ -584,25 +617,6 @@ pub fn report_bridge_status_from_xcm_bridge_router_works< .with_tracing() .build() .execute_with(|| { - // check transfer works - limited_reserve_transfer_assets_for_native_asset_works::< - Runtime, - AllPalletsWithoutSystem, - XcmConfig, - HrmpChannelOpener, - HrmpChannelSource, - LocationToAccountId, - >( - collator_session_keys, - existential_deposit, - alice_account, - unwrap_pallet_xcm_event, - unwrap_xcmp_queue_event, - prepare_configuration, - weight_limit, - maybe_paid_export_message, - ); - let report_bridge_status = |is_congested: bool| { // prepare bridge config let TestBridgingConfig { local_bridge_hub_location, .. } = prepare_configuration();