Fix bbbnc test bug

pallets/bb-bnc/src/benchmarking.rs

@@ -284,6 +284,18 @@ mod benchmarks {
 			BalanceOf::<T>::unique_saturated_from(100_000_000_000_000_u128),
 		)?;
 
+		T::MultiCurrency::mint_into(
+			CurrencyId::Native(TokenSymbol::BNC),
+			&test_account,
+			BalanceOf::<T>::unique_saturated_from(900_000_000_000_000_u128),
+		)?;
+
+		T::MultiCurrency::mint_into(
+			CurrencyId::VToken(TokenSymbol::BNC),
+			&test_account,
+			BalanceOf::<T>::unique_saturated_from(900_000_000_000_000_u128),
+		)?;
+
 		let issuance_adjustment: i128 =
 			<T::MultiCurrency as frame_support::traits::fungibles::Inspect<T::AccountId>>::total_issuance(CurrencyId::VToken(TokenSymbol::BNC))
 				.unique_saturated_into();
@@ -301,19 +313,18 @@ mod benchmarks {
 			rewards
 		));
 
-		assert_ok!(BbBNC::<T>::create_lock(
-			RawOrigin::Signed(test_account.clone()).into(),
-			BalanceOf::<T>::unique_saturated_from(10_000_000_000_000_u128),
-			(365 * 86400 / 12u32).into()
-		));
+		for _ in 0..T::MaxPositions::get() {
+			assert_ok!(BbBNC::<T>::create_lock(
+				RawOrigin::Signed(test_account.clone()).into(),
+				BalanceOf::<T>::unique_saturated_from(10_000_000_000_000_u128),
+				(7 * 86400 / 12u32).into()
+			));
+		}
 
-		<frame_system::Pallet<T>>::set_block_number((30 * 86400 / 12u32).into());
+		<frame_system::Pallet<T>>::set_block_number((14 * 86400 / 12u32).into());
 
 		#[extrinsic_call]
-		_(
-			RawOrigin::Signed(test_account),
-			ClaimOption::CreatePosition { lock_months: 12 },
-		);
+		_(RawOrigin::Signed(test_account), ClaimOption::DirectBnc);
 
 		Ok(())
 	}

pallets/bb-bnc/src/incentive.rs

@@ -25,7 +25,7 @@ use frame_support::{
 };
 pub use pallet::*;
 use sp_runtime::Permill;
-use sp_std::collections::btree_map::BTreeMap;
+use sp_std::{collections::btree_map::BTreeMap, vec::Vec};
 
 /// Type alias for the complex IncentiveConfig with generic parameters
 pub type IncentiveConfigOf<T> =
@@ -214,7 +214,7 @@ impl<T: Config> Pallet<T> {
 		Self::calculate_reward_per_token_at(pool_id, current_block_number)
 	}
 
