fix: don't ignore valid deposits when another one fails (#4165)

Co-authored-by: Martin Rieke <martin@chainflip.io>

state-chain/pallets/cf-ingress-egress/src/lib.rs

@@ -12,7 +12,7 @@ mod mock;
 mod tests;
 pub mod weights;
 use cf_runtime_utilities::log_or_panic;
-use frame_support::{sp_runtime::SaturatedConversion, traits::OnRuntimeUpgrade};
+use frame_support::{sp_runtime::SaturatedConversion, traits::OnRuntimeUpgrade, transactional};
 pub use weights::WeightInfo;
 
 use cf_chains::{
@@ -382,7 +382,7 @@ pub mod pallet {
 			asset: TargetChainAsset<T, I>,
 			minimum_deposit: TargetChainAmount<T, I>,
 		},
-		///The deposits is rejected because the amount is below the minimum allowed.
+		/// The deposits was rejected because the amount was below the minimum allowed.
 		DepositIgnored {
 			deposit_address: TargetChainAccount<T, I>,
 			asset: TargetChainAsset<T, I>,
@@ -394,6 +394,11 @@ pub mod pallet {
 			amount: TargetChainAmount<T, I>,
 			destination_address: TargetChainAccount<T, I>,
 		},
+		/// The deposit witness was rejected.
+		DepositWitnessRejected {
+			reason: DispatchError,
+			deposit_witness: DepositWitness<T::TargetChain>,
+		},
 	}
 
 	#[pallet::error]
@@ -535,16 +540,26 @@ pub mod pallet {
 		) -> DispatchResult {
 			T::EnsureWitnessed::ensure_origin(origin)?;
 
-			for DepositWitness { deposit_address, asset, amount, deposit_details } in
-				deposit_witnesses
+			for ref deposit_witness @ DepositWitness {
+				ref deposit_address,
+				asset,
+				amount,
+				ref deposit_details,
+			} in deposit_witnesses
 			{
 				Self::process_single_deposit(
-					deposit_address,
+					deposit_address.clone(),
 					asset,
 					amount,
-					deposit_details,
+					deposit_details.clone(),
 					block_height,
-				)?;
+				)
+				.unwrap_or_else(|e| {
+					Self::deposit_event(Event::<T, I>::DepositWitnessRejected {
+						reason: e,
+						deposit_witness: deposit_witness.clone(),
+					});
+				})
 			}
 			Ok(())
 		}
@@ -769,6 +784,7 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
 	}
 
 	/// Completes a single deposit request.
+	#[transactional]
 	fn process_single_deposit(
 		deposit_address: TargetChainAccount<T, I>,
 		asset: TargetChainAsset<T, I>,

state-chain/pallets/cf-ingress-egress/src/tests.rs

@@ -1,6 +1,6 @@
 use crate::{
 	mock::*, Call as PalletCall, ChannelAction, ChannelIdCounter, CrossChainMessage,
-	DepositChannelLookup, DepositChannelPool, DepositWitness, DisabledEgressAssets, Error,
+	DepositChannelLookup, DepositChannelPool, DepositWitness, DisabledEgressAssets,
 	Event as PalletEvent, FailedVaultTransfers, FetchOrTransfer, MinimumDeposit, Pallet,
 	ScheduledEgressCcm, ScheduledEgressFetchOrTransfer, TargetChainAccount, VaultTransfer,
 };
@@ -20,7 +20,7 @@ use cf_traits::{
 	DepositApi, EgressApi, GetBlockHeight,
 };
 use frame_support::{
-	assert_noop, assert_ok,
+	assert_ok,
 	traits::{Hooks, OriginTrait},
 	weights::Weight,
 };
@@ -574,15 +574,100 @@ fn can_egress_ccm() {
 	});
 }
 
