farming-pool: unstake() cannot partially withdraw a stake, unlike unlock_assets()'s partial-unlock support

[prodbycorne]

Problem

The lock/unlock system supports partial withdrawal — unlock_assets(env, user, amount) takes an explicit amount and leaves the remainder locked:

pub fn unlock_assets(env: Env, user: Address, amount: i128) -> Result<(), PoolError> {
    // ...
    assert!(amount <= position.amount, "insufficient locked balance");
    // ...
    position.amount -= amount;
    // ...
    if position.amount == 0 {
        remove_position(&env, &user);
    } else {
        set_position(&env, &user, &position);
    }
    // ...
}

(confirmed exercised by test_unlock_assets_partial_keeps_remaining_position). The boost/stake system's unstake has no such capability — it takes no amount parameter at all and always withdraws the entire UserStake.amount:

pub fn unstake(env: Env, from: Address) -> Result<i128, PoolError> {
    // ...
    let mut stake = get_user_stake(&env, &from).expect("no active stake");
    checkpoint(&env, &from, &mut stake);
    let total_credits = stake.credits_banked;
    // ...
    token::TokenClient::new(&env, &stake_token).transfer(
        &env.current_contract_address(), &from, &stake.amount,   // always the FULL amount
    );
    remove_user_stake(&env, &from);
    Ok(total_credits)
}

A user who wants to withdraw only part of their staked balance while keeping the rest earning credits (exactly the flexibility unlock_assets already provides for the lock system) has no way to do so in the boost/stake system: they must fully unstake() — losing their UserBoost allocation association for the withdrawn-and-redeposited remainder — and re-stake() the portion they want to keep, incurring an unnecessary round-trip through the token contract and resetting start_ledger/checkpoint timing in a way unlock_assets doesn't force on lock-system users.

Impact

• Inconsistent, more restrictive UX for the boost/stake system than the lock/unlock system it otherwise mirrors closely (both use the same checkpoint-then-persist pattern, both track credit_rate snapshots).
• Forces unnecessary token round-trips for a common operation (partial withdrawal), each incurring the full transfer-in/transfer-out cost and gas, purely because the API doesn't support the natural partial-amount case.

Fix

Add an amount parameter to unstake, mirroring unlock_assets's partial-withdrawal pattern:

pub fn unstake(env: Env, from: Address, amount: i128) -> Result<i128, PoolError> {
    from.require_auth();
    require_not_paused(&env)?;
    require_initialized(&env)?;

    let mut stake = get_user_stake(&env, &from).ok_or(PoolError::NoActiveStake)?;
    if amount <= 0 || amount > stake.amount {
        return Err(PoolError::InvalidAmount);
    }
    checkpoint(&env, &from, &mut stake);
    let total_credits = stake.credits_banked;
    stake.amount -= amount;

    let stake_token = get_stake_token(&env)?;
    token::TokenClient::new(&env, &stake_token).transfer(
        &env.current_contract_address(), &from, &amount,
    );

    if stake.amount == 0 {
        remove_user_stake(&env, &from);
    } else {
        set_user_stake(&env, &from, &stake);
    }
    Ok(total_credits)
}

This is a breaking ABI change (new required parameter); consider keeping a full_unstake convenience wrapper for backward compatibility with existing integrators if that's a concern.

Acceptance Criteria

• unstake accepts an amount parameter and supports partial withdrawal, leaving the remainder staked and still earning credits.
• unstake rejects amount <= 0 or amount > stake.amount with a typed error.
• Add test_unstake_partial_keeps_remaining_stake mirroring test_unlock_assets_partial_keeps_remaining_position.
• Update existing test_unstake_returns_tokens_and_credits, test_pause_blocks_unstake, test_unpause_restores_unstake call sites for the new signature.
• Document the ABI/signature change in the PR description for downstream client updates.

Additional Notes

