Improve deposits migration (#1636)

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]
@@ -109,6 +106,15 @@ pub mod pallet {
 	pub type Deposits<T: Config> =
 		StorageMap<_, Blake2_128Concat, T::AccountId, BoundedVec<Deposit, ConstU32<512>>>;
 
+	/// Failures of migration.
+	///
+	/// The first value is the deposits that failed to migrate,
+	/// the second value is the type of failure.
+	#[pallet::storage]
+	#[pallet::getter(fn migration_failure_of)]
+	pub type MigrationFailures<T: Config> =
+		StorageMap<_, Blake2_128Concat, T::AccountId, (BoundedVec<Deposit, ConstU32<512>>, u8)>;
+
 	// Deposit contract address.
 	#[pallet::storage]
 	#[pallet::getter(fn deposit_contract)]
@@ -137,14 +143,16 @@ pub mod pallet {
 				return remaining_weight;
 			};
 
-			if let Ok(remaining_deposits) = Self::migrate_for_inner(&k, v.clone()) {
-				if !remaining_deposits.is_empty() {
+			match Self::migrate_for_inner(&k, v.clone()) {
+				Ok(remaining_deposits) if !remaining_deposits.is_empty() => {
 					// There are still some deposits left for this account.
 					<Deposits<T>>::insert(&k, remaining_deposits);
-				}
-			} else {
-				// Put the deposits back if migration failed.
-				<Deposits<T>>::insert(&k, v);
+				},
+				Err((_, ty)) => {
+					// Migration failed, record the failures.
+					<MigrationFailures<T>>::insert(&k, (v, ty));
+				},
+				_ => (),
 			}
 
 			remaining_weight
@@ -159,7 +167,7 @@ pub mod pallet {
 			ensure_signed(origin)?;
 
 			let deposits = <Deposits<T>>::take(&who).ok_or(<Error<T>>::NoDeposit)?;
-			let deposits = Self::migrate_for_inner(&who, deposits)?;
+			let deposits = Self::migrate_for_inner(&who, deposits).map_err(|(e, _)| e)?;
 
 			// Put the rest deposits back.
 			if !deposits.is_empty() {
@@ -194,7 +202,7 @@ pub mod pallet {
 		fn migrate_for_inner<I>(
 			who: &T::AccountId,
 			deposits: I,
-		) -> Result<BoundedVec<Deposit, ConstU32<512>>, DispatchError>
+		) -> Result<BoundedVec<Deposit, ConstU32<512>>, (DispatchError, u8)>
 		where
 			I: IntoIterator<Item = Deposit>,
 		{
@@ -204,7 +212,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,15 +224,16 @@ 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)
+					.map_err(|e| (e, 0))?;
 				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)?;
+				T::DepositMigrator::migrate(who.clone(), to_migrate.0, to_migrate.2)
+					.map_err(|e| (e, 1))?;
 				Self::deposit_event(Event::DepositsMigrated {
 					owner: who.clone(),
 					deposits: to_migrate.1,
@@ -338,11 +347,3 @@ where
 		}
 	}
 }
-
-/// 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,36 @@ 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());
+	});
+	new_test_ext().execute_with(|| {
+		<Deposits<Runtime>>::insert(1, mock_deposits(512));
+		assert_eq!(Deposit::deposit_of(1).unwrap().len(), 512);
+
+		<Deposit as OnIdle<_>>::on_idle(0, Weight::MAX);
+		assert!(Deposit::deposit_of(1).is_none());
+		assert_eq!(Deposit::migration_failure_of(1).unwrap().0.into_iter().count(), 512);
+	});
 }

pallet/staking/src/lib.rs

@@ -192,8 +192,6 @@ pub mod pallet {
 	pub enum Event<T: Config> {
 		/// Reward allocated to the account.
 		RewardAllocated { who: T::AccountId, amount: Balance },
-		/// Fail to allocate the reward to the account.
-		RewardAllocationFailed { who: T::AccountId, amount: Balance },
 		/// Unstake all stakes for the account.
 		UnstakeAllFor { who: T::AccountId },
 	}
@@ -225,6 +223,11 @@ pub mod pallet {
 	pub type AuthoredBlockCount<T: Config> =
 		StorageValue<_, (BlockNumberFor<T>, BTreeMap<T::AccountId, BlockNumberFor<T>>), ValueQuery>;
 
+	/// Unissued reward to Treasury.
+	#[pallet::storage]
+	#[pallet::getter(fn unissued_reward)]
+	pub type UnissuedReward<T: Config> = StorageValue<_, Balance, ValueQuery>;
+
 	/// All outstanding rewards since the last payment.
 	#[pallet::storage]
 	#[pallet::unbounded]
@@ -395,7 +398,7 @@ pub mod pallet {
 			if T::IssuingManager::reward(amount).is_ok() {
 				Self::deposit_event(Event::RewardAllocated { who: treasury, amount });
 			} else {
-				Self::deposit_event(Event::RewardAllocationFailed { who: treasury, amount });
+				<UnissuedReward<T>>::mutate(|v| *v += amount);
 			}
 
 			let reward_to_ring_staking = amount.saturating_div(2);

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

@@ -36,8 +36,6 @@ impl frame_support::traits::OnRuntimeUpgrade for CustomOnRuntimeUpgrade {
 	fn post_upgrade(_state: Vec<u8>) -> Result<(), sp_runtime::DispatchError> {
 		log::info!("post");
 
-		darwinia_staking::migration::post_check::<Runtime>();
-
 		Ok(())
 	}
 
@@ -47,11 +45,15 @@ impl frame_support::traits::OnRuntimeUpgrade for CustomOnRuntimeUpgrade {
 }
 
 fn migrate() -> frame_support::weights::Weight {
-	if let Ok(k) = array_bytes::hex2bytes("0x1da53b775b270400e7e61ed5cbc5a146ab1160471b1418779239ba8e2b847e42d53de13b56da115d3342f0588bc3614108837de0ae21c270383d9f2de4db03c7b1314632314d8c74970d627c9b4f4c42e06688a9f7a2866905a810c4b1a49b8cb0dca3f1bc953905609869b6e9d4fb794cd36c5f") {
-		let _ = System::kill_storage(RuntimeOrigin::root(), vec![k]);
-	}
+	/// polkadot-sdk
+	use frame_support::PalletId;
+	use sp_runtime::traits::AccountIdConversion;
 
-	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(0, 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)
 }