From 7a91c092998b7b71552a0bdef3cf52fe64ad5953 Mon Sep 17 00:00:00 2001 From: Xavier Lau Date: Fri, 29 Nov 2024 22:12:12 +0800 Subject: [PATCH 1/9] Improve deposits migration Signed-off-by: Xavier Lau --- pallet/deposit/Cargo.toml | 2 +- pallet/deposit/src/lib.rs | 16 ++-------------- pallet/deposit/src/mock.rs | 4 ++++ pallet/deposit/src/tests.rs | 30 ++++++++++++++++++++++++++++-- runtime/darwinia/src/migration.rs | 8 +++++++- 5 files changed, 42 insertions(+), 18 deletions(-) diff --git a/pallet/deposit/Cargo.toml b/pallet/deposit/Cargo.toml index be7df7233..1f9d69549 100644 --- a/pallet/deposit/Cargo.toml +++ b/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"] diff --git a/pallet/deposit/src/lib.rs b/pallet/deposit/src/lib.rs index e3980b825..a09d77b1c 100644 --- a/pallet/deposit/src/lib.rs +++ b/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] @@ -204,7 +201,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 +213,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(), @@ -338,11 +334,3 @@ where } } } - -/// The account of the deposit pot. -pub fn account_id() -> A -where - A: FullCodec, -{ - PalletId(*b"dar/depo").into_account_truncating() -} diff --git a/pallet/deposit/src/mock.rs b/pallet/deposit/src/mock.rs index 195437844..391d086b3 100644 --- a/pallet/deposit/src/mock.rs +++ b/pallet/deposit/src/mock.rs @@ -71,3 +71,7 @@ pub fn new_test_ext() -> TestExternalities { storage.into() } + +pub fn events() -> Vec> { + System::read_events_for_pallet() +} diff --git a/pallet/deposit/src/tests.rs b/pallet/deposit/src/tests.rs index f4e1fedf7..0d08a4aeb 100644 --- a/pallet/deposit/src/tests.rs +++ b/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); >::insert( 1, @@ -52,9 +52,18 @@ fn on_idle_should_work() { .collect(), ) } + fn mock_zero_deposits(count: u16) -> BoundedVec> { + 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); >::insert(1, mock_deposits(512)); >::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(|_| { >::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); + + >::insert(1, mock_zero_deposits(512)); + assert_eq!(Deposit::deposit_of(1).unwrap().len(), 512); + + System::reset_events(); + (0..52).for_each(|_| { + >::on_idle(0, Weight::MAX); + }); + assert_eq!(events().into_iter().count(), 0); + assert!(Deposit::deposit_of(1).is_none()); + }); } diff --git a/runtime/darwinia/src/migration.rs b/runtime/darwinia/src/migration.rs index e45355693..797dd2f26 100644 --- a/runtime/darwinia/src/migration.rs +++ b/runtime/darwinia/src/migration.rs @@ -86,5 +86,11 @@ fn migrate() -> frame_support::weights::Weight { ]), ); - ::DbWeight::get().reads_writes(r, w + 5) + let _ = Balances::transfer_all( + RuntimeOrigin::signed(PalletId(*b"dar/depo").into_account_truncating()), + &Treasury::account_id(), + false, + ); + + ::DbWeight::get().reads_writes(r, w + 10) } From 9469224aa4ba4b3ae5901e2e6cd64e49157a2f74 Mon Sep 17 00:00:00 2001 From: Xavier Lau Date: Sat, 30 Nov 2024 10:36:40 +0800 Subject: [PATCH 2/9] Fix --- pallet/deposit/src/benchmarking.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/pallet/deposit/src/benchmarking.rs b/pallet/deposit/src/benchmarking.rs index 70a1df34e..b50c01e32 100644 --- a/pallet/deposit/src/benchmarking.rs +++ b/pallet/deposit/src/benchmarking.rs @@ -35,7 +35,6 @@ mod benchmarks { >::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: From df9ebc8517ea3405bff9fa8d02a3db4876980a64 Mon Sep 17 00:00:00 2001 From: Xavier Lau Date: Sat, 30 Nov 2024 18:57:29 +0800 Subject: [PATCH 3/9] Fix --- pallet/account-migration/src/lib.rs | 2 +- runtime/common/src/test.rs | 5 +---- runtime/darwinia/src/migration.rs | 5 +++-- 3 files changed, 5 insertions(+), 7 deletions(-) diff --git a/pallet/account-migration/src/lib.rs b/pallet/account-migration/src/lib.rs index cebaa79c5..ea9265d21 100644 --- a/pallet/account-migration/src/lib.rs +++ b/pallet/account-migration/src/lib.rs @@ -446,7 +446,7 @@ pub mod pallet { if let Some(ds) = >::take(from) { as Currency<_>>::transfer( to, - &darwinia_deposit::account_id(), + &::Treasury::get(), ds.iter().map(|d| d.value).sum(), AllowDeath, )?; diff --git a/runtime/common/src/test.rs b/runtime/common/src/test.rs index a516370e2..e65bffd31 100644 --- a/runtime/common/src/test.rs +++ b/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::()), - 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); }); diff --git a/runtime/darwinia/src/migration.rs b/runtime/darwinia/src/migration.rs index 797dd2f26..50f6f9a92 100644 --- a/runtime/darwinia/src/migration.rs +++ b/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::(); let _ = migration::clear_storage_prefix(b"Vesting", b"Vesting", &[], None, None); @@ -88,7 +89,7 @@ fn migrate() -> frame_support::weights::Weight { let _ = Balances::transfer_all( RuntimeOrigin::signed(PalletId(*b"dar/depo").into_account_truncating()), - &Treasury::account_id(), + Treasury::account_id(), false, ); From 754e22bc5c9aeecc4353f02e1bd14a1afbbc837a Mon Sep 17 00:00:00 2001 From: Xavier Lau Date: Mon, 2 Dec 2024 11:14:23 +0800 Subject: [PATCH 4/9] Transfer all Signed-off-by: Xavier Lau --- runtime/crab/src/migration.rs | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/runtime/crab/src/migration.rs b/runtime/crab/src/migration.rs index 8b4d07d4a..f7678eb32 100644 --- a/runtime/crab/src/migration.rs +++ b/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::(); + let _ = Balances::transfer_all( + RuntimeOrigin::signed(PalletId(*b"dar/depo").into_account_truncating()), + Treasury::account_id(), + false, + ); - ::DbWeight::get().reads_writes(r, w + 1) + ::DbWeight::get().reads_writes(r, w + 6) } From 0c8e69af2fa83f3071269dab4ca2a8fa0d8e3490 Mon Sep 17 00:00:00 2001 From: Xavier Lau Date: Mon, 2 Dec 2024 11:42:16 +0800 Subject: [PATCH 5/9] Deposit exit reason event --- pallet/deposit/src/lib.rs | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/pallet/deposit/src/lib.rs b/pallet/deposit/src/lib.rs index a09d77b1c..d44cf90b8 100644 --- a/pallet/deposit/src/lib.rs +++ b/pallet/deposit/src/lib.rs @@ -86,6 +86,8 @@ pub mod pallet { DepositsClaimed { owner: T::AccountId, deposits: Vec }, /// Deposits have been migrated. DepositsMigrated { owner: T::AccountId, deposits: Vec }, + /// Migration interaction with deposit contract failed. + MigrationFailedOnContract { exit_reason: ExitReason }, } #[pallet::error] @@ -330,7 +332,11 @@ where match exit_reason { ExitReason::Succeed(_) => Ok(()), - _ => Err(>::MigrationFailedOnContract)?, + exit_reason => { + >::deposit_event(Event::MigrationFailedOnContract { exit_reason }); + + Err(>::MigrationFailedOnContract)? + }, } } } From ac3f0a3945c43a2d8065cb298e9ce574b2712218 Mon Sep 17 00:00:00 2001 From: Xavier Lau Date: Mon, 2 Dec 2024 11:46:13 +0800 Subject: [PATCH 6/9] Add event for migration failure on refund Signed-off-by: Xavier Lau --- pallet/deposit/src/lib.rs | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/pallet/deposit/src/lib.rs b/pallet/deposit/src/lib.rs index d44cf90b8..34e0407be 100644 --- a/pallet/deposit/src/lib.rs +++ b/pallet/deposit/src/lib.rs @@ -88,6 +88,8 @@ pub mod pallet { DepositsMigrated { owner: T::AccountId, deposits: Vec }, /// Migration interaction with deposit contract failed. MigrationFailedOnContract { exit_reason: ExitReason }, + /// Migration interaction with refund failed. + MigrationFailedOnRefund, } #[pallet::error] @@ -215,7 +217,13 @@ pub mod pallet { } if to_claim.0 != 0 { - T::Ring::transfer(&T::Treasury::get(), who, to_claim.0, AllowDeath)?; + if let Err(e) = T::Ring::transfer(&T::Treasury::get(), who, to_claim.0, AllowDeath) + { + Self::deposit_event(Event::MigrationFailedOnRefund); + + Err(e)?; + } + Self::deposit_event(Event::DepositsClaimed { owner: who.clone(), deposits: to_claim.1, From 4bdbf50687b3fb088b3eba727a025a41ab9234cc Mon Sep 17 00:00:00 2001 From: Xavier Lau Date: Mon, 2 Dec 2024 11:49:33 +0800 Subject: [PATCH 7/9] Remove old migration Signed-off-by: Xavier Lau --- runtime/crab/src/migration.rs | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/runtime/crab/src/migration.rs b/runtime/crab/src/migration.rs index f7678eb32..d05257643 100644 --- a/runtime/crab/src/migration.rs +++ b/runtime/crab/src/migration.rs @@ -36,8 +36,6 @@ impl frame_support::traits::OnRuntimeUpgrade for CustomOnRuntimeUpgrade { fn post_upgrade(_state: Vec) -> Result<(), sp_runtime::DispatchError> { log::info!("post"); - darwinia_staking::migration::post_check::(); - Ok(()) } @@ -47,16 +45,11 @@ 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]); - } - - let (r, w) = darwinia_staking::migration::migrate::(); let _ = Balances::transfer_all( RuntimeOrigin::signed(PalletId(*b"dar/depo").into_account_truncating()), Treasury::account_id(), false, ); - ::DbWeight::get().reads_writes(r, w + 6) + ::DbWeight::get().reads_writes(0, 6) } From 934968593cb08b488649c4d9000e7639d857e824 Mon Sep 17 00:00:00 2001 From: Xavier Lau Date: Mon, 2 Dec 2024 15:43:57 +0800 Subject: [PATCH 8/9] Add missing items --- runtime/crab/src/migration.rs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/runtime/crab/src/migration.rs b/runtime/crab/src/migration.rs index d05257643..a0238283b 100644 --- a/runtime/crab/src/migration.rs +++ b/runtime/crab/src/migration.rs @@ -21,7 +21,7 @@ use crate::*; // polkadot-sdk #[allow(unused_imports)] -use frame_support::{migration, storage::unhashed, PalletId}; +use frame_support::{migration, storage::unhashed}; pub struct CustomOnRuntimeUpgrade; impl frame_support::traits::OnRuntimeUpgrade for CustomOnRuntimeUpgrade { @@ -45,6 +45,10 @@ impl frame_support::traits::OnRuntimeUpgrade for CustomOnRuntimeUpgrade { } fn migrate() -> frame_support::weights::Weight { + /// polkadot-sdk + use frame_support::PalletId; + use sp_runtime::traits::AccountIdConversion; + let _ = Balances::transfer_all( RuntimeOrigin::signed(PalletId(*b"dar/depo").into_account_truncating()), Treasury::account_id(), From 6a6eaf9030ddb682ff28375a5f370086367889f4 Mon Sep 17 00:00:00 2001 From: Xavier Lau Date: Tue, 3 Dec 2024 10:48:22 +0800 Subject: [PATCH 9/9] Record failures on automation tasks Signed-off-by: Xavier Lau --- pallet/deposit/src/lib.rs | 49 ++++++++++++++++++------------------- pallet/deposit/src/tests.rs | 8 ++++++ pallet/staking/src/lib.rs | 9 ++++--- 3 files changed, 38 insertions(+), 28 deletions(-) diff --git a/pallet/deposit/src/lib.rs b/pallet/deposit/src/lib.rs index 34e0407be..f3942926c 100644 --- a/pallet/deposit/src/lib.rs +++ b/pallet/deposit/src/lib.rs @@ -86,10 +86,6 @@ pub mod pallet { DepositsClaimed { owner: T::AccountId, deposits: Vec }, /// Deposits have been migrated. DepositsMigrated { owner: T::AccountId, deposits: Vec }, - /// Migration interaction with deposit contract failed. - MigrationFailedOnContract { exit_reason: ExitReason }, - /// Migration interaction with refund failed. - MigrationFailedOnRefund, } #[pallet::error] @@ -110,6 +106,15 @@ pub mod pallet { pub type Deposits = StorageMap<_, Blake2_128Concat, T::AccountId, BoundedVec>>; + /// 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 = + StorageMap<_, Blake2_128Concat, T::AccountId, (BoundedVec>, u8)>; + // Deposit contract address. #[pallet::storage] #[pallet::getter(fn deposit_contract)] @@ -138,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. >::insert(&k, remaining_deposits); - } - } else { - // Put the deposits back if migration failed. - >::insert(&k, v); + }, + Err((_, ty)) => { + // Migration failed, record the failures. + >::insert(&k, (v, ty)); + }, + _ => (), } remaining_weight @@ -160,7 +167,7 @@ pub mod pallet { ensure_signed(origin)?; let deposits = >::take(&who).ok_or(>::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() { @@ -195,7 +202,7 @@ pub mod pallet { fn migrate_for_inner( who: &T::AccountId, deposits: I, - ) -> Result>, DispatchError> + ) -> Result>, (DispatchError, u8)> where I: IntoIterator, { @@ -217,20 +224,16 @@ pub mod pallet { } if to_claim.0 != 0 { - if let Err(e) = T::Ring::transfer(&T::Treasury::get(), who, to_claim.0, AllowDeath) - { - Self::deposit_event(Event::MigrationFailedOnRefund); - - Err(e)?; - } - + 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::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, @@ -340,11 +343,7 @@ where match exit_reason { ExitReason::Succeed(_) => Ok(()), - exit_reason => { - >::deposit_event(Event::MigrationFailedOnContract { exit_reason }); - - Err(>::MigrationFailedOnContract)? - }, + _ => Err(>::MigrationFailedOnContract)?, } } } diff --git a/pallet/deposit/src/tests.rs b/pallet/deposit/src/tests.rs index 0d08a4aeb..b0e7a7ce3 100644 --- a/pallet/deposit/src/tests.rs +++ b/pallet/deposit/src/tests.rs @@ -123,4 +123,12 @@ fn on_idle_should_work() { assert_eq!(events().into_iter().count(), 0); assert!(Deposit::deposit_of(1).is_none()); }); + new_test_ext().execute_with(|| { + >::insert(1, mock_deposits(512)); + assert_eq!(Deposit::deposit_of(1).unwrap().len(), 512); + + >::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); + }); } diff --git a/pallet/staking/src/lib.rs b/pallet/staking/src/lib.rs index 6c9451604..5be8087e1 100644 --- a/pallet/staking/src/lib.rs +++ b/pallet/staking/src/lib.rs @@ -192,8 +192,6 @@ pub mod pallet { pub enum Event { /// 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 = StorageValue<_, (BlockNumberFor, BTreeMap>), ValueQuery>; + /// Unissued reward to Treasury. + #[pallet::storage] + #[pallet::getter(fn unissued_reward)] + pub type UnissuedReward = 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 }); + >::mutate(|v| *v += amount); } let reward_to_ring_staking = amount.saturating_div(2);