Handle fee with error instead of silently ignoring

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::<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(|| {

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

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<Balance, AccountId, FeeAssetLocation, EthereumNetwork, AssetTransactor, Fee
 		FeeProvider,
 	> where
 	Balance: BaseArithmetic + Unsigned + Copy + From<u128> + Into<u128>,
-	AccountId: Clone + Into<[u8; 32]> + From<[u8; 32]>,
+	AccountId: Clone + FullCodec,
 	FeeAssetLocation: Get<MultiLocation>,
 	EthereumNetwork: Get<NetworkId>,
 	AssetTransactor: TransactAsset,
@@ -55,7 +59,7 @@ impl<Balance, AccountId, FeeAssetLocation, EthereumNetwork, AssetTransactor, Fee
 		fees: MultiAssets,
 		context: Option<&XcmContext>,
 		reason: FeeReason,
-	) -> MultiAssets {
+	) -> Result<MultiAssets, XcmError> {
 		let token_location = FeeAssetLocation::get();
 
 		// Check the reason to see if this export is for snowbridge.
@@ -64,25 +68,24 @@ impl<Balance, AccountId, FeeAssetLocation, EthereumNetwork, AssetTransactor, Fee
 			FeeReason::Export { network: bridged_network, destination }
 				if bridged_network == EthereumNetwork::get() && destination == Here
 		) {
-			return fees
+			return Ok(fees)
 		}
 
 		// Get the parachain sovereign from the `context`.
-		let para_sovereign = if let Some(XcmContext {
+		let maybe_para_id: Option<u32> = 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, AccountId, FeeAssetLocation, EthereumNetwork, AssetTransactor, Fee
 				None
 			})
 			.next();
+		let (fee_index, total_fee) = maybe_total_supplied_fee.ok_or(XcmError::FeesNotMet)?;
+		let local_fee = FeeProvider::local_fee();
+		let remote_fee = total_fee.checked_sub(&local_fee).ok_or(XcmError::FeesNotMet)?;
+		ensure!(remote_fee > 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::<AssetTransactor, _>(
-					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())
 	}
 }