Repository: interlay/interbtc (language: rust)

Issue:
Prevent activating an already activated loans market
The [activate_market](https://github.com/interlay/interbtc/blob/f0a0144a0cd7e512bf9b2c55e57b12e4b3b5e9c4/crates/loans/src/lib.rs#L685) extrinsic should fail if the market is already active, so as not to re-emit the `ActivatedMarket` event.



Discussion / hints:
## Solutions

**Solution 1: Add an `is_active` flag when updating `mutate_market`**

Pros:
- Allows updating the `mutate_market` function to include an `is_active` flag.
- Only emits an event if the flag is not set, reducing unnecessary event emissions.

Cons:
- If additional conditions need to be checked in the future, adding more flags can cause issues and make the code more complex.

**Solution 2: Add the `check_market_state_before_activating` function**

Pros:
- Introduces a separate function, `check_market_state_before_activating`, to ensure all necessary checks are performed before updating the market.

Cons: 
- Time complexity increases


**Solution 3: Update `mutate_market` to accept result**

Pros:
- Time complexity remains the same as that of the existing solution. 







Fix patch:
diff --git a/crates/loans/src/lib.rs b/crates/loans/src/lib.rs
index e5099356c9..f4e805b67f 100644
--- a/crates/loans/src/lib.rs
+++ b/crates/loans/src/lib.rs
@@ -281,6 +281,8 @@ pub mod pallet {
         InvalidLendTokenId,
         /// Market does not exist
         MarketDoesNotExist,
+        /// Market already activated
+        MarketAlreadyActivated,
         /// Market already exists
         MarketAlreadyExists,
         /// New markets must have a pending state
@@ -684,14 +686,13 @@ pub mod pallet {
         #[transactional]
         pub fn activate_market(origin: OriginFor<T>, asset_id: CurrencyId<T>) -> DispatchResultWithPostInfo {
             T::UpdateOrigin::ensure_origin(origin)?;
-            // TODO: if the market is already active throw an error,
-            // to avoid emitting the `ActivatedMarket` event again.
             Self::mutate_market(asset_id, |stored_market| {
+                // Ensure that market is not already active
                 if let MarketState::Active = stored_market.state {
-                    return stored_market.clone();
+                    return Err(Error::<T>::MarketAlreadyActivated.into());
                 }
                 stored_market.state = MarketState::Active;
-                stored_market.clone()
+                Ok(stored_market.clone())
             })?;
             Self::deposit_event(Event::<T>::ActivatedMarket {
                 underlying_currency_id: asset_id,
@@ -716,7 +717,7 @@ pub mod pallet {
             ensure!(rate_model.check_model(), Error::<T>::InvalidRateModelParam);
             let market = Self::mutate_market(asset_id, |stored_market| {
                 stored_market.rate_model = rate_model;
-                stored_market.clone()
+                Ok(stored_market.clone())
             })?;
             Self::deposit_event(Event::<T>::UpdatedMarket {
                 underlying_currency_id: asset_id,
@@ -798,7 +799,7 @@ pub mod pallet {
                     supply_cap,
                     borrow_cap,
                 };
-                stored_market.clone()
+                Ok(stored_market.clone())
             })?;
             Self::deposit_event(Event::<T>::UpdatedMarket {
                 underlying_currency_id: asset_id,
@@ -832,7 +833,7 @@ pub mod pallet {
             UnderlyingAssetId::<T>::insert(market.lend_token_id, asset_id);
             let updated_market = Self::mutate_market(asset_id, |stored_market| {
                 *stored_market = market;
-                stored_market.clone()
+                Ok(stored_market.clone())
             })?;
 
             Self::deposit_event(Event::<T>::UpdatedMarket {
@@ -1831,11 +1832,11 @@ impl<T: Config> Pallet<T> {
     // Returns `Err` if market does not exist.
     pub(crate) fn mutate_market<F>(asset_id: CurrencyId<T>, cb: F) -> Result<Market<BalanceOf<T>>, DispatchError>
     where
-        F: FnOnce(&mut Market<BalanceOf<T>>) -> Market<BalanceOf<T>>,
+        F: FnOnce(&mut Market<BalanceOf<T>>) -> Result<Market<BalanceOf<T>>, DispatchError>,
     {
         Markets::<T>::try_mutate(asset_id, |opt| -> Result<Market<BalanceOf<T>>, DispatchError> {
             if let Some(market) = opt {
-                return Ok(cb(market));
+                return cb(market);
             }
             Err(Error::<T>::MarketDoesNotExist.into())
         })


Test patch:
diff --git a/crates/loans/src/tests.rs b/crates/loans/src/tests.rs
index 843c150720..30c0a9495b 100644
--- a/crates/loans/src/tests.rs
+++ b/crates/loans/src/tests.rs
@@ -1577,3 +1577,29 @@ fn redeem_amount_matches_freed_underlying() {
         );
     })
 }
+
+#[test]
+fn fail_to_activate_already_activated_market() {
+    new_test_ext().execute_with(|| {
+        assert_noop!(
+            Loans::activate_market(RuntimeOrigin::root(), Token(DOT_CURRENCY)),
+            Error::<Test>::MarketAlreadyActivated
+        );
+        assert_noop!(
+            Loans::activate_market(RuntimeOrigin::root(), Token(KINT_CURRENCY)),
+            Error::<Test>::MarketAlreadyActivated
+        );
+        assert_noop!(
+            Loans::activate_market(RuntimeOrigin::root(), Token(KSM_CURRENCY)),
+            Error::<Test>::MarketAlreadyActivated
+        );
+        assert_noop!(
+            Loans::activate_market(RuntimeOrigin::root(), Token(KBTC_CURRENCY)),
+            Error::<Test>::MarketAlreadyActivated
+        );
+        assert_noop!(
+            Loans::activate_market(RuntimeOrigin::root(), Token(IBTC_CURRENCY)),
+            Error::<Test>::MarketAlreadyActivated
+        );
+    })
+}
diff --git a/crates/loans/src/tests/liquidate_borrow.rs b/crates/loans/src/tests/liquidate_borrow.rs
index 0ea351c889..1c01d2ebc0 100644
--- a/crates/loans/src/tests/liquidate_borrow.rs
+++ b/crates/loans/src/tests/liquidate_borrow.rs
@@ -77,7 +77,7 @@ fn collateral_value_must_be_greater_than_liquidation_value() {
         CurrencyConvert::convert.mock_safe(with_price(Some((KSM, Rate::from_float(2000.0)))));
         Loans::mutate_market(KSM, |market| {
             market.liquidate_incentive = Rate::from_float(200.0);
-            market.clone()
+            Ok(market.clone())
         })
         .unwrap();
         assert_noop!(
@@ -265,7 +265,7 @@ fn liquidator_cannot_take_inactive_market_currency() {
         CurrencyConvert::convert.mock_safe(with_price(Some((KSM, 2.into()))));
         assert_ok!(Loans::mutate_market(DOT, |stored_market| {
             stored_market.state = MarketState::Supervision;
-            stored_market.clone()
+            Ok(stored_market.clone())
         }));
         assert_noop!(
             Loans::liquidate_borrow(RuntimeOrigin::signed(BOB), ALICE, KSM, unit(50), DOT),