From 4ad51494278c573cd787f6618a1d44e524726f28 Mon Sep 17 00:00:00 2001 From: drewstone Date: Wed, 20 Nov 2024 07:51:08 -0800 Subject: [PATCH] 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 <106580853+1xstj@users.noreply.github.com> --- .../src/functions/delegate.rs | 4 +- .../src/functions/deposit.rs | 23 +++++++---- .../src/functions/operator.rs | 38 ++++++++++++++--- pallets/multi-asset-delegation/src/lib.rs | 41 ++++++++++++++++++- .../src/tests/operator.rs | 32 +++++++++++++-- .../multi-asset-delegation/src/tests.rs | 2 +- 6 files changed, 118 insertions(+), 22 deletions(-) diff --git a/pallets/multi-asset-delegation/src/functions/delegate.rs b/pallets/multi-asset-delegation/src/functions/delegate.rs index ea09f9eb..41abfc20 100644 --- a/pallets/multi-asset-delegation/src/functions/delegate.rs +++ b/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 Pallet { @@ -49,7 +49,7 @@ impl Pallet { ensure!(*balance >= amount, Error::::InsufficientBalance); // Reduce the balance in deposits - *balance -= amount; + *balance = balance.checked_sub(&amount).ok_or(Error::::InsufficientBalance)?; if *balance == Zero::zero() { metadata.deposits.remove(&asset_id); } diff --git a/pallets/multi-asset-delegation/src/functions/deposit.rs b/pallets/multi-asset-delegation/src/functions/deposit.rs index 8dc0b534..be0cf2fc 100644 --- a/pallets/multi-asset-delegation/src/functions/deposit.rs +++ b/pallets/multi-asset-delegation/src/functions/deposit.rs @@ -15,15 +15,12 @@ // along with Tangle. If not, see . 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 Pallet { /// Returns the account ID of the pallet. @@ -57,13 +54,21 @@ impl Pallet { &Self::pallet_account(), amount, Preservation::Expendable, - )?; // Transfer the assets to the pallet account + )?; // Update storage - Delegators::::mutate(&who, |maybe_metadata| { + Delegators::::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::::DepositOverflow)?; + metadata.deposits.insert(asset_id, new_amount); + } else { + metadata.deposits.insert(asset_id, amount); + } + Ok(()) + })?; Ok(()) } diff --git a/pallets/multi-asset-delegation/src/functions/operator.rs b/pallets/multi-asset-delegation/src/functions/operator.rs index 7b2676ee..296351ba 100644 --- a/pallets/multi-asset-delegation/src/functions/operator.rs +++ b/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 Pallet { match operator.status { OperatorStatus::Leaving(leaving_round) => { - ensure!(current_round >= leaving_round, Error::::NotLeavingRound); + ensure!(current_round >= leaving_round, Error::::LeavingRoundNotReached); }, _ => return Err(Error::::NotLeavingOperator.into()), }; @@ -136,7 +137,8 @@ impl Pallet { 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 Pallet { additional_bond: BalanceOf, ) -> Result<(), DispatchError> { let mut operator = Operators::::get(who).ok_or(Error::::NotAnOperator)?; + + // Check for potential overflow before reserving funds + operator.stake = + operator.stake.checked_add(&additional_bond).ok_or(Error::::StakeOverflow)?; + + // Only reserve funds if the addition would be safe T::Currency::reserve(who, additional_bond)?; - operator.stake += additional_bond; Operators::::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 Pallet { let mut operator = Operators::::get(who).ok_or(Error::::NotAnOperator)?; ensure!(T::ServiceManager::can_exit(who), Error::::CannotExit); + // Ensure there's no existing unstake request + ensure!(operator.request.is_none(), Error::::PendingUnstakeRequestExists); + + // Ensure the unstake amount doesn't exceed current stake + ensure!(unstake_amount <= operator.stake, Error::::UnstakeAmountTooLarge); + + // Ensure operator maintains minimum required stake after unstaking + let remaining_stake = operator + .stake + .checked_sub(&unstake_amount) + .ok_or(Error::::UnstakeAmountTooLarge)?; + ensure!( + remaining_stake >= T::MinOperatorBondAmount::get(), + Error::::InsufficientStakeRemaining + ); + operator.request = Some(OperatorBondLessRequest { amount: unstake_amount, request_time: Self::current_round(), @@ -205,7 +229,11 @@ impl Pallet { Error::::BondLessRequestNotSatisfied ); - operator.stake -= request.amount; + operator.stake = operator + .stake + .checked_sub(&request.amount) + .ok_or(Error::::UnstakeAmountTooLarge)?; + operator.request = None; Operators::::insert(who, operator); diff --git a/pallets/multi-asset-delegation/src/lib.rs b/pallets/multi-asset-delegation/src/lib.rs index 84910fc5..4674aad8 100644 --- a/pallets/multi-asset-delegation/src/lib.rs +++ b/pallets/multi-asset-delegation/src/lib.rs @@ -77,6 +77,7 @@ 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::*, @@ -84,7 +85,7 @@ pub mod pallet { 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::::APYExceedsMaximum); + + // Validate cap is not zero + ensure!(!cap.is_zero(), Error::::CapCannotBeZero); + + // Validate the cap is not greater than the total supply + let asset_ids = RewardVaults::::get(vault_id).ok_or(Error::::VaultNotFound)?; + for asset_id in asset_ids.iter() { + ensure!( + T::Fungibles::total_issuance(*asset_id) >= cap, + Error::::CapExceedsTotalSupply + ); + } + // Initialize the reward config if not already initialized RewardConfigStorage::::mutate(|maybe_config| { let mut config = maybe_config.take().unwrap_or_else(|| RewardConfig { diff --git a/pallets/multi-asset-delegation/src/tests/operator.rs b/pallets/multi-asset-delegation/src/tests/operator.rs index 2108b3e7..f8cb66f6 100644 --- a/pallets/multi-asset-delegation/src/tests/operator.rs +++ b/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,6 +241,9 @@ 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 }, @@ -248,6 +251,27 @@ fn schedule_operator_unstake_success() { }); } +// 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::::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 diff --git a/precompiles/multi-asset-delegation/src/tests.rs b/precompiles/multi-asset-delegation/src/tests.rs index b0a9bf27..be8f0c20 100644 --- a/precompiles/multi-asset-delegation/src/tests.rs +++ b/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); });