fix: cap maxSecondsPerCollection instead of reverting

RembrandtK · RembrandtK · commit fa9951427d70 · 2026-03-01T23:06:24.000Z
Replace the hard revert (RecurringCollectorCollectionTooLate) with a
Math.min cap in _getCollectionInfo. Collections past maxSecondsPerCollection
now succeed with tokens capped at maxSecondsPerCollection worth of service,
rather than failing entirely.

Changes:
- _getCollectionInfo caps elapsed seconds at maxSecondsPerCollection
- Remove RecurringCollectorCollectionTooLate error from interface
- Replace test_Collect_Revert_WhenCollectingTooLate with
  test_Collect_OK_WhenCollectingPastMaxSeconds
- Update maxSecondsPerCollection NatSpec to reflect cap semantics
- Fix zero-token test to use correct _sensibleAuthorizeAndAccept API
diff --git a/packages/horizon/contracts/payments/collectors/MaxSecondsPerCollectionCap.md b/packages/horizon/contracts/payments/collectors/MaxSecondsPerCollectionCap.md
@@ -0,0 +1,56 @@
+# maxSecondsPerCollection: Cap, Not Deadline
+
+## Problem
+
+`_requireValidCollect` treats `maxSecondsPerCollection` as a hard deadline:
+
+```solidity
+require(
+    _collectionSeconds <= _agreement.maxSecondsPerCollection,
+    RecurringCollectorCollectionTooLate(...)
+);
+uint256 maxTokens = _agreement.maxOngoingTokensPerSecond * _collectionSeconds;
+```
+
+If the indexer collects even 1 second past `maxSecondsPerCollection`, the transaction reverts and the agreement becomes permanently stuck. The only recovery is a zero-token collect that bypasses temporal validation entirely (since `_requireValidCollect` is inside `if (tokens != 0)`), which works but is an unnatural mechanism.
+
+## Fix
+
+Cap `collectionSeconds` at `maxSecondsPerCollection` in `_getCollectionInfo`, so all callers (RC's `_collect` and SS's `IndexingAgreement.collect`) receive consistent capped seconds:
+
+```solidity
+uint256 elapsed = collectionEnd - collectionStart;
+return (true, Math.min(elapsed, uint256(_agreement.maxSecondsPerCollection)), ...);
+```
+
+The payer's per-collection exposure is still bounded by `maxOngoingTokensPerSecond * maxSecondsPerCollection`. The indexer can collect after the window closes, but receives no more tokens than if they had collected exactly at the deadline.
+
+## Why this is correct
+
+1. **`_getMaxNextClaim` already caps.** The view function (used by escrow to compute worst-case exposure) clamps `windowSeconds` at `maxSecondsPerCollection` rather than returning 0. The mutation function should be consistent.
+
+2. **`collectionSeconds` is derived from on-chain state**, not caller-supplied. The indexer's only leverage is _when_ they call. Capping means they can't extract more by waiting longer.
+
+3. **No stuck agreements.** A missed window no longer requires cancellation or a zero-token hack to recover.
+
+4. **`minSecondsPerCollection` is unaffected.** If elapsed time exceeds `maxSecondsPerCollection`, it trivially exceeds `minSecondsPerCollection` (since `max > min` is enforced at accept time).
+
+5. **Initial tokens preserved.** `maxInitialTokens` is added on top of the capped ongoing amount on first collection. With a hard deadline, a late first collection reverts and the indexer loses both the initial bonus and the ongoing amount — misaligning incentives. With a cap, the initial bonus is always available.
+
+6. **Late collection loses unclaimed seconds, not ability to collect.** After a capped collection, `lastCollectionAt` resets to `block.timestamp`, not `lastCollectionAt + maxSecondsPerCollection`. The indexer permanently loses tokens for the gap beyond the cap. This incentivizes timely collection without the cliff-edge of a hard revert.
+
+## Zero-token temporal validation enforced
+
+`_requireValidCollect` was previously inside `if (tokens != 0)`, allowing zero-token collections to update `lastCollectionAt` without temporal checks. With the cap in place there is no legitimate bypass scenario, so temporal validation now runs unconditionally.
+
+This also makes `lastCollectionAt` (publicly readable via `getAgreement`) trustworthy as a liveness signal. Previously it could be advanced to `block.timestamp` without any real collection. Now it can only be updated through a validated collection, making it reliable for external consumers (e.g. payers or SAM operators checking indexer activity to decide whether to cancel).
+
+## Zero-POI special case removed
+
+The old code special-cased `entities == 0 && poi == bytes32(0)` to force `tokens = 0`, bypassing `_tokensToCollect` and RC temporal validation. This existed as a reset mechanism for stuck agreements. With the cap, there are no stuck agreements, so the special case is removed. Every collection now goes through `_tokensToCollect` and RC validation uniformly, and every POI is disputable.
+
+## Contrast with indexing rewards
+
+Indexing rewards require a zero-POI "heartbeat" to keep allocations alive because reward rates change per epoch and snapshots are influenced by other participants' activity. That reset mechanism exists because the system is inherently snapshot-driven.
+
+RCA indexing fees have no snapshots. The rate (`tokensPerSecond`, `tokensPerEntityPerSecond`) is fixed at agreement accept/update time. No external state changes the per-second rate between collections. The amount owed for N seconds of service is deterministic regardless of when collection happens, so capping is strictly correct — there is no reason to penalize a late collection beyond limiting it to `maxSecondsPerCollection` worth of tokens.
diff --git a/packages/horizon/contracts/payments/collectors/RecurringCollector.sol b/packages/horizon/contracts/payments/collectors/RecurringCollector.sol
@@ -459,16 +459,7 @@ contract RecurringCollector is EIP712, GraphDirectory, Authorizable, IRecurringC
                 )
             );
         }