Precise references: unstake is at soroban/contracts/farming-pool/src/lib.rs:438-457; the full-withdrawal transfer is line 449-453 (&stake.amount); remove_user_stake unconditionally removes the DataKey::UserStake(from) entry at line 455 regardless of how much is withdrawn — this is the exact line that needs to become conditional (mirroring unlock_assets's if position.amount == 0 { remove_position } else { set_position } at lib.rs:292-296).

Edge cases beyond the issue body's sketch

• DataKey::UserBoost(user) orphaning: unlike Position (lock system), a stake's allocation_pct boost is stored in a separate key (DataKey::UserBoost, lib.rs:74-81) that unstake never touches today. If partial unstake leaves stake.amount > 0, the existing UserBoost entry correctly continues to apply to the remainder — that's fine and is actually the desired behavior. But if partial unstake reduces the stake to exactly the remainder and the user later calls set_boost again, checkpoint (line 148-162) is invoked from set_boost too (line 470) — confirm the interaction: checkpoint reads get_user_boost fresh each time (line 149), so no staleness risk there. Worth an explicit test to lock this in since it's a two-key (UserStake + UserBoost) invariant that's easy to break in a future refactor.
• Interaction with farming-pool: no property-based test enforces that total accrued credits are independent of checkpoint frequency #75's path-independence property test: once unstake takes a partial amount, the property test in farming-pool: no property-based test enforces that total accrued credits are independent of checkpoint frequency #75 should be extended to include partial-unstake-then-restake sequences in its random operation generator — a partial withdrawal followed by an equal re-stake should be checkpoint-neutral (same total credits as never withdrawing), and that's a good additional invariant to assert once both issues land.
• Zero-amount / full-amount boundary: the sketch's amount <= 0 || amount > stake.amount check should have an explicit test for amount == stake.amount (should behave identically to today's full-unstake path, including remove_user_stake) as well as amount == stake.amount + 1 (should reject) — the existing test_unlock_assets_rejects_more_than_locked (farming-pool/src/test.rs:685) is the template.
• PoolError variant reuse: the sketch introduces PoolError::InvalidAmount, but check types.rs (farming-pool/src/types.rs:7-13) first — no such variant exists yet (current variants: AlreadyInitialized=1, NotInitialized=2, InvalidCreditRate=3, NotPaused=13, NoActiveStake=14); note the existing gap between 3 and 13 in the discriminant sequence, suggesting other variants were removed/reserved at some point — pick a discriminant that doesn't collide with any other in-flight issue in this batch that also proposes new PoolError variants (e.g. farming-pool: set_global_multiplier and set_credit_rate accept unbounded values with no sanity ceiling, guaranteeing compute_credits overflow at scale #89's InvalidGlobalMultiplier).

Implementation sketch (refining the issue body's)

The issue body's sketch calls .ok_or(PoolError::NoActiveStake) — NoActiveStake already exists (value 14) and is reused correctly from emergency_withdraw (lib.rs:379-381), good. Add:

pub fn unstake(env: Env, from: Address, amount: i128) -> Result<i128, PoolError> {
    from.require_auth();
    assert!(!pool_is_paused(&env), "pool is paused");
    require_initialized(&env)?;
    let mut stake = get_user_stake(&env, &from).ok_or(PoolError::NoActiveStake)?;
    if amount <= 0 || amount > stake.amount {
        return Err(PoolError::InvalidAmount); // new variant, pick unused discriminant
    }
    bump_instance(&env);
    checkpoint(&env, &from, &mut stake);
    let total_credits = stake.credits_banked;
    stake.amount -= amount;
    let stake_token = get_stake_token(&env)?;
    token::TokenClient::new(&env, &stake_token).transfer(&env.current_contract_address(), &from, &amount);
    if stake.amount == 0 {
        remove_user_stake(&env, &from);
    } else {
        set_user_stake(&env, &from, &stake);
    }
    Ok(total_credits)
}

Test plan

• Add test_unstake_partial_keeps_remaining_stake (named in issue body).
• Add test_unstake_partial_preserves_boost_allocation for the UserBoost orphaning edge case above.
• Update call sites in existing tests: test_unstake_returns_tokens_and_credits (test.rs:424), and pause/unpause tests that call unstake (grep for .unstake( across farming-pool/src/test.rs) for the new required amount argument.
• Cross-references: farming-pool: no property-based test enforces that total accrued credits are independent of checkpoint frequency #75 (extend path-independence property generator with partial-unstake ops), farming-pool: set_global_multiplier and set_credit_rate accept unbounded values with no sanity ceiling, guaranteeing compute_credits overflow at scale #89 (new PoolError variant discriminant coordination), infrastructure: CI never runs cargo clippy or cargo fmt --check, letting lint regressions and panic-safety fixes silently regress #88 (CI clippy run should catch any missed call-site update if -D warnings is enabled and an unused-variable/type-mismatch results from the signature change).

[Cyber-Mitch]

i'm a smart contract engineer, i have experience in writing reviewing and testing Soroban Smart contracts. I will complete this issue in 2 days

[grantfox-oss]

🦊 GrantFox — @Cyber-Mitch has been assigned to this issue as part of the Official Campaign | FWC26 campaign!

Next steps:

1. Open a Pull Request referencing this issue (e.g., Closes #77)
2. Your PR will be reviewed by the SmartDropLabs maintainers

Good luck! Track your progress on GrantFox.