Make currency swap return the original imbalance on failure

crates/bridge-pot-currency-swap/src/existence_optional.rs

@@ -19,19 +19,30 @@ impl<T: Config> primitives_currency_swap::CurrencySwap<T::AccountIdFrom, T::Acco
     type Error = DispatchError;
 
     fn swap(
-        imbalance: <Self::From as Currency<T::AccountIdFrom>>::NegativeImbalance,
-    ) -> Result<<Self::To as Currency<T::AccountIdTo>>::NegativeImbalance, Self::Error> {
-        let amount = imbalance.peek();
-
-        T::CurrencyFrom::resolve_creating(&T::PotFrom::get(), imbalance);
-
-        let imbalance = T::CurrencyTo::withdraw(
+        incoming_imbalance: <Self::From as Currency<T::AccountIdFrom>>::NegativeImbalance,
+    ) -> Result<
+        <Self::To as Currency<T::AccountIdTo>>::NegativeImbalance,
+        primitives_currency_swap::ErrorFor<Self, T::AccountIdFrom, T::AccountIdTo>,
+    > {
+        let amount = incoming_imbalance.peek();
+
+        let outgoing_imbalance = match T::CurrencyTo::withdraw(
             &T::PotTo::get(),
             T::BalanceCoverter::convert(amount),
             WithdrawReasons::TRANSFER,
             ExistenceRequirement::AllowDeath,
-        )?;
-
-        Ok(imbalance)
+        ) {
+            Ok(imbalance) => imbalance,
+            Err(error) => {
+                return Err(primitives_currency_swap::Error {
+                    cause: error,
+                    incoming_imbalance,
+                })
+            }
+        };
+
+        T::CurrencyFrom::resolve_creating(&T::PotFrom::get(), incoming_imbalance);
+
+        Ok(outgoing_imbalance)
     }
 }

crates/bridge-pot-currency-swap/src/existence_required.rs

@@ -12,17 +12,19 @@ pub enum Marker {}
 
 /// An error that can occur while doing the swap operation.
 #[derive(Debug, Clone, Copy, PartialEq, Eq)]
-pub enum Error<ImbalanceFrom> {
+pub enum Error {
     /// Unable to resolve the incoming balance into the corresponding pot.
-    ResolvingIncomingImbalance(ImbalanceFrom),
+    ResolvingIncomingImbalance,
     /// Unable to withdraw the outgoing balance from the corresponding pot.
     IssuingOutgoingImbalance(DispatchError),
 }
 
-impl<T> From<Error<T>> for DispatchError {
-    fn from(value: Error<T>) -> Self {
+impl From<Error> for DispatchError {
+    fn from(value: Error) -> Self {
         match value {
-            Error::ResolvingIncomingImbalance(_) => DispatchError::NoProviders,
+            Error::ResolvingIncomingImbalance => {
+                DispatchError::Other("swap pot account does not exist")
+            }
             Error::IssuingOutgoingImbalance(err) => err,
         }
     }
@@ -33,25 +35,42 @@ impl<T: Config> primitives_currency_swap::CurrencySwap<T::AccountIdFrom, T::Acco
 {
     type From = T::CurrencyFrom;
     type To = T::CurrencyTo;
-    type Error =
-        Error<<<T as Config>::CurrencyFrom as Currency<T::AccountIdFrom>>::NegativeImbalance>;
+    type Error = Error;
 
     fn swap(
-        imbalance: <Self::From as Currency<T::AccountIdFrom>>::NegativeImbalance,
-    ) -> Result<<Self::To as Currency<T::AccountIdTo>>::NegativeImbalance, Self::Error> {
-        let amount = imbalance.peek();
+        incoming_imbalance: <Self::From as Currency<T::AccountIdFrom>>::NegativeImbalance,
+    ) -> Result<
+        <Self::To as Currency<T::AccountIdTo>>::NegativeImbalance,
+        primitives_currency_swap::ErrorFor<Self, T::AccountIdFrom, T::AccountIdTo>,
+    > {
+        let amount = incoming_imbalance.peek();
 
-        T::CurrencyFrom::resolve_into_existing(&T::PotFrom::get(), imbalance)
-            .map_err(Error::ResolvingIncomingImbalance)?;
-
-        let imbalance = T::CurrencyTo::withdraw(
+        let outgoing_imbalance = match T::CurrencyTo::withdraw(
             &T::PotTo::get(),
             T::BalanceCoverter::convert(amount),
             WithdrawReasons::TRANSFER,
             ExistenceRequirement::KeepAlive,
-        )
-        .map_err(Error::IssuingOutgoingImbalance)?;
+        ) {
+            Ok(imbalance) => imbalance,
+            Err(error) => {
+                return Err(primitives_currency_swap::Error {
+                    cause: Error::IssuingOutgoingImbalance(error),
+                    incoming_imbalance,
+                });
+            }
+        };
+
+        match T::CurrencyFrom::resolve_into_existing(&T::PotFrom::get(), incoming_imbalance) {
+            Ok(()) => {}
+            Err(imbalance) => {
+                T::CurrencyTo::resolve_creating(&T::PotTo::get(), outgoing_imbalance);
+                return Err(primitives_currency_swap::Error {
+                    cause: Error::ResolvingIncomingImbalance,
+                    incoming_imbalance: imbalance,
+                });
+            }
+        }
 
-        Ok(imbalance)
+        Ok(outgoing_imbalance)
     }
 }

crates/precompile-currency-swap/src/lib.rs

@@ -134,9 +134,13 @@ where
             },
         })?;
 
-        let imbalance = CurrencySwapT::swap(imbalance).map_err(|_| PrecompileFailure::Revert {
-            exit_status: ExitRevert::Reverted,
-            output: "unable to swap the currency".into(),
+        let imbalance = CurrencySwapT::swap(imbalance).map_err(|error| {
+            // Here we undo the withdrawal to avoid having a dangling imbalance.
+            CurrencySwapT::From::resolve_creating(&from, error.incoming_imbalance);
+            PrecompileFailure::Revert {
+                exit_status: ExitRevert::Reverted,
+                output: "unable to swap the currency".into(),
+            }
         })?;
 
         CurrencySwapT::To::resolve_creating(&to, imbalance);

crates/precompile-currency-swap/src/mock.rs

@@ -13,7 +13,7 @@ use frame_support::{
         traits::{BlakeTwo256, IdentityLookup},
         BuildStorage, DispatchError,
     },
-    traits::{ConstU16, ConstU32, ConstU64, Currency},
+    traits::{ConstU16, ConstU32, ConstU64},
     weights::Weight,
 };
 use frame_system as system;
@@ -190,8 +190,11 @@ mock! {
         type Error = DispatchError;
 
         fn swap(
-            imbalance: <EvmBalances as Currency<EvmAccountId>>::NegativeImbalance,
-        ) -> Result<<Balances as Currency<AccountId>>::NegativeImbalance, DispatchError>;
+            imbalance: primitives_currency_swap::FromNegativeImbalanceFor<Self, EvmAccountId, AccountId>,
+        ) -> Result<
+            primitives_currency_swap::ToNegativeImbalanceFor<Self, EvmAccountId, AccountId>,
+            primitives_currency_swap::ErrorFor<Self, EvmAccountId, AccountId>,
+        >;
     }
 }
 

