Refactoring XcmExportFeeToSibling and more tests for outbound fee (#1109

) * Handle fee with error instead of silently failing * Fix for pallet rename * AssetTransactor::deposit_asset directly with error thrown * Move invalid params to test * Check more strictly * More tests * More tests * Handle fee without error * Update sdk * More tests for outbound fee * More tests * Update parachain/runtime/runtime-common/src/lib.rs Co-authored-by: Alistair Singh <[email protected]> * Update parachain/runtime/runtime-common/src/lib.rs Co-authored-by: Alistair Singh <[email protected]> * Update parachain/runtime/runtime-common/src/lib.rs Co-authored-by: Alistair Singh <[email protected]> * Update parachain/runtime/runtime-common/src/lib.rs Co-authored-by: Alistair Singh <[email protected]> * More param checks & more tests * Rename test case less confusing * Remove sanity check for Pricing params * Remove test unnecessary * Revert to unroutable * Revert the checks * Rename as MockAssetTransactor * Update sdk --------- Co-authored-by: Alistair Singh <[email protected]>
Snowfork · Jan 19, 2024 · 4e24732 · 4e24732
1 parent 28e4f09
commit 4e24732
Show file tree

Hide file tree

Showing 13 changed files with 315 additions and 66 deletions.
diff --git a/parachain/Cargo.lock b/parachain/Cargo.lock
diff --git 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 <[email protected]>
 //! 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
@@ -1,3 +1,5 @@
+// SPDX-License-Identifier: Apache-2.0
+// SPDX-FileCopyrightText: 2023 Snowfork <[email protected]>
 //! 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
@@ -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<<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_valid_exchange_rate_but_remote_fee_calculated_as_zero() {
+	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);
+	});
+}
diff --git 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 <[email protected]>
 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
@@ -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 };

diff --git 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<Initializer> =
 			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::<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 +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<Initializer> =
 			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
@@ -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 {

diff --git 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
@@ -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<MultiLocation>,
 	EthereumNetwork: Get<NetworkId>,
 	AssetTransactor: TransactAsset,
@@ -68,21 +75,28 @@ impl<Balance, AccountId, FeeAssetLocation, EthereumNetwork, AssetTransactor, Fee
 		}
 
 		// 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
 		};
+		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<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, _>(
-					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()
 	}
 }