feat: add docs and additional error cases (#827)

* feat: add docs and additional error cases

* chore: add another test

* chore: fix tests

* cleanup tests

---------

Co-authored-by: 1xstj <[email protected]>

pallets/multi-asset-delegation/src/functions/delegate.rs

@@ -16,7 +16,7 @@
 use super::*;
 use crate::{types::*, Pallet};
 use frame_support::{ensure, pallet_prelude::DispatchResult, traits::Get};
-use sp_runtime::traits::Zero;
+use sp_runtime::traits::{CheckedSub, Zero};
 use sp_std::vec::Vec;
 
 impl<T: Config> Pallet<T> {
@@ -49,7 +49,7 @@ impl<T: Config> Pallet<T> {
 			ensure!(*balance >= amount, Error::<T>::InsufficientBalance);
 
 			// Reduce the balance in deposits
-			*balance -= amount;
+			*balance = balance.checked_sub(&amount).ok_or(Error::<T>::InsufficientBalance)?;
 			if *balance == Zero::zero() {
 				metadata.deposits.remove(&asset_id);
 			}

pallets/multi-asset-delegation/src/functions/deposit.rs

@@ -15,15 +15,12 @@
 // along with Tangle.  If not, see <http://www.gnu.org/licenses/>.
 use super::*;
 use crate::{types::*, Pallet};
-use frame_support::{ensure, pallet_prelude::DispatchResult};
-
 use frame_support::traits::fungibles::Mutate;
-
+use frame_support::{ensure, pallet_prelude::DispatchResult};
 use frame_support::{
-	sp_runtime::traits::AccountIdConversion,
+	sp_runtime::traits::{AccountIdConversion, CheckedAdd, Zero},
 	traits::{tokens::Preservation, Get},
 };
-use sp_runtime::traits::Zero;
 
 impl<T: Config> Pallet<T> {
 	/// Returns the account ID of the pallet.
@@ -57,13 +54,21 @@ impl<T: Config> Pallet<T> {
 			&Self::pallet_account(),
 			amount,
 			Preservation::Expendable,
-		)?; // Transfer the assets to the pallet account
+		)?;
 
 		// Update storage
-		Delegators::<T>::mutate(&who, |maybe_metadata| {
+		Delegators::<T>::try_mutate(&who, |maybe_metadata| -> DispatchResult {
 			let metadata = maybe_metadata.get_or_insert_with(Default::default);
-			metadata.deposits.entry(asset_id).and_modify(|e| *e += amount).or_insert(amount);
-		});
+			// Handle checked addition first to avoid ? operator in closure
+			if let Some(existing) = metadata.deposits.get(&asset_id) {
+				let new_amount =
+					existing.checked_add(&amount).ok_or(Error::<T>::DepositOverflow)?;
+				metadata.deposits.insert(asset_id, new_amount);
+			} else {
+				metadata.deposits.insert(asset_id, amount);
+			}
+			Ok(())
+		})?;
 
 		Ok(())
 	}

pallets/multi-asset-delegation/src/functions/operator.rs

@@ -22,6 +22,7 @@ use frame_support::{
 	pallet_prelude::DispatchResult,
 	traits::{Get, ReservableCurrency},
 };
+use sp_runtime::traits::{CheckedAdd, CheckedSub};
 use sp_runtime::DispatchError;
 use tangle_primitives::ServiceManager;
 
@@ -125,7 +126,7 @@ impl<T: Config> Pallet<T> {
 
 		match operator.status {
 			OperatorStatus::Leaving(leaving_round) => {
-				ensure!(current_round >= leaving_round, Error::<T>::NotLeavingRound);
+				ensure!(current_round >= leaving_round, Error::<T>::LeavingRoundNotReached);
 			},
 			_ => return Err(Error::<T>::NotLeavingOperator.into()),
 		};
@@ -136,7 +137,8 @@ impl<T: Config> Pallet<T> {
 		Ok(())
 	}
 
-	/// Processes an additional stake for an operator.
+	/// Processes an additional TNT stake for an operator, called
+	/// by themselves.
 	///
 	/// # Arguments
 	///
@@ -151,15 +153,21 @@ impl<T: Config> Pallet<T> {
 		additional_bond: BalanceOf<T>,
 	) -> Result<(), DispatchError> {
 		let mut operator = Operators::<T>::get(who).ok_or(Error::<T>::NotAnOperator)?;
+
+		// Check for potential overflow before reserving funds
+		operator.stake =
+			operator.stake.checked_add(&additional_bond).ok_or(Error::<T>::StakeOverflow)?;
+
+		// Only reserve funds if the addition would be safe
 		T::Currency::reserve(who, additional_bond)?;
 
-		operator.stake += additional_bond;
 		Operators::<T>::insert(who, operator);
 
 		Ok(())
 	}
 
-	/// Schedules a stake reduction for an operator.
+	/// Schedules a native TNT stake reduction for an operator, called
+	/// by themselves.
 	///
 	/// # Arguments
 	///
@@ -176,6 +184,22 @@ impl<T: Config> Pallet<T> {
 		let mut operator = Operators::<T>::get(who).ok_or(Error::<T>::NotAnOperator)?;
 		ensure!(T::ServiceManager::can_exit(who), Error::<T>::CannotExit);
 
+		// Ensure there's no existing unstake request
+		ensure!(operator.request.is_none(), Error::<T>::PendingUnstakeRequestExists);
+
+		// Ensure the unstake amount doesn't exceed current stake
+		ensure!(unstake_amount <= operator.stake, Error::<T>::UnstakeAmountTooLarge);
+
+		// Ensure operator maintains minimum required stake after unstaking
+		let remaining_stake = operator
+			.stake
+			.checked_sub(&unstake_amount)
+			.ok_or(Error::<T>::UnstakeAmountTooLarge)?;
+		ensure!(
+			remaining_stake >= T::MinOperatorBondAmount::get(),
+			Error::<T>::InsufficientStakeRemaining
+		);
+
 		operator.request = Some(OperatorBondLessRequest {
 			amount: unstake_amount,
 			request_time: Self::current_round(),
@@ -205,7 +229,11 @@ impl<T: Config> Pallet<T> {
 			Error::<T>::BondLessRequestNotSatisfied
 		);
 
-		operator.stake -= request.amount;
+		operator.stake = operator
+			.stake
+			.checked_sub(&request.amount)
+			.ok_or(Error::<T>::UnstakeAmountTooLarge)?;
+
 		operator.request = None;
 		Operators::<T>::insert(who, operator);
 

pallets/multi-asset-delegation/src/lib.rs

@@ -77,14 +77,15 @@ pub use functions::*;
 #[frame_support::pallet]
 pub mod pallet {
 	use crate::types::*;
+	use frame_support::traits::fungibles::Inspect;
 	use frame_support::{
 		dispatch::DispatchResult,
 		pallet_prelude::*,
 		traits::{fungibles, Currency, LockableCurrency, ReservableCurrency},
 		PalletId,
 	};
 	use frame_system::pallet_prelude::*;
-	use sp_runtime::traits::{AtLeast32BitUnsigned, MaybeSerializeDeserialize};
+	use sp_runtime::traits::{AtLeast32BitUnsigned, MaybeSerializeDeserialize, Zero};
 	use sp_std::{collections::btree_map::BTreeMap, vec::Vec};
 	use tangle_primitives::{traits::ServiceManager, RoundIndex};
 
@@ -298,6 +299,8 @@ pub mod pallet {
 		NotLeavingOperator,
 		/// The round does not match the scheduled leave round.
 		NotLeavingRound,
+		/// Leaving round not reached
+		LeavingRoundNotReached,
 		/// There is no scheduled unstake request.
 		NoScheduledBondLess,
 		/// The unstake request is not satisfied.
@@ -344,6 +347,22 @@ pub mod pallet {
 		AssetNotInVault,
 		/// The reward vault does not exist
 		VaultNotFound,
+		/// Deposit amount overflow
+		DepositOverflow,
+		/// Unstake underflow
+		UnstakeAmountTooLarge,
+		/// Overflow while adding stake
+		StakeOverflow,
+		/// Underflow while reducing stake
+		InsufficientStakeRemaining,
+		/// APY exceeds maximum allowed by the extrinsic
+		APYExceedsMaximum,
+		/// Cap cannot be zero
+		CapCannotBeZero,
+		/// Cap exceeds total supply of asset
+		CapExceedsTotalSupply,
+		/// An unstake request is already pending
+		PendingUnstakeRequestExists,
 	}
 
 	/// Hooks for the pallet.
@@ -577,6 +596,11 @@ pub mod pallet {
 		}
 
 		/// Sets the APY and cap for a specific asset.
+		/// The APY is the annual percentage yield that the asset will earn.
+		/// The cap is the amount of assets required to be deposited to distribute the entire APY.
+		/// The APY is capped at 10% and will require runtime upgrade to change.
+		///
+		/// While the cap is not met, the APY distributed will be `amount_deposited / cap * APY`.
 		#[pallet::call_index(19)]
 		#[pallet::weight(Weight::from_parts(10_000, 0) + T::DbWeight::get().writes(1))]
 		pub fn set_incentive_apy_and_cap(
@@ -588,6 +612,21 @@ pub mod pallet {
 			// Ensure that the origin is authorized
 			T::ForceOrigin::ensure_origin(origin)?;
 
+			// Validate APY is not greater than 10%
+			ensure!(apy <= sp_runtime::Percent::from_percent(10), Error::<T>::APYExceedsMaximum);
+
+			// Validate cap is not zero
+			ensure!(!cap.is_zero(), Error::<T>::CapCannotBeZero);
+
+			// Validate the cap is not greater than the total supply
+			let asset_ids = RewardVaults::<T>::get(vault_id).ok_or(Error::<T>::VaultNotFound)?;
+			for asset_id in asset_ids.iter() {
+				ensure!(
+					T::Fungibles::total_issuance(*asset_id) >= cap,
+					Error::<T>::CapExceedsTotalSupply
+				);
+			}
+
 			// Initialize the reward config if not already initialized
 			RewardConfigStorage::<T>::mutate(|maybe_config| {
 				let mut config = maybe_config.take().unwrap_or_else(|| RewardConfig {

pallets/multi-asset-delegation/src/tests/operator.rs

@@ -225,7 +225,7 @@ fn operator_bond_more_insufficient_balance() {
 #[test]
 fn schedule_operator_unstake_success() {
 	new_test_ext().execute_with(|| {
-		let bond_amount = 10_000;
+		let bond_amount = 20_000; // Increased initial bond
 		let unstake_amount = 5_000;
 
 		// Join operator first
@@ -241,13 +241,37 @@ fn schedule_operator_unstake_success() {
 		let operator_info = MultiAssetDelegation::operator_info(1).unwrap();
 		assert_eq!(operator_info.request.unwrap().amount, unstake_amount);
 
+		// Verify remaining stake is above minimum
+		assert!(operator_info.stake.saturating_sub(unstake_amount) >= MinOperatorBondAmount::get());
+
 		// Verify event
 		System::assert_has_event(RuntimeEvent::MultiAssetDelegation(
 			Event::OperatorBondLessScheduled { who: 1, unstake_amount },
 		));
 	});
 }
 
+// Add test for minimum stake requirement
+#[test]
+fn schedule_operator_unstake_respects_minimum_stake() {
+	new_test_ext().execute_with(|| {
+		let bond_amount = 20_000;
+		let unstake_amount = 15_000; // Would leave less than minimum required
+
+		// Join operator first
+		assert_ok!(MultiAssetDelegation::join_operators(RuntimeOrigin::signed(1), bond_amount));
+
+		// Attempt to schedule unstake that would leave less than minimum
+		assert_noop!(
+			MultiAssetDelegation::schedule_operator_unstake(
+				RuntimeOrigin::signed(1),
+				unstake_amount
+			),
+			Error::<Test>::InsufficientStakeRemaining
+		);
+	});
+}
+
 #[test]
 fn schedule_operator_unstake_not_an_operator() {
 	new_test_ext().execute_with(|| {
@@ -292,7 +316,7 @@ fn schedule_operator_unstake_not_an_operator() {
 #[test]
 fn execute_operator_unstake_success() {
 	new_test_ext().execute_with(|| {
-		let bond_amount = 10_000;
+		let bond_amount = 20_000;
 		let unstake_amount = 5_000;
 
 		// Join operator first
@@ -352,7 +376,7 @@ fn execute_operator_unstake_no_scheduled_unstake() {
 #[test]
 fn execute_operator_unstake_request_not_satisfied() {
 	new_test_ext().execute_with(|| {
-		let bond_amount = 10_000;
+		let bond_amount = 20_000;
 		let unstake_amount = 5_000;
 
 		// Join operator first
@@ -375,7 +399,7 @@ fn execute_operator_unstake_request_not_satisfied() {
 #[test]
 fn cancel_operator_unstake_success() {
 	new_test_ext().execute_with(|| {
-		let bond_amount = 10_000;
+		let bond_amount = 20_000;
 		let unstake_amount = 5_000;
 
 		// Join operator first

precompiles/multi-asset-delegation/src/tests.rs

@@ -159,7 +159,7 @@ fn test_delegate_assets_insufficient_balance() {
 					amount: U256::from(300),
 				},
 			)
-			.execute_reverts(|output| output == b"Dispatched call failed with error: Module(ModuleError { index: 5, error: [14, 0, 0, 0], message: Some(\"InsufficientBalance\") })");
+			.execute_reverts(|output| output == b"Dispatched call failed with error: Module(ModuleError { index: 5, error: [15, 0, 0, 0], message: Some(\"InsufficientBalance\") })");
 
 		assert_eq!(Balances::free_balance(delegator_account), 500);
 	});