farming-pool: checkpoint_position and calculate_credits duplicate the credit-accrual formula instead of sharing one implementation

[prodbycorne]

Problem

The lock/unlock (Position) credit-accrual formula is implemented inline, twice, instead of through a single shared function:

fn checkpoint_position(env: &Env, position: &mut Position) {
    let current = env.ledger().sequence();
    let elapsed = current.saturating_sub(position.checkpoint_ledger);
    position.total_credits += position.amount * position.credit_rate * elapsed as i128;
    position.checkpoint_ledger = current;
    position.credit_rate = read_credit_rate(env);
}

pub fn calculate_credits(env: Env, user: Address) -> Result<i128, PoolError> {
    // ...
    let elapsed = env.ledger().sequence().saturating_sub(position.checkpoint_ledger);
    Ok(position.total_credits + position.amount * position.credit_rate * elapsed as i128)
}

Both compute amount * credit_rate * elapsed, but they are two independently-maintained copies of the same formula, one mutating state (checkpoint_position) and one previewing it (calculate_credits). Unlike the boost/stake system, which funnels all accrual math through the single compute_credits/compute_total_stake helpers, the lock/unlock system has no equivalent shared primitive.

This is a real maintenance hazard, not a style nit: any future change to the accrual formula (e.g. adding overflow checks per issue "compute_credits' unchecked i128 multiplication chain...", or extending Position to support boost like UserStake does) requires remembering to update two call sites in lockstep. A change applied to only one (e.g. patching checkpoint_position to use checked_mul but forgetting calculate_credits) would silently reintroduce the exact overflow/precision bug being fixed in only one of the two paths — an easy mistake given the current code has no compiler-enforced single-source-of-truth for this formula, unlike the compute_credits helper used by the stake/boost system.

Impact

• Latent correctness risk: any accrual-formula bugfix or precision improvement applied to one of the two copies but not the other creates a preview-vs-actual mismatch — calculate_credits() would show one number, but the credits actually banked on unlock_assets would differ.
• Inconsistent architecture: the codebase already demonstrates the correct pattern (compute_credits/compute_total_stake shared by checkpoint/get_credits) for the stake/boost system; the lock/unlock system does not follow it, making the codebase harder to reason about and review.

Fix

Extract a single compute_position_credits (or reuse compute_credits with allocation_pct = 0/multiplier = 1 if Position and UserStake are meant to share semantics) and call it from both checkpoint_position and calculate_credits:

fn compute_position_credits(amount: i128, credit_rate: i128, elapsed: u32) -> i128 {
    amount * credit_rate * elapsed as i128
}

fn checkpoint_position(env: &Env, position: &mut Position) {
    let current = env.ledger().sequence();
    let elapsed = current.saturating_sub(position.checkpoint_ledger);
    position.total_credits += compute_position_credits(position.amount, position.credit_rate, elapsed);
    position.checkpoint_ledger = current;
    position.credit_rate = read_credit_rate(env);
}

pub fn calculate_credits(env: Env, user: Address) -> Result<i128, PoolError> {
    // ...
    let elapsed = env.ledger().sequence().saturating_sub(position.checkpoint_ledger);
    Ok(position.total_credits + compute_position_credits(position.amount, position.credit_rate, elapsed))
}

Acceptance Criteria

• checkpoint_position and calculate_credits call one shared helper for the accrual formula; no inline duplication remains.
• Add a unit test asserting the shared helper's output for checkpoint_position matches calculate_credits's preview for the identical (amount, credit_rate, elapsed) inputs across several cases.
• Any future overflow/precision fix (see the overflow-chain issue) needs to touch only one function, not two.
• All existing test_calculate_credits_* and test_lock_assets_*/test_unlock_assets_* tests continue to pass unmodified.

Additional Notes