crates/precompile-currency-swap/src/tests.rs

@@ -88,6 +88,7 @@ fn swap_works() {
             alice_evm_balance - swap_balance - expected_fee
         );
         assert_eq!(Balances::total_balance(&alice), swap_balance);
+        assert_eq!(EvmBalances::total_balance(&PRECOMPILE_ADDRESS), 0);
 
         // Assert mock invocations.
         swap_ctx.checkpoint();
@@ -174,6 +175,7 @@ fn swap_works_almost_full_balance() {
         );
         assert_eq!(EvmBalances::total_balance(&alice_evm), 1);
         assert_eq!(Balances::total_balance(&alice), swap_balance);
+        assert_eq!(EvmBalances::total_balance(&PRECOMPILE_ADDRESS), 0);
 
         // Assert mock invocations.
         swap_ctx.checkpoint();
@@ -243,6 +245,7 @@ fn swap_fail_no_funds() {
         // Assert state changes.
         assert_eq!(EvmBalances::total_balance(&alice_evm), alice_evm_balance);
         assert_eq!(Balances::total_balance(&alice), 0);
+        assert_eq!(EvmBalances::total_balance(&PRECOMPILE_ADDRESS), 0);
 
         // Assert mock invocations.
         swap_ctx.checkpoint();
@@ -286,7 +289,12 @@ fn swap_fail_trait_error() {
             .with(predicate::eq(
                 <EvmBalances as Currency<EvmAccountId>>::NegativeImbalance::new(swap_balance),
             ))
-            .return_once(move |_| Err(sp_runtime::DispatchError::Other("test")));
+            .return_once(move |incoming_imbalance| {
+                Err(primitives_currency_swap::Error {
+                    cause: sp_runtime::DispatchError::Other("test"),
+                    incoming_imbalance,
+                })
+            });
 
         // Prepare EVM call.
         let input = EvmDataWriter::new_with_selector(Action::Swap)
@@ -325,6 +333,7 @@ fn swap_fail_trait_error() {
             alice_evm_balance - expected_fee
         );
         assert_eq!(Balances::total_balance(&alice), 0);
+        assert_eq!(EvmBalances::total_balance(&PRECOMPILE_ADDRESS), 0);
 
         // Assert mock invocations.
         swap_ctx.checkpoint();
@@ -401,6 +410,7 @@ fn swap_fail_full_balance() {
         // Assert state changes.
         assert_eq!(EvmBalances::total_balance(&alice_evm), alice_evm_balance);
         assert_eq!(Balances::total_balance(&alice), 0);
+        assert_eq!(EvmBalances::total_balance(&PRECOMPILE_ADDRESS), 0);
 
         // Assert mock invocations.
         swap_ctx.checkpoint();
@@ -475,6 +485,7 @@ fn swap_fail_bad_selector() {
             alice_evm_balance - expected_fee
         );
         assert_eq!(Balances::total_balance(&alice), 0);
+        assert_eq!(EvmBalances::total_balance(&PRECOMPILE_ADDRESS), 0);
 
         // Assert mock invocations.
         swap_ctx.checkpoint();
@@ -547,6 +558,7 @@ fn swap_fail_value_overflow() {
         // Assert state changes.
         assert_eq!(EvmBalances::total_balance(&alice_evm), alice_evm_balance);
         assert_eq!(Balances::total_balance(&alice), 0);
+        assert_eq!(EvmBalances::total_balance(&PRECOMPILE_ADDRESS), 0);
 
         // Assert mock invocations.
         swap_ctx.checkpoint();
@@ -621,6 +633,7 @@ fn swap_fail_no_arguments() {
             alice_evm_balance - expected_fee
         );
         assert_eq!(Balances::total_balance(&alice), 0);
+        assert_eq!(EvmBalances::total_balance(&PRECOMPILE_ADDRESS), 0);
 
         // Assert mock invocations.
         swap_ctx.checkpoint();
@@ -696,6 +709,7 @@ fn swap_fail_short_argument() {
             alice_evm_balance - expected_fee
         );
         assert_eq!(Balances::total_balance(&alice), 0);
+        assert_eq!(EvmBalances::total_balance(&PRECOMPILE_ADDRESS), 0);
 
         // Assert mock invocations.
         swap_ctx.checkpoint();
@@ -771,6 +785,7 @@ fn swap_fail_trailing_junk() {
             alice_evm_balance - expected_fee
         );
         assert_eq!(Balances::total_balance(&alice), 0);
+        assert_eq!(EvmBalances::total_balance(&PRECOMPILE_ADDRESS), 0);
 
         // Assert mock invocations.
         swap_ctx.checkpoint();

crates/primitives-currency-swap/src/lib.rs

@@ -18,6 +18,36 @@ pub trait CurrencySwap<AccountIdFrom, AccountIdTo> {
 
     /// The actual swap logic.
     fn swap(
-        imbalance: <Self::From as Currency<AccountIdFrom>>::NegativeImbalance,
-    ) -> Result<<Self::To as Currency<AccountIdTo>>::NegativeImbalance, Self::Error>;
+        imbalance: FromNegativeImbalanceFor<Self, AccountIdFrom, AccountIdTo>,
+    ) -> Result<
+        ToNegativeImbalanceFor<Self, AccountIdFrom, AccountIdTo>,
+        ErrorFor<Self, AccountIdFrom, AccountIdTo>,
+    >;
+}
+
+/// An easy way to access the [`Currency::NegativeImbalance`] of [`CurrencySwap::From`] of `T`.
+pub type FromNegativeImbalanceFor<T, AccountIdFrom, AccountIdTo> = <<T as CurrencySwap<
+    AccountIdFrom,
+    AccountIdTo,
+>>::From as Currency<AccountIdFrom>>::NegativeImbalance;
+
+/// An easy way to access the [`Currency::NegativeImbalance`] of [`CurrencySwap::To`] of `T`.
+pub type ToNegativeImbalanceFor<T, AccountIdFrom, AccountIdTo> = <<T as CurrencySwap<
+    AccountIdFrom,
+    AccountIdTo,
+>>::To as Currency<AccountIdTo>>::NegativeImbalance;
+
+/// A type alias for compact declaration of the error type for the [`CurrencySwap::swap`] call.
+pub type ErrorFor<T, AccountIdFrom, AccountIdTo> = Error<
+    FromNegativeImbalanceFor<T, AccountIdFrom, AccountIdTo>,
+    <T as CurrencySwap<AccountIdFrom, AccountIdTo>>::Error,
+>;
+
+/// An error that can occur while doing a currency swap.
+#[derive(Debug)]
+pub struct Error<I, E> {
+    /// The underlying cause of this error.
+    pub cause: E,
+    /// The original imbalance that was passed to the swap operation.
+    pub incoming_imbalance: I,
 }