farming-pool: lock_assets/unlock_assets/stake/set_boost validate amounts with untyped assert! instead of typed PoolError variants

[prodbycorne]

Problem

Amount and allocation validation across the fund-handling entry points uses untyped assert! panics instead of the typed PoolError pattern the contract otherwise uses consistently:

pub fn lock_assets(env: Env, user: Address, amount: i128) -> Result<(), PoolError> {
    // ...
    assert!(amount > 0, "amount must be positive");
    // ...
}

pub fn unlock_assets(env: Env, user: Address, amount: i128) -> Result<(), PoolError> {
    // ...
    assert!(amount > 0, "amount must be positive");
    // ...
    assert!(amount <= position.amount, "insufficient locked balance");
    // ...
    assert!(current >= position.unlock_ledger, "minimum lock period not elapsed");
}

pub fn stake(env: Env, from: Address, amount: i128) -> Result<(), PoolError> {
    // ...
    assert!(amount > 0, "amount must be positive");
}

pub fn set_boost(env: Env, user: Address, allocation_pct: u32) -> Result<(), PoolError> {
    // ...
    assert!(allocation_pct >= 1 && allocation_pct <= 100, "allocation_pct must be 1-100");
}

Every one of these functions already declares Result<(), PoolError> as its return type — the plumbing for typed errors is already in place — yet none of these six validation checks use it. set_credit_rate (PoolError::InvalidCreditRate) is the only place in the whole contract that validates fund/rate parameters via a typed error instead of a panic, making the contract internally inconsistent about how it signals invalid input.

Impact

• Poor integrator experience: a frontend calling try_lock_assets(&user, &-5) or try_unlock_assets(&user, &(position.amount + 1)) gets an opaque host trap it cannot pattern-match on, instead of a typed error like the rest of the API promises.
• Inconsistent contract surface: reviewers and integrators cannot rely on PoolError as the single source of truth for "what can go wrong" — some failures are typed, others are untyped panics, for what are conceptually the same class of validation failure (bad caller input).

Fix

Add dedicated typed variants and replace each assert! with a Result-returning check:

pub enum PoolError {
    // ...
    InvalidAmount = 16,
    InsufficientBalance = 17,
    LockPeriodNotElapsed = 18,
    InvalidAllocation = 19,
}

// lock_assets / stake:
if amount <= 0 { return Err(PoolError::InvalidAmount); }

// unlock_assets:
if amount <= 0 { return Err(PoolError::InvalidAmount); }
if amount > position.amount { return Err(PoolError::InsufficientBalance); }
if current < position.unlock_ledger { return Err(PoolError::LockPeriodNotElapsed); }

// set_boost:
if !(1..=100).contains(&allocation_pct) { return Err(PoolError::InvalidAllocation); }

Acceptance Criteria

• New PoolError variants added for amount/allocation/lock-period validation (exact names/numbering at maintainer discretion, but must not collide with existing variants).
• lock_assets, unlock_assets, stake, set_boost return typed Err(PoolError::_) instead of panicking via assert! for all input-validation failures.
• Existing tests using try_lock_assets/try_unlock_assets/try_stake/try_set_boost and asserting .is_err() continue to pass, updated to assert the specific PoolError variant where practical (e.g. test_set_boost_rejects_zero_allocation, test_lock_assets_rejects_negative_amount).
• No assert! remains for caller-input validation in lock_assets, unlock_assets, stake, set_boost.

Additional Notes

Confirmed against current source: lock_assets (farming-pool/src/lib.rs:225-263) asserts amount > 0 at line 229; unlock_assets (265-303) asserts amount > 0 (269), amount <= position.amount (273), and current >= position.unlock_ledger (276-279); stake (404-436) asserts amount > 0 (408); set_boost (459-484) asserts allocation_pct in 1..=100 (463-466). Confirmed set_credit_rate (513-530) is the only validation in the contract using a typed error (PoolError::InvalidCreditRate, line 517) instead of assert!.

Edge cases:

