diff --git a/bridges/bin/node/runtime/src/lib.rs b/bridges/bin/node/runtime/src/lib.rs index 450bd4d8791d..b967cf07f760 100644 --- a/bridges/bin/node/runtime/src/lib.rs +++ b/bridges/bin/node/runtime/src/lib.rs @@ -49,7 +49,7 @@ use sp_version::RuntimeVersion; // A few exports that help ease life for downstream crates. pub use frame_support::{ construct_runtime, parameter_types, - traits::{Currency, ExistenceRequirement, KeyOwnerProofSystem, Randomness}, + traits::{Currency, ExistenceRequirement, Imbalance, KeyOwnerProofSystem, Randomness}, weights::{IdentityFee, RuntimeDbWeight, Weight}, StorageValue, }; @@ -246,26 +246,53 @@ impl sp_currency_exchange::DepositInto for DepositInto { type Amount = Balance; fn deposit_into(recipient: Self::Recipient, amount: Self::Amount) -> sp_currency_exchange::Result<()> { - as Currency>::deposit_into_existing(&recipient, amount) - .map(|_| { + // let balances module make all checks for us (it won't allow depositing lower than existential + // deposit, balance overflow, ...) + let deposited = as Currency>::deposit_creating(&recipient, amount); + + // I'm dropping deposited here explicitly to illustrate the fact that it'll update `TotalIssuance` + // on drop + let deposited_amount = deposited.peek(); + drop(deposited); + + // we have 3 cases here: + // - deposited == amount: success + // - deposited == 0: deposit has failed and no changes to storage were made + // - deposited != 0: (should never happen in practice) deposit has been partially completed + match deposited_amount { + _ if deposited_amount == amount => { frame_support::debug::trace!( target: "runtime", "Deposited {} to {:?}", amount, recipient, ); - }) - .map_err(|e| { + + Ok(()) + } + _ if deposited_amount == 0 => { + frame_support::debug::error!( + target: "runtime", + "Deposit of {} to {:?} has failed", + amount, + recipient, + ); + + Err(sp_currency_exchange::Error::DepositFailed) + } + _ => { frame_support::debug::error!( target: "runtime", - "Deposit of {} to {:?} has failed with: {:?}", + "Deposit of {} to {:?} has partially competed. {} has been deposited", amount, recipient, - e + deposited_amount, ); - sp_currency_exchange::Error::DepositFailed - }) + // we can't return DepositFailed error here, because storage changes were made + Err(sp_currency_exchange::Error::DepositPartiallyFailed) + } + } } } @@ -599,6 +626,7 @@ impl_runtime_apis! { #[cfg(test)] mod tests { use super::*; + use sp_currency_exchange::DepositInto; #[test] fn shift_session_manager_works() { @@ -639,4 +667,77 @@ mod tests { vec![acc1, acc2, acc3], ); } + + fn run_deposit_into_test(test: impl Fn(AccountId) -> Balance) { + let mut ext: sp_io::TestExternalities = SystemConfig::default().build_storage::().unwrap().into(); + ext.execute_with(|| { + // initially issuance is zero + assert_eq!( + as Currency>::total_issuance(), + 0, + ); + + // create account + let account: AccountId = [1u8; 32].into(); + let initial_amount = ExistentialDeposit::get(); + let deposited = + as Currency>::deposit_creating(&account, initial_amount); + drop(deposited); + assert_eq!( + as Currency>::total_issuance(), + initial_amount, + ); + assert_eq!( + as Currency>::free_balance(&account), + initial_amount, + ); + + // run test + let total_issuance_change = test(account); + + // check that total issuance has changed by `run_deposit_into_test` + assert_eq!( + as Currency>::total_issuance(), + initial_amount + total_issuance_change, + ); + }); + } + + #[test] + fn deposit_into_existing_account_works() { + run_deposit_into_test(|existing_account| { + let initial_amount = + as Currency>::free_balance(&existing_account); + let additional_amount = 10_000; + ::DepositInto::deposit_into( + existing_account.clone(), + additional_amount, + ) + .unwrap(); + assert_eq!( + as Currency>::free_balance(&existing_account), + initial_amount + additional_amount, + ); + additional_amount + }); + } + + #[test] + fn deposit_into_new_account_works() { + run_deposit_into_test(|_| { + let initial_amount = 0; + let additional_amount = ExistentialDeposit::get() + 10_000; + let new_account: AccountId = [42u8; 32].into(); + ::DepositInto::deposit_into( + new_account.clone(), + additional_amount, + ) + .unwrap(); + assert_eq!( + as Currency>::free_balance(&new_account), + initial_amount + additional_amount, + ); + additional_amount + }); + } } diff --git a/bridges/modules/currency-exchange/src/lib.rs b/bridges/modules/currency-exchange/src/lib.rs index a4ef7360f9ed..cfefd5b4adb3 100644 --- a/bridges/modules/currency-exchange/src/lib.rs +++ b/bridges/modules/currency-exchange/src/lib.rs @@ -83,6 +83,8 @@ decl_error! { FailedToConvertCurrency, /// Deposit has failed. DepositFailed, + /// Deposit has partially failed (changes to recipient account were made). + DepositPartiallyFailed, /// Transaction is not finalized. UnfinalizedTransaction, /// Transaction funds are already claimed. @@ -121,7 +123,14 @@ decl_module! { // make sure to update the mapping if we deposit successfully to avoid double spending, // i.e. whenever `deposit_into` is successful we MUST update `Transfers`. { - T::DepositInto::deposit_into(recipient, amount).map_err(Error::::from)?; + // if any changes were made to the storage, we can't just return error here, because + // otherwise the same proof may be imported again + let deposit_result = T::DepositInto::deposit_into(recipient, amount); + match deposit_result { + Ok(_) => (), + Err(ExchangeError::DepositPartiallyFailed) => (), + Err(error) => Err(Error::::from(error))?, + } Transfers::::insert(transfer_id, ()) } @@ -149,6 +158,7 @@ impl From for Error { ExchangeError::FailedToMapRecipients => Error::FailedToMapRecipients, ExchangeError::FailedToConvertCurrency => Error::FailedToConvertCurrency, ExchangeError::DepositFailed => Error::DepositFailed, + ExchangeError::DepositPartiallyFailed => Error::DepositPartiallyFailed, } } } @@ -254,9 +264,12 @@ mod tests { type Amount = u64; fn deposit_into(_recipient: Self::Recipient, amount: Self::Amount) -> sp_currency_exchange::Result<()> { - match amount > MAX_DEPOSIT_AMOUNT { - true => Err(sp_currency_exchange::Error::DepositFailed), - _ => Ok(()), + if amount < MAX_DEPOSIT_AMOUNT * 10 { + Ok(()) + } else if amount == MAX_DEPOSIT_AMOUNT * 10 { + Err(ExchangeError::DepositPartiallyFailed) + } else { + Err(ExchangeError::DepositFailed) } } } @@ -393,7 +406,7 @@ mod tests { fn transaction_with_invalid_deposit_rejected() { new_test_ext().execute_with(|| { let mut transaction = transaction(0); - transaction.amount = MAX_DEPOSIT_AMOUNT; + transaction.amount = MAX_DEPOSIT_AMOUNT + 1; assert_noop!( Exchange::import_peer_transaction(Origin::signed(SUBMITTER), (true, transaction)), Error::::DepositFailed, @@ -401,6 +414,23 @@ mod tests { }); } + #[test] + fn valid_transaction_accepted_even_if_deposit_partially_fails() { + new_test_ext().execute_with(|| { + let mut transaction = transaction(0); + transaction.amount = MAX_DEPOSIT_AMOUNT; + assert_ok!(Exchange::import_peer_transaction( + Origin::signed(SUBMITTER), + (true, transaction), + ),); + + // ensure that the transfer has been marked as completed + assert!(::Transfers::contains_key(0u64)); + // ensure that submitter has been rewarded + assert!(::Transfers::contains_key(SUBMITTER)); + }); + } + #[test] fn valid_transaction_accepted() { new_test_ext().execute_with(|| { diff --git a/bridges/primitives/currency-exchange/src/lib.rs b/bridges/primitives/currency-exchange/src/lib.rs index 59e411c1bcfb..f864d4b733a9 100644 --- a/bridges/primitives/currency-exchange/src/lib.rs +++ b/bridges/primitives/currency-exchange/src/lib.rs @@ -35,6 +35,8 @@ pub enum Error { FailedToConvertCurrency, /// Deposit has failed. DepositFailed, + /// Deposit has partially failed (changes to recipient account were made). + DepositPartiallyFailed, } /// Result of all exchange operations.