Improve deposits migration

pallet/account-migration/src/lib.rs

@@ -446,7 +446,7 @@ pub mod pallet {
 				if let Some(ds) = <Deposits<T>>::take(from) {
 					<pallet_balances::Pallet<T> as Currency<_>>::transfer(
 						to,
-						&darwinia_deposit::account_id(),
+						&<T as darwinia_deposit::Config>::Treasury::get(),
 						ds.iter().map(|d| d.value).sum(),
 						AllowDeath,
 					)?;

pallet/deposit/Cargo.toml

@@ -25,7 +25,6 @@ frame-support    = { workspace = true }
 frame-system     = { workspace = true }
 pallet-timestamp = { workspace = true }
 sp-core          = { workspace = true }
-sp-runtime       = { workspace = true }
 sp-std           = { workspace = true }
 # polkadot-sdk optional
 frame-benchmarking = { workspace = true, optional = true }
@@ -34,6 +33,7 @@ frame-benchmarking = { workspace = true, optional = true }
 # polkadot-sdk
 pallet-balances = { workspace = true, features = ["std"] }
 sp-io           = { workspace = true, features = ["std"] }
+sp-runtime      = { workspace = true, features = ["std"] }
 
 [features]
 default = ["std"]

pallet/deposit/src/benchmarking.rs

@@ -35,7 +35,6 @@ mod benchmarks {
 
 		<Pallet<T>>::set_deposit_contract(RawOrigin::Root.into(), a.clone()).unwrap();
 
-		T::Ring::make_free_balance_be(&account_id(), 2 << 126);
 		T::Ring::make_free_balance_be(&T::Treasury::get(), 2 << 126);
 
 		// Worst-case scenario:

pallet/deposit/src/lib.rs

@@ -40,7 +40,6 @@ pub use weights::WeightInfo;
 // core
 use core::marker::PhantomData;
 // crates.io
-use codec::FullCodec;
 use ethabi::{Function, Param, ParamType, StateMutability, Token};
 // darwinia
 use dc_types::{Balance, Moment};
@@ -50,11 +49,9 @@ use fp_evm::{CallOrCreateInfo, ExitReason};
 use frame_support::{
 	pallet_prelude::*,
 	traits::{Currency, ExistenceRequirement::AllowDeath, UnixTime},
-	PalletId,
 };
 use frame_system::pallet_prelude::*;
 use sp_core::H160;
-use sp_runtime::traits::AccountIdConversion;
 use sp_std::prelude::*;
 
 #[frame_support::pallet]
@@ -89,6 +86,8 @@ pub mod pallet {
 		DepositsClaimed { owner: T::AccountId, deposits: Vec<DepositId> },
 		/// Deposits have been migrated.
 		DepositsMigrated { owner: T::AccountId, deposits: Vec<DepositId> },
+		/// Migration interaction with deposit contract failed.
+		MigrationFailedOnContract { exit_reason: ExitReason },
 	}
 
 	#[pallet::error]
@@ -204,7 +203,7 @@ pub mod pallet {
 			let mut to_migrate = (0, Vec::new(), Vec::new());
 
 			// Take 0~10 deposits to migrate.
-			for d in deposits.by_ref().take(10) {
+			for d in deposits.by_ref().take(10).filter(|d| d.value != 0) {
 				if d.expired_time <= now {
 					to_claim.0 += d.value;
 					to_claim.1.push(d.id);
@@ -216,14 +215,13 @@ pub mod pallet {
 			}
 
 			if to_claim.0 != 0 {
-				T::Ring::transfer(&account_id(), who, to_claim.0, AllowDeath)?;
+				T::Ring::transfer(&T::Treasury::get(), who, to_claim.0, AllowDeath)?;
 				Self::deposit_event(Event::DepositsClaimed {
 					owner: who.clone(),
 					deposits: to_claim.1,
 				});
 			}
 			if to_migrate.0 != 0 {
-				T::Ring::transfer(&account_id(), &T::Treasury::get(), to_migrate.0, AllowDeath)?;
 				T::DepositMigrator::migrate(who.clone(), to_migrate.0, to_migrate.2)?;
 				Self::deposit_event(Event::DepositsMigrated {
 					owner: who.clone(),
@@ -334,15 +332,11 @@ where
 
 		match exit_reason {
 			ExitReason::Succeed(_) => Ok(()),
-			_ => Err(<Error<T>>::MigrationFailedOnContract)?,
+			exit_reason => {
+				<Pallet<T>>::deposit_event(Event::MigrationFailedOnContract { exit_reason });
+
+				Err(<Error<T>>::MigrationFailedOnContract)?
+			},
 		}
 	}
 }
-
-/// The account of the deposit pot.
-pub fn account_id<A>() -> A
-where
-	A: FullCodec,
-{
-	PalletId(*b"dar/depo").into_account_truncating()
-}

pallet/deposit/src/mock.rs

@@ -71,3 +71,7 @@ pub fn new_test_ext() -> TestExternalities {
 
 	storage.into()
 }
+
+pub fn events() -> Vec<Event<Runtime>> {
+	System::read_events_for_pallet()
+}

pallet/deposit/src/tests.rs

@@ -27,7 +27,7 @@ use frame_support::{assert_ok, traits::OnIdle};
 #[test]
 fn migrate_should_work() {
 	new_test_ext().execute_with(|| {
-		let _ = Balances::deposit_creating(&account_id(), 2);
+		let _ = Balances::deposit_creating(&0, 2);
 
 		<Deposits<Runtime>>::insert(
 			1,
@@ -52,9 +52,18 @@ fn on_idle_should_work() {
 				.collect(),
 		)
 	}
+	fn mock_zero_deposits(count: u16) -> BoundedVec<DepositS, ConstU32<512>> {
+		BoundedVec::truncate_from(
+			(0..count)
+				.map(|id| DepositS { id, value: 0, start_time: 0, expired_time: 0, in_use: false })
+				.collect(),
+		)
+	}
 
 	new_test_ext().execute_with(|| {
-		let _ = Balances::deposit_creating(&account_id(), 10_000);
+		System::set_block_number(1);
+
+		let _ = Balances::deposit_creating(&0, 10_000);
 
 		<Deposits<Runtime>>::insert(1, mock_deposits(512));
 		<Deposits<Runtime>>::insert(2, mock_deposits(512));
@@ -90,11 +99,28 @@ fn on_idle_should_work() {
 		assert_eq!(Deposit::deposit_of(2).unwrap().len(), 512);
 		assert!(Deposit::deposit_of(3).is_none());
 
+		System::reset_events();
 		(0..54).for_each(|_| {
 			<Deposit as OnIdle<_>>::on_idle(0, Weight::MAX);
 		});
+		assert_eq!(events().into_iter().count(), 54);
 		assert!(Deposit::deposit_of(1).is_none());
 		assert!(Deposit::deposit_of(2).is_none());
 		assert!(Deposit::deposit_of(3).is_none());
 	});
+	new_test_ext().execute_with(|| {
+		System::set_block_number(1);
+
+		let _ = Balances::deposit_creating(&0, 10_000);
+
+		<Deposits<Runtime>>::insert(1, mock_zero_deposits(512));
+		assert_eq!(Deposit::deposit_of(1).unwrap().len(), 512);
+
+		System::reset_events();
+		(0..52).for_each(|_| {
+			<Deposit as OnIdle<_>>::on_idle(0, Weight::MAX);
+		});
+		assert_eq!(events().into_iter().count(), 0);
+		assert!(Deposit::deposit_of(1).is_none());
+	});
 }

runtime/common/src/test.rs

@@ -304,10 +304,7 @@ macro_rules! impl_account_migration_tests {
 
 						assert_ok!(migrate(from, to));
 						assert_eq!(Balances::free_balance(to), 80);
-						assert_eq!(
-							Balances::free_balance(&darwinia_deposit::account_id::<AccountId>()),
-							20
-						);
+						assert_eq!(Balances::free_balance(&Treasury::account_id()), 20);
 						assert_eq!(Deposit::deposit_of(to).unwrap().len(), 2);
 						assert_eq!(Assets::maybe_balance(KTON_ID, to).unwrap(), 100);
 					});

runtime/crab/src/migration.rs

@@ -21,7 +21,7 @@
 use crate::*;
 // polkadot-sdk
 #[allow(unused_imports)]
-use frame_support::{migration, storage::unhashed};
+use frame_support::{migration, storage::unhashed, PalletId};
 
 pub struct CustomOnRuntimeUpgrade;
 impl frame_support::traits::OnRuntimeUpgrade for CustomOnRuntimeUpgrade {
@@ -52,6 +52,11 @@ fn migrate() -> frame_support::weights::Weight {
 	}
 
 	let (r, w) = darwinia_staking::migration::migrate::<Runtime>();
+	let _ = Balances::transfer_all(
+		RuntimeOrigin::signed(PalletId(*b"dar/depo").into_account_truncating()),
+		Treasury::account_id(),
+		false,
+	);
 
-	<Runtime as frame_system::Config>::DbWeight::get().reads_writes(r, w + 1)
+	<Runtime as frame_system::Config>::DbWeight::get().reads_writes(r, w + 6)
 }

runtime/darwinia/src/migration.rs

@@ -73,7 +73,8 @@ impl frame_support::traits::OnRuntimeUpgrade for CustomOnRuntimeUpgrade {
 
 fn migrate() -> frame_support::weights::Weight {
 	// polkadot-sdk
-	use frame_support::traits::LockableCurrency;
+	use frame_support::{traits::LockableCurrency, PalletId};
+	use sp_runtime::traits::AccountIdConversion;
 
 	let (r, w) = darwinia_staking::migration::migrate::<Runtime>();
 	let _ = migration::clear_storage_prefix(b"Vesting", b"Vesting", &[], None, None);
@@ -86,5 +87,11 @@ fn migrate() -> frame_support::weights::Weight {
 		]),
 	);
 
-	<Runtime as frame_system::Config>::DbWeight::get().reads_writes(r, w + 5)
+	let _ = Balances::transfer_all(
+		RuntimeOrigin::signed(PalletId(*b"dar/depo").into_account_truncating()),
+		Treasury::account_id(),
+		false,
+	);
+
+	<Runtime as frame_system::Config>::DbWeight::get().reads_writes(r, w + 10)
 }