fix(fee_collector): extend TTL of KEY_TOTAL on every collect_fee write, add get_total_collected_opt, document ambiguous-zero caveat

fee_collector/src/lib.rs

@@ -32,6 +32,30 @@ const KEY_INIT: Symbol = symbol_short!("INIT");
 /// Persistent key prefix for lifetime-total-collected per token.
 const KEY_TOTAL: Symbol = symbol_short!("TOTAL");
 
+// ---------------------------------------------------------------------------
+// TTL policy for (KEY_TOTAL, token) persistent entries
+// ---------------------------------------------------------------------------
+
+/// If the remaining TTL of a `(KEY_TOTAL, token)` entry falls below this
+/// threshold when `collect_fee` is called, the TTL is extended up to
+/// `TOTAL_TTL_TARGET`.
+///
+/// Set to ~30 days at 5 s/ledger (≈518 400 ledgers) so that a token which
+/// stops receiving traffic has about a month before its entry needs an
+/// external restore. On mainnet today the minimum persistent TTL is already
+/// well above this, but the guard here keeps the entry alive even when the
+/// network-minimum drops or a token becomes low-frequency.
+const TOTAL_TTL_THRESHOLD: u32 = 518_400; // ~30 days at 5 s/ledger
+
+/// Target TTL to extend a `(KEY_TOTAL, token)` entry to when its TTL falls
+/// below `TOTAL_TTL_THRESHOLD`.
+///
+/// ~120 days at 5 s/ledger (≈2 073 600 ledgers).  This is intentionally
+/// generous: the lifetime-total counter is the only on-chain audit surface
+/// for fee accounting, and the cost of re-extending a healthy entry is
+/// negligible compared to the cost of silently losing historical data.
+const TOTAL_TTL_TARGET: u32 = 2_073_600; // ~120 days at 5 s/ledger
+
 // ---------------------------------------------------------------------------
 // Error type
 // ---------------------------------------------------------------------------