-        require(
-            // solhint-disable-next-line gas-strict-inequalities
-            _collectionSeconds <= _agreement.maxSecondsPerCollection,
-            RecurringCollectorCollectionTooLate(
-                _agreementId,
-                uint64(_collectionSeconds),
-                _agreement.maxSecondsPerCollection
-            )
-        );
-
+        // _collectionSeconds is already capped at maxSecondsPerCollection by _getCollectionInfo
         uint256 maxTokens = _agreement.maxOngoingTokensPerSecond * _collectionSeconds;
         maxTokens += _agreement.lastCollectionAt == 0 ? _agreement.maxInitialTokens : 0;
 
@@ -631,7 +622,12 @@ contract RecurringCollector is EIP712, GraphDirectory, Authorizable, IRecurringC
             return (false, 0, AgreementNotCollectableReason.ZeroCollectionSeconds);
         }
 
-        return (true, collectionEnd - collectionStart, AgreementNotCollectableReason.None);
+        uint256 elapsed = collectionEnd - collectionStart;
+        return (
+            true,
+            Math.min(elapsed, uint256(_agreement.maxSecondsPerCollection)),
+            AgreementNotCollectableReason.None
+        );
     }
 
     /**
diff --git a/packages/horizon/test/unit/payments/recurring-collector/collect.t.sol b/packages/horizon/test/unit/payments/recurring-collector/collect.t.sol
@@ -168,7 +168,7 @@ contract RecurringCollectorCollectTest is RecurringCollectorSharedTest {
         _recurringCollector.collect(_paymentType(fuzzy.unboundedPaymentType), data);
     }
 
-    function test_Collect_Revert_WhenCollectingTooLate(
+    function test_Collect_OK_WhenCollectingPastMaxSeconds(
         FuzzyTestCollect calldata fuzzy,
         uint256 unboundedFirstCollectionSeconds,
         uint256 unboundedSecondCollectionSeconds
@@ -177,16 +177,15 @@ contract RecurringCollectorCollectTest is RecurringCollectorSharedTest {
             fuzzy.fuzzyTestAccept
         );
 
-        // skip to collectable time
-
+        // First valid collection to establish lastCollectionAt
         skip(
             boundSkip(
                 unboundedFirstCollectionSeconds,
                 accepted.rca.minSecondsPerCollection,
                 accepted.rca.maxSecondsPerCollection
             )
         );
-        bytes memory data = _generateCollectData(
+        bytes memory firstData = _generateCollectData(
             _generateCollectParams(
                 accepted.rca,
                 agreementId,
@@ -195,35 +194,41 @@ contract RecurringCollectorCollectTest is RecurringCollectorSharedTest {
                 fuzzy.collectParams.dataServiceCut
             )
         );
-        vm.prank(accepted.rca.dataService);
-        _recurringCollector.collect(_paymentType(fuzzy.unboundedPaymentType), data);
+        vm.prank(acceptedRca.dataService);
+        _recurringCollector.collect(_paymentType(fuzzy.unboundedPaymentType), firstData);
 
-        // skip beyond collectable time but still within the agreement endsAt
+        // Skip PAST maxSecondsPerCollection (but still within agreement endsAt)
         uint256 collectionSeconds = boundSkip(
             unboundedSecondCollectionSeconds,
             accepted.rca.maxSecondsPerCollection + 1,
             accepted.rca.endsAt - block.timestamp
         );
         skip(collectionSeconds);
 
-        data = _generateCollectData(
-            _generateCollectParams(
-                accepted.rca,
-                agreementId,
-                fuzzy.collectParams.collectionId,
-                bound(fuzzy.collectParams.tokens, 1, type(uint256).max),
-                fuzzy.collectParams.dataServiceCut
-            )
+        // Request more tokens than the cap allows
+        uint256 cappedMaxTokens = acceptedRca.maxOngoingTokensPerSecond * acceptedRca.maxSecondsPerCollection;
+        uint256 requestedTokens = cappedMaxTokens + 1;
+
+        IRecurringCollector.CollectParams memory collectParams = _generateCollectParams(
+            acceptedRca,
+            agreementId,
+            fuzzy.collectParams.collectionId,
+            requestedTokens,
+            fuzzy.collectParams.dataServiceCut
         );
-        bytes memory expectedErr = abi.encodeWithSelector(
-            IRecurringCollector.RecurringCollectorCollectionTooLate.selector,
+        bytes memory data = _generateCollectData(collectParams);
+
+        // Collection should SUCCEED with tokens capped at maxSecondsPerCollection worth
+        _expectCollectCallAndEmit(
+            acceptedRca,
             agreementId,
-            collectionSeconds,
-            accepted.rca.maxSecondsPerCollection
+            _paymentType(fuzzy.unboundedPaymentType),
+            collectParams,
+            cappedMaxTokens
         );
-        vm.expectRevert(expectedErr);
-        vm.prank(accepted.rca.dataService);
-        _recurringCollector.collect(_paymentType(fuzzy.unboundedPaymentType), data);
+        vm.prank(acceptedRca.dataService);
+        uint256 collected = _recurringCollector.collect(_paymentType(fuzzy.unboundedPaymentType), data);
+        assertEq(collected, cappedMaxTokens, "Tokens should be capped at maxSecondsPerCollection worth");
     }
 
     function test_Collect_OK_WhenCollectingTooMuch(
diff --git a/packages/interfaces/contracts/horizon/IRecurringCollector.sol b/packages/interfaces/contracts/horizon/IRecurringCollector.sol
@@ -59,7 +59,7 @@ interface IRecurringCollector is IAuthorizable, IPaymentsCollector {
      * @param maxOngoingTokensPerSecond The maximum amount of tokens that can be collected per second
      * except for the first collection
      * @param minSecondsPerCollection The minimum amount of seconds that must pass between collections
-     * @param maxSecondsPerCollection The maximum amount of seconds that can pass between collections
+     * @param maxSecondsPerCollection The maximum seconds of service that can be collected in a single collection
      * @param nonce A unique nonce for preventing collisions (user-chosen)
      * @param metadata Arbitrary metadata to extend functionality if a data service requires it
      *
@@ -99,7 +99,7 @@ interface IRecurringCollector is IAuthorizable, IPaymentsCollector {
      * @param maxOngoingTokensPerSecond The maximum amount of tokens that can be collected per second
      * except for the first collection
      * @param minSecondsPerCollection The minimum amount of seconds that must pass between collections
-     * @param maxSecondsPerCollection The maximum amount of seconds that can pass between collections
+     * @param maxSecondsPerCollection The maximum seconds of service that can be collected in a single collection
      * @param nonce The nonce for preventing replay attacks (must be current nonce + 1)
      * @param metadata Arbitrary metadata to extend functionality if a data service requires it
      */
