bugfix: balances::transfer for new_account issue#722

srml/balances/src/lib.rs

@@ -284,7 +284,8 @@ impl<T: Trait> Module<T> {
 
 		let dest = Self::lookup(dest)?;
 		let from_balance = Self::free_balance(&transactor);
-		let would_create = from_balance.is_zero();
+		let to_balance = Self::free_balance(&dest);
+		let would_create = to_balance.is_zero();
 		let fee = if would_create { Self::creation_fee() } else { Self::transfer_fee() };
 		let liability = value + fee;
 
@@ -297,7 +298,6 @@ impl<T: Trait> Module<T> {
 		}
 		T::EnsureAccountLiquid::ensure_account_liquid(&transactor)?;
 
-		let to_balance = Self::free_balance(&dest);
 		// NOTE: total stake being stored in the same type means that this could never overflow
 		// but better to be safe than sorry.
 		let new_to_balance = match to_balance.checked_add(&value) {

srml/balances/src/mock.rs

@@ -73,5 +73,28 @@ pub fn new_test_ext(ext_deposit: u64, monied: bool) -> runtime_io::TestExternali
 	t.into()
 }
 
+pub fn new_test_ext2(ext_deposit: u64, monied: bool) -> runtime_io::TestExternalities<Blake2Hasher> {
+	let mut t = system::GenesisConfig::<Runtime>::default().build_storage().unwrap();
+	let balance_factor = if ext_deposit > 0 {
+		256
+	} else {
+		1
+	};
+	t.extend(GenesisConfig::<Runtime>{
+		balances: if monied {
+			vec![(1, 10 * balance_factor), (2, 20 * balance_factor), (3, 30 * balance_factor), (4, 40 * balance_factor)]
+		} else {
+			vec![(10, balance_factor), (20, balance_factor)]
+		},
+		transaction_base_fee: 0,
+		transaction_byte_fee: 0,
+		existential_deposit: ext_deposit,
+		transfer_fee: 10,  // transfer_fee not zero
+		creation_fee: 50, // creation_fee not zero
+		reclaim_rebate: 0,
+	}.build_storage().unwrap());
+	t.into()
+}
+
 pub type System = system::Module<Runtime>;
 pub type Balances = Module<Runtime>;

srml/balances/src/tests.rs

@@ -20,7 +20,7 @@
 
 use super::*;
 use runtime_io::with_externalities;
-use mock::{Balances, System, Runtime, new_test_ext};
+use mock::{Balances, System, Runtime, new_test_ext, new_test_ext2};
 
 #[test]
 fn reward_should_work() {
@@ -52,6 +52,29 @@ fn default_indexing_on_new_accounts_should_work() {
 	});
 }
 
