Handle fee with error instead of silently failing

cumulus/parachains/runtimes/bridge-hubs/bridge-hub-rococo/src/xcm_config.rs

@@ -39,15 +39,15 @@ use frame_system::EnsureRoot;
 use pallet_xcm::XcmPassthrough;
 use parachains_common::{
 	impls::ToStakingPot,
+	rococo::snowbridge::EthereumNetwork,
 	xcm_config::{
 		AllSiblingSystemParachains, ConcreteAssetFromSystem, ParentRelayOrSiblingParachains,
 		RelayOrOtherSystemParachains,
 	},
-	TREASURY_PALLET_ID,
+	Balance, TREASURY_PALLET_ID,
 };
 use polkadot_parachain_primitives::primitives::Sibling;
 use polkadot_runtime_common::xcm_sender::ExponentialPrice;
-use parachains_common::rococo::snowbridge::EthereumNetwork;
 use snowbridge_runtime_common::XcmExportFeeToSibling;
 use sp_core::Get;
 use sp_runtime::traits::AccountIdConversion;
@@ -312,7 +312,7 @@ impl xcm_executor::Config for XcmConfig {
 				crate::bridge_to_westend_config::AssetHubRococoToAssetHubWestendMessagesLane,
 			>,
 			XcmExportFeeToSibling<
-				bp_rococo::Balance,
+				Balance,
 				AccountId,
 				TokenLocation,
 				EthereumNetwork,
@@ -416,7 +416,7 @@ impl<
 		fee: MultiAssets,
 		maybe_context: Option<&XcmContext>,
 		reason: FeeReason,
-	) -> MultiAssets {
+	) -> Result<MultiAssets, XcmError> {
 		if matches!(reason, FeeReason::Export { network: bridged_network, destination }
 				if bridged_network == DestNetwork::get() &&
 					destination == X1(Parachain(DestParaId::get().into())))
@@ -472,10 +472,10 @@ impl<
 				}
 			}
 
-			return MultiAssets::new()
+			return Ok(MultiAssets::new())
 		}
 
-		fee
+		Ok(fee)
 	}
 }
 
@@ -493,7 +493,8 @@ impl<WaivedLocations: Contains<MultiLocation>, FeeHandler: HandleFee> FeeManager
 		WaivedLocations::contains(loc)
 	}
 
-	fn handle_fee(fee: MultiAssets, context: Option<&XcmContext>, reason: FeeReason) {
-		FeeHandler::handle_fee(fee, context, reason);
+	fn handle_fee(fee: MultiAssets, context: Option<&XcmContext>, reason: FeeReason) -> XcmResult {
+		FeeHandler::handle_fee(fee, context, reason)?;
+		Ok(())
 	}
 }

cumulus/parachains/runtimes/bridge-hubs/bridge-hub-rococo/tests/snowbridge.rs

@@ -20,15 +20,22 @@ use bridge_hub_rococo_runtime::{
 	xcm_config::XcmConfig, MessageQueueServiceWeight, Runtime, RuntimeEvent, SessionKeys,
 };
 use codec::Decode;
-use cumulus_primitives_core::XcmError::{FailedToTransactAsset, NotHoldingFees};
-use frame_support::parameter_types;
+use cumulus_primitives_core::XcmError::{FailedToTransactAsset, FeesNotMet, NotHoldingFees};
+use frame_support::{parameter_types, traits::fungible::Inspect};
 use parachains_common::{AccountId, AuraId, Balance};
+use snowbridge_core::{PricingParameters, Rewards};
 use snowbridge_pallet_ethereum_client::WeightInfo;
 use sp_core::H160;
 use sp_keyring::AccountKeyring::Alice;
