diff --git a/parachain/Cargo.lock b/parachain/Cargo.lock index 4720ee8148..2115b1b471 100644 --- a/parachain/Cargo.lock +++ b/parachain/Cargo.lock @@ -9173,6 +9173,7 @@ dependencies = [ "frame-support", "frame-system", "log", + "parity-scale-codec", "snowbridge-core", "sp-arithmetic", "staging-xcm", diff --git a/parachain/pallets/system/src/tests.rs b/parachain/pallets/system/src/tests.rs index e07481c1e3..e36cf6826e 100644 --- a/parachain/pallets/system/src/tests.rs +++ b/parachain/pallets/system/src/tests.rs @@ -3,7 +3,7 @@ use crate::{mock::*, *}; use frame_support::{assert_noop, assert_ok}; use hex_literal::hex; -use snowbridge_core::{eth, sibling_sovereign_account_raw}; +use snowbridge_core::eth; use sp_core::H256; use sp_runtime::{AccountId32, DispatchError::BadOrigin, TokenError}; @@ -551,22 +551,6 @@ fn force_transfer_native_from_agent_bad_origin() { // conversions for devops purposes. They need to be removed here and incorporated into a command // line utility. -#[ignore] -#[test] -fn check_sibling_sovereign_account() { - new_test_ext(true).execute_with(|| { - let para_id = 1001; - let sovereign_account = sibling_sovereign_account::(para_id.into()); - let sovereign_account_raw = sibling_sovereign_account_raw(para_id.into()); - println!( - "Sovereign account for parachain {}: {:#?}", - para_id, - hex::encode(sovereign_account.clone()) - ); - assert_eq!(sovereign_account, sovereign_account_raw.into()); - }); -} - #[test] fn charge_fee_for_create_agent() { new_test_ext(true).execute_with(|| { diff --git a/parachain/primitives/core/src/lib.rs b/parachain/primitives/core/src/lib.rs index ecbc3bb365..5a17aa3f0b 100644 --- a/parachain/primitives/core/src/lib.rs +++ b/parachain/primitives/core/src/lib.rs @@ -48,10 +48,6 @@ where SiblingParaId::from(para_id).into_account_truncating() } -pub fn sibling_sovereign_account_raw(para_id: ParaId) -> [u8; 32] { - SiblingParaId::from(para_id).into_account_truncating() -} - pub struct AllowSiblingsOnly; impl Contains for AllowSiblingsOnly { fn contains(location: &MultiLocation) -> bool { diff --git a/parachain/runtime/runtime-common/Cargo.toml b/parachain/runtime/runtime-common/Cargo.toml index d3a4e9e62f..ac907bd9ff 100644 --- a/parachain/runtime/runtime-common/Cargo.toml +++ b/parachain/runtime/runtime-common/Cargo.toml @@ -13,7 +13,7 @@ workspace = true [dependencies] log = { version = "0.4.20", default-features = false } - +codec = { package = "parity-scale-codec", version = "3.6.1", default-features = false } frame-support = { path = "../../../polkadot-sdk/substrate/frame/support", default-features = false } frame-system = { path = "../../../polkadot-sdk/substrate/frame/system", default-features = false } sp-arithmetic = { path = "../../../polkadot-sdk/substrate/primitives/arithmetic", default-features = false } @@ -28,6 +28,7 @@ snowbridge-core = { path = "../../primitives/core", default-features = false } [features] default = ["std"] std = [ + "codec/std", "frame-support/std", "frame-system/std", "log/std", diff --git a/parachain/runtime/runtime-common/src/lib.rs b/parachain/runtime/runtime-common/src/lib.rs index b7f54d262b..4dfd0fa79d 100644 --- a/parachain/runtime/runtime-common/src/lib.rs +++ b/parachain/runtime/runtime-common/src/lib.rs @@ -5,12 +5,16 @@ //! Common traits and types shared by runtimes. #![cfg_attr(not(feature = "std"), no_std)] +#[cfg(test)] +mod tests; + +use codec::FullCodec; use core::marker::PhantomData; -use frame_support::traits::Get; -use snowbridge_core::{outbound::SendMessageFeeProvider, sibling_sovereign_account_raw}; +use frame_support::{ensure, traits::Get}; +use snowbridge_core::outbound::SendMessageFeeProvider; use sp_arithmetic::traits::{BaseArithmetic, Unsigned}; use xcm::prelude::*; -use xcm_builder::{deposit_or_burn_fee, HandleFee}; +use xcm_builder::HandleFee; use xcm_executor::traits::{FeeReason, TransactAsset}; /// A `HandleFee` implementation that takes fees from `ExportMessage` XCM instructions @@ -45,7 +49,7 @@ impl where Balance: BaseArithmetic + Unsigned + Copy + From + Into, - AccountId: Clone + Into<[u8; 32]> + From<[u8; 32]>, + AccountId: Clone + FullCodec, FeeAssetLocation: Get, EthereumNetwork: Get, AssetTransactor: TransactAsset, @@ -55,7 +59,7 @@ impl, reason: FeeReason, - ) -> MultiAssets { + ) -> Result { let token_location = FeeAssetLocation::get(); // Check the reason to see if this export is for snowbridge. @@ -64,25 +68,24 @@ impl = if let Some(XcmContext { origin: Some(MultiLocation { parents: 1, interior }), .. }) = context { if let Some(Parachain(sibling_para_id)) = interior.first() { - let account: AccountId = - sibling_sovereign_account_raw((*sibling_para_id).into()).into(); - account + Some(*sibling_para_id) } else { - return fees + None } } else { - return fees + None }; + let para_id = maybe_para_id.ok_or(XcmError::InvalidLocation)?; // Get the total fee offered by export message. let maybe_total_supplied_fee: Option<(usize, Balance)> = fees @@ -98,32 +101,22 @@ impl Balance::one(), XcmError::FeesNotMet); + // Refund remote component of fee to physical origin + AssetTransactor::deposit_asset( + &MultiAsset { id: Concrete(token_location), fun: Fungible(remote_fee.into()) }, + &MultiLocation { parents: 1, interior: X1(Parachain(para_id)) }, + context, + )?; - if let Some((fee_index, total_fee)) = maybe_total_supplied_fee { - let remote_fee = total_fee.saturating_sub(FeeProvider::local_fee()); - if remote_fee > (0u128).into() { - // Refund remote component of fee to physical origin - deposit_or_burn_fee::( - MultiAsset { id: Concrete(token_location), fun: Fungible(remote_fee.into()) } - .into(), - context, - para_sovereign, - ); - // Return remaining fee to the next fee handler in the chain. - let mut modified_fees = fees.inner().clone(); - modified_fees.remove(fee_index); - modified_fees.push(MultiAsset { - id: Concrete(token_location), - fun: Fungible((total_fee - remote_fee).into()), - }); - return modified_fees.into() - } - } - - log::info!( - target: "xcm::fees", - "XcmExportFeeToSibling skipped: {fees:?}, context: {context:?}, reason: {reason:?}", - ); - fees + // Return remaining fee to the next fee handler in the chain. + let mut modified_fees = fees.inner().clone(); + modified_fees.remove(fee_index); + modified_fees + .push(MultiAsset { id: Concrete(token_location), fun: Fungible(local_fee.into()) }); + Ok(modified_fees.into()) } } diff --git a/parachain/runtime/runtime-common/src/tests.rs b/parachain/runtime/runtime-common/src/tests.rs new file mode 100644 index 0000000000..f8ee3e142a --- /dev/null +++ b/parachain/runtime/runtime-common/src/tests.rs @@ -0,0 +1,248 @@ +use crate::XcmExportFeeToSibling; +use frame_support::{parameter_types, sp_runtime::testing::H256}; +use snowbridge_core::outbound::{Fee, Message, SendError, SendMessage, SendMessageFeeProvider}; +use xcm::prelude::{ + Here, Kusama, MultiAsset, MultiAssets, MultiLocation, NetworkId, Parachain, XcmContext, + XcmError, XcmHash, XcmResult, X1, +}; +use xcm_builder::HandleFee; +use xcm_executor::{ + traits::{FeeReason, TransactAsset}, + Assets, +}; + +parameter_types! { + pub EthereumNetwork: NetworkId = NetworkId::Ethereum { chain_id: 11155111 }; + pub TokenLocation: MultiLocation = MultiLocation::parent(); +} + +struct MockOkOutboundQueue; +impl SendMessage for MockOkOutboundQueue { + type Ticket = (); + + fn validate(_: &Message) -> Result<(Self::Ticket, Fee), SendError> { + Ok(((), Fee { local: 1, remote: 1 })) + } + + fn deliver(_: Self::Ticket) -> Result { + Ok(H256::zero()) + } +} + +impl SendMessageFeeProvider for MockOkOutboundQueue { + type Balance = u128; + + fn local_fee() -> Self::Balance { + 1 + } +} +struct MockErrOutboundQueue; +impl SendMessage for MockErrOutboundQueue { + type Ticket = (); + + fn validate(_: &Message) -> Result<(Self::Ticket, Fee), SendError> { + Err(SendError::MessageTooLarge) + } + + fn deliver(_: Self::Ticket) -> Result { + Err(SendError::MessageTooLarge) + } +} + +impl SendMessageFeeProvider for MockErrOutboundQueue { + type Balance = u128; + + fn local_fee() -> Self::Balance { + 1 + } +} + +pub struct SuccessfulTransactor; +impl TransactAsset for SuccessfulTransactor { + fn can_check_in( + _origin: &MultiLocation, + _what: &MultiAsset, + _context: &XcmContext, + ) -> XcmResult { + Ok(()) + } + + fn can_check_out( + _dest: &MultiLocation, + _what: &MultiAsset, + _context: &XcmContext, + ) -> XcmResult { + Ok(()) + } + + fn deposit_asset( + _what: &MultiAsset, + _who: &MultiLocation, + _context: Option<&XcmContext>, + ) -> XcmResult { + Ok(()) + } + + fn withdraw_asset( + _what: &MultiAsset, + _who: &MultiLocation, + _context: Option<&XcmContext>, + ) -> Result { + Ok(Assets::default()) + } + + fn internal_transfer_asset( + _what: &MultiAsset, + _from: &MultiLocation, + _to: &MultiLocation, + _context: &XcmContext, + ) -> Result { + Ok(Assets::default()) + } +} + +pub struct NotFoundTransactor; +impl TransactAsset for NotFoundTransactor { + fn can_check_in( + _origin: &MultiLocation, + _what: &MultiAsset, + _context: &XcmContext, + ) -> XcmResult { + Err(XcmError::AssetNotFound) + } + + fn can_check_out( + _dest: &MultiLocation, + _what: &MultiAsset, + _context: &XcmContext, + ) -> XcmResult { + Err(XcmError::AssetNotFound) + } + + fn deposit_asset( + _what: &MultiAsset, + _who: &MultiLocation, + _context: Option<&XcmContext>, + ) -> XcmResult { + Err(XcmError::AssetNotFound) + } + + fn withdraw_asset( + _what: &MultiAsset, + _who: &MultiLocation, + _context: Option<&XcmContext>, + ) -> Result { + Err(XcmError::AssetNotFound) + } + + fn internal_transfer_asset( + _what: &MultiAsset, + _from: &MultiLocation, + _to: &MultiLocation, + _context: &XcmContext, + ) -> Result { + Err(XcmError::AssetNotFound) + } +} + +#[test] +fn handle_fee_success() { + let fee: MultiAssets = MultiAsset::from((MultiLocation::parent(), 10_u128)).into(); + let ctx = XcmContext { + origin: Some(MultiLocation { parents: 1, interior: X1(Parachain(1000)) }), + message_id: XcmHash::default(), + topic: None, + }; + let reason = FeeReason::Export { network: EthereumNetwork::get(), destination: Here }; + let result = XcmExportFeeToSibling::< + u128, + u64, + TokenLocation, + EthereumNetwork, + SuccessfulTransactor, + MockOkOutboundQueue, + >::handle_fee(fee, Some(&ctx), reason) + .unwrap(); + let local_fee = + MultiAsset::from((MultiLocation::parent(), MockOkOutboundQueue::local_fee())).into(); + // assert only local fee left + assert_eq!(result, local_fee) +} + +#[test] +fn handle_fee_success_but_not_for_ethereum() { + let fee: MultiAssets = MultiAsset::from((MultiLocation::parent(), 10_u128)).into(); + let ctx = XcmContext { origin: None, message_id: XcmHash::default(), topic: None }; + // invalid network not for ethereum + let reason = FeeReason::Export { network: Kusama, destination: Here }; + let result = XcmExportFeeToSibling::< + u128, + u64, + TokenLocation, + EthereumNetwork, + SuccessfulTransactor, + MockOkOutboundQueue, + >::handle_fee(fee.clone(), Some(&ctx), reason) + .unwrap(); + // assert fee not touched and just forward to the next handler + assert_eq!(result, fee) +} + +#[test] +fn handle_fee_fail_for_invalid_location() { + let fee: MultiAssets = MultiAsset::from((MultiLocation::parent(), 10_u128)).into(); + // invalid origin not from sibling chain + let ctx = XcmContext { origin: None, message_id: XcmHash::default(), topic: None }; + let reason = FeeReason::Export { network: EthereumNetwork::get(), destination: Here }; + let result = XcmExportFeeToSibling::< + u128, + u64, + TokenLocation, + EthereumNetwork, + SuccessfulTransactor, + MockOkOutboundQueue, + >::handle_fee(fee.clone(), Some(&ctx), reason); + assert_eq!(result, Err(XcmError::InvalidLocation)) +} + +#[test] +fn handle_fee_fail_for_fees_not_met() { + // insufficient fee not met + let fee: MultiAssets = MultiAsset::from((MultiLocation::parent(), 1_u128)).into(); + let ctx = XcmContext { + origin: Some(MultiLocation { parents: 1, interior: X1(Parachain(1000)) }), + message_id: XcmHash::default(), + topic: None, + }; + let reason = FeeReason::Export { network: EthereumNetwork::get(), destination: Here }; + let result = XcmExportFeeToSibling::< + u128, + u64, + TokenLocation, + EthereumNetwork, + SuccessfulTransactor, + MockOkOutboundQueue, + >::handle_fee(fee.clone(), Some(&ctx), reason); + assert_eq!(result, Err(XcmError::FeesNotMet)) +} + +#[test] +fn handle_fee_fail_for_transact() { + let fee: MultiAssets = MultiAsset::from((MultiLocation::parent(), 10_u128)).into(); + let ctx = XcmContext { + origin: Some(MultiLocation { parents: 1, interior: X1(Parachain(1000)) }), + message_id: XcmHash::default(), + topic: None, + }; + let reason = FeeReason::Export { network: EthereumNetwork::get(), destination: Here }; + let result = XcmExportFeeToSibling::< + u128, + u64, + TokenLocation, + EthereumNetwork, + // invalid transactor + NotFoundTransactor, + MockOkOutboundQueue, + >::handle_fee(fee.clone(), Some(&ctx), reason); + assert_eq!(result, Err(XcmError::AssetNotFound)) +} diff --git a/parachain/runtime/test-common/src/lib.rs b/parachain/runtime/test-common/src/lib.rs index 8a6834db2e..c1ba8a38b2 100644 --- a/parachain/runtime/test-common/src/lib.rs +++ b/parachain/runtime/test-common/src/lib.rs @@ -2,11 +2,15 @@ // SPDX-FileCopyrightText: 2023 Snowfork use codec::Encode; -use frame_support::{assert_err, assert_ok, traits::fungible::Mutate}; +use frame_support::{ + assert_err, assert_ok, + traits::fungible::{Inspect, Mutate}, +}; pub use parachains_runtimes_test_utils::test_cases::change_storage_constant_by_governance_works; use parachains_runtimes_test_utils::{ AccountIdOf, BalanceOf, CollatorSessionKeys, ExtBuilder, ValidatorIdOf, XcmReceivedFrom, }; +use snowbridge_core::PricingParameters; use sp_core::H160; use sp_runtime::SaturatedConversion; use xcm::{ @@ -18,6 +22,10 @@ use xcm_executor::XcmExecutor; type RuntimeHelper = parachains_runtimes_test_utils::RuntimeHelper; +type TokenBalanceOf = <::Token as Inspect< + ::AccountId, +>>::Balance; + pub fn initial_fund(assethub_parachain_id: u32, initial_amount: u128) where Runtime: frame_system::Config + pallet_balances::Config, @@ -292,3 +300,58 @@ pub fn send_transfer_token_message_failure( assert_err!(outcome.ensure_complete(), expected_error); }); } + +#[allow(clippy::too_many_arguments)] +pub fn send_transfer_token_message_failure_with_invalid_fee_params( + collator_session_key: CollatorSessionKeys, + runtime_para_id: u32, + assethub_parachain_id: u32, + initial_amount: u128, + weth_contract_address: H160, + destination_address: H160, + fee_amount: u128, + price_params: PricingParameters>, + expected_error: Error, +) where + Runtime: frame_system::Config + + pallet_balances::Config + + pallet_session::Config + + pallet_xcm::Config + + parachain_info::Config + + pallet_collator_selection::Config + + cumulus_pallet_parachain_system::Config + + snowbridge_pallet_outbound_queue::Config + + snowbridge_pallet_system::Config, + XcmConfig: xcm_executor::Config, + ValidatorIdOf: From>, +{ + ExtBuilder::::default() + .with_collators(collator_session_key.collators()) + .with_session_keys(collator_session_key.session_keys()) + .with_para_id(runtime_para_id.into()) + .with_tracing() + .build() + .execute_with(|| { + // initialize price params + snowbridge_pallet_system::PricingParameters::::put(price_params); + + // initialize agent and channel + >::initialize( + runtime_para_id.into(), + assethub_parachain_id.into(), + ) + .unwrap(); + + // fund asset hub sovereign account enough so it can pay fees + initial_fund::(assethub_parachain_id, initial_amount); + + let outcome = send_transfer_token_message::( + assethub_parachain_id, + weth_contract_address, + destination_address, + fee_amount, + ); + // check err is NotHoldingFees + assert_err!(outcome.ensure_complete(), expected_error); + }); +} diff --git a/polkadot-sdk b/polkadot-sdk index c23c50f2cb..fa290541ba 160000 --- a/polkadot-sdk +++ b/polkadot-sdk @@ -1 +1 @@ -Subproject commit c23c50f2cb4697885e1dd891da6fdd33036631a8 +Subproject commit fa290541baf4181cb464290332d87e7a25961575