fix: guard factory pool count overflow

.github/workflows/ci.yml

@@ -32,6 +32,17 @@ jobs:
           restore-keys: |
             ${{ runner.os }}-cargo-
 
+      - name: Build WASM fixtures for factory integration tests
+        working-directory: soroban
+        run: cargo build --workspace --target wasm32v1-none --release
+
+      - name: Check formatting
+        working-directory: soroban
+        run: cargo fmt --all -- --check
+
+      - name: Clippy
+        working-directory: soroban
+        run: cargo clippy --workspace --all-targets -- -D warnings
       - name: Pre-build farming-pool WASM for integration tests
         working-directory: soroban
         # `factory/src/test.rs` embeds the real farming-pool WASM via

soroban/contracts/factory/Cargo.toml

@@ -12,6 +12,8 @@ doctest    = false
 soroban-sdk = { workspace = true }
 
 [dev-dependencies]
+farming-pool = { path = "../farming-pool" }
+soroban-sdk = { workspace = true, features = ["testutils"] }
 soroban-sdk   = { workspace = true, features = ["testutils"] }
 # Provides `farming_pool::WASM` (compiled contract bytes) and `FarmingPoolClient`
 # used in `src/test.rs` to prove that `create_pool` deploys a *real*, driveable

soroban/contracts/factory/src/lib.rs

