pool: chore: move max positions check logic to once per request process

pool/src/pool/actions.rs

@@ -71,6 +71,7 @@ pub fn build_actions_from_request(
 ) -> (Actions, User, bool) {
     let mut actions = Actions::new(e);
     let mut from_state = User::load(e, from);
+    let prev_positions_count = from_state.positions.effective_count();
     let mut check_health = false;
     for request in requests.iter() {
         // verify the request is allowed
@@ -280,9 +281,11 @@ pub fn build_actions_from_request(
             }
             _ => panic_with_error!(e, PoolError::BadRequest),
         }
-        // Verify max positions haven't been exceeded
-        pool.require_under_max(e, from_state.positions.clone(), request.request_type)
     }
+
+    // Verify max positions haven't been exceeded
+    pool.require_under_max(e, &from_state.positions, prev_positions_count);
+
     (actions, from_state, check_health)
 }
 
@@ -1133,7 +1136,7 @@ mod tests {
             oracle: oracle_address,
             bstop_rate: 0_100_000_000,
             status: 0,
-            max_positions: 2,
+            max_positions: 4,
         };
         let positions: Positions = Positions {
             collateral: map![
@@ -1515,6 +1518,69 @@ mod tests {
             assert_eq!(actions.spender_transfer.len(), 0);
         });
     }
