Add Deposit and Withdraw Events to Balances Pallet (#9425)

* add Deposit and Withdraw events to balances

+ add deposit_event() calls where appropriate to signal fund movement
+ adjust and extend tests

* line length

* move events to the end to avoid changing indices

* bump spec_version

* cargo fmt

* adjust block import bench to new event count

* fix node executor tests

* adjust import bench comment

* fix typo and formatting

* adjust event number

* fix copy pasta

* fix contracts pallets tests

* cargo fmt

* WIP fix events in tests

* fix offences tests

* fix tests

* cargo +nightly fmt

* fix contracts pallets tests

* cargo +nightly fmt

* fix offences tests

* formatting and compile fixes

Co-authored-by: Shawn Tabrizi <shawntabrizi@gmail.com>

substrate/frame/balances/src/lib.rs

@@ -463,8 +463,6 @@ pub mod pallet {
 		Transfer(T::AccountId, T::AccountId, T::Balance),
 		/// A balance was set by root. \[who, free, reserved\]
 		BalanceSet(T::AccountId, T::Balance, T::Balance),
-		/// Some amount was deposited (e.g. for transaction fees). \[who, deposit\]
-		Deposit(T::AccountId, T::Balance),
 		/// Some balance was reserved (moved from free to reserved). \[who, value\]
 		Reserved(T::AccountId, T::Balance),
 		/// Some balance was unreserved (moved from reserved to free). \[who, value\]
@@ -473,6 +471,14 @@ pub mod pallet {
 		/// Final argument indicates the destination balance type.
 		/// \[from, to, balance, destination_status\]
 		ReserveRepatriated(T::AccountId, T::AccountId, T::Balance, Status),
+		/// Some amount was deposited into the account (e.g. for transaction fees). \[who,
+		/// deposit\]
+		Deposit(T::AccountId, T::Balance),
+		/// Some amount was withdrawn from the account (e.g. for transaction fees). \[who, value\]
+		Withdraw(T::AccountId, T::Balance),
+		/// Some amount was removed from the account (e.g. for misbehavior). \[who,
+		/// amount_slashed\]
+		Slashed(T::AccountId, T::Balance),
 	}
 
 	/// Old name generated by `decl_event`.
@@ -1103,6 +1109,7 @@ impl<T: Config<I>, I: 'static> fungible::Mutate<T::AccountId> for Pallet<T, I> {
 			Ok(())
 		})?;
 		TotalIssuance::<T, I>::mutate(|t| *t += amount);
+		Self::deposit_event(Event::Deposit(who.clone(), amount));
 		Ok(())
 	}
 
@@ -1123,6 +1130,7 @@ impl<T: Config<I>, I: 'static> fungible::Mutate<T::AccountId> for Pallet<T, I> {
 			},
 		)?;
 		TotalIssuance::<T, I>::mutate(|t| *t -= actual);
+		Self::deposit_event(Event::Withdraw(who.clone(), amount));
 		Ok(actual)
 	}
 }
@@ -1141,7 +1149,10 @@ impl<T: Config<I>, I: 'static> fungible::Transfer<T::AccountId> for Pallet<T, I>
 
 impl<T: Config<I>, I: 'static> fungible::Unbalanced<T::AccountId> for Pallet<T, I> {
 	fn set_balance(who: &T::AccountId, amount: Self::Balance) -> DispatchResult {
-		Self::mutate_account(who, |account| account.free = amount)?;
+		Self::mutate_account(who, |account| {
+			account.free = amount;
+			Self::deposit_event(Event::BalanceSet(who.clone(), account.free, account.reserved));
+		})?;
 		Ok(())
 	}
 
@@ -1583,7 +1594,13 @@ where
 					}
 				},
 			) {
-				Ok(r) => return r,
+				Ok((imbalance, not_slashed)) => {
+					Self::deposit_event(Event::Slashed(
+						who.clone(),
+						value.saturating_sub(not_slashed),
+					));
+					return (imbalance, not_slashed)
+				},
 				Err(_) => (),
 			}
 		}
