diff --git a/parachain/Cargo.lock b/parachain/Cargo.lock index 4720ee8148..70f685f423 100644 --- a/parachain/Cargo.lock +++ b/parachain/Cargo.lock @@ -9173,8 +9173,10 @@ dependencies = [ "frame-support", "frame-system", "log", + "parity-scale-codec", "snowbridge-core", "sp-arithmetic", + "sp-std 8.0.0", "staging-xcm", "staging-xcm-builder", "staging-xcm-executor", diff --git a/parachain/pallets/outbound-queue/src/process_message_impl.rs b/parachain/pallets/outbound-queue/src/process_message_impl.rs index 575ed9e0e7..731aa6fa6d 100644 --- a/parachain/pallets/outbound-queue/src/process_message_impl.rs +++ b/parachain/pallets/outbound-queue/src/process_message_impl.rs @@ -1,3 +1,5 @@ +// SPDX-License-Identifier: Apache-2.0 +// SPDX-FileCopyrightText: 2023 Snowfork //! Implementation for [`frame_support::traits::ProcessMessage`] use super::*; use crate::weights::WeightInfo; diff --git a/parachain/pallets/outbound-queue/src/send_message_impl.rs b/parachain/pallets/outbound-queue/src/send_message_impl.rs index a84e2c520e..03be618199 100644 --- a/parachain/pallets/outbound-queue/src/send_message_impl.rs +++ b/parachain/pallets/outbound-queue/src/send_message_impl.rs @@ -1,3 +1,5 @@ +// SPDX-License-Identifier: Apache-2.0 +// SPDX-FileCopyrightText: 2023 Snowfork //! Implementation for [`snowbridge_core::outbound::SendMessage`] use super::*; use bridge_hub_common::AggregateMessageOrigin; diff --git a/parachain/pallets/outbound-queue/src/test.rs b/parachain/pallets/outbound-queue/src/test.rs index 0028d75e7b..8ed4a318d6 100644 --- a/parachain/pallets/outbound-queue/src/test.rs +++ b/parachain/pallets/outbound-queue/src/test.rs @@ -11,7 +11,7 @@ use frame_support::{ use codec::Encode; use snowbridge_core::{ outbound::{Command, SendError, SendMessage}, - ParaId, + ParaId, PricingParameters, Rewards, }; use sp_arithmetic::FixedU128; use sp_core::H256; @@ -266,3 +266,47 @@ fn encode_digest_item() { ); }); } + +#[test] +fn validate_messages_with_fees() { + new_tester().execute_with(|| { + let message = mock_message(1000); + let (_, fee) = OutboundQueue::validate(&message).unwrap(); + assert_eq!(fee.local, 698000000); + assert_eq!(fee.remote, 2680000000000); + }); +} + +#[test] +fn test_calculate_fees() { + new_tester().execute_with(|| { + let gas_used: u64 = 250000; + let illegal_price_params: PricingParameters<::Balance> = + PricingParameters { + exchange_rate: FixedU128::from_rational(1, 400), + fee_per_gas: 10000_u32.into(), + rewards: Rewards { local: 1_u32.into(), remote: 1_u32.into() }, + }; + let fee = OutboundQueue::calculate_fee(gas_used, illegal_price_params); + assert_eq!(fee.local, 698000000); + assert_eq!(fee.remote, 1000000); + }); +} + +#[test] +fn test_calculate_fees_with_valid_exchange_rate_but_remote_fee_calculated_as_zero() { + new_tester().execute_with(|| { + let gas_used: u64 = 250000; + let illegal_price_params: PricingParameters<::Balance> = + PricingParameters { + exchange_rate: FixedU128::from_rational(1, 1), + fee_per_gas: 1_u32.into(), + rewards: Rewards { local: 1_u32.into(), remote: 1_u32.into() }, + }; + let fee = OutboundQueue::calculate_fee(gas_used, illegal_price_params.clone()); + assert_eq!(fee.local, 698000000); + // Though none zero pricing params the remote fee calculated here is invalid + // which should be avoided + assert_eq!(fee.remote, 0); + }); +} diff --git a/parachain/pallets/outbound-queue/src/types.rs b/parachain/pallets/outbound-queue/src/types.rs index 28d400bb9d..f65a15d3d2 100644 --- a/parachain/pallets/outbound-queue/src/types.rs +++ b/parachain/pallets/outbound-queue/src/types.rs @@ -1,3 +1,5 @@ +// SPDX-License-Identifier: Apache-2.0 +// SPDX-FileCopyrightText: 2023 Snowfork use codec::{Decode, Encode}; use ethabi::Token; use frame_support::traits::ProcessMessage; diff --git a/parachain/pallets/system/src/lib.rs b/parachain/pallets/system/src/lib.rs index 0042093ee6..93cb9a4404 100644 --- a/parachain/pallets/system/src/lib.rs +++ b/parachain/pallets/system/src/lib.rs @@ -226,6 +226,7 @@ pub mod pallet { Send(SendError), InvalidTokenTransferFees, InvalidPricingParameters, + InvalidUpgradeParameters, } /// The set of registered agents @@ -282,6 +283,11 @@ pub mod pallet { ) -> DispatchResult { ensure_root(origin)?; + ensure!( + !impl_address.eq(&H160::zero()) && !impl_code_hash.eq(&H256::zero()), + Error::::InvalidUpgradeParameters + ); + let initializer_params_hash: Option = initializer.as_ref().map(|i| H256::from(blake2_256(i.params.as_ref()))); let command = Command::Upgrade { impl_address, impl_code_hash, initializer }; diff --git a/parachain/pallets/system/src/tests.rs b/parachain/pallets/system/src/tests.rs index e07481c1e3..b0d5f15d02 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}; @@ -87,8 +87,8 @@ fn create_agent_bad_origin() { fn upgrade_as_root() { new_test_ext(true).execute_with(|| { let origin = RuntimeOrigin::root(); - let address: H160 = Default::default(); - let code_hash: H256 = Default::default(); + let address: H160 = [1_u8; 20].into(); + let code_hash: H256 = [1_u8; 32].into(); assert_ok!(EthereumSystem::upgrade(origin, address, code_hash, None)); @@ -115,8 +115,8 @@ fn upgrade_as_signed_fails() { fn upgrade_with_params() { new_test_ext(true).execute_with(|| { let origin = RuntimeOrigin::root(); - let address: H160 = Default::default(); - let code_hash: H256 = Default::default(); + let address: H160 = [1_u8; 20].into(); + let code_hash: H256 = [1_u8; 32].into(); let initializer: Option = Some(Initializer { params: [0; 256].into(), maximum_required_gas: 10000 }); assert_ok!(EthereumSystem::upgrade(origin, address, code_hash, initializer)); @@ -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(|| { @@ -636,8 +620,8 @@ fn charge_fee_for_upgrade() { new_test_ext(true).execute_with(|| { let para_id: u32 = TestParaId::get(); let origin = RuntimeOrigin::root(); - let address: H160 = Default::default(); - let code_hash: H256 = Default::default(); + let address: H160 = [1_u8; 20].into(); + let code_hash: H256 = [1_u8; 32].into(); let initializer: Option = Some(Initializer { params: [0; 256].into(), maximum_required_gas: 10000 }); assert_ok!(EthereumSystem::upgrade(origin, address, code_hash, initializer.clone())); 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..08b9a8b229 100644 --- a/parachain/runtime/runtime-common/Cargo.toml +++ b/parachain/runtime/runtime-common/Cargo.toml @@ -13,9 +13,10 @@ 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-std = { path = "../../../polkadot-sdk/substrate/primitives/std", default-features = false } sp-arithmetic = { path = "../../../polkadot-sdk/substrate/primitives/arithmetic", default-features = false } xcm = { package = "staging-xcm", path = "../../../polkadot-sdk/polkadot/xcm", default-features = false } xcm-builder = { package = "staging-xcm-builder", path = "../../../polkadot-sdk/polkadot/xcm/xcm-builder", default-features = false } @@ -28,11 +29,13 @@ snowbridge-core = { path = "../../primitives/core", default-features = false } [features] default = ["std"] std = [ + "codec/std", "frame-support/std", "frame-system/std", "log/std", "snowbridge-core/std", "sp-arithmetic/std", + "sp-std/std", "xcm-builder/std", "xcm-executor/std", "xcm/std", diff --git a/parachain/runtime/runtime-common/src/lib.rs b/parachain/runtime/runtime-common/src/lib.rs index b7f54d262b..e909b301ab 100644 --- a/parachain/runtime/runtime-common/src/lib.rs +++ b/parachain/runtime/runtime-common/src/lib.rs @@ -5,14 +5,21 @@ //! 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 snowbridge_core::outbound::SendMessageFeeProvider; use sp_arithmetic::traits::{BaseArithmetic, Unsigned}; +use sp_std::fmt::Debug; use xcm::prelude::*; -use xcm_builder::{deposit_or_burn_fee, HandleFee}; +use xcm_builder::HandleFee; use xcm_executor::traits::{FeeReason, TransactAsset}; +pub const LOG_TARGET: &str = "xcm::export-fee-to-sibling"; + /// A `HandleFee` implementation that takes fees from `ExportMessage` XCM instructions /// to Snowbridge and splits off the remote fee and deposits it to the origin /// parachain sovereign account. The local fee is then returned back to be handled by @@ -44,8 +51,8 @@ impl where - Balance: BaseArithmetic + Unsigned + Copy + From + Into, - AccountId: Clone + Into<[u8; 32]> + From<[u8; 32]>, + Balance: BaseArithmetic + Unsigned + Copy + From + Into + Debug, + AccountId: Clone + FullCodec, FeeAssetLocation: Get, EthereumNetwork: Get, AssetTransactor: TransactAsset, @@ -68,21 +75,28 @@ 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 }; + if maybe_para_id.is_none() { + log::error!( + target: LOG_TARGET, + "invalid location in context {:?}", + context, + ); + return fees + } + let para_id = maybe_para_id.unwrap(); // Get the total fee offered by export message. let maybe_total_supplied_fee: Option<(usize, Balance)> = fees @@ -98,32 +112,46 @@ impl (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() - } + if maybe_total_supplied_fee.is_none() { + log::error!( + target: LOG_TARGET, + "could not find fee asset item in fees: {:?}", + fees, + ); + return fees } - - log::info!( - target: "xcm::fees", - "XcmExportFeeToSibling skipped: {fees:?}, context: {context:?}, reason: {reason:?}", + let (fee_index, total_fee) = maybe_total_supplied_fee.unwrap(); + let local_fee = FeeProvider::local_fee(); + let remote_fee = total_fee.saturating_sub(local_fee); + if local_fee == Balance::zero() || remote_fee == Balance::zero() { + log::error!( + target: LOG_TARGET, + "calculated refund incorrect with local_fee: {:?} and remote_fee: {:?}", + local_fee, + remote_fee, + ); + return fees + } + // Refund remote component of fee to physical origin + let result = AssetTransactor::deposit_asset( + &MultiAsset { id: Concrete(token_location), fun: Fungible(remote_fee.into()) }, + &MultiLocation { parents: 1, interior: X1(Parachain(para_id)) }, + context, ); - fees + if result.is_err() { + log::error!( + target: LOG_TARGET, + "transact fee asset failed: {:?}", + result.unwrap_err() + ); + return 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()) }); + 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..39c0ddbc75 --- /dev/null +++ b/parachain/runtime/runtime-common/src/tests.rs @@ -0,0 +1,181 @@ +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 MockAssetTransactor; +impl TransactAsset for MockAssetTransactor { + 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()) + } +} + +#[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, + MockAssetTransactor, + MockOkOutboundQueue, + >::handle_fee(fee, Some(&ctx), reason); + 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, + MockAssetTransactor, + MockOkOutboundQueue, + >::handle_fee(fee.clone(), Some(&ctx), reason); + // assert fee not touched and just forward to the next handler + assert_eq!(result, fee) +} + +#[test] +fn handle_fee_success_even_from_an_invalid_none_origin_location() { + let fee: MultiAssets = MultiAsset::from((MultiLocation::parent(), 10_u128)).into(); + // invalid origin None here not from a 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, + MockAssetTransactor, + MockOkOutboundQueue, + >::handle_fee(fee.clone(), Some(&ctx), reason); + assert_eq!(result, fee) +} + +#[test] +fn handle_fee_success_even_when_fee_insufficient() { + // insufficient fee not cover the (local_fee + remote_fee) required + 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, + MockAssetTransactor, + MockOkOutboundQueue, + >::handle_fee(fee.clone(), Some(&ctx), reason); + assert_eq!(result, fee) +} diff --git a/parachain/runtime/test-common/src/lib.rs b/parachain/runtime/test-common/src/lib.rs index 8a6834db2e..1feceaf13a 100644 --- a/parachain/runtime/test-common/src/lib.rs +++ b/parachain/runtime/test-common/src/lib.rs @@ -288,7 +288,6 @@ pub fn send_transfer_token_message_failure( destination_address, fee_amount, ); - // check err is NotHoldingFees assert_err!(outcome.ensure_complete(), expected_error); }); } diff --git a/polkadot-sdk b/polkadot-sdk index d0e0b38618..988945bb4b 160000 --- a/polkadot-sdk +++ b/polkadot-sdk @@ -1 +1 @@ -Subproject commit d0e0b386187b5958b0d6e7ae9aadec9e0b88388e +Subproject commit 988945bb4b7e3b29bf4bc3f239444414afd0be1d