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).

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

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