+use sp_runtime::FixedU128;
+
+type TokenBalanceOf<Runtime> = <<Runtime as snowbridge_pallet_system::Config>::Token as Inspect<
+	<Runtime as frame_system::Config>::AccountId,
+>>::Balance;
 
 parameter_types! {
 		pub const DefaultBridgeHubEthereumBaseFee: Balance = 2_750_872_500_000;
+		pub const InsufficientBridgeHubEthereumBaseFee: Balance = 1_000_000_000;
 }
 
 fn collator_session_keys() -> bridge_hub_test_utils::CollatorSessionKeys<Runtime> {
@@ -69,7 +76,7 @@ pub fn unpaid_transfer_token_to_ethereum_fails_with_barrier() {
 }
 
 #[test]
-pub fn transfer_token_to_ethereum_fee_not_enough() {
+pub fn transfer_token_to_ethereum_not_holding_fees() {
 	snowbridge_runtime_test_common::send_transfer_token_message_failure::<Runtime, XcmConfig>(
 		collator_session_keys(),
 		1013,
@@ -78,18 +85,19 @@ pub fn transfer_token_to_ethereum_fee_not_enough() {
 		H160::random(),
 		H160::random(),
 		// fee not enough
-		1_000_000_000,
+		InsufficientBridgeHubEthereumBaseFee::get(),
 		NotHoldingFees,
 	)
 }
 
 #[test]
-pub fn transfer_token_to_ethereum_insufficient_fund() {
+pub fn transfer_token_to_ethereum_failed_to_transact_asset() {
 	snowbridge_runtime_test_common::send_transfer_token_message_failure::<Runtime, XcmConfig>(
 		collator_session_keys(),
 		1013,
 		1000,
-		1_000_000_000,
+		// initial fund not enough
+		InsufficientBridgeHubEthereumBaseFee::get(),
 		H160::random(),
 		H160::random(),
 		DefaultBridgeHubEthereumBaseFee::get(),
@@ -107,3 +115,27 @@ fn max_message_queue_service_weight_is_more_than_beacon_extrinsic_weights() {
 	max_message_queue_weight.all_gt(force_checkpoint);
 	max_message_queue_weight.all_gt(submit_checkpoint);
 }
+
+#[test]
+pub fn transfer_token_to_ethereum_fees_not_met() {
+	let illegal_params: PricingParameters<TokenBalanceOf<Runtime>> = PricingParameters {
+		exchange_rate: FixedU128::from_rational(1, 1),
+		fee_per_gas: 1_u32.into(),
+		rewards: Rewards { local: 1_u32.into(), remote: 1_u32.into() },
+	};
+	snowbridge_runtime_test_common::send_transfer_token_message_failure_with_invalid_fee_params::<
+		Runtime,
+		XcmConfig,
+	>(
+		collator_session_keys(),
+		1013,
+		1000,
+		DefaultBridgeHubEthereumBaseFee::get() + 1_000_000_000,
+		H160::random(),
+		H160::random(),
+		// fee not enough
+		InsufficientBridgeHubEthereumBaseFee::get(),
+		illegal_params,
+		FeesNotMet,
+	)
+}

polkadot/xcm/xcm-builder/src/fee_handling.rs

@@ -21,18 +21,23 @@ use xcm_executor::traits::{FeeManager, FeeReason, TransactAsset};
 
 /// Handles the fees that are taken by certain XCM instructions.
 pub trait HandleFee {
-	/// Do something with the fee which has been paid. Doing nothing here silently burns the
-	/// fees.
-	///
+	/// Do something with the fee which has been paid.
 	/// Returns any part of the fee that wasn't consumed.
-	fn handle_fee(fee: MultiAssets, context: Option<&XcmContext>, reason: FeeReason)
-		-> MultiAssets;
+	fn handle_fee(
+		fee: MultiAssets,
+		context: Option<&XcmContext>,
+		reason: FeeReason,
+	) -> Result<MultiAssets, XcmError>;
 }
 
 // Default `HandleFee` implementation that just burns the fee.
 impl HandleFee for () {
-	fn handle_fee(_: MultiAssets, _: Option<&XcmContext>, _: FeeReason) -> MultiAssets {
-		MultiAssets::new()
+	fn handle_fee(
+		_: MultiAssets,
+		_: Option<&XcmContext>,
+		_: FeeReason,
+	) -> Result<MultiAssets, XcmError> {
+		Ok(MultiAssets::new())
 	}
 }
 
@@ -42,16 +47,16 @@ impl HandleFee for Tuple {
 		fee: MultiAssets,
 		context: Option<&XcmContext>,
 		reason: FeeReason,
-	) -> MultiAssets {
+	) -> Result<MultiAssets, XcmError> {
 		let mut unconsumed_fee = fee;
 		for_tuples!( #(
-			unconsumed_fee = Tuple::handle_fee(unconsumed_fee, context, reason);
+			unconsumed_fee = Tuple::handle_fee(unconsumed_fee, context, reason)?;
 			if unconsumed_fee.is_none() {
-				return unconsumed_fee;
+				return Ok(unconsumed_fee);
 			}
 		)* );
 
-		unconsumed_fee
+		Ok(unconsumed_fee)
 	}
 }
 
@@ -68,8 +73,9 @@ impl<WaivedLocations: Contains<MultiLocation>, FeeHandler: HandleFee> FeeManager
 		WaivedLocations::contains(loc)
 	}
 
-	fn handle_fee(fee: MultiAssets, context: Option<&XcmContext>, reason: FeeReason) {
-		FeeHandler::handle_fee(fee, context, reason);
+	fn handle_fee(fee: MultiAssets, context: Option<&XcmContext>, reason: FeeReason) -> XcmResult {
+		FeeHandler::handle_fee(fee, context, reason)?;
+		Ok(())
 	}
 }
 
@@ -113,9 +119,9 @@ impl<
 		fee: MultiAssets,
 		context: Option<&XcmContext>,
 		_reason: FeeReason,
-	) -> MultiAssets {
+	) -> Result<MultiAssets, XcmError> {
 		deposit_or_burn_fee::<AssetTransactor, _>(fee, context, ReceiverAccount::get());
 
-		MultiAssets::new()
+		Ok(MultiAssets::new())
 	}
 }

polkadot/xcm/xcm-executor/src/assets.rs

@@ -400,7 +400,7 @@ impl Assets {
 			.expect("general_take never results in error when saturating")
 	}
 
-	/// Mutates `self` to its original value less `mask` and returns `true` iff it contains at least
+	/// Mutates `self` to its original value less `mask` and returns `true` if it contains at least
 	/// `mask`.
 	///
 	/// Returns `Ok` with the non-wildcard equivalence of `asset` taken and mutates `self` to its

polkadot/xcm/xcm-executor/src/lib.rs

@@ -248,7 +248,7 @@ impl<Config: config::Config> ExecuteXcm<Config::RuntimeCall> for XcmExecutor<Con
 			for asset in fees.inner() {
 				Config::AssetTransactor::withdraw_asset(&asset, &origin, None)?;
 			}
-			Config::FeeManager::handle_fee(fees, None, FeeReason::ChargeFees);
+			Config::FeeManager::handle_fee(fees, None, FeeReason::ChargeFees)?;
 		}
 		Ok(())
 	}
