diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 5dd1cf9..1640f12 100644 --- a/.github/workflows/ci.yml +++ b/.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 diff --git a/soroban/contracts/factory/Cargo.toml b/soroban/contracts/factory/Cargo.toml index 691d4f1..24f7fd0 100644 --- a/soroban/contracts/factory/Cargo.toml +++ b/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 diff --git a/soroban/contracts/factory/src/lib.rs b/soroban/contracts/factory/src/lib.rs index ebc9bcd..8b93608 100644 --- a/soroban/contracts/factory/src/lib.rs +++ b/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)] diff --git a/soroban/contracts/factory/src/test.rs b/soroban/contracts/factory/src/test.rs index e2a70ca..fdc2740 100644 --- a/soroban/contracts/factory/src/test.rs +++ b/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); diff --git a/soroban/contracts/factory/src/types.rs b/soroban/contracts/factory/src/types.rs index d738bf0..c24f234 100644 --- a/soroban/contracts/factory/src/types.rs +++ b/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, } diff --git a/soroban/contracts/factory/tests/factory_pool_integration.rs b/soroban/contracts/factory/tests/factory_pool_integration.rs index 743528f..1db43cd 100644 --- a/soroban/contracts/factory/tests/factory_pool_integration.rs +++ b/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, ); diff --git a/soroban/contracts/farming-pool/src/lib.rs b/soroban/contracts/farming-pool/src/lib.rs index 63ed9aa..8887f59 100644 --- a/soroban/contracts/farming-pool/src/lib.rs +++ b/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 { 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); diff --git a/soroban/contracts/farming-pool/src/mock_reentrant_token.rs b/soroban/contracts/farming-pool/src/mock_reentrant_token.rs index 2f71b09..c05627e 100644 --- a/soroban/contracts/farming-pool/src/mock_reentrant_token.rs +++ b/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 diff --git a/soroban/contracts/farming-pool/src/test.rs b/soroban/contracts/farming-pool/src/test.rs index 6d2d005..134c293 100644 --- a/soroban/contracts/farming-pool/src/test.rs +++ b/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);