Skip to content

farming-pool: no test verifies persistent-storage TTL extension for UserStake/UserPosition/UserBoost/BankedCredits #74

Description

@prodbycorne

Problem

The factory contract has explicit TTL-extension test coverage — test_get_pool_bumps_pool_record_ttl in factory/src/test.rs advances ledgers past the threshold, asserts the TTL has dropped below TTL_THRESHOLD, calls get_pool, and asserts the TTL is restored to TTL_EXTEND_TO:

#[test]
fn test_get_pool_bumps_pool_record_ttl() {
    let t = setup();
    let id = t.client.create_pool(&Address::generate(&t.env), &250u128, &50u64);

    assert_eq!(pool_record_ttl(&t.env, &t.factory_addr, id), TTL_EXTEND_TO);
    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_eq!(pool_record_ttl(&t.env, &t.factory_addr, id), TTL_EXTEND_TO);
}

farming-pool/src/test.rs (1039 lines, extensive coverage of accrual, pause, and admin-transfer scenarios) has no equivalent test for any of its four per-user persistent keys (UserStake, UserPosition, UserBoost, BankedCredits), despite bump_user/USER_TTL_THRESHOLD/USER_TTL_EXTEND_TO existing specifically to manage their TTL, mirroring the factory's constants almost exactly. There is no test asserting:

  • that get_user_stake/set_user_stake actually extend a UserStake entry's TTL,
  • that get_position/set_position extend a Position entry's TTL,
  • that get_user_boost extends a UserBoost entry's TTL,
  • or that these entries' TTLs decay over elapsed ledgers the way the factory's Pool records are proven to.

This is a coverage gap in a test suite that is otherwise unusually rigorous (per the project's own testing standards), and it's exactly the kind of gap that would hide a regression if bump_user's threshold/extension constants were ever changed incorrectly, or if a code path were refactored to accidentally skip the bump_user call (as already happens for remove_user_stake/remove_position, which correctly don't need to bump a key they're deleting, but a similar omission elsewhere would go unnoticed).

Impact

  • Undetected regression risk: a future refactor could accidentally break bump_user for one of the four key variants without any test failing.
  • Asymmetric confidence between the two contracts: the factory contract has proof its archival-avoidance mechanism works; the farming pool, which arguably has a more acute archival risk (see the companion "no restore path for archived per-user storage" issue, since per-user keys are only touched by that specific user, unlike pool records which any indexer's read can bump), has none.

Fix

Add a key_ttl test helper mirroring the factory's pool_record_ttl helper, exercised against each of the four DataKey variants:

fn key_ttl(env: &Env, contract_id: &Address, key: &DataKey) -> u32 {
    env.as_contract(contract_id, || env.storage().persistent().get_ttl(key))
}

#[test]
fn test_stake_bumps_user_stake_ttl() {
    let t = setup(1, 1);
    t.client.stake(&t.user, &1_000);
    let key = DataKey::UserStake(t.user.clone());
    assert_eq!(key_ttl(&t.env, &t.contract_id, &key), USER_TTL_EXTEND_TO);

    advance_ledgers(&t.env, USER_TTL_EXTEND_TO - USER_TTL_THRESHOLD + 1);
    assert!(key_ttl(&t.env, &t.contract_id, &key) < USER_TTL_THRESHOLD);

    t.client.get_stake(&t.user);
    assert_eq!(key_ttl(&t.env, &t.contract_id, &key), USER_TTL_EXTEND_TO);
}

Acceptance Criteria

  • key_ttl test helper added to farming-pool/src/test.rs.
  • test_stake_bumps_user_stake_ttl (or equivalent) verifying UserStake TTL decays and is restored on read/write.
  • test_lock_assets_bumps_user_position_ttl verifying UserPosition TTL behavior.
  • test_set_boost_bumps_user_boost_ttl verifying UserBoost TTL behavior.
  • test_get_banked_credits_bumps_ttl verifying BankedCredits TTL behavior after emergency_withdraw.

Additional Notes

Confirmed against current source: factory/src/test.rs:559-572 contains test_get_pool_bumps_pool_record_ttl exactly as quoted in the issue, using pool_record_ttl/advance_ledgers/TTL_THRESHOLD/TTL_EXTEND_TO helpers (factory/src/test.rs:118-133). Confirmed farming-pool/src/test.rs (1039 lines) has no key_ttl-equivalent helper and no test asserting TTL behavior for any of the four DataKey variants (UserStake, UserPosition/Position — note the issue's prose says "UserPosition" but the actual DataKey variant name confirmed in farming-pool/src/types.rs:59 is UserPosition(Address), matching), UserBoost, BankedCredits. farming-pool/src/lib.rs:19-23 defines bump_user with the same USER_TTL_THRESHOLD/USER_TTL_EXTEND_TO constant pattern (lines 10-11) mirroring the factory's TTL_THRESHOLD/TTL_EXTEND_TO (factory/src/lib.rs:9-10) almost exactly, confirming the "mirrors the factory's constants" claim.

Edge cases:

Implementation sketch: add fn key_ttl(env: &Env, contract_id: &Address, key: &DataKey) -> u32 to farming-pool/src/test.rs, mirroring pool_record_ttl (factory/src/test.rs:133) but using env.as_contract(contract_id, || env.storage().persistent().get_ttl(key)) against the farming-pool's own DataKey enum (already imported via use super::* at test.rs:3).

Test plan: add test_stake_bumps_user_stake_ttl, test_lock_assets_bumps_user_position_ttl, test_set_boost_bumps_user_boost_ttl, and test_get_banked_credits_bumps_ttl (the last requiring a pause() + emergency_withdraw() setup as noted above), each following the three-phase pattern: assert TTL is USER_TTL_EXTEND_TO immediately after the write, advance ledgers past USER_TTL_EXTEND_TO - USER_TTL_THRESHOLD + 1, assert TTL has dropped below USER_TTL_THRESHOLD, then perform a read/write and assert TTL is restored to USER_TTL_EXTEND_TO.

Cross-references: #73 (this issue's test infrastructure is a direct prerequisite/precedent for testing that issue's eventual fix), #72 (the BankedCredits-focused test here depends on emergency_withdraw's pause-gated setup, same function that issue's reentrancy fix touches — no code conflict since #74 doesn't modify lib.rs, but worth sequencing test additions after #72's behavior is stable so the BankedCredits test doesn't need updating for arg/behavior changes made by #72).

Metadata

Metadata

Labels

GrantFox OSSIssue tracked in GrantFox OSSMaybe RewardedIssue may be eligible for a GrantFox rewardOfficial CampaignCampaign: Official CampaignOfficial Campaign | FWC26Campaign: Official Campaign | FWC26farming-poolFarmingPool contracttestingTests and test coveragevery hardExtremely hard — deep expertise, careful design, and significant time required

Type

No type

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions