Refactoring XcmExportFeeToSibling and more tests for outbound fee

parachain/pallets/outbound-queue/src/send_message_impl.rs

@@ -15,6 +15,7 @@ use snowbridge_core::{
 	},
 	ChannelId, PRIMARY_GOVERNANCE_CHANNEL,
 };
+use sp_arithmetic::traits::Zero;
 use sp_core::H256;
 use sp_runtime::BoundedVec;
 
@@ -57,7 +58,14 @@ where
 			.unwrap_or_else(|| unique((message.channel_id, &message.command)).into());
 
 		let gas_used_at_most = T::GasMeter::maximum_gas_used_at_most(&message.command);
+		ensure!(
+			gas_used_at_most > T::GasMeter::MAXIMUM_BASE_GAS &&
+				gas_used_at_most < T::GasMeter::MAXIMUM_GAS_CAP,
+			SendError::MessageInvalidGasFees
+		);
 		let fee = Self::calculate_fee(gas_used_at_most, T::PricingParameters::get());
+		let zero_fee = <Self as SendMessageFeeProvider>::Balance::zero();
+		ensure!(fee.local > zero_fee && fee.remote > zero_fee, SendError::MessageInvalidGasFees);
 
 		let queued_message: VersionedQueuedMessage = QueuedMessage {
 			id: message_id,

parachain/pallets/outbound-queue/src/test.rs

@@ -11,7 +11,8 @@ use frame_support::{
 use codec::Encode;
 use snowbridge_core::{
 	outbound::{Command, SendError, SendMessage},
-	ParaId,
+	pricing::InvalidPricingParameters,
+	ParaId, PricingParameters, Rewards,
 };
 use sp_arithmetic::FixedU128;
 use sp_core::H256;
@@ -266,3 +267,49 @@ 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<<Test as Config>::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_exchange_rate_invalid() {
+	new_tester().execute_with(|| {
+		let gas_used: u64 = 250000;
+		let illegal_price_params: PricingParameters<<Test as Config>::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);
+		// assert the pricing params as illegal as expected
+		assert_err!(illegal_price_params.validate(), InvalidPricingParameters)
+	});
+}

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::<T>::InvalidUpgradeParameters
+			);
+
 			let initializer_params_hash: Option<H256> =
 				initializer.as_ref().map(|i| H256::from(blake2_256(i.params.as_ref())));
 			let command = Command::Upgrade { impl_address, impl_code_hash, initializer };

parachain/pallets/system/src/tests.rs

@@ -3,9 +3,9 @@
 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};
+use sp_runtime::{AccountId32, DispatchError::BadOrigin, FixedU128, TokenError};
 
 #[test]
 fn create_agent() {
@@ -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<Initializer> =
 			Some(Initializer { params: [0; 256].into(), maximum_required_gas: 10000 });
 		assert_ok!(EthereumSystem::upgrade(origin, address, code_hash, initializer));
@@ -202,6 +202,22 @@ fn set_pricing_parameters_invalid() {
 		);
 		params = Parameters::get();
 		params.rewards.remote = sp_core::U256::zero();
+		assert_noop!(
+			EthereumSystem::set_pricing_parameters(origin.clone(), params),
+			Error::<Test>::InvalidPricingParameters
+		);
+
+		// Invalid exchange rate with DOT expensive than Ether
+		params = Parameters::get();
+		params.exchange_rate = FixedU128::from_rational(2, 1);
+		assert_noop!(
+			EthereumSystem::set_pricing_parameters(origin.clone(), params),
+			Error::<Test>::InvalidPricingParameters
+		);
+
+		// Invalid gas fee too cheap
+		params = Parameters::get();
+		params.fee_per_gas = 1_u128.into();
 		assert_noop!(
 			EthereumSystem::set_pricing_parameters(origin, params),
 			Error::<Test>::InvalidPricingParameters
@@ -551,22 +567,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::<Test>(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 +636,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<Initializer> =
 			Some(Initializer { params: [0; 256].into(), maximum_required_gas: 10000 });
 		assert_ok!(EthereumSystem::upgrade(origin, address, code_hash, initializer.clone()));

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<MultiLocation> for AllowSiblingsOnly {
 	fn contains(location: &MultiLocation) -> bool {

parachain/primitives/core/src/outbound.rs

@@ -339,13 +339,18 @@ pub enum SendError {
 	Halted,
 	/// Invalid Channel
 	InvalidChannel,
+	/// Message with gas or fees calculated invalid
+	MessageInvalidGasFees,
 }
 
 pub trait GasMeter {
 	/// All the gas used for submitting a message to Ethereum, minus the cost of dispatching
 	/// the command within the message
 	const MAXIMUM_BASE_GAS: u64;
 
+	/// Hard limit for the maximum gas allowed among all commands
+	const MAXIMUM_GAS_CAP: u64;
+
 	fn maximum_gas_used_at_most(command: &Command) -> u64 {
 		Self::MAXIMUM_BASE_GAS + Self::maximum_dispatch_gas_used_at_most(command)
 	}
@@ -371,6 +376,8 @@ impl GasMeter for ConstantGasMeter {
 	// for message verification
 	const MAXIMUM_BASE_GAS: u64 = 185_000;
 
+	const MAXIMUM_GAS_CAP: u64 = 5_000_000;
+
 	fn maximum_dispatch_gas_used_at_most(command: &Command) -> u64 {
 		match command {
 			Command::CreateAgent { .. } => 275_000,
@@ -405,6 +412,8 @@ impl GasMeter for ConstantGasMeter {
 impl GasMeter for () {
 	const MAXIMUM_BASE_GAS: u64 = 1;
 
+	const MAXIMUM_GAS_CAP: u64 = 100;
+
 	fn maximum_dispatch_gas_used_at_most(_: &Command) -> u64 {
 		1
 	}

parachain/primitives/core/src/pricing.rs

@@ -1,6 +1,7 @@
+use crate::gwei;
 use codec::{Decode, Encode, MaxEncodedLen};
 use scale_info::TypeInfo;
-use sp_arithmetic::traits::{BaseArithmetic, Unsigned, Zero};
+use sp_arithmetic::traits::{BaseArithmetic, Unsigned};
 use sp_core::U256;
 use sp_runtime::{FixedU128, RuntimeDebug};
 use sp_std::prelude::*;
@@ -23,18 +24,20 @@ pub struct Rewards<Balance> {
 	pub remote: U256,
 }
 
-#[derive(RuntimeDebug)]
+#[derive(Clone, PartialEq, RuntimeDebug)]
 pub struct InvalidPricingParameters;
 
 impl<Balance> PricingParameters<Balance>
 where
 	Balance: BaseArithmetic + Unsigned + Copy,
 {
 	pub fn validate(&self) -> Result<(), InvalidPricingParameters> {
-		if self.exchange_rate == FixedU128::zero() {
+		if self.exchange_rate < FixedU128::from_rational(1, 10000) ||
+			self.exchange_rate > FixedU128::from_rational(1, 1)
+		{
 			return Err(InvalidPricingParameters)
 		}
-		if self.fee_per_gas == U256::zero() {
+		if self.fee_per_gas < gwei(1) || self.fee_per_gas > gwei(1000) {
 			return Err(InvalidPricingParameters)
 		}
 		if self.rewards.local.is_zero() {

parachain/primitives/router/src/outbound/mod.rs

@@ -113,7 +113,7 @@ where
 		// validate the message
 		let (ticket, fee) = OutboundQueue::validate(&outbound_message).map_err(|err| {
 			log::error!(target: "xcm::ethereum_blob_exporter", "OutboundQueue validation of message failed. {err:?}");
-			SendError::Unroutable
+			SendError::Fees
 		})?;
 
 		// convert fee to MultiAsset

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",