From fa274a8dc77c8ede5c1ea720d83947ea3c5cb9e1 Mon Sep 17 00:00:00 2001 From: Liam Aharon Date: Fri, 26 May 2023 16:34:36 +0400 Subject: [PATCH 01/16] unreserve all tip funds migration --- frame/tips/Cargo.toml | 2 +- frame/tips/src/migrations/mod.rs | 3 + .../src/migrations/unreserve_all_funds.rs | 241 ++++++++++++++++++ 3 files changed, 245 insertions(+), 1 deletion(-) create mode 100644 frame/tips/src/migrations/unreserve_all_funds.rs diff --git a/frame/tips/Cargo.toml b/frame/tips/Cargo.toml index caed4edec5f48..b4b2b48fa7d5e 100644 --- a/frame/tips/Cargo.toml +++ b/frame/tips/Cargo.toml @@ -25,9 +25,9 @@ sp-core = { version = "7.0.0", default-features = false, path = "../../primitive sp-io = { version = "7.0.0", default-features = false, path = "../../primitives/io" } sp-runtime = { version = "7.0.0", default-features = false, path = "../../primitives/runtime" } sp-std = { version = "5.0.0", default-features = false, path = "../../primitives/std" } +pallet-balances = { version = "4.0.0-dev", default-features = false, path = "../balances" } [dev-dependencies] -pallet-balances = { version = "4.0.0-dev", path = "../balances" } sp-storage = { version = "7.0.0", path = "../../primitives/storage" } [features] diff --git a/frame/tips/src/migrations/mod.rs b/frame/tips/src/migrations/mod.rs index f7f144adcdb6e..38cdba49340f3 100644 --- a/frame/tips/src/migrations/mod.rs +++ b/frame/tips/src/migrations/mod.rs @@ -21,3 +21,6 @@ /// before calling this migration. After calling this migration, it will get replaced with /// own storage identifier. pub mod v4; + +/// A migration that unreserves all funds held in the context of this pallet. +pub mod unreserve_all_funds; diff --git a/frame/tips/src/migrations/unreserve_all_funds.rs b/frame/tips/src/migrations/unreserve_all_funds.rs new file mode 100644 index 0000000000000..7d398b3d4832f --- /dev/null +++ b/frame/tips/src/migrations/unreserve_all_funds.rs @@ -0,0 +1,241 @@ +// This file is part of Substrate. + +// Copyright (C) Parity Technologies (UK) Ltd. +// SPDX-License-Identifier: Apache-2.0 + +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +//! A migration that unreserves all deposit and unlocks all stake held in the context of this +//! pallet. + +use core::iter::Sum; +use frame_support::{ + traits::{OnRuntimeUpgrade, ReservableCurrency}, + weights::constants::RocksDbWeight, +}; +use pallet_treasury::BalanceOf; +use sp_runtime::{traits::Zero, Saturating}; +use sp_std::{collections::btree_map::BTreeMap, vec::Vec}; + +#[cfg(feature = "try-runtime")] +use codec::{Decode, Encode}; + +/// A migration that unreserves all tip deposits. +/// +/// Useful to prevent funds from being locked up when the pallet is deprecated. +/// +/// The pallet should be made inoperable before or immediately after this migration is run. +/// +/// (See also the `RemovePallet` migration in `frame/support/src/migrations.rs`) +pub struct UnreserveAllFunds, I: 'static>(sp_std::marker::PhantomData<(T, I)>); + +impl, I: 'static> UnreserveAllFunds { + /// Calculates and returns the total amount reserved by each account by this pallet from open + /// tips. + /// + /// # Returns + /// + /// * `BTreeMap`: Map of account IDs to their respective total + /// reserved balance by this pallet + /// * `frame_support::weights::Weight`: The weight of this operation. + fn get_deposits() -> (BTreeMap>, frame_support::weights::Weight) { + let tips = crate::Tips::::iter().collect::>(); + let tips_len = tips.len(); + let account_deposits: BTreeMap> = tips + .into_iter() + .map(|(_hash, open_tip)| open_tip) + .fold(BTreeMap::new(), |mut acc, tip| { + // Add the balance to the account's existing deposit in the accumulator + *acc.entry(tip.finder).or_insert(Zero::zero()) = acc + .entry(tip.finder.clone()) + .or_insert(Zero::zero()) + .saturating_add(tip.deposit); + acc + }); + + (account_deposits, RocksDbWeight::get().reads(tips_len as u64)) + } +} + +impl + pallet_treasury::Config, I: 'static> OnRuntimeUpgrade + for UnreserveAllFunds +where + BalanceOf: Sum, +{ + /// Gets the actual reserved amount for each account before the migration, performs integrity + /// checks and prints some summary information. + /// + /// Steps: + /// 1. Gets the deposited balances for each account stored in this pallet. + /// 2. Collects actual pre-migration reserved balances for each account. + /// 3. Checks the integrity of the deposited balances. + /// 4. Prints summary statistics about the state to be migrated. + /// 5. Returns the pre-migration actual reserved balance for each account that will + /// be part of the migration. + /// + /// Fails with a `TryRuntimeError` if somehow the amount reserved by this pallet is greater than + /// the actual total reserved amount for any accounts. + #[cfg(feature = "try-runtime")] + fn pre_upgrade() -> Result, sp_runtime::TryRuntimeError> { + use frame_support::ensure; + + // Get the Tips pallet view of balances it has reserved + let (account_deposits, _) = Self::get_deposits(); + + // Get the actual amounts reserved for accounts with open tips + let account_reserved_before: BTreeMap> = account_deposits + .keys() + .map(|account| (account.clone(), T::Currency::reserved_balance(&account))) + .collect(); + + // The deposit amount must be less than or equal to the reserved amount. + // If it is higher, there is either a bug with the pallet or a bug in the calculation of the + // deposit amount. + ensure!( + account_deposits.iter().all(|(account, deposit)| *deposit <= + *account_reserved_before.get(account).unwrap_or(&Zero::zero())), + "Deposit amount is greater than reserved amount" + ); + + // Print some summary stats + let total_deposits_to_unreserve = + account_deposits.clone().into_values().sum::>(); + log::info!("Total accounts: {}", account_deposits.keys().count()); + log::info!("Total amount to unreserve: {:?}", total_deposits_to_unreserve); + + // Return the actual amount reserved before the upgrade to verify integrity of the upgrade + // in the post_upgrade hook. + Ok(account_reserved_before.encode()) + } + + /// Executes the migration, unreserving funds that are locked in Tip deposits. + fn on_runtime_upgrade() -> frame_support::weights::Weight { + // Get staked and deposited balances as reported by this pallet. + let (account_deposits, initial_reads_weight) = Self::get_deposits(); + + // Deposited funds need to be unreserved. + for (account, unreserve_amount) in account_deposits.iter() { + if unreserve_amount.is_zero() { + continue + } + T::Currency::unreserve(&account, *unreserve_amount); + } + + // Question for reviewers: how do I know the weight of the unreserve & remove_lock calls? + RocksDbWeight::get().reads_writes(1, 1) + initial_reads_weight + } + + /// Verifies that the account reserved balances were reduced by the actual expected amounts. + #[cfg(feature = "try-runtime")] + fn post_upgrade( + account_reserved_before_bytes: Vec, + ) -> Result<(), sp_runtime::TryRuntimeError> { + let account_reserved_before = BTreeMap::>::decode( + &mut &account_reserved_before_bytes[..], + ) + .map_err(|_| "Failed to decode account_reserved_before_bytes")?; + + // Get deposited balances as reported by this pallet. + let (account_deposits, _) = Self::get_deposits(); + + // Check that the reserved balance is reduced by the expected deposited amount. + for (account, actual_reserved_before) in account_reserved_before { + let actual_reserved_after = T::Currency::reserved_balance(&account); + let expected_amount_deducted = *account_deposits + .get(&account) + .expect("account deposit must exist to be in account_reserved_before, qed"); + let expected_reserved_after = + actual_reserved_before.saturating_sub(expected_amount_deducted); + assert!( + actual_reserved_after == expected_reserved_after, + "Reserved balance for {:?} is incorrect. actual before: {:?}, actual after, {:?}, expected deducted: {:?}", + account, + actual_reserved_before, + actual_reserved_after, + expected_amount_deducted, + ); + } + + Ok(()) + } +} + +#[cfg(test)] +mod test { + use super::*; + use crate::{ + migrations::unreserve_all_funds::UnreserveAllFunds, + tests::{new_test_ext, RuntimeOrigin, Test, Tips}, + }; + use frame_support::assert_ok; + use sp_core::TypedGet; + + #[test] + fn unreserve_all_funds_works() { + let tipper_0 = 0; + let tipper_1 = 1; + let recipient = 100; + let tip_0_reason = b"whats_not_awesome?".to_vec(); + let tip_1_reason = b"pinaple_on_pizza".to_vec(); + new_test_ext().execute_with(|| { + // Assert no amounts are reserved pre-tip. + assert_eq!( + ::Currency::reserved_balance(&tipper_0), + 0u64 + ); + assert_eq!( + ::Currency::reserved_balance(&tipper_1), + 0u64 + ); + + // Make some tips + assert_ok!(Tips::report_awesome( + RuntimeOrigin::signed(tipper_0), + tip_0_reason.clone(), + recipient + )); + assert_ok!(Tips::report_awesome( + RuntimeOrigin::signed(tipper_1), + tip_1_reason.clone(), + recipient + )); + + // Verify the expected amount is reserved + assert_eq!( + ::Currency::reserved_balance(&tipper_0), + ::TipReportDepositBase::get() + + ::DataDepositPerByte::get() * + tip_0_reason.len() as u64 + ); + assert_eq!( + ::Currency::reserved_balance(&tipper_1), + ::TipReportDepositBase::get() + + ::DataDepositPerByte::get() * + tip_1_reason.len() as u64 + ); + + // Execute the migration + UnreserveAllFunds::::on_runtime_upgrade(); + + // Check the deposits were were unreserved + assert_eq!( + ::Currency::reserved_balance(&tipper_0), + 0u64 + ); + assert_eq!( + ::Currency::reserved_balance(&tipper_1), + 0u64 + ); + }); + } +} From 484862232ca04b8fe14dce5956c9481595307a87 Mon Sep 17 00:00:00 2001 From: Liam Aharon Date: Fri, 26 May 2023 20:29:54 +0400 Subject: [PATCH 02/16] improve test --- .../src/migrations/unreserve_all_funds.rs | 32 +++++++++++-------- 1 file changed, 18 insertions(+), 14 deletions(-) diff --git a/frame/tips/src/migrations/unreserve_all_funds.rs b/frame/tips/src/migrations/unreserve_all_funds.rs index 7d398b3d4832f..9bffbfd2c8663 100644 --- a/frame/tips/src/migrations/unreserve_all_funds.rs +++ b/frame/tips/src/migrations/unreserve_all_funds.rs @@ -184,19 +184,21 @@ mod test { fn unreserve_all_funds_works() { let tipper_0 = 0; let tipper_1 = 1; + let tipper_0_initial_reserved = 0; + let tipper_1_initial_reserved = 5; let recipient = 100; - let tip_0_reason = b"whats_not_awesome?".to_vec(); - let tip_1_reason = b"pinaple_on_pizza".to_vec(); + let tip_0_reason = b"what_is_really_not_awesome".to_vec(); + let tip_1_reason = b"pineapple_on_pizza".to_vec(); new_test_ext().execute_with(|| { // Assert no amounts are reserved pre-tip. - assert_eq!( - ::Currency::reserved_balance(&tipper_0), - 0u64 - ); - assert_eq!( - ::Currency::reserved_balance(&tipper_1), - 0u64 - ); + assert_ok!(::Currency::reserve( + &tipper_0, + tipper_0_initial_reserved + )); + assert_ok!(::Currency::reserve( + &tipper_1, + tipper_1_initial_reserved + )); // Make some tips assert_ok!(Tips::report_awesome( @@ -213,13 +215,15 @@ mod test { // Verify the expected amount is reserved assert_eq!( ::Currency::reserved_balance(&tipper_0), - ::TipReportDepositBase::get() + + tipper_0_initial_reserved + + ::TipReportDepositBase::get() + ::DataDepositPerByte::get() * tip_0_reason.len() as u64 ); assert_eq!( ::Currency::reserved_balance(&tipper_1), - ::TipReportDepositBase::get() + + tipper_1_initial_reserved + + ::TipReportDepositBase::get() + ::DataDepositPerByte::get() * tip_1_reason.len() as u64 ); @@ -230,11 +234,11 @@ mod test { // Check the deposits were were unreserved assert_eq!( ::Currency::reserved_balance(&tipper_0), - 0u64 + tipper_0_initial_reserved ); assert_eq!( ::Currency::reserved_balance(&tipper_1), - 0u64 + tipper_1_initial_reserved ); }); } From 232a59ff4ed51cf79d788c7e8042631c4e1fb6b9 Mon Sep 17 00:00:00 2001 From: Liam Aharon Date: Fri, 26 May 2023 20:49:41 +0400 Subject: [PATCH 03/16] fix comment --- frame/tips/src/migrations/unreserve_all_funds.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/tips/src/migrations/unreserve_all_funds.rs b/frame/tips/src/migrations/unreserve_all_funds.rs index 9bffbfd2c8663..9ecd9246f260d 100644 --- a/frame/tips/src/migrations/unreserve_all_funds.rs +++ b/frame/tips/src/migrations/unreserve_all_funds.rs @@ -190,7 +190,7 @@ mod test { let tip_0_reason = b"what_is_really_not_awesome".to_vec(); let tip_1_reason = b"pineapple_on_pizza".to_vec(); new_test_ext().execute_with(|| { - // Assert no amounts are reserved pre-tip. + // Set up assert_ok!(::Currency::reserve( &tipper_0, tipper_0_initial_reserved From 3dc164346e8907f297bad98242a4072629b11274 Mon Sep 17 00:00:00 2001 From: Liam Aharon Date: Tue, 30 May 2023 15:24:49 +0400 Subject: [PATCH 04/16] implement weights --- frame/tips/src/migrations/unreserve_all_funds.rs | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/frame/tips/src/migrations/unreserve_all_funds.rs b/frame/tips/src/migrations/unreserve_all_funds.rs index 9ecd9246f260d..58d710d4e7f8a 100644 --- a/frame/tips/src/migrations/unreserve_all_funds.rs +++ b/frame/tips/src/migrations/unreserve_all_funds.rs @@ -27,9 +27,6 @@ use pallet_treasury::BalanceOf; use sp_runtime::{traits::Zero, Saturating}; use sp_std::{collections::btree_map::BTreeMap, vec::Vec}; -#[cfg(feature = "try-runtime")] -use codec::{Decode, Encode}; - /// A migration that unreserves all tip deposits. /// /// Useful to prevent funds from being locked up when the pallet is deprecated. @@ -87,6 +84,7 @@ where /// the actual total reserved amount for any accounts. #[cfg(feature = "try-runtime")] fn pre_upgrade() -> Result, sp_runtime::TryRuntimeError> { + use codec::Encode; use frame_support::ensure; // Get the Tips pallet view of balances it has reserved @@ -121,7 +119,7 @@ where /// Executes the migration, unreserving funds that are locked in Tip deposits. fn on_runtime_upgrade() -> frame_support::weights::Weight { // Get staked and deposited balances as reported by this pallet. - let (account_deposits, initial_reads_weight) = Self::get_deposits(); + let (account_deposits, initial_reads) = Self::get_deposits(); // Deposited funds need to be unreserved. for (account, unreserve_amount) in account_deposits.iter() { @@ -131,8 +129,9 @@ where T::Currency::unreserve(&account, *unreserve_amount); } - // Question for reviewers: how do I know the weight of the unreserve & remove_lock calls? - RocksDbWeight::get().reads_writes(1, 1) + initial_reads_weight + RocksDbWeight::get() + .reads_writes(account_deposits.len() as u64, account_deposits.len() as u64) + + initial_reads } /// Verifies that the account reserved balances were reduced by the actual expected amounts. @@ -140,6 +139,8 @@ where fn post_upgrade( account_reserved_before_bytes: Vec, ) -> Result<(), sp_runtime::TryRuntimeError> { + use codec::Decode; + let account_reserved_before = BTreeMap::>::decode( &mut &account_reserved_before_bytes[..], ) From 55c64f3a816e6628d4a982ccbc24674c868aaaee Mon Sep 17 00:00:00 2001 From: Liam Aharon Date: Fri, 2 Jun 2023 10:24:24 +0200 Subject: [PATCH 05/16] saturating_accrue --- frame/tips/src/migrations/unreserve_all_funds.rs | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/frame/tips/src/migrations/unreserve_all_funds.rs b/frame/tips/src/migrations/unreserve_all_funds.rs index 58d710d4e7f8a..070af2056d21a 100644 --- a/frame/tips/src/migrations/unreserve_all_funds.rs +++ b/frame/tips/src/migrations/unreserve_all_funds.rs @@ -53,10 +53,7 @@ impl, I: 'static> UnreserveAllFunds { .map(|(_hash, open_tip)| open_tip) .fold(BTreeMap::new(), |mut acc, tip| { // Add the balance to the account's existing deposit in the accumulator - *acc.entry(tip.finder).or_insert(Zero::zero()) = acc - .entry(tip.finder.clone()) - .or_insert(Zero::zero()) - .saturating_add(tip.deposit); + acc.entry(tip.finder).or_insert(Zero::zero()).saturating_accrue(tip.deposit); acc }); From 8cba18bea845952ecc01a2528cc679b89872b591 Mon Sep 17 00:00:00 2001 From: Liam Aharon Date: Fri, 2 Jun 2023 10:28:04 +0200 Subject: [PATCH 06/16] remove unnecessary collect --- frame/tips/src/migrations/unreserve_all_funds.rs | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/frame/tips/src/migrations/unreserve_all_funds.rs b/frame/tips/src/migrations/unreserve_all_funds.rs index 070af2056d21a..49e809a99a5f5 100644 --- a/frame/tips/src/migrations/unreserve_all_funds.rs +++ b/frame/tips/src/migrations/unreserve_all_funds.rs @@ -25,7 +25,7 @@ use frame_support::{ }; use pallet_treasury::BalanceOf; use sp_runtime::{traits::Zero, Saturating}; -use sp_std::{collections::btree_map::BTreeMap, vec::Vec}; +use sp_std::collections::btree_map::BTreeMap; /// A migration that unreserves all tip deposits. /// @@ -46,18 +46,19 @@ impl, I: 'static> UnreserveAllFunds { /// reserved balance by this pallet /// * `frame_support::weights::Weight`: The weight of this operation. fn get_deposits() -> (BTreeMap>, frame_support::weights::Weight) { - let tips = crate::Tips::::iter().collect::>(); - let tips_len = tips.len(); - let account_deposits: BTreeMap> = tips - .into_iter() + let mut tips_len = 0; + let account_deposits: BTreeMap> = crate::Tips::::iter() .map(|(_hash, open_tip)| open_tip) .fold(BTreeMap::new(), |mut acc, tip| { + // Count the total number of tips + tips_len += 1; + // Add the balance to the account's existing deposit in the accumulator acc.entry(tip.finder).or_insert(Zero::zero()).saturating_accrue(tip.deposit); acc }); - (account_deposits, RocksDbWeight::get().reads(tips_len as u64)) + (account_deposits, RocksDbWeight::get().reads(tips_len)) } } From c9903ee5a0c93d4760c6f67fb2d61eed1d2524b9 Mon Sep 17 00:00:00 2001 From: Liam Aharon Date: Fri, 2 Jun 2023 10:30:10 +0200 Subject: [PATCH 07/16] prefer ensure --- frame/tips/src/migrations/unreserve_all_funds.rs | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/frame/tips/src/migrations/unreserve_all_funds.rs b/frame/tips/src/migrations/unreserve_all_funds.rs index 49e809a99a5f5..2a78e6fccea94 100644 --- a/frame/tips/src/migrations/unreserve_all_funds.rs +++ b/frame/tips/src/migrations/unreserve_all_funds.rs @@ -138,6 +138,7 @@ where account_reserved_before_bytes: Vec, ) -> Result<(), sp_runtime::TryRuntimeError> { use codec::Decode; + use frame_support::ensure; let account_reserved_before = BTreeMap::>::decode( &mut &account_reserved_before_bytes[..], @@ -155,13 +156,15 @@ where .expect("account deposit must exist to be in account_reserved_before, qed"); let expected_reserved_after = actual_reserved_before.saturating_sub(expected_amount_deducted); - assert!( + ensure!( actual_reserved_after == expected_reserved_after, - "Reserved balance for {:?} is incorrect. actual before: {:?}, actual after, {:?}, expected deducted: {:?}", - account, - actual_reserved_before, - actual_reserved_after, - expected_amount_deducted, + format!( + "Reserved balance for {:?} is incorrect. actual before: {:?}, actual after, {:?}, expected deducted: {:?}", + account, + actual_reserved_before, + actual_reserved_after, + expected_amount_deducted + ), ); } From 871b98b717d8be563ed182b9f500d858db069142 Mon Sep 17 00:00:00 2001 From: Liam Aharon Date: Fri, 2 Jun 2023 10:38:24 +0200 Subject: [PATCH 08/16] use assert --- .../tips/src/migrations/unreserve_all_funds.rs | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/frame/tips/src/migrations/unreserve_all_funds.rs b/frame/tips/src/migrations/unreserve_all_funds.rs index 2a78e6fccea94..133191165b04b 100644 --- a/frame/tips/src/migrations/unreserve_all_funds.rs +++ b/frame/tips/src/migrations/unreserve_all_funds.rs @@ -27,6 +27,9 @@ use pallet_treasury::BalanceOf; use sp_runtime::{traits::Zero, Saturating}; use sp_std::collections::btree_map::BTreeMap; +#[cfg(feature = "try-runtime")] +use sp_std::vec::Vec; + /// A migration that unreserves all tip deposits. /// /// Useful to prevent funds from being locked up when the pallet is deprecated. @@ -138,7 +141,6 @@ where account_reserved_before_bytes: Vec, ) -> Result<(), sp_runtime::TryRuntimeError> { use codec::Decode; - use frame_support::ensure; let account_reserved_before = BTreeMap::>::decode( &mut &account_reserved_before_bytes[..], @@ -156,15 +158,13 @@ where .expect("account deposit must exist to be in account_reserved_before, qed"); let expected_reserved_after = actual_reserved_before.saturating_sub(expected_amount_deducted); - ensure!( + assert!( actual_reserved_after == expected_reserved_after, - format!( - "Reserved balance for {:?} is incorrect. actual before: {:?}, actual after, {:?}, expected deducted: {:?}", - account, - actual_reserved_before, - actual_reserved_after, - expected_amount_deducted - ), + "Reserved balance for {:?} is incorrect. actual before: {:?}, actual after, {:?}, expected deducted: {:?}", + account, + actual_reserved_before, + actual_reserved_after, + expected_amount_deducted ); } From c2896f5e3f57b659db5a6e712f441b0b179b83fd Mon Sep 17 00:00:00 2001 From: Liam Aharon Date: Fri, 2 Jun 2023 10:40:48 +0200 Subject: [PATCH 09/16] use saturating_add --- frame/tips/src/migrations/unreserve_all_funds.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/frame/tips/src/migrations/unreserve_all_funds.rs b/frame/tips/src/migrations/unreserve_all_funds.rs index 133191165b04b..e1a260f20ef72 100644 --- a/frame/tips/src/migrations/unreserve_all_funds.rs +++ b/frame/tips/src/migrations/unreserve_all_funds.rs @@ -131,8 +131,8 @@ where } RocksDbWeight::get() - .reads_writes(account_deposits.len() as u64, account_deposits.len() as u64) + - initial_reads + .reads_writes(account_deposits.len() as u64, account_deposits.len() as u64) + .saturating_add(initial_reads) } /// Verifies that the account reserved balances were reduced by the actual expected amounts. From eba34afaf58598e68111cef55048c5fb3cd16038 Mon Sep 17 00:00:00 2001 From: Liam Aharon Date: Fri, 2 Jun 2023 11:04:27 +0200 Subject: [PATCH 10/16] use saturating_accrue --- frame/tips/src/migrations/unreserve_all_funds.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/tips/src/migrations/unreserve_all_funds.rs b/frame/tips/src/migrations/unreserve_all_funds.rs index e1a260f20ef72..c79cb9ffe91cc 100644 --- a/frame/tips/src/migrations/unreserve_all_funds.rs +++ b/frame/tips/src/migrations/unreserve_all_funds.rs @@ -54,7 +54,7 @@ impl, I: 'static> UnreserveAllFunds { .map(|(_hash, open_tip)| open_tip) .fold(BTreeMap::new(), |mut acc, tip| { // Count the total number of tips - tips_len += 1; + tips_len.saturating_accrue(1); // Add the balance to the account's existing deposit in the accumulator acc.entry(tip.finder).or_insert(Zero::zero()).saturating_accrue(tip.deposit); From 9885ffacc1edf4f35c354db85b38ef7f94f0ccd8 Mon Sep 17 00:00:00 2001 From: Liam Aharon Date: Fri, 2 Jun 2023 12:47:38 +0200 Subject: [PATCH 11/16] test pre_upgrade and post_upgrade --- frame/tips/src/migrations/unreserve_all_funds.rs | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/frame/tips/src/migrations/unreserve_all_funds.rs b/frame/tips/src/migrations/unreserve_all_funds.rs index c79cb9ffe91cc..8df7875296d1e 100644 --- a/frame/tips/src/migrations/unreserve_all_funds.rs +++ b/frame/tips/src/migrations/unreserve_all_funds.rs @@ -172,7 +172,7 @@ where } } -#[cfg(test)] +#[cfg(all(feature = "try-runtime", test))] mod test { use super::*; use crate::{ @@ -231,7 +231,12 @@ mod test { ); // Execute the migration + let bytes = match UnreserveAllFunds::::pre_upgrade() { + Ok(bytes) => bytes, + Err(e) => panic!("pre_upgrade failed: {:?}", e), + }; UnreserveAllFunds::::on_runtime_upgrade(); + assert_ok!(UnreserveAllFunds::::post_upgrade(bytes)); // Check the deposits were were unreserved assert_eq!( From af4b2a72d917284fc967389c932cafa979dd3972 Mon Sep 17 00:00:00 2001 From: Liam Aharon Date: Sun, 4 Jun 2023 12:05:14 +0200 Subject: [PATCH 12/16] remove pallet_treasury bound --- frame/tips/src/migrations/unreserve_all_funds.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/frame/tips/src/migrations/unreserve_all_funds.rs b/frame/tips/src/migrations/unreserve_all_funds.rs index 8df7875296d1e..5e5d97eebecb3 100644 --- a/frame/tips/src/migrations/unreserve_all_funds.rs +++ b/frame/tips/src/migrations/unreserve_all_funds.rs @@ -65,8 +65,7 @@ impl, I: 'static> UnreserveAllFunds { } } -impl + pallet_treasury::Config, I: 'static> OnRuntimeUpgrade - for UnreserveAllFunds +impl, I: 'static> OnRuntimeUpgrade for UnreserveAllFunds where BalanceOf: Sum, { From 6e9d9a43d992389f911950b5bbe3d93559afde6b Mon Sep 17 00:00:00 2001 From: Liam Aharon Date: Sun, 4 Jun 2023 12:28:47 +0200 Subject: [PATCH 13/16] resolve pr comments --- .../src/migrations/unreserve_all_funds.rs | 35 ++++++++++--------- 1 file changed, 19 insertions(+), 16 deletions(-) diff --git a/frame/tips/src/migrations/unreserve_all_funds.rs b/frame/tips/src/migrations/unreserve_all_funds.rs index 5e5d97eebecb3..2e44c1ce61ff8 100644 --- a/frame/tips/src/migrations/unreserve_all_funds.rs +++ b/frame/tips/src/migrations/unreserve_all_funds.rs @@ -19,10 +19,7 @@ //! pallet. use core::iter::Sum; -use frame_support::{ - traits::{OnRuntimeUpgrade, ReservableCurrency}, - weights::constants::RocksDbWeight, -}; +use frame_support::traits::{OnRuntimeUpgrade, ReservableCurrency}; use pallet_treasury::BalanceOf; use sp_runtime::{traits::Zero, Saturating}; use sp_std::collections::btree_map::BTreeMap; @@ -49,6 +46,8 @@ impl, I: 'static> UnreserveAllFunds { /// reserved balance by this pallet /// * `frame_support::weights::Weight`: The weight of this operation. fn get_deposits() -> (BTreeMap>, frame_support::weights::Weight) { + use frame_support::traits::Get; + let mut tips_len = 0; let account_deposits: BTreeMap> = crate::Tips::::iter() .map(|(_hash, open_tip)| open_tip) @@ -61,7 +60,7 @@ impl, I: 'static> UnreserveAllFunds { acc }); - (account_deposits, RocksDbWeight::get().reads(tips_len)) + (account_deposits, T::DbWeight::get().reads(tips_len)) } } @@ -118,6 +117,8 @@ where /// Executes the migration, unreserving funds that are locked in Tip deposits. fn on_runtime_upgrade() -> frame_support::weights::Weight { + use frame_support::traits::Get; + // Get staked and deposited balances as reported by this pallet. let (account_deposits, initial_reads) = Self::get_deposits(); @@ -129,7 +130,7 @@ where T::Currency::unreserve(&account, *unreserve_amount); } - RocksDbWeight::get() + T::DbWeight::get() .reads_writes(account_deposits.len() as u64, account_deposits.len() as u64) .saturating_add(initial_reads) } @@ -157,14 +158,17 @@ where .expect("account deposit must exist to be in account_reserved_before, qed"); let expected_reserved_after = actual_reserved_before.saturating_sub(expected_amount_deducted); - assert!( - actual_reserved_after == expected_reserved_after, - "Reserved balance for {:?} is incorrect. actual before: {:?}, actual after, {:?}, expected deducted: {:?}", - account, - actual_reserved_before, - actual_reserved_after, - expected_amount_deducted - ); + + if actual_reserved_after != expected_reserved_after { + log::error!( + "Reserved balance for {:?} is incorrect. actual before: {:?}, actual after, {:?}, expected deducted: {:?}", + account, + actual_reserved_before, + actual_reserved_after, + expected_amount_deducted + ); + return Err("Reserved balance is incorrect".into()) + } } Ok(()) @@ -178,8 +182,7 @@ mod test { migrations::unreserve_all_funds::UnreserveAllFunds, tests::{new_test_ext, RuntimeOrigin, Test, Tips}, }; - use frame_support::assert_ok; - use sp_core::TypedGet; + use frame_support::{assert_ok, traits::TypedGet}; #[test] fn unreserve_all_funds_works() { From 7b51e6b1165bfa283573c053eea15fbbb85a3fe1 Mon Sep 17 00:00:00 2001 From: Liam Aharon Date: Sun, 4 Jun 2023 12:31:18 +0200 Subject: [PATCH 14/16] rename migration --- frame/tips/src/migrations/mod.rs | 2 +- ...nreserve_all_funds.rs => unreserve_deposits.rs} | 14 +++++++------- 2 files changed, 8 insertions(+), 8 deletions(-) rename frame/tips/src/migrations/{unreserve_all_funds.rs => unreserve_deposits.rs} (95%) diff --git a/frame/tips/src/migrations/mod.rs b/frame/tips/src/migrations/mod.rs index 38cdba49340f3..9cdd01c17fbf6 100644 --- a/frame/tips/src/migrations/mod.rs +++ b/frame/tips/src/migrations/mod.rs @@ -23,4 +23,4 @@ pub mod v4; /// A migration that unreserves all funds held in the context of this pallet. -pub mod unreserve_all_funds; +pub mod unreserve_deposits; diff --git a/frame/tips/src/migrations/unreserve_all_funds.rs b/frame/tips/src/migrations/unreserve_deposits.rs similarity index 95% rename from frame/tips/src/migrations/unreserve_all_funds.rs rename to frame/tips/src/migrations/unreserve_deposits.rs index 2e44c1ce61ff8..878d80fe1bb4e 100644 --- a/frame/tips/src/migrations/unreserve_all_funds.rs +++ b/frame/tips/src/migrations/unreserve_deposits.rs @@ -34,9 +34,9 @@ use sp_std::vec::Vec; /// The pallet should be made inoperable before or immediately after this migration is run. /// /// (See also the `RemovePallet` migration in `frame/support/src/migrations.rs`) -pub struct UnreserveAllFunds, I: 'static>(sp_std::marker::PhantomData<(T, I)>); +pub struct UnreserveDeposits, I: 'static>(sp_std::marker::PhantomData<(T, I)>); -impl, I: 'static> UnreserveAllFunds { +impl, I: 'static> UnreserveDeposits { /// Calculates and returns the total amount reserved by each account by this pallet from open /// tips. /// @@ -64,7 +64,7 @@ impl, I: 'static> UnreserveAllFunds { } } -impl, I: 'static> OnRuntimeUpgrade for UnreserveAllFunds +impl, I: 'static> OnRuntimeUpgrade for UnreserveDeposits where BalanceOf: Sum, { @@ -179,7 +179,7 @@ where mod test { use super::*; use crate::{ - migrations::unreserve_all_funds::UnreserveAllFunds, + migrations::unreserve_deposits::UnreserveDeposits, tests::{new_test_ext, RuntimeOrigin, Test, Tips}, }; use frame_support::{assert_ok, traits::TypedGet}; @@ -233,12 +233,12 @@ mod test { ); // Execute the migration - let bytes = match UnreserveAllFunds::::pre_upgrade() { + let bytes = match UnreserveDeposits::::pre_upgrade() { Ok(bytes) => bytes, Err(e) => panic!("pre_upgrade failed: {:?}", e), }; - UnreserveAllFunds::::on_runtime_upgrade(); - assert_ok!(UnreserveAllFunds::::post_upgrade(bytes)); + UnreserveDeposits::::on_runtime_upgrade(); + assert_ok!(UnreserveDeposits::::post_upgrade(bytes)); // Check the deposits were were unreserved assert_eq!( From e4c8cbbc7a7e016278cc4ba4af401ccf6a4eb3b0 Mon Sep 17 00:00:00 2001 From: Liam Aharon Date: Mon, 5 Jun 2023 10:25:50 +0200 Subject: [PATCH 15/16] kick ci --- frame/multisig/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/multisig/src/lib.rs b/frame/multisig/src/lib.rs index 64058be9c8fbf..3586b4a81a4fb 100644 --- a/frame/multisig/src/lib.rs +++ b/frame/multisig/src/lib.rs @@ -175,7 +175,7 @@ pub mod pallet { #[pallet::storage_version(STORAGE_VERSION)] pub struct Pallet(_); - /// The set of open multisig operations. + /// The set of open multisig operations #[pallet::storage] pub type Multisigs = StorageDoubleMap< _, From 1aeb2911696b3334041efa87ba20878afad04528 Mon Sep 17 00:00:00 2001 From: Liam Aharon Date: Mon, 5 Jun 2023 10:26:25 +0200 Subject: [PATCH 16/16] kick ci --- frame/multisig/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/multisig/src/lib.rs b/frame/multisig/src/lib.rs index 3586b4a81a4fb..64058be9c8fbf 100644 --- a/frame/multisig/src/lib.rs +++ b/frame/multisig/src/lib.rs @@ -175,7 +175,7 @@ pub mod pallet { #[pallet::storage_version(STORAGE_VERSION)] pub struct Pallet(_); - /// The set of open multisig operations + /// The set of open multisig operations. #[pallet::storage] pub type Multisigs = StorageDoubleMap< _,