farming-pool: unlock_assets and unstake panic via untyped .expect() instead of returning typed PoolError

[prodbycorne]

Problem

unlock_assets and unstake panic with untyped .expect() calls on a path directly reachable by any non-privileged caller who simply never staked/locked anything:

pub fn unlock_assets(env: Env, user: Address, amount: i128) -> Result<(), PoolError> {
    // ...
    let mut position = get_position(&env, &user).expect("no active position");
    // ...
}

pub fn unstake(env: Env, from: Address) -> Result<i128, PoolError> {
    // ...
    let mut stake = get_user_stake(&env, &from).expect("no active stake");
    // ...
}

Both functions already declare Result<_, PoolError> return types and both already have a matching typed error available — PoolError::NoActiveStake is used correctly by emergency_withdraw for exactly this scenario (if total_returned == 0 { return Err(PoolError::NoActiveStake); }) — yet unlock_assets/unstake don't reuse it and instead trap the whole invocation.

This is trivial for anyone to trigger: client.unlock_assets(&random_user, &1) or client.unstake(&random_user) for a user who never called lock_assets/stake panics rather than returning a matchable error, even though the function signature promises a Result.

Impact

• Poor integrator experience / DoS-adjacent: any frontend or bot calling unlock_assets/unstake speculatively (e.g. "try to withdraw if you have a position") gets an opaque host trap instead of a typed error it could catch and ignore.
• Inconsistent with the rest of the contract: PoolError::NoActiveStake already exists specifically for this condition but is only used in one of the three functions where it logically applies.

Fix

pub fn unlock_assets(env: Env, user: Address, amount: i128) -> Result<(), PoolError> {
    user.require_auth();
    require_initialized(&env)?;
    // ...
    let mut position = get_position(&env, &user).ok_or(PoolError::NoActiveStake)?;
    // ...
}

pub fn unstake(env: Env, from: Address) -> Result<i128, PoolError> {
    from.require_auth();
    // ...
    let mut stake = get_user_stake(&env, &from).ok_or(PoolError::NoActiveStake)?;
    // ...
}

Acceptance Criteria

• unlock_assets returns Err(PoolError::NoActiveStake) instead of panicking when no Position exists.
• unstake returns Err(PoolError::NoActiveStake) instead of panicking when no UserStake exists.
• Add test_unlock_assets_no_position_returns_typed_error asserting Err(Ok(PoolError::NoActiveStake)) via try_unlock_assets.
• Add test_unstake_no_stake_returns_typed_error asserting Err(Ok(PoolError::NoActiveStake)) via try_unstake.
• No .expect(...) remains in any non-test, non-initialization code path in farming-pool/src/lib.rs.

Additional Notes

Confirmed against current source: unlock_assets (farming-pool/src/lib.rs:265-303) calls get_position(&env, &user).expect("no active position") at line 272; unstake (lib.rs:438-457) calls get_user_stake(&env, &from).expect("no active stake") at line 444. Both functions already declare Result<_, PoolError> (Result<(), PoolError> and Result<i128, PoolError> respectively), and PoolError::NoActiveStake = 14 (types.rs:12) already exists and is used correctly in emergency_withdraw (lib.rs:379-381: if total_returned == 0 { return Err(PoolError::NoActiveStake); }).

Edge cases:

• Confirmed there is an existing test, test_unlock_assets_rejects_when_no_position (farming-pool/src/test.rs:692-695), but it only asserts try_unlock_assets(...).is_err() — it does not (and currently cannot, since the failure is an untyped panic/trap rather than a matchable PoolError) assert the specific error variant. After this fix, that test should be tightened to assert Err(Ok(PoolError::NoActiveStake)) specifically, not just .is_err(), per this issue's own acceptance criteria — flagging this because the issue's acceptance criteria asks for a new test name (test_unlock_assets_no_position_returns_typed_error) without explicitly saying to also strengthen the existing one; both should happen (either strengthen in place or add the new test and note the old one is now a strict subset).
• No equivalent existing test for unstake's no-stake case was found by scanning test.rs's test_unstake_*/test_additional_stake_* names — confirms test_unstake_no_stake_returns_typed_error is a genuinely new addition, not a strengthening of an existing test.
• Interacts with farming-pool: stake() and unstake() perform the token transfer before persisting UserStake changes #71 (stake/unstake CEI reordering): farming-pool: stake() and unstake() perform the token transfer before persisting UserStake changes #71's proposed fix for unstake (lib.rs:438-457) already rewrites the .expect(...) call to .ok_or(PoolError::NoActiveStake)? as part of its own reordering fix (see farming-pool: stake() and unstake() perform the token transfer before persisting UserStake changes #71's fix snippet, which changes line 444's get_user_stake(...).expect(...) to .ok_or(PoolError::NoActiveStake)?). These two issues touch the identical line — implement together or ensure whichever lands first doesn't get silently reverted by the other's patch.

Implementation sketch: replace .expect("no active position") (line 272) with .ok_or(PoolError::NoActiveStake)? and .expect("no active stake") (line 444) with .ok_or(PoolError::NoActiveStake)?, exactly as the issue's fix snippet shows.

Test plan: strengthen test_unlock_assets_rejects_when_no_position (test.rs:692-695) or add test_unlock_assets_no_position_returns_typed_error asserting t.client.try_unlock_assets(&t.user, &100i128) == Err(Ok(PoolError::NoActiveStake)); add test_unstake_no_stake_returns_typed_error with the analogous assertion via try_unstake. Grep farming-pool/src/lib.rs post-fix for .expect( to confirm none remain outside test/init code, per the issue's last acceptance-criteria bullet.

Cross-references: #64 (same untyped-panic-vs-typed-Result theme), #71 (touches the exact same unstake line as part of its CEI-reordering fix — coordinate merge order), #66 (same theme applied to input-validation asserts rather than missing-record lookups).

[Dickson2015]

The issue description is crystal clear, Maintainer I would love to be given the opportunity to work on issue