+
+    /********** positions_under_max **********/
+
+    #[test]
+    fn test_actions_requires_positions_under_max_with_decrease() {
+        let e = Env::default();
+        e.mock_all_auths();
+
+        let bombadil = Address::generate(&e);
+        let samwise = Address::generate(&e);
+        let pool = testutils::create_pool(&e);
+
+        let (underlying, _) = testutils::create_token_contract(&e, &bombadil);
+        let (reserve_config, reserve_data) = testutils::default_reserve_meta();
+        testutils::create_reserve(&e, &pool, &underlying, &reserve_config, &reserve_data);
+
+        let (underlying_1, _) = testutils::create_token_contract(&e, &bombadil);
+        let (reserve_config, reserve_data) = testutils::default_reserve_meta();
+        testutils::create_reserve(&e, &pool, &underlying_1, &reserve_config, &reserve_data);
+
+        e.ledger().set(LedgerInfo {
+            timestamp: 600,
+            protocol_version: 20,
+            sequence_number: 1234,
+            network_id: Default::default(),
+            base_reserve: 10,
+            min_temp_entry_ttl: 10,
+            min_persistent_entry_ttl: 10,
+            max_entry_ttl: 2000000,
+        });
+
+        let pool_config = PoolConfig {
+            oracle: Address::generate(&e),
+            bstop_rate: 0_200_000_000,
+            status: 0,
+            max_positions: 2,
+        };
+
+        let user_positions = Positions {
+            liabilities: map![&e, (0, 5_0000000), (1, 1_0000000)],
+            collateral: map![&e, (0, 20_0000000), (1, 10)],
+            supply: map![&e],
+        };
+        e.as_contract(&pool, || {
+            storage::set_pool_config(&e, &pool_config);
+            storage::set_user_positions(&e, &samwise, &user_positions);
+
+            let mut pool = Pool::load(&e);
+
+            let requests = vec![
+                &e,
+                Request {
+                    request_type: 3,
+                    address: underlying_1.clone(),
+                    amount: 20,
+                },
+            ];
+
+            let (_, user, _) = build_actions_from_request(&e, &mut pool, &samwise, requests);
+            assert_eq!(user.positions.effective_count(), 3)
+        });
+    }
+
     #[test]
     #[should_panic(expected = "Error(Contract, #13)")]
     fn test_actions_requires_positions_under_max() {
@@ -1528,6 +1594,7 @@ mod tests {
         let (underlying, _) = testutils::create_token_contract(&e, &bombadil);
         let (reserve_config, reserve_data) = testutils::default_reserve_meta();
         testutils::create_reserve(&e, &pool, &underlying, &reserve_config, &reserve_data);
+
         e.ledger().set(LedgerInfo {
             timestamp: 600,
             protocol_version: 20,
@@ -1538,14 +1605,22 @@ mod tests {
             min_persistent_entry_ttl: 10,
             max_entry_ttl: 2000000,
         });
+
         let pool_config = PoolConfig {
             oracle: Address::generate(&e),
             bstop_rate: 0_200_000_000,
             status: 0,
-            max_positions: 0,
+            max_positions: 1,
+        };
+
+        let user_positions = Positions {
+            liabilities: map![&e],
+            collateral: map![&e, (0, 20_0000000)],
+            supply: map![&e],
         };
         e.as_contract(&pool, || {
             storage::set_pool_config(&e, &pool_config);
+            storage::set_user_positions(&e, &samwise, &user_positions);
 
             let mut pool = Pool::load(&e);
 
@@ -1554,9 +1629,10 @@ mod tests {
                 Request {
                     request_type: 4,
                     address: underlying.clone(),
-                    amount: 10_1234567,
+                    amount: 1_0000000,
                 },
             ];
+
             build_actions_from_request(&e, &mut pool, &samwise, requests);
         });
     }

pool/src/pool/pool.rs

@@ -76,16 +76,18 @@ impl Pool {
         }
     }
 
-    /// Require that an action does not violate the maximum number of positions, or panic.
+    /// Require that a position does not violate the maximum number of positions, or panic.
     ///
     /// ### Arguments
     /// * `positions` - The user's positions
-    /// * `action_type` - The type of action being performed
-    pub fn require_under_max(&self, e: &Env, positions: Positions, action_type: u32) {
-        if self.config.max_positions
-            < positions.liabilities.len() + positions.collateral.len() as u32
-            && (action_type == 2 || action_type == 4)
-        {
+    /// * `previous_num` - The number of positions the user previously had
+    ///
+    /// ### Panics
+    /// If the user has more positions than the maximum allowed and they are not
+    /// decreasing their number of positions
+    pub fn require_under_max(&self, e: &Env, positions: &Positions, previous_num: u32) {
+        let new_num = positions.effective_count();
+        if new_num > previous_num && self.config.max_positions < new_num {
             panic_with_error!(e, PoolError::MaxPositionsExceeded)
         }
     }
@@ -541,8 +543,9 @@ mod tests {
             assert!(false);
         });
     }
+
     #[test]
-    fn test_require_under_max_passes() {
+    fn test_require_under_max_empty() {
         let e = Env::default();
         let samwise = Address::generate(&e);
         let pool = testutils::create_pool(&e);
@@ -560,105 +563,97 @@ mod tests {
             max_positions: 2,
         };
         e.as_contract(&pool, || {
-            user.add_collateral(&e, &mut reserve_0, 123);
             storage::set_pool_config(&e, &pool_config);
+            let prev_positions = user.positions.effective_count();
+
             let pool = Pool::load(&e);
+            user.add_collateral(&e, &mut reserve_0, 1);
 
-            pool.require_under_max(&e, user.positions, 4);
+            pool.require_under_max(&e, &user.positions, prev_positions);
         });
     }
+
     #[test]
-    #[should_panic(expected = "Error(Contract, #13)")]
-    fn test_require_under_max_fails_borrow() {
+    fn test_require_under_max_ignores_supply() {
         let e = Env::default();
         let samwise = Address::generate(&e);
         let pool = testutils::create_pool(&e);
 
         let mut reserve_0 = testutils::default_reserve(&e);
+        let mut reserve_1 = testutils::default_reserve(&e);
+        reserve_1.index = 1;
 
+        let (oracle, _) = testutils::create_mock_oracle(&e);
         let mut user = User {
             address: samwise.clone(),
             positions: Positions::env_default(&e),
         };
-        let (oracle, _) = testutils::create_mock_oracle(&e);
         let pool_config = PoolConfig {
             oracle,
             bstop_rate: 0_200_000_000,
             status: 0,
-            max_positions: 1,
+            max_positions: 2,
         };
         e.as_contract(&pool, || {
             storage::set_pool_config(&e, &pool_config);
-            user.add_collateral(&e, &mut reserve_0, 123);
-            user.add_liabilities(&e, &mut reserve_0, 789);
+            user.add_supply(&e, &mut reserve_0, 42);
+            user.add_supply(&e, &mut reserve_1, 42);
+            user.add_collateral(&e, &mut reserve_1, 1);
+            let prev_positions = user.positions.effective_count();
+
             let pool = Pool::load(&e);
+            user.add_liabilities(&e, &mut reserve_1, 2);
 
-            pool.require_under_max(&e, user.positions, 4);
+            pool.require_under_max(&e, &user.positions, prev_positions);
         });
     }
-    #[test]
-    #[should_panic(expected = "Error(Contract, #13)")]
-    fn test_require_under_max_fails_deposit() {
-        let e = Env::default();
-        let samwise = Address::generate(&e);
-        let pool = testutils::create_pool(&e);
 
-        let mut reserve_0 = testutils::default_reserve(&e);
-
-        let mut user = User {
-            address: samwise.clone(),
-            positions: Positions::env_default(&e),
-        };
-        let (oracle, _) = testutils::create_mock_oracle(&e);
-        let pool_config = PoolConfig {
-            oracle,
-            bstop_rate: 0_200_000_000,
-            status: 0,
-            max_positions: 1,
-        };
-        e.as_contract(&pool, || {
-            storage::set_pool_config(&e, &pool_config);
-            let pool = Pool::load(&e);
-            user.add_collateral(&e, &mut reserve_0, 123);
-            user.add_liabilities(&e, &mut reserve_0, 789);
-            pool.require_under_max(&e, user.positions, 2);
-        });
-    }
     #[test]
-    fn test_require_under_max_passes_withdraw() {
+    fn test_require_under_max_allows_decreasing_change() {
         let e = Env::default();
         let samwise = Address::generate(&e);
         let pool = testutils::create_pool(&e);
 
         let mut reserve_0 = testutils::default_reserve(&e);
+        let mut reserve_1 = testutils::default_reserve(&e);
+        reserve_1.index = 1;
 
+        let (oracle, _) = testutils::create_mock_oracle(&e);
         let mut user = User {
             address: samwise.clone(),
             positions: Positions::env_default(&e),
         };
-        let (oracle, _) = testutils::create_mock_oracle(&e);
         let pool_config = PoolConfig {
             oracle,
             bstop_rate: 0_200_000_000,
             status: 0,
-            max_positions: 1,
+            max_positions: 2,
         };
         e.as_contract(&pool, || {
             storage::set_pool_config(&e, &pool_config);
+            user.add_collateral(&e, &mut reserve_0, 42);
+            user.add_collateral(&e, &mut reserve_1, 42);
+            user.add_liabilities(&e, &mut reserve_0, 123);
+            user.add_liabilities(&e, &mut reserve_1, 123);
+            let prev_positions = user.positions.effective_count();
+
             let pool = Pool::load(&e);
-            user.add_collateral(&e, &mut reserve_0, 123);
-            user.add_liabilities(&e, &mut reserve_0, 789);
+            user.remove_collateral(&e, &mut reserve_1, 42);
 
-            pool.require_under_max(&e, user.positions, 3);
+            pool.require_under_max(&e, &user.positions, prev_positions);
         });
     }
+
     #[test]
-    fn test_require_under_max_passes_repay() {
+    #[should_panic(expected = "Error(Contract, #13)")]
+    fn test_require_under_max_panics_if_over() {
         let e = Env::default();
         let samwise = Address::generate(&e);
         let pool = testutils::create_pool(&e);
 
         let mut reserve_0 = testutils::default_reserve(&e);
+        let mut reserve_1 = testutils::default_reserve(&e);
+        reserve_1.index = 1;
 
         let mut user = User {
             address: samwise.clone(),
@@ -669,14 +664,18 @@ mod tests {
             oracle,
             bstop_rate: 0_200_000_000,
             status: 0,
-            max_positions: 1,
+            max_positions: 2,
         };
         e.as_contract(&pool, || {
             storage::set_pool_config(&e, &pool_config);
-            let pool = Pool::load(&e);
             user.add_collateral(&e, &mut reserve_0, 123);
             user.add_liabilities(&e, &mut reserve_0, 789);
-            pool.require_under_max(&e, user.positions, 5);
+            let prev_positions = user.positions.effective_count();
+
+            let pool = Pool::load(&e);
+            user.add_liabilities(&e, &mut reserve_1, 42);
+
+            pool.require_under_max(&e, &user.positions, prev_positions);
         });
     }
 }

pool/src/pool/submit.rs

@@ -161,7 +161,7 @@ mod tests {
             assert_eq!(underlying_1_client.balance(&merry), 1_5000000);
         });
     }
-  
+
     #[test]
     #[should_panic(expected = "Error(Contract, #10)")]
     fn test_submit_requires_healhty() {

pool/src/pool/user.rs

@@ -22,6 +22,14 @@ impl Positions {
             supply: Map::new(e),
         }
     }
+
+    /// Get the number of effective (impacts health factor) posiitons the user holds.
+    ///
+    /// This function ignores non-collateralized supply positions, as they are not relevant to the
+    /// max number of allowed positions by the pool.
+    pub fn effective_count(&self) -> u32 {
+        self.liabilities.len() + self.collateral.len()
+    }
 }
 
 /// A user / contracts position's with the pool