-	fn calculate_reward_per_token_at(
+	pub(crate) fn calculate_reward_per_token_at(
 		pool_id: PoolId,
 		block_number: BlockNumberFor<T>,
 	) -> Result<IncentiveConfigOf<T>, DispatchError> {
@@ -261,24 +261,42 @@ impl<T: Config> Pallet<T> {
 		share_info: Option<(BalanceOf<T>, BalanceOf<T>)>,
 	) -> DispatchResult {
 		let mut rewards = Rewards::<T>::get(who).unwrap_or_default();
+		let user_reward_per_token_paid = UserRewardPerTokenPaid::<T>::get(who);
+		Self::accumulate_reward_delta(
+			&mut rewards,
+			bbbnc_balance,
+			&reward_per_token,
+			&user_reward_per_token_paid,
+			share_info,
+		)?;
+
+		if rewards != BTreeMap::<CurrencyIdOf<T>, BalanceOf<T>>::default() {
+			Rewards::<T>::insert(who, rewards);
+		}
+		UserRewardPerTokenPaid::<T>::insert(who, reward_per_token);
+		Ok(())
+	}
+
+	pub(crate) fn accumulate_reward_delta(
+		rewards: &mut BTreeMap<CurrencyIdOf<T>, BalanceOf<T>>,
+		bbbnc_balance: BalanceOf<T>,
+		reward_per_token: &BTreeMap<CurrencyIdOf<T>, BalanceOf<T>>,
+		user_reward_per_token_paid: &BTreeMap<CurrencyIdOf<T>, BalanceOf<T>>,
+		share_info: Option<(BalanceOf<T>, BalanceOf<T>)>,
+	) -> DispatchResult {
+		let zero = BalanceOf::<T>::zero();
 		reward_per_token
 			.iter()
 			.try_for_each(|(currency, reward)| -> DispatchResult {
+				let reward_delta = reward
+					.saturating_sub(*user_reward_per_token_paid.get(currency).unwrap_or(&zero));
 				let increment = U256::from(bbbnc_balance.saturated_into::<u128>())
-					.checked_mul(U256::from(
-						reward
-							.saturating_sub(
-								*UserRewardPerTokenPaid::<T>::get(who)
-									.get(currency)
-									.unwrap_or(&BalanceOf::<T>::zero()),
-							)
-							.saturated_into::<u128>(),
-					))
+					.checked_mul(U256::from(reward_delta.saturated_into::<u128>()))
 					.ok_or(ArithmeticError::Overflow)?
 					.checked_div(U256::from(T::Multiplier::get().saturated_into::<u128>()))
 					.ok_or(ArithmeticError::Overflow)?;
 
-				let reward = match share_info {
+				let reward: BalanceOf<T> = match share_info {
 					Some((share, total_share)) => increment
 						.checked_mul(U256::from(share.saturated_into::<u128>()))
 						.ok_or(ArithmeticError::Overflow)?
@@ -292,20 +310,16 @@ impl<T: Config> Pallet<T> {
 						.unique_saturated_into(),
 				};
 
-				rewards
-					.entry(*currency)
-					.and_modify(|total_reward| {
-						*total_reward = total_reward.saturating_add(reward);
-					})
-					.or_insert(reward);
+				if !reward.is_zero() {
+					rewards
+						.entry(*currency)
+						.and_modify(|total_reward| {
+							*total_reward = total_reward.saturating_add(reward);
+						})
+						.or_insert(reward);
+				}
 				Ok(())
-			})?;
-
-		if rewards != BTreeMap::<CurrencyIdOf<T>, BalanceOf<T>>::default() {
-			Rewards::<T>::insert(who, rewards);
-		}
-		UserRewardPerTokenPaid::<T>::insert(who, reward_per_token);
-		Ok(())
+			})
 	}
 
 	pub(crate) fn update_reward_before_expiry(
@@ -322,6 +336,63 @@ impl<T: Config> Pallet<T> {
 		Self::update_account_reward(who, bbbnc_balance, reward_per_token_stored, None)
 	}
 
+	pub(crate) fn settle_unprocessed_expiry_rewards(who: &AccountIdOf<T>) -> DispatchResult {
+		let current_block_number: BlockNumberFor<T> =
+			T::BlockNumberProvider::current_block_number();
+		let next_expiring = NextExpiringBlock::<T>::get();
+		if next_expiring.is_zero() || next_expiring > current_block_number {
+			return Ok(());
+		}
+
+		for expiry_block in Self::unprocessed_expiry_blocks(who) {
+			Self::update_reward_before_expiry(who, expiry_block)?;
+		}
+
+		Ok(())
+	}
+
+	pub(crate) fn unprocessed_expiry_blocks(who: &AccountIdOf<T>) -> Vec<BlockNumberFor<T>> {
+		let current_block_number: BlockNumberFor<T> =
+			T::BlockNumberProvider::current_block_number();
+		let mut expiry_blocks = Vec::new();
+
+		for position in UserPositions::<T>::get(who) {
+			let locked = Locked::<T>::get(position);
+			if !locked.end.is_zero()
+				&& locked.end <= current_block_number
+				&& ExpiringPositions::<T>::contains_key(locked.end, position)
+			{
+				expiry_blocks.push(locked.end);
+			}
+		}
+
+		expiry_blocks.sort();
+		expiry_blocks.dedup();
+		expiry_blocks
+	}
+
+	pub(crate) fn reward_per_token_before_expiry(
+		expiry_block: BlockNumberFor<T>,
+		conf: &IncentiveConfigOf<T>,
+	) -> Result<BTreeMap<CurrencyIdOf<T>, BalanceOf<T>>, DispatchError> {
+		let reward_block = expiry_block.saturating_sub(One::one());
+		if let Some(snapshot) = ExpiryRewardPerToken::<T>::get(expiry_block) {
+			return Ok(snapshot);
+		}
+
+		if reward_block <= conf.last_update_time {
+			if conf.last_update_time > expiry_block {
+				return Ok(BTreeMap::new());
+			}
+			return Ok(conf.reward_per_token_stored.clone());
+		}
+
+		Ok(
+			Self::calculate_reward_per_token_at(BB_BNC_SYSTEM_POOL_ID, reward_block)?
+				.reward_per_token_stored,
+		)
+	}
+
 	pub(crate) fn ensure_expiry_reward_snapshot(expiry_block: BlockNumberFor<T>) -> DispatchResult {
 		if ExpiryRewardPerToken::<T>::contains_key(expiry_block) {
 			return Ok(());
@@ -366,6 +437,12 @@ impl<T: Config> Pallet<T> {
 		who: Option<&AccountIdOf<T>>,
 		share_info: Option<(BalanceOf<T>, BalanceOf<T>)>,
 	) -> DispatchResult {
+		if pool_id == BB_BNC_SYSTEM_POOL_ID {
+			if let Some(account) = who {
+				Self::settle_unprocessed_expiry_rewards(account)?;
+			}
+		}
+
 		let reward_per_token_stored = Self::reward_per_token(pool_id)?;
 
 		IncentiveConfigs::<T>::mutate(pool_id, |item| {

pallets/bb-bnc/src/lib.rs

@@ -643,8 +643,6 @@ pub mod pallet {
 						.take(remaining_limit as usize)
 						.map(|(position, _)| position)
 						.collect::<Vec<_>>();
-					let positions_len = positions.len() as u32;
-					let block_limit_reached = positions_len == remaining_limit;
 					let mut block_has_retained_positions = false;
 
 					if !positions.is_empty() {
@@ -701,7 +699,9 @@ pub mod pallet {
 						}
 					}
 
-					if block_has_retained_positions || block_limit_reached {
+					let block_has_remaining_positions =
+						ExpiringPositions::<T>::iter_prefix(block).next().is_some();
+					if block_has_retained_positions || block_has_remaining_positions {
 						blocks_exhausted = true;
 					} else {
 						Self::cleanup_expiry_reward_snapshot(block);
@@ -729,7 +729,7 @@ pub mod pallet {
 							"Failed to create reward snapshot for throttled expiry block {block:?}: {e:?}",
 						);
 					}
-					weight = weight.saturating_add(T::DbWeight::get().reads_writes(2, 1));
+					weight = weight.saturating_add(T::DbWeight::get().reads_writes(60, 2));
 				}
 			}
 
@@ -1130,33 +1130,36 @@ pub mod pallet {
 				Error::<T>::NoController
 			);
 
-			let mut rewards = BTreeMap::new();
-			let user_reward_per_token_paid = UserRewardPerTokenPaid::<T>::get(who);
+			let mut rewards = Rewards::<T>::get(who).unwrap_or_default();
+			let mut user_reward_per_token_paid = UserRewardPerTokenPaid::<T>::get(who);
+
+			for expiry_block in Self::unprocessed_expiry_blocks(who) {
+				let reward_block = expiry_block.saturating_sub(One::one());
+				let reward_per_token_stored =
+					Self::reward_per_token_before_expiry(expiry_block, &conf)?;
+				let bbbnc_balance = Self::balance_of(who, Some(reward_block))?;
+				Self::accumulate_reward_delta(
+					&mut rewards,
+					bbbnc_balance,
+					&reward_per_token_stored,
+					&user_reward_per_token_paid,
+					None,
+				)?;
+				user_reward_per_token_paid = reward_per_token_stored;
+			}
 
 			// Get current reward per token for all currencies
 			let reward_per_token =
 				Self::calculate_reward_per_token(BB_BNC_SYSTEM_POOL_ID)?.reward_per_token_stored;
 
-			let zero = Zero::zero();
-			// Calculate pending rewards for each currency
-			for (currency_id, current_reward_per_token) in reward_per_token {
-				let paid = user_reward_per_token_paid
-					.get(&currency_id)
-					.unwrap_or(&zero);
-
-				if let Some(reward) = current_reward_per_token.checked_sub(*paid) {
-					let balance = Self::balance_of_current_block(who)?;
-					// pending = reward * balance / multiplier
-					let pending: BalanceOf<T> = FixedU128::checked_from_rational(
-						balance.saturated_into::<u128>(),
-						T::Multiplier::get().saturated_into::<u128>(),
-					)
-					.and_then(|ratio| ratio.checked_mul_int(reward.saturated_into::<u128>()))
-					.ok_or(ArithmeticError::Overflow)?
-					.saturated_into();
-					rewards.insert(currency_id, pending);
-				}
-			}
+			let balance = Self::balance_of_current_block(who)?;
+			Self::accumulate_reward_delta(
+				&mut rewards,
+				balance,
+				&reward_per_token,
+				&user_reward_per_token_paid,
+				None,
+			)?;
 
 			Ok(rewards.into_iter().collect())
 		}
@@ -2104,6 +2107,8 @@ pub mod pallet {
 		) -> DispatchResult {
 			if if_fast.is_none() && !locked.end.is_zero() {
 				Self::update_reward_before_expiry(who, locked.end)?;
+			} else if if_fast.is_some() {
+				Self::update_reward_all(who)?;
 			}
 			let value = locked.amount;
 			let old_locked: LockedBalance<BalanceOf<T>, BlockNumberFor<T>> = locked.clone();
@@ -2389,10 +2394,13 @@ impl<T: Config> BbBNCInterface<AccountIdOf<T>, CurrencyIdOf<T>, BalanceOf<T>, Bl
 			T::BlockNumberProvider::current_block_number();
 		ensure!(current_block_number >= locked.end, Error::<T>::Expired);
 
-		// Remove position from expiring mapping
-		Self::remove_expiring_position(position, locked.end);
+		let expiry_block = locked.end;
+		Self::withdraw_no_ensure(who, position, locked, None)?;
 
-		Self::withdraw_no_ensure(who, position, locked, None)
+		// Remove position from expiring mapping after expiry reward settlement so the
+		// last position at an expiry block cannot drop the shared reward snapshot.
+		Self::remove_expiring_position(position, expiry_block);
+		Ok(())
 	}
 
 	fn balance_of(

pallets/bb-bnc/src/mock.rs

@@ -342,18 +342,11 @@ impl pallet_xcm::Config for Runtime {
 	type AuthorizedAliasConsideration = Disabled;
 }
 
+#[derive(Default)]
 pub struct ExtBuilder {
 	endowed_accounts: Vec<(AccountId, CurrencyId, Balance)>,
 }
 
-impl Default for ExtBuilder {
-	fn default() -> Self {
-		Self {
-			endowed_accounts: vec![],
-		}
-	}
-}
-
 impl ExtBuilder {
 	pub fn balances(mut self, endowed_accounts: Vec<(AccountId, CurrencyId, Balance)>) -> Self {
 		self.endowed_accounts = endowed_accounts;
@@ -441,7 +434,7 @@ impl ExtBuilder {
 }
 
 pub fn asset_registry() {
-	let items = vec![
+	let items = [
 		(
 			DOT,
 			AssetMetadata {