NODE-79, bifrost pallets improvements (#34)

* NODE-79, feature: migrate to standard storage version for bfc-staking

* NODE-79, feature: migrate to use new standard storage version for `bfc-offences`, `bfc-utility`, `relay-manager`.

* NODE-79, refactor: replace `OrderedSet` with `BTreeMap` & `BTreeSet`

* NODE-79, refactor: calculate translate weight info

* NODE-79, refactor: Remove redundant code

* NODE-79, refactor: Remove panic point in `bfc-staking` as much as possible

* NODE-79, refactor: use btree set in `SelectedCandidates`, `SelectedFullCandidates`, `SelectedBasicCandidates`, `CachedSelectedCandidates`

* NODE-79, refactor: remove unnecessary path prefix

* NODE-79, refactor: remove unnecessary reverse

* NODE-79, refactor: migrate `CandidatePool` to use BoundedBTreeMap

* NODE-79, refactor: kill storage `MinTotalSelected`

* NODE-79, refactor: remove unnecessary path prefix

* NODE-79, fix: typo

* NODE-79, refactor: use iter_mut

* NODE-79, chore: remove deadcode

* NODE-79, refactor: use mutate

* NODE-79, refactor: clone in small range

* NODE-79, refactor: no need to be complicate

* NODE-79, refactor: use getter

* NODE-79, fix: should use take instead get

* NODE-79, refactor: remove dead code

* NODE-79, refactor: no need to be complicated

* NODE-79, refactor: use getter

* NODE-79, refactor: `relay-manager`

* NODE-79, refactor: .

* NODE-79, refactor: remove redundant

* NODE-79, fix: revert message

* NODE-79, refactor: `bfc-staking`

* NODE-79, refactor: `relay-manager`

* NODE-79, refactor: move under util methods

* NODE-79, refactor: impl From<Offence> for EvmOffence

* NODE-79, refactor: reduce getter call

* NODE-79, refactor: remove unnecessary clone

* NODE-79, refactor: `Nominator.initial_nominations` type to BTreeMap

* NODE-79, refactor: .

* NODE-79, chore: remove dead code

* NODE-79, fix:

* NODE-79, fix:

* NODE-79, test: fit into new storage format

* NODE-79, fix: modify if exists else insert

* NODE-79, refactor: remove dead code

* NODE-79, fix: condition check on OnRuntimeUpgrade

* NODE-79, chore: comment

* NODE-79, fix: remove duplicate translate in migrations

pallets/bfc-offences/src/lib.rs

@@ -27,6 +27,22 @@ pub type NegativeImbalanceOf<T> = <<T as Config>::Currency as Currency<
 	<T as frame_system::Config>::AccountId,
 >>::NegativeImbalance;
 
+pub(crate) const LOG_TARGET: &'static str = "runtime::bfc-offences";
+
+// syntactic sugar for logging.
+#[macro_export]
+macro_rules! log {
+	($level:tt, $patter:expr $(, $values:expr)* $(,)?) => {
+		log::$level!(
+			target: crate::LOG_TARGET,
+			concat!("[{:?}] 💸 ", $patter), <frame_system::Pallet<T>>::block_number() $(, $values)*
+		)
+	};
+}
+
+/// Used for release versioning upto v2_0_0.
+///
+/// Obsolete from v3. Keeping around to make encoding/decoding of old migration code easier.
 #[derive(Encode, Decode, Clone, Copy, PartialEq, Eq, RuntimeDebug, TypeInfo, MaxEncodedLen)]
 /// A value that represents the current storage version of this pallet.
 ///

pallets/bfc-offences/src/migrations.rs

@@ -1,11 +1,65 @@
 use super::*;
+use frame_support::{dispatch::Weight, pallet_prelude::*, storage_alias, traits::OnRuntimeUpgrade};
+
+#[storage_alias]
+pub type StorageVersion<T: Config> = StorageValue<Pallet<T>, Releases, ValueQuery>;
+
+pub mod v3 {
+	use super::*;
+	#[cfg(feature = "try-runtime")]
+	use sp_runtime::TryRuntimeError;
+
+	pub struct MigrateToV3<T>(PhantomData<T>);
+
+	impl<T: Config> OnRuntimeUpgrade for MigrateToV3<T> {
+		fn on_runtime_upgrade() -> Weight {
+			let mut weight = Weight::zero();
+
+			let current = Pallet::<T>::current_storage_version();
+			let onchain = StorageVersion::<T>::get();
+
+			if current == 3 && onchain == Releases::V2_0_0 {
+				// migrate to new standard storage version
+				StorageVersion::<T>::kill();
+				current.put::<Pallet<T>>();
+
+				log!(info, "bfc-offences storage migration passes v3 update ✅");
+				weight = weight.saturating_add(T::DbWeight::get().reads_writes(1, 2));
+			} else {
+				log!(warn, "Skipping v3, should be removed");
+				weight = weight.saturating_add(T::DbWeight::get().reads(1));
+			}
+
+			weight
+		}
+
+		#[cfg(feature = "try-runtime")]
+		fn pre_upgrade() -> Result<Vec<u8>, TryRuntimeError> {
+			ensure!(
+				StorageVersion::<T>::get() == Releases::V2_0_0,
+				"Required v2_0_0 before upgrading to v3"
+			);
+
+			Ok(Default::default())
+		}
+
+		#[cfg(feature = "try-runtime")]
+		fn post_upgrade(_state: Vec<u8>) -> Result<(), TryRuntimeError> {
+			ensure!(Pallet::<T>::on_chain_storage_version() == 3, "v3 not applied");
+
+			ensure!(!StorageVersion::<T>::exists(), "Storage version not migrated correctly");
+
+			Ok(())
+		}
+	}
+}
 
 pub mod v2 {
 	use super::*;
 	use frame_support::{pallet_prelude::Weight, traits::Get};
 
 	pub fn pre_migrate<T: Config>() -> Result<(), &'static str> {
-		frame_support::ensure!(
+		ensure!(
 			StorageVersion::<T>::get() == Releases::V1_0_0,
 			"Storage version must match to v1.0.0",
 		);
@@ -27,7 +81,7 @@ pub mod v1 {
 	use frame_support::{pallet_prelude::Weight, traits::Get};
 
 	pub fn pre_migrate<T: Config>() -> Result<(), &'static str> {
-		frame_support::ensure!(
+		ensure!(
 			StorageVersion::<T>::get() == Releases::V1_0_0,
 			"Storage version must match to v1.0.0",
 		);

pallets/bfc-offences/src/pallet/impls.rs

@@ -61,33 +61,33 @@ impl<T: Config> OffenceHandler<T::AccountId, BalanceOf<T>> for Pallet<T> {
 		slash_fraction: Perbill,
 		bond: BalanceOf<T>,
 	) -> BalanceOf<T> {
-		let mut slash_amount = BalanceOf::<T>::zero();
 		// slash bonds only if activated
-		if IsSlashActive::<T>::get() {
-			slash_amount = slash_fraction * bond;
+		if Self::is_slash_active() {
+			let slash_amount = slash_fraction * bond;
 			// slash the validator's reserved self bond
 			// the slashed imbalance will be reserved to the treasury
 			T::Slash::on_unbalanced(T::Currency::slash_reserved(stash, slash_amount).0);
 			Self::deposit_event(Event::Slashed { who: who.clone(), amount: slash_amount });
+			return slash_amount;
 		}
-		slash_amount
+		BalanceOf::<T>::zero()
 	}
 
 	fn refresh_offences(session_index: SessionIndex) {
-		for offences in ValidatorOffences::<T>::iter() {
+		<ValidatorOffences<T>>::iter().for_each(|offences| {
 			if (session_index - offences.1.latest_offence_session_index)
-				> OffenceExpirationInSessions::<T>::get()
+				> Self::offence_expiration_in_sessions()
 			{
-				ValidatorOffences::<T>::remove(&offences.0);
+				<ValidatorOffences<T>>::remove(&offences.0);
 			}
-		}
+		});
 	}
 
 	fn is_offence_count_exceeds(count: u32, tier: TierType) -> bool {
 		// if offence count exceeds the configured limit
 		return match tier {
-			TierType::Full => count > FullMaximumOffenceCount::<T>::get(),
-			_ => count > BasicMaximumOffenceCount::<T>::get(),
+			TierType::Full => count > Self::full_maximum_offence_count(),
+			_ => count > Self::basic_maximum_offence_count(),
 		};
 	}
 }

pallets/bfc-offences/src/pallet/mod.rs

@@ -1,13 +1,13 @@
 mod impls;
 
 use crate::{
-	BalanceOf, NegativeImbalanceOf, OffenceCount, Releases, ValidatorOffenceInfo, WeightInfo,
+	migrations, BalanceOf, NegativeImbalanceOf, OffenceCount, ValidatorOffenceInfo, WeightInfo,
 };
 
 use bp_staking::TierType;
 use frame_support::{
 	pallet_prelude::*,
-	traits::{Currency, OnUnbalanced, ReservableCurrency},
+	traits::{Currency, OnRuntimeUpgrade, OnUnbalanced, ReservableCurrency, StorageVersion},
 };
 use frame_system::pallet_prelude::*;
 use sp_staking::SessionIndex;
@@ -16,8 +16,12 @@ use sp_staking::SessionIndex;
 pub mod pallet {
 	use super::*;
 
+	/// The current storage version.
+	const STORAGE_VERSION: StorageVersion = StorageVersion::new(3);
+
 	/// Pallet for bfc offences
 	#[pallet::pallet]
+	#[pallet::storage_version(STORAGE_VERSION)]
 	pub struct Pallet<T>(_);
 
 	/// Configuration trait of this pallet
@@ -71,10 +75,6 @@ pub mod pallet {
 		Slashed { who: T::AccountId, amount: BalanceOf<T> },
 	}
 
-	#[pallet::storage]
-	/// Storage version of the pallet
-	pub(crate) type StorageVersion<T: Config> = StorageValue<_, Releases, ValueQuery>;
-
 	#[pallet::storage]
 	#[pallet::unbounded]
 	#[pallet::getter(fn validator_offences)]
@@ -107,6 +107,13 @@ pub mod pallet {
 	/// The current activation of validator slashing
 	pub type IsSlashActive<T: Config> = StorageValue<_, bool, ValueQuery>;
 
+	#[pallet::hooks]
+	impl<T: Config> Hooks<BlockNumberFor<T>> for Pallet<T> {
+		fn on_runtime_upgrade() -> Weight {
+			migrations::v3::MigrateToV3::<T>::on_runtime_upgrade()
+		}
+	}
+
 	#[pallet::genesis_config]
 	pub struct GenesisConfig {}
 
@@ -120,7 +127,6 @@ pub mod pallet {
 	#[pallet::genesis_build]
 	impl<T: Config> GenesisBuild<T> for GenesisConfig {
 		fn build(&self) {
-			StorageVersion::<T>::put(Releases::V2_0_0);
 			OffenceExpirationInSessions::<T>::put(T::DefaultOffenceExpirationInSessions::get());
 			FullMaximumOffenceCount::<T>::put(T::DefaultFullMaximumOffenceCount::get());
 			BasicMaximumOffenceCount::<T>::put(T::DefaultBasicMaximumOffenceCount::get());
@@ -140,7 +146,7 @@ pub mod pallet {
 			origin: OriginFor<T>,
 			new: SessionIndex,
 		) -> DispatchResultWithPostInfo {
-			frame_system::ensure_root(origin)?;
+			ensure_root(origin)?;
 			ensure!(new > 0u32, Error::<T>::CannotSetBelowMin);
 			let old = <OffenceExpirationInSessions<T>>::get();
 			ensure!(old != new, Error::<T>::NoWritingSameValue);
@@ -159,7 +165,7 @@ pub mod pallet {
 			new: OffenceCount,
 			tier: TierType,
 		) -> DispatchResultWithPostInfo {
-			frame_system::ensure_root(origin)?;
+			ensure_root(origin)?;
 			ensure!(new > 0u32, Error::<T>::CannotSetBelowMin);
 			match tier {
 				TierType::Full => {
@@ -207,7 +213,7 @@ pub mod pallet {
 			origin: OriginFor<T>,
 			is_active: bool,
 		) -> DispatchResultWithPostInfo {
-			frame_system::ensure_root(origin)?;
+			ensure_root(origin)?;
 			ensure!(is_active != <IsOffenceActive<T>>::get(), Error::<T>::NoWritingSameValue);
 			<IsOffenceActive<T>>::put(is_active);
 			Self::deposit_event(Event::OffenceActivationSet { is_active });
@@ -223,7 +229,7 @@ pub mod pallet {
 			origin: OriginFor<T>,
 			is_active: bool,
 		) -> DispatchResultWithPostInfo {
-			frame_system::ensure_root(origin)?;
+			ensure_root(origin)?;
 			ensure!(is_active != <IsSlashActive<T>>::get(), Error::<T>::NoWritingSameValue);
 			<IsSlashActive<T>>::put(is_active);
 			Self::deposit_event(Event::SlashActivationSet { is_active });