diff --git a/state-chain/chains/src/btc/api.rs b/state-chain/chains/src/btc/api.rs index 50dab3f79f..6433b486ef 100644 --- a/state-chain/chains/src/btc/api.rs +++ b/state-chain/chains/src/btc/api.rs @@ -67,7 +67,7 @@ where transfer_params: Vec>, ) -> Result { let agg_key @ AggKey { current, .. } = - >::lookup(()).ok_or(AllBatchError::Other)?; + >::lookup(()).ok_or(AllBatchError::AggKeyNotSet)?; let bitcoin_change_script = DepositAddress::new(current, CHANGE_ADDRESS_SALT).script_pubkey(); let mut total_output_amount: u64 = 0; @@ -89,7 +89,7 @@ where .ok_or(AllBatchError::NotRequired)?, number_of_outputs: (btc_outputs.len() + 1) as u64, // +1 for the change output }) - .ok_or(AllBatchError::Other)?; + .ok_or(AllBatchError::UtxoSelectionFailed)?; if change_amount >= BITCOIN_DUST_LIMIT { btc_outputs.push(BitcoinOutput { amount: change_amount, diff --git a/state-chain/chains/src/dot/api.rs b/state-chain/chains/src/dot/api.rs index e2ccb1ac10..0d90eb7120 100644 --- a/state-chain/chains/src/dot/api.rs +++ b/state-chain/chains/src/dot/api.rs @@ -66,7 +66,7 @@ where E::replay_protection(false), fetch_params, transfer_params, - E::try_vault_account().ok_or(AllBatchError::Other)?, + E::try_vault_account().ok_or(AllBatchError::VaultAccountNotSet)?, ))) } } diff --git a/state-chain/chains/src/eth/api.rs b/state-chain/chains/src/eth/api.rs index 31aea4d797..d121f0bb09 100644 --- a/state-chain/chains/src/eth/api.rs +++ b/state-chain/chains/src/eth/api.rs @@ -232,7 +232,7 @@ where EvmFetchId::NotRequired => (), }; } else { - return Err(AllBatchError::Other) + return Err(AllBatchError::UnsupportedToken) } } if fetch_only_params.is_empty() && @@ -255,7 +255,7 @@ where amount, asset: address, }) - .ok_or(AllBatchError::Other) + .ok_or(AllBatchError::UnsupportedToken) }) .collect::, _>>()?, ), diff --git a/state-chain/chains/src/lib.rs b/state-chain/chains/src/lib.rs index 050403d5ed..566b4979ac 100644 --- a/state-chain/chains/src/lib.rs +++ b/state-chain/chains/src/lib.rs @@ -341,10 +341,31 @@ pub trait RegisterRedemption: ApiCall<::ChainCrypto> { fn amount(&self) -> u128; } -#[derive(Debug)] +#[derive(Debug, Encode, Decode, Clone, PartialEq, Eq, TypeInfo)] pub enum AllBatchError { + /// Empty transaction - the call is not required. NotRequired, - Other, + + /// The token address lookup failed. The token is not supported on the target chain. + UnsupportedToken, + + /// The vault account is not set. + VaultAccountNotSet, + + /// The Aggregate key lookup failed + AggKeyNotSet, + + /// Unable to select Utxos. + UtxoSelectionFailed, + + /// Some other DispatchError occurred. + DispatchError(DispatchError), +} + +impl From for AllBatchError { + fn from(e: DispatchError) -> Self { + AllBatchError::DispatchError(e) + } } #[derive(Debug)] diff --git a/state-chain/pallets/cf-ingress-egress/src/lib.rs b/state-chain/pallets/cf-ingress-egress/src/lib.rs index c4cc9257dd..aabdb995aa 100644 --- a/state-chain/pallets/cf-ingress-egress/src/lib.rs +++ b/state-chain/pallets/cf-ingress-egress/src/lib.rs @@ -36,7 +36,7 @@ use cf_traits::{ }; use frame_support::{ pallet_prelude::*, - sp_runtime::{traits::Zero, DispatchError, Saturating, TransactionOutcome}, + sp_runtime::{traits::Zero, DispatchError, Saturating}, }; use frame_system::pallet_prelude::*; pub use pallet::*; @@ -140,7 +140,6 @@ pub mod pallet { use cf_traits::LpDepositHandler; use core::marker::PhantomData; use frame_support::{ - storage::with_transaction, traits::{ConstU128, EnsureOrigin, IsType}, DefaultNoBound, }; @@ -533,6 +532,9 @@ pub mod pallet { UtxoConsolidation { broadcast_id: BroadcastId, }, + FailedToBuildAllBatchCall { + error: AllBatchError, + }, ChannelOpeningFeePaid { fee: T::Amount, }, @@ -598,8 +600,9 @@ pub mod pallet { /// Take all scheduled Egress and send them out fn on_finalize(_n: BlockNumberFor) { // Send all fetch/transfer requests as a batch. Revert storage if failed. - if let Err(e) = with_transaction(|| Self::do_egress_scheduled_fetch_transfer()) { - log::error!("Ingress-Egress failed to send BatchAll. Error: {e:?}"); + if let Err(error) = Self::do_egress_scheduled_fetch_transfer() { + log::error!("Ingress-Egress failed to send BatchAll. Error: {error:?}"); + Self::deposit_event(Event::::FailedToBuildAllBatchCall { error }); } if let Ok(egress_transaction) = @@ -885,7 +888,8 @@ impl, I: 'static> Pallet { /// Take all scheduled egress requests and send them out in an `AllBatch` call. /// /// Note: Egress transactions with Blacklisted assets are not sent, and kept in storage. - fn do_egress_scheduled_fetch_transfer() -> TransactionOutcome { + #[transactional] + fn do_egress_scheduled_fetch_transfer() -> Result<(), AllBatchError> { let batch_to_send: Vec<_> = ScheduledEgressFetchOrTransfer::::mutate(|requests: &mut Vec<_>| { // Filter out disabled assets and requests that are not ready to be egressed. @@ -927,7 +931,7 @@ impl, I: 'static> Pallet { }); if batch_to_send.is_empty() { - return TransactionOutcome::Commit(Ok(())) + return Ok(()) } let mut fetch_params = vec![]; @@ -983,12 +987,10 @@ impl, I: 'static> Pallet { broadcast_id, egress_ids, }); - TransactionOutcome::Commit(Ok(())) + Ok(()) }, - Err(AllBatchError::NotRequired) => TransactionOutcome::Commit(Ok(())), - Err(AllBatchError::Other) => TransactionOutcome::Rollback(Err(DispatchError::Other( - "AllBatch ApiCall creation failed, rolled back storage", - ))), + Err(AllBatchError::NotRequired) => Ok(()), + Err(other) => Err(other), } } diff --git a/state-chain/pallets/cf-ingress-egress/src/tests.rs b/state-chain/pallets/cf-ingress-egress/src/tests.rs index 6a4e8bbbf7..25d107d71a 100644 --- a/state-chain/pallets/cf-ingress-egress/src/tests.rs +++ b/state-chain/pallets/cf-ingress-egress/src/tests.rs @@ -1489,6 +1489,30 @@ fn consolidation_tx_gets_broadcasted_on_finalize() { }); } +#[test] +fn all_batch_errors_are_logged_as_event() { + new_test_ext() + .execute_with(|| { + ScheduledEgressFetchOrTransfer::::set(vec![ + FetchOrTransfer::::Transfer { + asset: ETH_ETH, + amount: 1_000, + destination_address: ALICE_ETH_ADDRESS, + egress_id: (ForeignChain::Ethereum, 1), + }, + ]); + MockEthAllBatch::set_success(false); + }) + .then_execute_at_next_block(|_| {}) + .then_execute_with(|_| { + System::assert_last_event(RuntimeEvent::IngressEgress( + crate::Event::::FailedToBuildAllBatchCall { + error: cf_chains::AllBatchError::UnsupportedToken, + }, + )); + }); +} + #[test] fn broker_pays_a_fee_for_each_deposit_address() { new_test_ext().execute_with(|| { diff --git a/state-chain/traits/src/mocks/api_call.rs b/state-chain/traits/src/mocks/api_call.rs index 789f3da257..eef70f756e 100644 --- a/state-chain/traits/src/mocks/api_call.rs +++ b/state-chain/traits/src/mocks/api_call.rs @@ -90,7 +90,7 @@ impl AllBatch for MockEthereumApiCall { _phantom: PhantomData, })) } else { - Err(AllBatchError::Other) + Err(AllBatchError::UnsupportedToken) } } } @@ -294,7 +294,7 @@ impl AllBatch for MockBitcoinApiCall { _phantom: PhantomData, })) } else { - Err(AllBatchError::Other) + Err(AllBatchError::UnsupportedToken) } } }