@@ -112,6 +136,16 @@ impl FeeCollectorContract {
             .ok_or(FeeCollectorError::ArithmeticOverflow)?;
         env.storage().persistent().set(&total_key, &new_total);
 
+        // Extend the TTL of the lifetime-total entry on every write so it
+        // stays live even if this token goes dormant for a long stretch.
+        // Without this, a token that stops receiving fees could have its
+        // entry expire, causing `get_total_collected` to silently return 0
+        // instead of the real historical total — indistinguishable from
+        // "never collected any fees" and strictly worse than a loud error.
+        env.storage()
+            .persistent()
+            .extend_ttl(&total_key, TOTAL_TTL_THRESHOLD, TOTAL_TTL_TARGET);
+
         // Emit event.
         env.events().publish(
             (symbol_short!("fee_rcvd"), token),
@@ -183,6 +217,32 @@ impl FeeCollectorContract {
     }
 
     /// Return the lifetime total amount of `token` ever collected as fees.
+    ///
+    /// # Caveat: ambiguous zero
+    ///
+    /// This function returns `0` in two distinct situations that it cannot
+    /// distinguish:
+    ///
+    /// 1. **Genuinely zero** — no fees have ever been collected for `token`.
+    /// 2. **Stale / archived** — fees *were* collected historically, but the
+    ///    `(KEY_TOTAL, token)` persistent entry's TTL lapsed (e.g. the token
+    ///    stopped receiving traffic long enough for the entry to be archived
+    ///    by the network), and no one has restored it yet.
+    ///
+    /// As of this version, `collect_fee` calls `extend_ttl` on every write,
+    /// which keeps the entry live for up to `TOTAL_TTL_TARGET` ledgers after
+    /// the last fee was collected (~120 days at 5 s/ledger).  This makes
+    /// scenario 2 unlikely for any token that has seen recent activity, but
+    /// it **cannot be ruled out** for tokens that have been dormant longer
+    /// than the target TTL.
+    ///
+    /// Callers that need to distinguish "genuinely zero" from "possibly
+    /// stale" should use [`Self::get_total_collected_opt`], which returns
+    /// `None` when the entry is absent rather than silently returning `0`.
+    ///
+    /// The `fee_rcvd` events emitted by `collect_fee` are the authoritative
+    /// source of truth for lifetime fee totals; this counter is a cache that
+    /// can, in principle, be reconstructed by replaying those events.
     pub fn get_total_collected(env: Env, token: Address) -> i128 {
         let total_key = (KEY_TOTAL, token);
         env.storage()
@@ -191,6 +251,32 @@ impl FeeCollectorContract {
             .unwrap_or(0i128)
     }
 
+    /// Return the lifetime total fees collected for `token`, or `None` if
+    /// the persistent entry is absent.
+    ///
+    /// Unlike [`Self::get_total_collected`], this function surfaces the
+    /// distinction between:
+    ///
+    /// * `Some(0)`  — the entry exists and the running total is zero (an
+    ///   edge case that should not occur in practice, but is technically
+    ///   valid if the only fee collected was then subtracted — currently the
+    ///   contract has no subtraction path, so `Some(0)` will only appear
+    ///   immediately after the first `collect_fee` when `amount` would have
+    ///   been 0, which is rejected, meaning `Some` is always `> 0` today).
+    /// * `Some(n)`  — the entry exists with a nonzero running total `n`.
+    /// * `None`     — the entry is absent: either this token has never had
+    ///   any fees collected, **or** the entry existed but its TTL lapsed and
+    ///   it has since been archived by the network.
+    ///
+    /// Use this variant in any context where silently returning `0` for a
+    /// stale-but-historically-active token would be incorrect (e.g. treasury
+    /// reporting, per-epoch withdrawal-limit calculations, or on-chain
+    /// callers that gate logic on whether any fees exist).
+    pub fn get_total_collected_opt(env: Env, token: Address) -> Option<i128> {
+        let total_key = (KEY_TOTAL, token);
+        env.storage().persistent().get(&total_key)
+    }
+
     /// Return the admin address.
     pub fn get_admin(env: Env) -> Result<Address, FeeCollectorError> {
         env.storage()

fee_collector/src/test.rs

@@ -3,7 +3,7 @@
 #![cfg(test)]
 
 use soroban_sdk::{
-    testutils::Address as _,
+    testutils::{Address as _, Ledger as _, storage::Persistent as _},
     token::{Client as TokenClient, StellarAssetClient},
     Address, Env,
 };
@@ -160,3 +160,161 @@ fn test_not_initialized_errors() {
     let result = client.try_collect_fee(&token, &10i128);
     assert_eq!(result, Err(Ok(FeeCollectorError::NotInitialized)));
 }
+
+// ---------------------------------------------------------------------------
+// TTL tests
+// ---------------------------------------------------------------------------
+
+/// Demonstrates that `collect_fee` extends the TTL of the `(KEY_TOTAL,
+/// token)` persistent entry on every write, so the running total remains
+/// readable — via both `get_total_collected` and `get_total_collected_opt` —
+/// after enough ledgers have elapsed that the entry would have gone stale
+/// without the TTL-extension fix.
+///
+/// Strategy
+/// ────────
+/// Rather than relying on an archived-entry error to prove the fix works
+/// (which in soroban-sdk v21 also archives the contract instance, making
+/// further calls impossible), this test uses `get_ttl` directly to verify
+/// that the entry's TTL was extended to `TOTAL_TTL_TARGET` by `collect_fee`,
+/// not just to the bare `min_persistent_entry_ttl`.
+///
+/// Steps:
+/// 1. Set `min_persistent_entry_ttl = 5_000` and a large `max_entry_ttl`.
+///    New persistent entries (including KEY_TOTAL) start at TTL = 4_999.
+/// 2. Call `collect_fee` to write the entry; the `extend_ttl` inside it
+///    should bump the TTL up to `TOTAL_TTL_TARGET` (2_073_600).
+/// 3. Verify via `env.as_contract` + `get_ttl` that the TTL equals
+///    `TOTAL_TTL_TARGET`, not 4_999 — direct proof the extension ran.
+/// 4. Advance the ledger by `ADVANCE` ledgers; confirm via `as_contract`
+///    that the TTL decayed by exactly `ADVANCE` and is still above
+///    `TOTAL_TTL_THRESHOLD` — the entry is alive and well past what
+///    the un-extended TTL would have been.
+/// 5. Verify the public API (`get_total_collected` + `get_total_collected_opt`)
+///    also returns the correct value, confirming end-to-end correctness.
+#[test]
+fn test_total_collected_ttl_extended_across_ledgers() {
+    use crate::{KEY_TOTAL, TOTAL_TTL_TARGET, TOTAL_TTL_THRESHOLD};
+
+    // ── Environment setup ────────────────────────────────────────────────
+    // A moderate min_persistent_entry_ttl chosen so that it's clearly
+    // distinct from TOTAL_TTL_TARGET: without extend_ttl entries would have
+    // a TTL of INITIAL_TTL_SETTING - 1 = 4_999; our fix sets it to
+    // TOTAL_TTL_TARGET = 2_073_600.
+    const INITIAL_TTL_SETTING: u32 = 5_000;
+    // Ledger advance: enough to decay past INITIAL_TTL_SETTING and
+    // prove the entry is alive purely because of extend_ttl.
+    const ADVANCE: u32 = 10_000; // > INITIAL_TTL_SETTING, << TOTAL_TTL_TARGET
+
+    let env = Env::default();
+    env.mock_all_auths();
+
+    env.ledger().with_mut(|li| {
+        li.sequence_number = 100_000;
+        li.min_persistent_entry_ttl = INITIAL_TTL_SETTING;
+        // max_entry_ttl must accommodate TOTAL_TTL_TARGET (2_073_600).
+        li.max_entry_ttl = 3_000_000;
+    });
+
+    // ── Contract + token setup ───────────────────────────────────────────
+    let admin = Address::generate(&env);
+    let treasury = Address::generate(&env);
+    let contract_id = env.register_contract(None, FeeCollectorContract);
+    let client = FeeCollectorContractClient::new(&env, &contract_id);
+    client.initialize(&admin, &treasury);
+
+    let token_admin = Address::generate(&env);
+    let token_id = env.register_stellar_asset_contract_v2(token_admin.clone());
+    let token = token_id.address();
+
+    // ── Step 1: collect fee and verify TTL was set to TOTAL_TTL_TARGET ───
+    mint(&env, &token, &token_admin, &client.address, 1_000);
+    client.collect_fee(&token, &1_000i128);
+
+    // Public API sanity check.
+    assert_eq!(client.get_total_collected(&token), 1_000);
+    assert_eq!(client.get_total_collected_opt(&token), Some(1_000i128));
+
+    // The critical assertion: the TTL must equal TOTAL_TTL_TARGET, not the
+    // bare `min_persistent_entry_ttl - 1 = 4_999`. If extend_ttl wasn't
+    // called, this would be 4_999.
+    let total_key = (KEY_TOTAL, token.clone());
+    env.as_contract(&contract_id, || {
+        let ttl = env.storage().persistent().get_ttl(&total_key);
+        assert_eq!(
+            ttl,
+            TOTAL_TTL_TARGET,
+            "TTL immediately after collect_fee must be TOTAL_TTL_TARGET \
+             ({TOTAL_TTL_TARGET}), not bare min_persistent_entry_ttl - 1 \
+             ({} - 1). Got: {ttl}",
+            INITIAL_TTL_SETTING,
+        );
+    });
+
+    // Also extend the instance's TTL so it survives the ledger advance
+    // below (the instance is a separate persistent entry that collect_fee
+    // doesn't touch; we extend it here so the contract remains callable
+    // after we advance the ledger).
+    env.as_contract(&contract_id, || {
+        env.storage()
+            .instance()
+            .extend_ttl(INITIAL_TTL_SETTING, ADVANCE * 2);
+    });
+
+    // ── Step 2: advance the ledger past the un-extended TTL ──────────────
+    // Without extend_ttl the KEY_TOTAL entry would have TTL = 4_999 and
+    // would be archived after 5_000 ledgers. We advance by ADVANCE = 10_000,
+    // which is well past that. The entry must still be alive.
+    env.ledger().with_mut(|li| {
+        li.sequence_number = 100_000 + ADVANCE;
+    });
+
+    // ── Step 3: verify the entry is still alive with a decayed TTL ───────
+    env.as_contract(&contract_id, || {
+        // Entry must be present and hold the correct value.
+        let stored: Option<i128> = env.storage().persistent().get(&total_key);
+        assert_eq!(
+            stored,
+            Some(1_000i128),
+            "KEY_TOTAL entry must still be Some(1000) after {ADVANCE} ledgers; \
+             without the extend_ttl fix it would have been archived after \
+             {} ledgers",
+            INITIAL_TTL_SETTING,
+        );
+
+        // Remaining TTL must have decayed by exactly ADVANCE, and must
+        // still be far above TOTAL_TTL_THRESHOLD.
+        let expected_ttl = TOTAL_TTL_TARGET - ADVANCE;
+        let ttl_after = env.storage().persistent().get_ttl(&total_key);
+        assert_eq!(
+            ttl_after,
+            expected_ttl,
+            "TTL after {ADVANCE} ledgers must be TOTAL_TTL_TARGET - ADVANCE \
+             ({TOTAL_TTL_TARGET} - {ADVANCE} = {expected_ttl}). Got: {ttl_after}",
+        );
+        assert!(
+            ttl_after > TOTAL_TTL_THRESHOLD,
+            "remaining TTL ({ttl_after}) must still exceed TOTAL_TTL_THRESHOLD \
+             ({TOTAL_TTL_THRESHOLD})",
+        );
+    });
+
+    // ── Step 4: public API still works after the advance ─────────────────
+    assert_eq!(
+        client.get_total_collected(&token),
+        1_000,
+        "get_total_collected must return 1000, not 0, after {ADVANCE} ledgers",
+    );
+    assert_eq!(
+        client.get_total_collected_opt(&token),
+        Some(1_000i128),
+        "get_total_collected_opt must return Some(1000), not None, \
+         after {ADVANCE} ledgers",
+    );
+
+    // ── Step 5: second collect_fee accumulates and re-extends the TTL ────
+    mint(&env, &token, &token_admin, &client.address, 500);
+    client.collect_fee(&token, &500i128);
+    assert_eq!(client.get_total_collected(&token), 1_500);
+    assert_eq!(client.get_total_collected_opt(&token), Some(1_500i128));
+}

fee_collector/test_snapshots/test/test_collect_fee_updates_total.1.json

@@ -249,7 +249,7 @@
             },
             "ext": "v0"
           },
-          4095
+          2073600
         ]
       ],
       [