@@ -1608,6 +1625,7 @@ where
 			|account, is_new| -> Result<Self::PositiveImbalance, DispatchError> {
 				ensure!(!is_new, Error::<T, I>::DeadAccount);
 				account.free = account.free.checked_add(&value).ok_or(ArithmeticError::Overflow)?;
+				Self::deposit_event(Event::Deposit(who.clone(), value));
 				Ok(PositiveImbalance::new(value))
 			},
 		)
@@ -1640,6 +1658,7 @@ where
 					None => return Ok(Self::PositiveImbalance::zero()),
 				};
 
+				Self::deposit_event(Event::Deposit(who.clone(), value));
 				Ok(PositiveImbalance::new(value))
 			},
 		)
@@ -1677,6 +1696,7 @@ where
 
 				account.free = new_free_account;
 
+				Self::deposit_event(Event::Withdraw(who.clone(), value));
 				Ok(NegativeImbalance::new(value))
 			},
 		)
@@ -1709,6 +1729,7 @@ where
 					SignedImbalance::Negative(NegativeImbalance::new(account.free - value))
 				};
 				account.free = value;
+				Self::deposit_event(Event::BalanceSet(who.clone(), account.free, account.reserved));
 				Ok(imbalance)
 			},
 		)
@@ -1824,7 +1845,13 @@ where
 				// underflow should never happen, but it if does, there's nothing to be done here.
 				(NegativeImbalance::new(actual), value - actual)
 			}) {
-				Ok(r) => return r,
+				Ok((imbalance, not_slashed)) => {
+					Self::deposit_event(Event::Slashed(
+						who.clone(),
+						value.saturating_sub(not_slashed),
+					));
+					return (imbalance, not_slashed)
+				},
 				Err(_) => (),
 			}
 		}
@@ -1965,6 +1992,7 @@ where
 					// `actual <= to_change` and `to_change <= amount`; qed;
 					reserves[index].amount -= actual;
 
+					Self::deposit_event(Event::Slashed(who.clone(), actual));
 					(imb, value - actual)
 				},
 				Err(_) => (NegativeImbalance::zero(), value),

substrate/frame/balances/src/tests.rs