@@ -968,8 +968,7 @@ impl<Config: config::Config> XcmExecutor<Config> {
 		} else {
 			self.holding.try_take(fee.into()).map_err(|_| XcmError::NotHoldingFees)?.into()
 		};
-		Config::FeeManager::handle_fee(paid, Some(&self.context), reason);
-		Ok(())
+		Config::FeeManager::handle_fee(paid, Some(&self.context), reason)
 	}
 
 	/// Calculates what `local_querier` would be from the perspective of `destination`.

polkadot/xcm/xcm-executor/src/traits/fee_manager.rs

@@ -21,9 +21,8 @@ pub trait FeeManager {
 	/// Determine if a fee should be waived.
 	fn is_waived(origin: Option<&MultiLocation>, r: FeeReason) -> bool;
 
-	/// Do something with the fee which has been paid. Doing nothing here silently burns the
-	/// fees.
-	fn handle_fee(fee: MultiAssets, context: Option<&XcmContext>, r: FeeReason);
+	/// Do something with the fee which has been paid.
+	fn handle_fee(fee: MultiAssets, context: Option<&XcmContext>, r: FeeReason) -> XcmResult;
 }
 
 /// Context under which a fee is paid.
@@ -56,5 +55,7 @@ impl FeeManager for () {
 		false
 	}
 
-	fn handle_fee(_: MultiAssets, _: Option<&XcmContext>, _: FeeReason) {}
+	fn handle_fee(_: MultiAssets, _: Option<&XcmContext>, _: FeeReason) -> XcmResult {
+		Ok(())
+	}
 }