From 07a2a4b3364c3d3ad2de4435f97a8faab1958d65 Mon Sep 17 00:00:00 2001 From: Roy Yang Date: Thu, 15 Feb 2024 14:16:48 +1300 Subject: [PATCH 1/4] Added more errors to AllBatchError so more information is provided. When AllBatch build fails, an event is emitted that contains the error, this way both the pallet and the error details is visible. --- state-chain/chains/src/btc/api.rs | 4 +-- state-chain/chains/src/dot/api.rs | 2 +- state-chain/chains/src/eth/api.rs | 4 +-- state-chain/chains/src/lib.rs | 17 ++++++++++++- .../pallets/cf-ingress-egress/src/lib.rs | 12 ++++++--- .../pallets/cf-ingress-egress/src/tests.rs | 25 +++++++++++++++++++ 6 files changed, 55 insertions(+), 9 deletions(-) diff --git a/state-chain/chains/src/btc/api.rs b/state-chain/chains/src/btc/api.rs index 50dab3f79f..1c5759e94d 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::InvalidUtxo)?; 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..1e6e64071c 100644 --- a/state-chain/chains/src/lib.rs +++ b/state-chain/chains/src/lib.rs @@ -341,9 +341,24 @@ 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, + + // 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, + + // Utxo lookup failed. + InvalidUtxo, + + // Other errors. Other, } diff --git a/state-chain/pallets/cf-ingress-egress/src/lib.rs b/state-chain/pallets/cf-ingress-egress/src/lib.rs index f93141e2ad..65187338aa 100644 --- a/state-chain/pallets/cf-ingress-egress/src/lib.rs +++ b/state-chain/pallets/cf-ingress-egress/src/lib.rs @@ -505,6 +505,9 @@ pub mod pallet { UtxoConsolidation { broadcast_id: BroadcastId, }, + FailedToBuildAllBatchCall { + error: AllBatchError, + }, } #[derive(CloneNoBound, PartialEqNoBound, EqNoBound)] @@ -942,9 +945,12 @@ impl, I: 'static> Pallet { TransactionOutcome::Commit(Ok(())) }, Err(AllBatchError::NotRequired) => TransactionOutcome::Commit(Ok(())), - Err(AllBatchError::Other) => TransactionOutcome::Rollback(Err(DispatchError::Other( - "AllBatch ApiCall creation failed, rolled back storage", - ))), + Err(error) => { + Self::deposit_event(Event::::FailedToBuildAllBatchCall { error }); + TransactionOutcome::Rollback(Err(DispatchError::Other( + "AllBatch ApiCall creation failed, rolled back storage", + ))) + }, } } diff --git a/state-chain/pallets/cf-ingress-egress/src/tests.rs b/state-chain/pallets/cf-ingress-egress/src/tests.rs index e6d2e9c770..6101dd8869 100644 --- a/state-chain/pallets/cf-ingress-egress/src/tests.rs +++ b/state-chain/pallets/cf-ingress-egress/src/tests.rs @@ -1474,3 +1474,28 @@ 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); + + // Expect the egress to fail + matches!( + IngressEgress::do_egress_scheduled_fetch_transfer(), + sp_runtime::TransactionOutcome::Rollback(..) + ); + + System::assert_last_event(RuntimeEvent::IngressEgress( + crate::Event::::FailedToBuildAllBatchCall { + error: cf_chains::AllBatchError::Other, + }, + )); + }); +} From 456f7f581289f640ee945f9e3a39690dac0ebe44 Mon Sep 17 00:00:00 2001 From: Roy Yang Date: Fri, 16 Feb 2024 19:32:51 +1300 Subject: [PATCH 2/4] Addressed PR comments --- state-chain/chains/src/btc/api.rs | 2 +- state-chain/chains/src/lib.rs | 15 +++---- .../pallets/cf-ingress-egress/src/lib.rs | 29 ++++++------- .../pallets/cf-ingress-egress/src/tests.rs | 42 +++++++++---------- state-chain/traits/src/mocks/api_call.rs | 4 +- 5 files changed, 45 insertions(+), 47 deletions(-) diff --git a/state-chain/chains/src/btc/api.rs b/state-chain/chains/src/btc/api.rs index 1c5759e94d..6433b486ef 100644 --- a/state-chain/chains/src/btc/api.rs +++ b/state-chain/chains/src/btc/api.rs @@ -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::InvalidUtxo)?; + .ok_or(AllBatchError::UtxoSelectionFailed)?; if change_amount >= BITCOIN_DUST_LIMIT { btc_outputs.push(BitcoinOutput { amount: change_amount, diff --git a/state-chain/chains/src/lib.rs b/state-chain/chains/src/lib.rs index 1e6e64071c..f2bf3cf0a7 100644 --- a/state-chain/chains/src/lib.rs +++ b/state-chain/chains/src/lib.rs @@ -343,23 +343,20 @@ pub trait RegisterRedemption: ApiCall<::ChainCrypto> { #[derive(Debug, Encode, Decode, Clone, PartialEq, Eq, TypeInfo)] pub enum AllBatchError { - // Empty transaction - the call is not required. + /// Empty transaction - the call is not required. NotRequired, - // The token address lookup failed. The token is not supported on the target chain. + /// The token address lookup failed. The token is not supported on the target chain. UnsupportedToken, - // The vault account is not set. + /// The vault account is not set. VaultAccountNotSet, - // The Aggregate key lookup failed + /// The Aggregate key lookup failed AggKeyNotSet, - // Utxo lookup failed. - InvalidUtxo, - - // Other errors. - Other, + /// Unable to select Utxos. + UtxoSelectionFailed, } #[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 65187338aa..c51fad60fa 100644 --- a/state-chain/pallets/cf-ingress-egress/src/lib.rs +++ b/state-chain/pallets/cf-ingress-egress/src/lib.rs @@ -36,10 +36,11 @@ 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::*; +use scale_info::prelude::{format, string::String}; use sp_runtime::traits::UniqueSaturatedInto; use sp_std::{vec, vec::Vec}; @@ -122,7 +123,6 @@ pub mod pallet { use cf_traits::LpDepositHandler; use core::marker::PhantomData; use frame_support::{ - storage::with_transaction, traits::{ConstU128, EnsureOrigin, IsType}, DefaultNoBound, }; @@ -506,7 +506,7 @@ pub mod pallet { broadcast_id: BroadcastId, }, FailedToBuildAllBatchCall { - error: AllBatchError, + error: String, }, } @@ -567,8 +567,11 @@ 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()) { + if let Err(e) = Self::do_egress_scheduled_fetch_transfer() { log::error!("Ingress-Egress failed to send BatchAll. Error: {e:?}"); + Self::deposit_event(Event::::FailedToBuildAllBatchCall { + error: format!("{:?}", e), + }); } if let Ok(egress_transaction) = @@ -844,7 +847,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() -> DispatchResult { let batch_to_send: Vec<_> = ScheduledEgressFetchOrTransfer::::mutate(|requests: &mut Vec<_>| { // Filter out disabled assets and requests that are not ready to be egressed. @@ -886,7 +890,7 @@ impl, I: 'static> Pallet { }); if batch_to_send.is_empty() { - return TransactionOutcome::Commit(Ok(())) + return Ok(()) } let mut fetch_params = vec![]; @@ -942,15 +946,12 @@ impl, I: 'static> Pallet { broadcast_id, egress_ids, }); - TransactionOutcome::Commit(Ok(())) - }, - Err(AllBatchError::NotRequired) => TransactionOutcome::Commit(Ok(())), - Err(error) => { - Self::deposit_event(Event::::FailedToBuildAllBatchCall { error }); - TransactionOutcome::Rollback(Err(DispatchError::Other( - "AllBatch ApiCall creation failed, rolled back storage", - ))) + Ok(()) }, + Err(AllBatchError::NotRequired) => Ok(()), + Err(error) => Err(DispatchError::Other( + format!("Failed to build AllBatch call. Error: {:?}", error).leak(), + )), } } diff --git a/state-chain/pallets/cf-ingress-egress/src/tests.rs b/state-chain/pallets/cf-ingress-egress/src/tests.rs index 6101dd8869..b75216eddf 100644 --- a/state-chain/pallets/cf-ingress-egress/src/tests.rs +++ b/state-chain/pallets/cf-ingress-egress/src/tests.rs @@ -1477,25 +1477,25 @@ 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); - - // Expect the egress to fail - matches!( - IngressEgress::do_egress_scheduled_fetch_transfer(), - sp_runtime::TransactionOutcome::Rollback(..) - ); - - System::assert_last_event(RuntimeEvent::IngressEgress( - crate::Event::::FailedToBuildAllBatchCall { - error: cf_chains::AllBatchError::Other, - }, - )); - }); + 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: "Other(\"Failed to build AllBatch call. Error: UnsupportedToken\")" + .to_string(), + }, + )); + }); } 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) } } } From 6d1cde52fc7790a8831205e7bc2682251f4622cb Mon Sep 17 00:00:00 2001 From: Daniel Date: Fri, 16 Feb 2024 12:50:35 +0100 Subject: [PATCH 3/4] fix: `AllBatchError: From` --- state-chain/chains/src/lib.rs | 9 +++++++++ .../pallets/cf-ingress-egress/src/lib.rs | 17 ++++++----------- 2 files changed, 15 insertions(+), 11 deletions(-) diff --git a/state-chain/chains/src/lib.rs b/state-chain/chains/src/lib.rs index f2bf3cf0a7..566b4979ac 100644 --- a/state-chain/chains/src/lib.rs +++ b/state-chain/chains/src/lib.rs @@ -357,6 +357,15 @@ pub enum AllBatchError { /// 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 1fa88f6426..aabdb995aa 100644 --- a/state-chain/pallets/cf-ingress-egress/src/lib.rs +++ b/state-chain/pallets/cf-ingress-egress/src/lib.rs @@ -40,7 +40,6 @@ use frame_support::{ }; use frame_system::pallet_prelude::*; pub use pallet::*; -use scale_info::prelude::{format, string::String}; use sp_runtime::traits::UniqueSaturatedInto; use sp_std::{vec, vec::Vec}; @@ -534,7 +533,7 @@ pub mod pallet { broadcast_id: BroadcastId, }, FailedToBuildAllBatchCall { - error: String, + error: AllBatchError, }, ChannelOpeningFeePaid { fee: T::Amount, @@ -601,11 +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) = Self::do_egress_scheduled_fetch_transfer() { - log::error!("Ingress-Egress failed to send BatchAll. Error: {e:?}"); - Self::deposit_event(Event::::FailedToBuildAllBatchCall { - error: format!("{:?}", 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) = @@ -892,7 +889,7 @@ impl, I: 'static> Pallet { /// /// Note: Egress transactions with Blacklisted assets are not sent, and kept in storage. #[transactional] - fn do_egress_scheduled_fetch_transfer() -> DispatchResult { + 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. @@ -993,9 +990,7 @@ impl, I: 'static> Pallet { Ok(()) }, Err(AllBatchError::NotRequired) => Ok(()), - Err(error) => Err(DispatchError::Other( - format!("Failed to build AllBatch call. Error: {:?}", error).leak(), - )), + Err(other) => Err(other), } } From 37a89980f3438319916485d64e9978eb4a2e8274 Mon Sep 17 00:00:00 2001 From: Daniel Date: Fri, 16 Feb 2024 14:34:42 +0100 Subject: [PATCH 4/4] fix: unit test --- state-chain/pallets/cf-ingress-egress/src/tests.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/state-chain/pallets/cf-ingress-egress/src/tests.rs b/state-chain/pallets/cf-ingress-egress/src/tests.rs index 971f2259e4..25d107d71a 100644 --- a/state-chain/pallets/cf-ingress-egress/src/tests.rs +++ b/state-chain/pallets/cf-ingress-egress/src/tests.rs @@ -1507,8 +1507,7 @@ fn all_batch_errors_are_logged_as_event() { .then_execute_with(|_| { System::assert_last_event(RuntimeEvent::IngressEgress( crate::Event::::FailedToBuildAllBatchCall { - error: "Other(\"Failed to build AllBatch call. Error: UnsupportedToken\")" - .to_string(), + error: cf_chains::AllBatchError::UnsupportedToken, }, )); });