@@ -130,7 +130,7 @@ interface IRecurringCollector is IAuthorizable, IPaymentsCollector {
      * @param maxOngoingTokensPerSecond The maximum amount of tokens that can be collected per second
      * except for the first collection
      * @param minSecondsPerCollection The minimum amount of seconds that must pass between collections
-     * @param maxSecondsPerCollection The maximum amount of seconds that can pass between collections
+     * @param maxSecondsPerCollection The maximum seconds of service that can be collected in a single collection
      * @param updateNonce The current nonce for updates (prevents replay attacks)
      * @param canceledAt The timestamp when the agreement was canceled
      * @param state The state of the agreement
@@ -180,7 +180,7 @@ interface IRecurringCollector is IAuthorizable, IPaymentsCollector {
      * @param maxInitialTokens The maximum amount of tokens that can be collected in the first collection
      * @param maxOngoingTokensPerSecond The maximum amount of tokens that can be collected per second
      * @param minSecondsPerCollection The minimum amount of seconds that must pass between collections
-     * @param maxSecondsPerCollection The maximum amount of seconds that can pass between collections
+     * @param maxSecondsPerCollection The maximum seconds of service that can be collected in a single collection
      */
     event AgreementAccepted(
         address indexed dataService,
@@ -224,7 +224,7 @@ interface IRecurringCollector is IAuthorizable, IPaymentsCollector {
      * @param maxInitialTokens The maximum amount of tokens that can be collected in the first collection
      * @param maxOngoingTokensPerSecond The maximum amount of tokens that can be collected per second
      * @param minSecondsPerCollection The minimum amount of seconds that must pass between collections
-     * @param maxSecondsPerCollection The maximum amount of seconds that can pass between collections
+     * @param maxSecondsPerCollection The maximum seconds of service that can be collected in a single collection
      */
     event AgreementUpdated(
         address indexed dataService,
@@ -373,14 +373,6 @@ interface IRecurringCollector is IAuthorizable, IPaymentsCollector {
      */
     error RecurringCollectorCollectionTooSoon(bytes16 agreementId, uint32 secondsSinceLast, uint32 minSeconds);
 
-    /**
-     * @notice Thrown when calling collect() too late
-     * @param agreementId The agreement ID
-     * @param secondsSinceLast Seconds since last collection
-     * @param maxSeconds Maximum seconds between collections
-     */
-    error RecurringCollectorCollectionTooLate(bytes16 agreementId, uint64 secondsSinceLast, uint32 maxSeconds);
-
     /**
      * @notice Thrown when calling update() with an invalid nonce
      * @param agreementId The agreement ID