• Interacts directly with farming-pool: unlock_assets and unstake panic via untyped .expect() instead of returning typed PoolError #65 and farming-pool: stake() and unstake() perform the token transfer before persisting UserStake changes #71: unlock_assets's .expect("no active position") (line 272, fixed by farming-pool: unlock_assets and unstake panic via untyped .expect() instead of returning typed PoolError #65) sits between two of the assert!s this issue targets (amount > 0 at 269, then the expect at 272, then amount <= position.amount at 273) — implementing farming-pool: unlock_assets and unstake panic via untyped .expect() instead of returning typed PoolError #65 and farming-pool: lock_assets/unlock_assets/stake/set_boost validate amounts with untyped assert! instead of typed PoolError variants #66 independently on the same function risks merge conflicts on adjacent lines; do them together or in a clearly sequenced order (typed missing-position check, then typed amount checks, all in one pass over unlock_assets).
• unlock_assets's three validation asserts have an ordering dependency worth preserving: amount > 0 must be checked before amount <= position.amount is meaningful (a negative amount would trivially satisfy <= position.amount), and both need to run before current >= position.unlock_ledger per current code order — confirm the typed-error rewrite preserves this exact check order, since some test likely depends on which error fires first for a call violating multiple constraints simultaneously (worth adding a test for that if none exists).
• stake's and lock_assets's amount > 0 checks are functionally identical asserts in two different functions (lines 229 and 408) — if a shared PoolError::InvalidAmount is introduced, consider (but the issue doesn't require) a shared fn require_positive_amount(amount: i128) -> Result<(), PoolError> to avoid the same copy-paste-drift risk called out in farming-pool: pause guard has no typed PoolError variant — every gated function panics via assert! on a paused pool #68 for the pause guard.
• New variants must not collide with the existing PoolError discriminants (AlreadyInitialized=1, NotInitialized=2, InvalidCreditRate=3, NotPaused=13, NoActiveStake=14) or with farming-pool: compute_credits' unchecked i128 multiplication chain traps the whole contract on overflow with no typed error #62's proposed CreditOverflow=15 or farming-pool: pause guard has no typed PoolError variant — every gated function panics via assert! on a paused pool #68's proposed Paused=20 — coordinate discriminant assignment across farming-pool: compute_credits' unchecked i128 multiplication chain traps the whole contract on overflow with no typed error #62/farming-pool: lock_assets/unlock_assets/stake/set_boost validate amounts with untyped assert! instead of typed PoolError variants #66/farming-pool: initialize() validates credit_rate/global_multiplier via assert! while set_credit_rate enforces the same rule with a typed error #67/farming-pool: pause guard has no typed PoolError variant — every gated function panics via assert! on a paused pool #68 since they're all adding variants to the same enum in the same batch of work.

Implementation sketch: add PoolError::InvalidAmount, InsufficientBalance, LockPeriodNotElapsed, InvalidAllocation (types.rs, after NoActiveStake = 14, e.g. 16-19, leaving 15 free for #62's CreditOverflow); replace each assert! at the six call sites above with the corresponding if ... { return Err(...); } exactly as the issue's fix snippet shows.

Test plan: update test_lock_assets_rejects_zero_amount, test_lock_assets_rejects_negative_amount, test_unlock_assets_rejects_zero_amount, test_unlock_assets_rejects_more_than_locked, test_set_boost_rejects_zero_allocation, test_set_boost_rejects_over_100_allocation (all confirmed present in farming-pool/src/test.rs) to assert the specific PoolError variant via try_* rather than just .is_err()/should_panic. Add a new test for the "multiple constraints violated simultaneously" ordering case in unlock_assets noted above (e.g. test_unlock_assets_zero_amount_and_no_position_reports_amount_error_first or the reverse, whichever the preserved order dictates).

Cross-references: #64 (same theme, admin-getter functions), #65 (adjacent lines in unlock_assets/unstake — coordinate), #67 (same theme, initialize/set_global_multiplier), #68 (same theme, pause guard; also same enum-discriminant coordination concern), #62 (discriminant-numbering coordination for CreditOverflow).

[Achievers-sketch]

Experienced Rust/Soroban contributor. I’ll replace validation panics with typed PoolError variants, standardize error handling, update tests, and improve API consistency for integrators.