@@ -241,9 +241,18 @@ impl Factory {
     /// maintained incrementally in create_pool) would avoid full-registry scans entirely.
     /// This would be a more robust long-term fix but requires changes to create_pool's
     /// write path and potentially a migration/backfill for existing pools.
-    pub fn get_pools_by_asset(env: Env, asset: Address, start_id: u32, limit: u32) -> ListPoolsResponse {
+    pub fn get_pools_by_asset(
+        env: Env,
+        asset: Address,
+        start_id: u32,
+        limit: u32,
+    ) -> ListPoolsResponse {
         bump_instance(&env);
-        let count: u32 = env.storage().instance().get(&DataKey::PoolCount).unwrap_or(0);
+        let count: u32 = env
+            .storage()
+            .instance()
+            .get(&DataKey::PoolCount)
+            .unwrap_or(0);
         let capped_limit = limit.min(20);
         let mut records: Vec<(u32, PoolRecord)> = vec![&env];
         let mut next_start_id = count;
@@ -298,7 +307,11 @@ impl Factory {
     /// ~60 days) to ensure all pool records remain accessible.
     pub fn refresh_pool_ttls(env: Env, start_id: u32, limit: u32) -> Result<(), FactoryError> {
         bump_instance(&env);
-        let count: u32 = env.storage().instance().get(&DataKey::PoolCount).unwrap_or(0);
+        let count: u32 = env
+            .storage()
+            .instance()
+            .get(&DataKey::PoolCount)
+            .unwrap_or(0);
         let capped_limit = limit.min(20);
         let end = start_id.saturating_add(capped_limit).min(count);
         for pool_id in start_id..end {
@@ -371,19 +384,14 @@ impl Factory {
     ///
     /// Emits a `wasm_set` event with `(old_hash, new_hash)` so that the previous
     /// hash is discoverable off-chain for rollback scenarios.
-    pub fn set_pool_wasm_hash(
-        env: Env,
-        new_hash: BytesN<32>,
-    ) -> Result<(), FactoryError> {
+    pub fn set_pool_wasm_hash(env: Env, new_hash: BytesN<32>) -> Result<(), FactoryError> {
         require_initialized(&env)?;
         let admin: Address = load_admin(&env);
         admin.require_auth();
         bump_instance(&env);
 
         let old_hash: BytesN<32> = env.storage().instance().get(&DataKey::WasmHash).unwrap();
-        env.storage()
-            .instance()
-            .set(&DataKey::WasmHash, &new_hash);
+        env.storage().instance().set(&DataKey::WasmHash, &new_hash);
         #[allow(deprecated)]
         env.events().publish(
             (symbol_short!("factory"), symbol_short!("wasm_set")),
@@ -444,6 +452,10 @@ impl Factory {
             .map_err(|_| FactoryError::MinLockPeriodOutOfRange)?;
 
         let pool_id: u32 = env.storage().instance().get(&DataKey::PoolCount).unwrap();
+        let next_count = pool_id
+            .checked_add(1)
+            .ok_or(FactoryError::PoolCountOverflow)?;
+        let wasm_hash: BytesN<32> = env.storage().instance().get(&DataKey::WasmHash).unwrap();
         let wasm_hash = load_wasm_hash(&env)?;
         let salt = pool_salt(&env, pool_id);
 
@@ -483,7 +495,7 @@ impl Factory {
         bump_pool(&env, pool_id);
         env.storage()
             .instance()
-            .set(&DataKey::PoolCount, &(pool_id + 1));
+            .set(&DataKey::PoolCount, &next_count);
 
         // Emit enriched event so indexers get the full pool parameters in one shot.
         #[allow(deprecated)]

soroban/contracts/factory/src/test.rs

@@ -343,7 +343,11 @@ fn test_get_pools_by_asset_returns_empty_when_no_pools() {
     let t = setup();
     let asset = Address::generate(&t.env);
     let page = t.client.get_pools_by_asset(&asset, &0u32, &10u32);
-    assert_eq!(page.records.len(), 0, "expected no pools for a fresh factory");
+    assert_eq!(
+        page.records.len(),
+        0,
+        "expected no pools for a fresh factory"
+    );
 }
 
 // ── transfer_admin ────────────────────────────────────────────────────────────
@@ -633,6 +637,12 @@ fn test_get_pools_by_asset_paginates_large_matching_registry() {
     // Create 25 pools all sharing the same asset
     let asset = Address::generate(&env);
     for i in 0..25 {
+        client.create_pool(
+            &asset,
+            &(1_728_000 + i as u128 * 17_280),
+            &2u32,
+            &(10 + i as u64),
+        );
         client.create_pool(&asset, &((100 + i as u128) * 17_280), &2u32, &(10 + i as u64));
     }
 
@@ -741,6 +751,28 @@ fn test_create_pool_increments_count_after_each_pool() {
     assert_eq!(t.client.pool_count(), 2);
 }
 
+#[test]
+fn test_create_pool_returns_typed_error_when_pool_count_overflows() {
+    let t = setup();
+
+    t.env.as_contract(&t.factory_addr, || {
+        t.env
+            .storage()
+            .instance()
+            .set(&DataKey::PoolCount, &u32::MAX);
+    });
+
+    let result =
+        t.client
+            .try_create_pool(&Address::generate(&t.env), &1_728_000u128, &2u32, &100u64);
+
+    assert_eq!(result, Err(Ok(FactoryError::PoolCountOverflow)));
+    assert_eq!(t.client.pool_count(), u32::MAX);
+    assert!(!t.env.as_contract(&t.factory_addr, || {
+        t.env.storage().persistent().has(&DataKey::Pool(u32::MAX))
+    }));
+}
+
 #[test]
 fn test_create_pool_uses_deterministic_pool_addresses() {
     let t = setup();
@@ -814,7 +846,7 @@ fn test_get_pool_bumps_pool_record_ttl() {
     advance_ledgers(&t.env, TTL_EXTEND_TO - TTL_THRESHOLD + 1);
     assert!(pool_record_ttl(&t.env, &t.factory_addr, id) < TTL_THRESHOLD);
 
-    assert_eq!(t.client.try_get_pool(&id).is_ok(), true);
+    assert!(t.client.try_get_pool(&id).is_ok());
     assert_eq!(pool_record_ttl(&t.env, &t.factory_addr, id), TTL_EXTEND_TO);
 }
 
@@ -823,6 +855,7 @@ fn test_refresh_pool_ttls_restores_ttl_for_unqueried_pool() {
     let t = setup();
     let id = t
         .client
+        .create_pool(&Address::generate(&t.env), &1_728_000u128, &2u32, &50u64);
         .create_pool(&Address::generate(&t.env), &(250u128 * 17_280), &2u32, &50u64);
 
     // Initial TTL after creation
@@ -833,10 +866,7 @@ fn test_refresh_pool_ttls_restores_ttl_for_unqueried_pool() {
     assert!(pool_record_ttl(&t.env, &t.factory_addr, id) < TTL_THRESHOLD);
 
     // Call refresh_pool_ttls to restore TTL without a specific get_pool query
-    assert_eq!(
-        t.client.try_refresh_pool_ttls(&id, &1u32),
-        Ok(Ok(()))
-    );
+    assert_eq!(t.client.try_refresh_pool_ttls(&id, &1u32), Ok(Ok(())));
 
     // Verify TTL is restored
     assert_eq!(pool_record_ttl(&t.env, &t.factory_addr, id), TTL_EXTEND_TO);

soroban/contracts/factory/src/types.rs

@@ -79,6 +79,8 @@ pub enum FactoryError {
     InvalidCreditRate = 5,
     /// `create_pool`'s `min_lock_period` does not fit in the pool's native `u32`.
     MinLockPeriodOutOfRange = 6,
+    /// `create_pool` cannot allocate another monotonically increasing pool ID.
+    PoolCountOverflow = 8,
     /// A function requiring initialization was called on an uninitialized factory.
     NotInitialized = 7,
 }

soroban/contracts/factory/tests/factory_pool_integration.rs

@@ -96,6 +96,8 @@ fn deploy_pool_via_factory(
     let factory_client = FactoryClient::new(env, &factory_addr);
     factory_client.initialize(admin, &wasm_hash);
 
+    let pool_id =
+        factory_client.create_pool(asset, &daily_rate, &global_multiplier, &min_lock_period);
     let pool_id = factory_client.create_pool(asset, &daily_rate, &global_multiplier, &min_lock_period);
     let record = factory_client.get_pool(&pool_id);
     let pool_address = record.address.clone();
@@ -122,6 +124,7 @@ fn smoke_create_pool_returns_live_pool_address() {
     let factory_client = FactoryClient::new(&env, &factory_addr);
     factory_client.initialize(&admin, &wasm_hash);
 
+    let pool_id = factory_client.create_pool(&asset, &1_728_000u128, &2u32, &10u64);
     let pool_id = factory_client.create_pool(&asset, &17_280u128, &1u32, &10u64);
     let record = factory_client.get_pool(&pool_id);
 
@@ -130,6 +133,7 @@ fn smoke_create_pool_returns_live_pool_address() {
     // panic on the first call below). create_pool (#79) already initialized
     // it atomically, so it must already report the factory's admin as its own.
     let pool_client = FarmingPoolClient::new(&env, &record.address);
+    // `create_pool` initializes the deployed contract atomically.
     assert_eq!(pool_client.admin(), admin);
 }
 
@@ -224,6 +228,10 @@ fn end_to_end_create_pool_then_stake_and_unstake() {
         &admin,
         &asset.address(),
         daily_rate,
+        global_multiplier,
+        min_lock_period as u64,
+    );
+
         1u32,
         min_lock_period as u64,
     );
@@ -300,6 +308,8 @@ fn end_to_end_create_pool_then_lock_and_unlock() {
     const INITIAL_MINT: i128 = 1_000_000_000;
     token_sac.mint(&user, &INITIAL_MINT);
 
+    let global_multiplier = 1u32;
+    let daily_rate = 34_560u128;
     // global_multiplier is passed as 1 at creation — this test only
     // exercises lock/unlock, which isn't affected by it.
     let credit_rate = 2i128;
@@ -314,6 +324,10 @@ fn end_to_end_create_pool_then_lock_and_unlock() {
         &admin,
         &asset.address(),
         daily_rate,
+        global_multiplier,
+        min_lock_period_ledgers as u64,
+    );
+
         1u32,
         min_lock_period_ledgers as u64,
     );

soroban/contracts/farming-pool/src/lib.rs

@@ -1,8 +1,9 @@
 #![no_std]
+#![allow(deprecated)]
 
-mod types;
 #[cfg(test)]
 mod mock_reentrant_token;
+mod types;
 
 use soroban_sdk::{contract, contractimpl, symbol_short, token, Address, BytesN, Env};
 pub use types::PoolError;
@@ -97,7 +98,6 @@ fn require_not_paused(env: &Env) -> Result<(), PoolError> {
     Ok(())
 }
 
-
 fn get_admin(env: &Env) -> Result<Address, PoolError> {
     env.storage()
         .instance()
@@ -382,7 +382,7 @@ impl FarmingPool {
         let stake_token = get_stake_token(&env)?;
         token::TokenClient::new(&env, &stake_token).transfer(
             &user,
-            &env.current_contract_address(),
+            env.current_contract_address(),
             &amount,
         );
 
@@ -560,7 +560,7 @@ impl FarmingPool {
         let stake_token = get_stake_token(&env)?;
         token::TokenClient::new(&env, &stake_token).transfer(
             &from,
-            &env.current_contract_address(),
+            env.current_contract_address(),
             &amount,
         );
 
@@ -594,10 +594,9 @@ impl FarmingPool {
         user.require_auth();
         require_not_paused(&env)?;
 
-
         require_initialized(&env)?;
         assert!(
-            allocation_pct >= 1 && allocation_pct <= 100,
+            (1..=100).contains(&allocation_pct),
             "allocation_pct must be 1-100"
         );
         bump_instance(&env);

soroban/contracts/farming-pool/src/mock_reentrant_token.rs

@@ -1,5 +1,3 @@
-#![cfg(test)]
-
 //! A minimal token-interface contract for exercising checks-effects-
 //! interactions (CEI) reentrancy scenarios in tests (#69). Configured with a
 //! target contract + user, its `transfer` attempts to call back into the

soroban/contracts/farming-pool/src/test.rs

@@ -1175,7 +1175,6 @@ fn test_pause_blocks_lock_assets() {
     }
 }
 
-
 #[test]
 fn test_pause_blocks_unlock_assets() {
     let t = setup(1, 1);
@@ -1188,7 +1187,6 @@ fn test_pause_blocks_unlock_assets() {
     }
 }
 
-
 #[test]
 fn test_unpause_restores_operations() {
     let t = setup(1, 1);
@@ -1232,7 +1230,6 @@ fn test_pause_blocks_stake() {
     }
 }
 
-
 #[test]
 fn test_unpause_restores_stake() {
     let t = setup(1, 1);