You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
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]fntest_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:
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:
BankedCredits is only ever written by set_banked_credits (lib.rs:104-108), which is only called from emergency_withdraw (lines 384-385, and only when banked_credits > 0) — so test_get_banked_credits_bumps_ttl (per the issue's acceptance criteria) needs a setup that first drives a user through emergency_withdraw (requiring pause() first, since that function is pause-gated) before it can even populate a BankedCredits entry to test TTL behavior on. This is a more involved setup than the other three tests, which can bump their target key via ordinary stake/lock_assets/set_boost calls.
UserBoost's TTL is bumped by both get_user_boost (a read, called internally by checkpoint/get_credits/get_boost_config) and indirectly whenever set_boost writes it (line 476) — the test should verify both the write-then-immediate-TTL and the later-read-after-decay cases, matching the two-phase pattern (set establishes TTL_EXTEND_TO, then decay, then a read/write restores it) used in the issue's own test_stake_bumps_user_stake_ttl sketch and the factory's existing test_get_pool_bumps_pool_record_ttl.
This issue is purely additive (new tests + one new helper) and should not require touching farming-pool/src/lib.rs at all — confirm no production code changes are needed, only farming-pool/src/test.rs additions, keeping this squarely a testing/coverage issue rather than a behavior change.
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).
Problem
The factory contract has explicit TTL-extension test coverage —
test_get_pool_bumps_pool_record_ttlinfactory/src/test.rsadvances ledgers past the threshold, asserts the TTL has dropped belowTTL_THRESHOLD, callsget_pool, and asserts the TTL is restored toTTL_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), despitebump_user/USER_TTL_THRESHOLD/USER_TTL_EXTEND_TOexisting specifically to manage their TTL, mirroring the factory's constants almost exactly. There is no test asserting:get_user_stake/set_user_stakeactually extend aUserStakeentry's TTL,get_position/set_positionextend aPositionentry's TTL,get_user_boostextends aUserBoostentry's TTL,Poolrecords 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 thebump_usercall (as already happens forremove_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
bump_userfor one of the four key variants without any test failing.Fix
Add a
key_ttltest helper mirroring the factory'spool_record_ttlhelper, exercised against each of the fourDataKeyvariants:Acceptance Criteria
key_ttltest helper added tofarming-pool/src/test.rs.test_stake_bumps_user_stake_ttl(or equivalent) verifyingUserStakeTTL decays and is restored on read/write.test_lock_assets_bumps_user_position_ttlverifyingUserPositionTTL behavior.test_set_boost_bumps_user_boost_ttlverifyingUserBoostTTL behavior.test_get_banked_credits_bumps_ttlverifyingBankedCreditsTTL behavior afteremergency_withdraw.Additional Notes
Confirmed against current source:
factory/src/test.rs:559-572containstest_get_pool_bumps_pool_record_ttlexactly as quoted in the issue, usingpool_record_ttl/advance_ledgers/TTL_THRESHOLD/TTL_EXTEND_TOhelpers (factory/src/test.rs:118-133). Confirmedfarming-pool/src/test.rs(1039 lines) has nokey_ttl-equivalent helper and no test asserting TTL behavior for any of the fourDataKeyvariants (UserStake,UserPosition/Position— note the issue's prose says "UserPosition" but the actualDataKeyvariant name confirmed infarming-pool/src/types.rs:59isUserPosition(Address), matching),UserBoost,BankedCredits.farming-pool/src/lib.rs:19-23definesbump_userwith the sameUSER_TTL_THRESHOLD/USER_TTL_EXTEND_TOconstant pattern (lines 10-11) mirroring the factory'sTTL_THRESHOLD/TTL_EXTEND_TO(factory/src/lib.rs:9-10) almost exactly, confirming the "mirrors the factory's constants" claim.Edge cases:
BankedCreditsis only ever written byset_banked_credits(lib.rs:104-108), which is only called fromemergency_withdraw(lines 384-385, and only whenbanked_credits > 0) — sotest_get_banked_credits_bumps_ttl(per the issue's acceptance criteria) needs a setup that first drives a user throughemergency_withdraw(requiringpause()first, since that function is pause-gated) before it can even populate aBankedCreditsentry to test TTL behavior on. This is a more involved setup than the other three tests, which can bump their target key via ordinarystake/lock_assets/set_boostcalls.UserBoost's TTL is bumped by bothget_user_boost(a read, called internally bycheckpoint/get_credits/get_boost_config) and indirectly wheneverset_boostwrites it (line 476) — the test should verify both the write-then-immediate-TTL and the later-read-after-decay cases, matching the two-phase pattern (setestablishesTTL_EXTEND_TO, then decay, then a read/write restores it) used in the issue's owntest_stake_bumps_user_stake_ttlsketch and the factory's existingtest_get_pool_bumps_pool_record_ttl.farming-pool/src/lib.rsat all — confirm no production code changes are needed, onlyfarming-pool/src/test.rsadditions, keeping this squarely a testing/coverage issue rather than a behavior change.keep_alive-style fix for farming-pool: no restore/recovery path exists for archived per-user persistent storage entries after TTL expiry #73 would also need test coverage for — landing farming-pool: no test verifies persistent-storage TTL extension for UserStake/UserPosition/UserBoost/BankedCredits #74 first gives farming-pool: no restore/recovery path exists for archived per-user persistent storage entries after TTL expiry #73's eventual fix akey_ttlhelper and decay-pattern precedent to build on directly, rather than inventing test infrastructure from scratch.Implementation sketch: add
fn key_ttl(env: &Env, contract_id: &Address, key: &DataKey) -> u32tofarming-pool/src/test.rs, mirroringpool_record_ttl(factory/src/test.rs:133) but usingenv.as_contract(contract_id, || env.storage().persistent().get_ttl(key))against the farming-pool's ownDataKeyenum (already imported viause 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, andtest_get_banked_credits_bumps_ttl(the last requiring apause()+emergency_withdraw()setup as noted above), each following the three-phase pattern: assert TTL isUSER_TTL_EXTEND_TOimmediately after the write, advance ledgers pastUSER_TTL_EXTEND_TO - USER_TTL_THRESHOLD + 1, assert TTL has dropped belowUSER_TTL_THRESHOLD, then perform a read/write and assert TTL is restored toUSER_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 onemergency_withdraw's pause-gated setup, same function that issue's reentrancy fix touches — no code conflict since #74 doesn't modifylib.rs, but worth sequencing test additions after #72's behavior is stable so theBankedCreditstest doesn't need updating for arg/behavior changes made by #72).