Create accounts from DepositInto::deposit_into() (paritytech#157)

* use deposit_creating in DepositInto * Update modules/currency-exchange/src/lib.rs Co-authored-by: Tomasz Drwięga <tomusdrw@users.noreply.github.com> * Update primitives/currency-exchange/src/lib.rs Co-authored-by: Tomasz Drwięga <tomusdrw@users.noreply.github.com> Co-authored-by: Tomasz Drwięga <tomusdrw@users.noreply.github.com>
jonathanudd · Apr 10, 2024 · 201740f · 201740f
1 parent b497505
commit 201740f
Show file tree

Hide file tree

Showing 3 changed files with 147 additions and 14 deletions.
diff --git 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<()> {
-		<pallet_balances::Module<Runtime> as Currency<AccountId>>::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 = <pallet_balances::Module<Runtime> as Currency<AccountId>>::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::<Runtime>().unwrap().into();
+		ext.execute_with(|| {
+			// initially issuance is zero
+			assert_eq!(
+				<pallet_balances::Module<Runtime> as Currency<AccountId>>::total_issuance(),
+				0,
+			);
+
+			// create account
+			let account: AccountId = [1u8; 32].into();
+			let initial_amount = ExistentialDeposit::get();
+			let deposited =
+				<pallet_balances::Module<Runtime> as Currency<AccountId>>::deposit_creating(&account, initial_amount);
+			drop(deposited);
+			assert_eq!(
+				<pallet_balances::Module<Runtime> as Currency<AccountId>>::total_issuance(),
+				initial_amount,
+			);
+			assert_eq!(
+				<pallet_balances::Module<Runtime> as Currency<AccountId>>::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!(
+				<pallet_balances::Module<Runtime> as Currency<AccountId>>::total_issuance(),
+				initial_amount + total_issuance_change,
+			);
+		});
+	}
+
+	#[test]
+	fn deposit_into_existing_account_works() {
+		run_deposit_into_test(|existing_account| {
+			let initial_amount =
+				<pallet_balances::Module<Runtime> as Currency<AccountId>>::free_balance(&existing_account);
+			let additional_amount = 10_000;
+			<Runtime as pallet_bridge_currency_exchange::Trait>::DepositInto::deposit_into(
+				existing_account.clone(),
+				additional_amount,
+			)
+			.unwrap();
+			assert_eq!(
+				<pallet_balances::Module<Runtime> as Currency<AccountId>>::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();
+			<Runtime as pallet_bridge_currency_exchange::Trait>::DepositInto::deposit_into(
+				new_account.clone(),
+				additional_amount,
+			)
+			.unwrap();
+			assert_eq!(
+				<pallet_balances::Module<Runtime> as Currency<AccountId>>::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
@@ -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::<T>::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::<T>::from(error))?,
+				}
 				Transfers::<T>::insert(transfer_id, ())
 			}
 
@@ -149,6 +158,7 @@ impl<T: Trait> From<ExchangeError> for Error<T> {
 			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,14 +406,31 @@ 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::<TestRuntime>::DepositFailed,
 			);
 		});
 	}
 
+	#[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!(<Exchange as crate::Store>::Transfers::contains_key(0u64));
+			// ensure that submitter has been rewarded
+			assert!(<Exchange as crate::Store>::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
@@ -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.