+#[test]
+fn multi_deposit_includes_deposit_beyond_recycle_height() {
+	const ETH: eth::Asset = eth::Asset::Eth;
+	new_test_ext()
+		.then_execute_at_next_block(|_| {
+			let (_, address, ..) =
+				IngressEgress::request_liquidity_deposit_address(ALICE, ETH).unwrap();
+			let address: <Ethereum as Chain>::ChainAccount = address.try_into().unwrap();
+			let recycles_at = IngressEgress::expiry_and_recycle_block_height().2;
+			(address, recycles_at)
+		})
+		.then_execute_at_next_block(|(address, recycles_at)| {
+			BlockHeightProvider::<MockEthereum>::set_block_height(recycles_at);
+			address
+		})
+		.then_execute_at_next_block(|address| {
+			let (_, address2, ..) =
+				IngressEgress::request_liquidity_deposit_address(ALICE, ETH).unwrap();
+			let address2: <Ethereum as Chain>::ChainAccount = address2.try_into().unwrap();
+			(address, address2)
+		})
+		.then_apply_extrinsics(|&(address, address2)| {
+			[(
+				RuntimeOrigin::root(),
+				crate::Call::<Test, _>::process_deposits {
+					deposit_witnesses: vec![
+						DepositWitness {
+							deposit_address: address,
+							asset: ETH,
+							amount: 1,
+							deposit_details: Default::default(),
+						},
+						DepositWitness {
+							deposit_address: address2,
+							asset: ETH,
+							amount: 1,
+							deposit_details: Default::default(),
+						},
+					],
+					// The block height is purely informative.
+					block_height: BlockHeightProvider::<MockEthereum>::get_block_height(),
+				},
+				Ok(()),
+			)]
+		})
+		.then_process_events(|_, event| match event {
+			RuntimeEvent::IngressEgress(crate::Event::DepositWitnessRejected { .. }) |
+			RuntimeEvent::IngressEgress(crate::Event::DepositReceived { .. }) => Some(event),
+			_ => None,
+		})
+		.inspect_context(|((expected_rejected_address, expected_accepted_address), emitted)| {
+			assert_eq!(emitted.len(), 2);
+			assert!(emitted.iter().any(|e| matches!(
+			e,
+			RuntimeEvent::IngressEgress(
+				crate::Event::DepositWitnessRejected {
+					deposit_witness,
+					..
+				}) if deposit_witness.deposit_address == *expected_rejected_address
+			)),);
+			assert!(emitted.iter().any(|e| matches!(
+			e,
+			RuntimeEvent::IngressEgress(
+				crate::Event::DepositReceived {
+					deposit_address,
+					..
+				}) if deposit_address == expected_accepted_address
+			)),);
+		});
+}
+
 #[test]
 fn multi_use_deposit_address_different_blocks() {
 	const ETH: eth::Asset = eth::Asset::Eth;
 
 	new_test_ext()
 		.then_execute_at_next_block(|_| request_address_and_deposit(ALICE, ETH))
+		.then_apply_extrinsics(|&(_, deposit_address)| {
+			[(
+				RuntimeOrigin::root(),
+				crate::Call::<Test, _>::process_deposits {
+					deposit_witnesses: vec![DepositWitness {
+						deposit_address,
+						asset: ETH,
+						amount: 1,
+						deposit_details: Default::default(),
+					}],
+					// block height is purely informative.
+					block_height: BlockHeightProvider::<MockEthereum>::get_block_height(),
+				},
+				Ok(()),
+			)]
+		})
 		.then_execute_at_next_block(|channel @ (_, deposit_address)| {
-			// Set the address to deployed.
-			// Do another, should succeed.
 			assert_ok!(Pallet::<Test, _>::process_single_deposit(
 				deposit_address,
 				ETH,
@@ -595,21 +680,32 @@ fn multi_use_deposit_address_different_blocks() {
 
 			channel
 		})
-		.then_execute_at_next_block(|(_, deposit_address)| {
-			// Closing the channel should invalidate the deposit address.
-			assert_noop!(
-				IngressEgress::process_deposits(
-					RuntimeOrigin::root(),
-					vec![DepositWitness {
+		// The channel should be closed at the next block.
+		.then_apply_extrinsics(|&(_, deposit_address)| {
+			[(
+				RuntimeOrigin::root(),
+				crate::Call::<Test, _>::process_deposits {
+					deposit_witnesses: vec![DepositWitness {
 						deposit_address,
-						asset: eth::Asset::Eth,
+						asset: ETH,
 						amount: 1,
-						deposit_details: Default::default()
+						deposit_details: Default::default(),
 					}],
-					Default::default()
-				),
-				Error::<Test, _>::InvalidDepositAddress
-			);
+					// block height is purely informative.
+					block_height: BlockHeightProvider::<MockEthereum>::get_block_height(),
+				},
+				Ok(()),
+			)]
+		})
+		.then_process_events(|_, event| match event {
+			RuntimeEvent::IngressEgress(crate::Event::DepositWitnessRejected {
+				deposit_witness,
+				..
+			}) => Some(deposit_witness.deposit_address),
+			_ => None,
+		})
+		.inspect_context(|((_, expected_address), emitted)| {
+			assert_eq!(*emitted, vec![*expected_address]);
 		});
 }