Snowbridge Ethereum Deneb fork preparation (#3029)

- Prepares for the Deneb hardfork on Sepolia testnet on 31 January
(needs to be deployed to Rococo before then)
- Removes `beacon-minimal-spec` flag for simpler config
- Adds test comments

---------

Co-authored-by: Ron <yrong1997@gmail.com>
Co-authored-by: claravanstaden <Cats 4 life!>
Co-authored-by: Alistair Singh <alistair.singh7@gmail.com>

bridges/snowbridge/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 = "../../../../../substrate/frame/support", default-features = false }
 frame-system = { path = "../../../../../substrate/frame/system", default-features = false }
+sp-std = { path = "../../../../../substrate/primitives/std", default-features = false }
 sp-arithmetic = { path = "../../../../../substrate/primitives/arithmetic", default-features = false }
 xcm = { package = "staging-xcm", path = "../../../../../polkadot/xcm", default-features = false }
 xcm-builder = { package = "staging-xcm-builder", path = "../../../../../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",

bridges/snowbridge/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<Balance, AccountId, FeeAssetLocation, EthereumNetwork, AssetTransactor, Fee
 		AssetTransactor,
 		FeeProvider,
 	> where
-	Balance: BaseArithmetic + Unsigned + Copy + From<u128> + Into<u128>,
-	AccountId: Clone + Into<[u8; 32]> + From<[u8; 32]>,
+	Balance: BaseArithmetic + Unsigned + Copy + From<u128> + Into<u128> + Debug,
+	AccountId: Clone + FullCodec,
 	FeeAssetLocation: Get<Location>,
 	EthereumNetwork: Get<NetworkId>,
 	AssetTransactor: TransactAsset,
@@ -64,20 +71,27 @@ impl<Balance, AccountId, FeeAssetLocation, EthereumNetwork, AssetTransactor, Fee
 		}
 
 		// Get the parachain sovereign from the `context`.
-		let para_sovereign =
+		let maybe_para_id: Option<u32> =
 			if let Some(XcmContext { origin: Some(Location { 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
@@ -93,32 +107,45 @@ impl<Balance, AccountId, FeeAssetLocation, EthereumNetwork, AssetTransactor, Fee
 				None
 			})
 			.next();
-
-		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, _>(
-					Asset { id: AssetId(token_location.clone()), 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(Asset {
-					id: AssetId(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
+		}
+		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(
+			&Asset { id: AssetId(token_location.clone()), fun: Fungible(remote_fee.into()) },
+			&Location::new(1, [Parachain(para_id)]),
+			context,
+		);
+		if result.is_err() {
+			log::error!(
+				target: LOG_TARGET,
+				"transact fee asset failed: {:?}",
+				result.unwrap_err()
+			);
+			return fees
 		}
 
-		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(Asset { id: AssetId(token_location), fun: Fungible(local_fee.into()) });
+		modified_fees.into()
 	}
 }

bridges/snowbridge/parachain/runtime/runtime-common/src/tests.rs

@@ -0,0 +1,168 @@
+use crate::XcmExportFeeToSibling;
+use frame_support::{parameter_types, sp_runtime::testing::H256};
+use snowbridge_core::outbound::{Fee, Message, SendError, SendMessage, SendMessageFeeProvider};
+use xcm::prelude::{
+	Asset, Assets, Here, Kusama, Location, NetworkId, Parachain, XcmContext, XcmError, XcmHash,
+	XcmResult,
+};
+use xcm_builder::HandleFee;
+use xcm_executor::{
+	traits::{FeeReason, TransactAsset},
+	AssetsInHolding,
+};
+
+parameter_types! {
+	pub EthereumNetwork: NetworkId = NetworkId::Ethereum { chain_id: 11155111 };
+	pub TokenLocation: Location = Location::parent();
+}
+
+struct MockOkOutboundQueue;
+impl SendMessage for MockOkOutboundQueue {
+	type Ticket = ();
+
+	fn validate(_: &Message) -> Result<(Self::Ticket, Fee<Self::Balance>), SendError> {
+		Ok(((), Fee { local: 1, remote: 1 }))
+	}
+
+	fn deliver(_: Self::Ticket) -> Result<H256, SendError> {
+		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<Self::Balance>), SendError> {
+		Err(SendError::MessageTooLarge)
+	}
+
+	fn deliver(_: Self::Ticket) -> Result<H256, SendError> {
+		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: &Location, _what: &Asset, _context: &XcmContext) -> XcmResult {
+		Ok(())
+	}
+
+	fn can_check_out(_dest: &Location, _what: &Asset, _context: &XcmContext) -> XcmResult {
+		Ok(())
+	}
+
+	fn deposit_asset(_what: &Asset, _who: &Location, _context: Option<&XcmContext>) -> XcmResult {
+		Ok(())
+	}
+
+	fn withdraw_asset(
+		_what: &Asset,
+		_who: &Location,
+		_context: Option<&XcmContext>,
+	) -> Result<AssetsInHolding, XcmError> {
+		Ok(Assets::default().into())
+	}
+
+	fn internal_transfer_asset(
+		_what: &Asset,
+		_from: &Location,
+		_to: &Location,
+		_context: &XcmContext,
+	) -> Result<AssetsInHolding, XcmError> {
+		Ok(Assets::default().into())
+	}
+}
+
+#[test]
+fn handle_fee_success() {
+	let fee: Assets = Asset::from((Location::parent(), 10_u128)).into();
+	let ctx = XcmContext {
+		origin: Some(Location::new(1, 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 = Asset::from((Location::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: Assets = Asset::from((Location::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: Assets = Asset::from((Location::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: Assets = Asset::from((Location::parent(), 1_u128)).into();
+	let ctx = XcmContext {
+		origin: Some(Location::new(1, 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)
+}