Confirmed against current source: checkpoint_position (farming-pool/src/lib.rs:164-170) computes position.total_credits += position.amount * position.credit_rate * elapsed as i128 at line 167; calculate_credits (lib.rs:305-317) computes the identical expression at line 316 (position.total_credits + position.amount * position.credit_rate * elapsed as i128). Confirmed these are the only two call sites of this formula, and confirmed the stake/boost side already has the shared-helper pattern this issue wants replicated: compute_credits/compute_total_stake (lib.rs:138-146, 131-136) are called from both checkpoint (line 153) and get_credits (line 579).

Edge cases:

• This issue is a pure refactor (extract compute_position_credits), but it becomes a prerequisite for farming-pool: compute_credits' unchecked i128 multiplication chain traps the whole contract on overflow with no typed error #62's overflow-check fix if that fix is meant to protect the Position path too — the overflow issue only names compute_credits/compute_total_stake in its acceptance criteria, not checkpoint_position/calculate_credits. If farming-pool: compute_credits' unchecked i128 multiplication chain traps the whole contract on overflow with no typed error #62 lands before this refactor, its checked_mul protection would need to be duplicated into both current inline copies, exactly reproducing the drift risk this issue warns about. Land this refactor first, or in the same PR as farming-pool: compute_credits' unchecked i128 multiplication chain traps the whole contract on overflow with no typed error #62's Position-side extension.
• farming-pool: get_credits retroactively reprices a user's entire uncheckpointed accrual window when the admin changes the global multiplier #60's UserStake.multiplier field does not apply to Position (the lock/unlock system doesn't use global_multiplier/allocation_pct at all — confirmed Position in types.rs:37-46 has no boost-related fields), so the new shared helper only needs (amount, credit_rate, elapsed), not a multiplier parameter — don't over-generalize it to match compute_credits's signature.
• The issue floats "reuse compute_credits with allocation_pct = 0/multiplier = 1" as an alternative — worth flagging that compute_total_stake(amount, 0, _) returns exactly amount (boosted = amount*0/100 = 0), so this works arithmetically, but conflates two independent domain concepts (a "position" has no boost concept at all) purely to save a few lines; a dedicated compute_position_credits(amount, credit_rate, elapsed) is clearer and matches the issue's primary suggestion.

Implementation sketch: extract fn compute_position_credits(amount: i128, credit_rate: i128, elapsed: u32) -> i128 (or Result<i128, PoolError> if merged with #62) next to compute_credits/compute_total_stake (lib.rs:131-146); replace line 167 in checkpoint_position and line 316 in calculate_credits with calls to it.

Test plan: add a unit test (e.g. test_compute_position_credits_matches_checkpoint_and_calculate_credits) directly calling the extracted helper and asserting its output equals both what checkpoint_position banks into total_credits and what calculate_credits previews, for several (amount, credit_rate, elapsed) triples including elapsed = 0 and a multi-checkpoint sequence. Confirm all existing test_calculate_credits_* (test_calculate_credits_zero_without_position, test_calculate_credits_accrues_over_time, test_calculate_credits_includes_banked_plus_accruing, test_calculate_credits_reflects_partial_unlock_checkpoint) and test_lock_assets_*/test_unlock_assets_* pass unmodified, per the issue's own acceptance criteria.

Cross-references: #60 (analogous snapshot-consistency concern, but on the UserStake side — no direct code overlap since Position doesn't use multiplier), #61 (this issue's helper should not import compute_total_stake's truncation bug — confirm Position accrual has no allocation-based multiplication at all, so #61 doesn't apply here), #62 (sequencing dependency — do this refactor first so the overflow fix touches one function, not two, on the Position side).

[Okorie2000-code]

Hi Maintainer,

I'd love to work on this issue. I understand the goal is to eliminate the duplicated credit-accrual logic by extracting a shared implementation while preserving existing behavior and ensuring comprehensive test coverage. I can submit a clean, review-ready PR within 48 hours.

Thank you for your consideration!

[grantfox-oss]

🦊 GrantFox — @Okorie2000-code 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 #63)
2. Your PR will be reviewed by the SmartDropLabs maintainers

Good luck! Track your progress on GrantFox.