@@ -314,6 +314,7 @@ macro_rules! decl_tests {
 			<$ext_builder>::default().monied(true).build().execute_with(|| {
 				assert_eq!(Balances::total_balance(&1), 10);
 				assert_ok!(Balances::deposit_into_existing(&1, 10).map(drop));
+				System::assert_last_event(Event::Balances(crate::Event::Deposit(1, 10)));
 				assert_eq!(Balances::total_balance(&1), 20);
 				assert_eq!(<TotalIssuance<$test>>::get(), 120);
 			});
@@ -341,6 +342,7 @@ macro_rules! decl_tests {
 		fn balance_works() {
 			<$ext_builder>::default().build().execute_with(|| {
 				let _ = Balances::deposit_creating(&1, 42);
+				System::assert_has_event(Event::Balances(crate::Event::Deposit(1, 42)));
 				assert_eq!(Balances::free_balance(1), 42);
 				assert_eq!(Balances::reserved_balance(1), 0);
 				assert_eq!(Balances::total_balance(&1), 42);
@@ -435,6 +437,19 @@ macro_rules! decl_tests {
 			});
 		}
 
+		#[test]
+		fn withdrawing_balance_should_work() {
+			<$ext_builder>::default().build().execute_with(|| {
+				let _ = Balances::deposit_creating(&2, 111);
+				let _ = Balances::withdraw(
+					&2, 11, WithdrawReasons::TRANSFER, ExistenceRequirement::KeepAlive
+				);
+				System::assert_last_event(Event::Balances(crate::Event::Withdraw(2, 11)));
+				assert_eq!(Balances::free_balance(2), 100);
+				assert_eq!(<TotalIssuance<$test>>::get(), 100);
+			});
+		}
+
 		#[test]
 		fn slashing_incomplete_balance_should_work() {
 			<$ext_builder>::default().build().execute_with(|| {
@@ -749,6 +764,7 @@ macro_rules! decl_tests {
 						[
 							Event::System(system::Event::KilledAccount(1)),
 							Event::Balances(crate::Event::DustLost(1, 99)),
+							Event::Balances(crate::Event::Slashed(1, 1)),
 						]
 					);
 				});
@@ -777,7 +793,8 @@ macro_rules! decl_tests {
 					assert_eq!(
 						events(),
 						[
-							Event::System(system::Event::KilledAccount(1))
+							Event::System(system::Event::KilledAccount(1)),
+							Event::Balances(crate::Event::Slashed(1, 100)),
 						]
 					);
 				});
@@ -797,6 +814,7 @@ macro_rules! decl_tests {
 					assert_eq!(Balances::slash(&1, 900), (NegativeImbalance::new(900), 0));
 					// Account is still alive
 					assert!(System::account_exists(&1));
+					System::assert_last_event(Event::Balances(crate::Event::Slashed(1, 900)));
 
 					// SCENARIO: Slash will kill account because not enough balance left.
 					assert_ok!(Balances::set_balance(Origin::root(), 1, 1_000, 0));

substrate/frame/balances/src/tests_local.rs

@@ -173,7 +173,7 @@ fn emit_events_with_no_existential_deposit_suicide_with_dust() {
 		assert_eq!(res, (NegativeImbalance::new(98), 0));
 
 		// no events
-		assert_eq!(events(), []);
+		assert_eq!(events(), [Event::Balances(crate::Event::Slashed(1, 98))]);
 
 		let res = Balances::slash(&1, 1);
 		assert_eq!(res, (NegativeImbalance::new(1), 0));
@@ -183,6 +183,7 @@ fn emit_events_with_no_existential_deposit_suicide_with_dust() {
 			[
 				Event::System(system::Event::KilledAccount(1)),
 				Event::Balances(crate::Event::DustLost(1, 1)),
+				Event::Balances(crate::Event::Slashed(1, 1)),
 			]
 		);
 	});

substrate/frame/balances/src/tests_reentrancy.rs

@@ -167,11 +167,11 @@ fn transfer_dust_removal_tst1_should_work() {
 		assert_eq!(Balances::free_balance(&1), 1050);
 
 		// Verify the events
-		// Number of events expected is 8
-		assert_eq!(System::events().len(), 11);
+		assert_eq!(System::events().len(), 12);
 
 		System::assert_has_event(Event::Balances(crate::Event::Transfer(2, 3, 450)));
 		System::assert_has_event(Event::Balances(crate::Event::DustLost(2, 50)));
+		System::assert_has_event(Event::Balances(crate::Event::Deposit(1, 50)));
 	});
 }
 
@@ -195,11 +195,11 @@ fn transfer_dust_removal_tst2_should_work() {
 		assert_eq!(Balances::free_balance(&1), 1500);
 
 		// Verify the events
-		// Number of events expected is 8
-		assert_eq!(System::events().len(), 9);
+		assert_eq!(System::events().len(), 10);
 
 		System::assert_has_event(Event::Balances(crate::Event::Transfer(2, 1, 450)));
 		System::assert_has_event(Event::Balances(crate::Event::DustLost(2, 50)));
+		System::assert_has_event(Event::Balances(crate::Event::Deposit(1, 50)));
 	});
 }
 
@@ -232,8 +232,7 @@ fn repatriating_reserved_balance_dust_removal_should_work() {
 		assert_eq!(Balances::free_balance(1), 1500);
 
 		// Verify the events
-		// Number of events expected is 10
-		assert_eq!(System::events().len(), 10);
+		assert_eq!(System::events().len(), 11);
 
 		System::assert_has_event(Event::Balances(crate::Event::ReserveRepatriated(
 			2,
@@ -241,7 +240,7 @@ fn repatriating_reserved_balance_dust_removal_should_work() {
 			450,
 			Status::Free,
 		)));
-
-		System::assert_last_event(Event::Balances(crate::Event::DustLost(2, 50)));
+		System::assert_has_event(Event::Balances(crate::Event::DustLost(2, 50)));
+		System::assert_last_event(Event::Balances(crate::Event::Deposit(1, 50)));
 	});
 }