+#[test]
+fn default_indexing_on_new_accounts_should_work2() {
+	with_externalities(&mut new_test_ext2(10, true), || {
+		assert_eq!(Balances::lookup_index(4), None);
+		// account 1 has 256 * 10 = 2560, account 5 is not exist, ext_deposit is 10, value is 10
+		assert_ok!(Balances::transfer(Some(1).into(), 5.into(), 10));
+		assert_eq!(Balances::lookup_index(4), Some(5));
+
+		assert_eq!(Balances::free_balance(&1), 256 * 10 - 10 - 50); // 10 is value, 50 is creation_free
+	});
+}
+
+#[test]
+fn default_indexing_on_new_accounts_should_not_work2() {
+	with_externalities(&mut new_test_ext2(10, true), || {
+		assert_eq!(Balances::lookup_index(4), None);
+		// account 1 has 256 * 10 = 2560, account 5 is not exist, ext_deposit is 10, value is 9, not satisfies for ext_deposit
+		assert_noop!(Balances::transfer(Some(1).into(), 5.into(), 9), "value too low to create account");
+		assert_eq!(Balances::lookup_index(4), None); // account 5 should not exist
+		assert_eq!(Balances::free_balance(&1), 256 * 10);
+	});
+}
+
 #[test]
 fn dust_account_removal_should_work() {
 	with_externalities(&mut new_test_ext(256 * 10, true), || {
@@ -66,6 +89,19 @@ fn dust_account_removal_should_work() {
 	});
 }
 
+#[test]
+fn dust_account_removal_should_work2() {
+	with_externalities(&mut new_test_ext2(256 * 10, true), || {
+		System::inc_account_nonce(&2);
+		assert_eq!(System::account_nonce(&2), 1);
+		assert_eq!(Balances::total_balance(&2), 256 * 20);
+		assert_ok!(Balances::transfer(Some(2).into(), 5.into(), 256 * 10));	// index 1 (account 2) becomes zombie for 256*10 + 50(fee) < 256 * 10 (ext_deposit)
+		assert_eq!(Balances::total_balance(&2), 0);
+		assert_eq!(Balances::total_balance(&5), 256 * 10);
+		assert_eq!(System::account_nonce(&2), 0);
+	});
+}
+
 #[test]
 fn reclaim_indexing_on_new_accounts_should_work() {
 	with_externalities(&mut new_test_ext(256 * 1, true), || {
@@ -82,6 +118,22 @@ fn reclaim_indexing_on_new_accounts_should_work() {
 	});
 }
 
+#[test]
+fn reclaim_indexing_on_new_accounts_should_work2() {
+	with_externalities(&mut new_test_ext2(256 * 1, true), || {
+		assert_eq!(Balances::lookup_index(1), Some(2));
+		assert_eq!(Balances::lookup_index(4), None);
+		assert_eq!(Balances::total_balance(&2), 256 * 20);
+
+		assert_ok!(Balances::transfer(Some(2).into(), 5.into(), 256 * 20 - 50));	// account 2 becomes zombie freeing index 1 for reclaim) 50 is creation fee
+		assert_eq!(Balances::total_balance(&2), 0);
+
+		assert_ok!(Balances::transfer(Some(5).into(), 6.into(), 256 * 1 + 0x69));	// account 6 takes index 1.
+		assert_eq!(Balances::total_balance(&6), 256 * 1 + 0x69);
+		assert_eq!(Balances::lookup_index(1), Some(6));
+	});
+}
+
 #[test]
 fn reserved_balance_should_prevent_reclaim_count() {
 	with_externalities(&mut new_test_ext(256 * 1, true), || {
@@ -110,6 +162,34 @@ fn reserved_balance_should_prevent_reclaim_count() {
 	});
 }
 
+#[test]
+fn reserved_balance_should_prevent_reclaim_count2() {
+	with_externalities(&mut new_test_ext2(256 * 1, true), || {
+		System::inc_account_nonce(&2);
+		assert_eq!(Balances::lookup_index(1), Some(2));
+		assert_eq!(Balances::lookup_index(4), None);
+		assert_eq!(Balances::total_balance(&2), 256 * 20);
+
+		assert_ok!(Balances::reserve(&2, 256 * 19 + 1));					// account 2 becomes mostly reserved
+		assert_eq!(Balances::free_balance(&2), 0);						// "free" account deleted."
+		assert_eq!(Balances::total_balance(&2), 256 * 19 + 1);			// reserve still exists.
+		assert_eq!(System::account_nonce(&2), 1);
+
+		assert_ok!(Balances::transfer(Some(4).into(), 5.into(), 256 * 1 + 0x69));	// account 4 tries to take index 1 for account 5.
+		assert_eq!(Balances::total_balance(&5), 256 * 1 + 0x69);
+		assert_eq!(Balances::lookup_index(1), Some(2));					// but fails.
+		assert_eq!(System::account_nonce(&2), 1);
+
+		assert_eq!(Balances::slash(&2, 256 * 18 + 2), None);				// account 2 gets slashed
+		assert_eq!(Balances::total_balance(&2), 0);						// "free" account deleted."
+		assert_eq!(System::account_nonce(&2), 0);
+
+		assert_ok!(Balances::transfer(Some(4).into(), 6.into(), 256 * 1 + 0x69));	// account 4 tries to take index 1 again for account 6.
+		assert_eq!(Balances::total_balance(&6), 256 * 1 + 0x69);
+		assert_eq!(Balances::lookup_index(1), Some(6));					// and succeeds.
+	});
+}
+
 #[test]
 fn balance_works() {
 	with_externalities(&mut new_test_ext(0, false), || {