fix: collect before cancelling an agreement

Author: Maikol

packages/indexer-common/src/indexing-fees/__tests__/dips.test.ts

@@ -749,15 +749,15 @@ describe('DipsManager', () => {
         })
       })
 
-      test('successful cancel + final collect attempt', async () => {
-        const mockReceipt = { hash: '0xcancel123' }
+      test('pre-cancel collect attempt + successful cancel', async () => {
         const mockCollectReceipt = { hash: '0xcollect456' }
+        const mockReceipt = { hash: '0xcancel123' }
 
-        // Mock cancel transaction
+        // Order: collect (best-effort, before cancel) then the SP cancel.
         network.transactionManager.executeTransaction = jest
           .fn()
+          .mockResolvedValueOnce(mockCollectReceipt) // collect (before cancel)
           .mockResolvedValueOnce(mockReceipt) // cancel
-          .mockResolvedValueOnce(mockCollectReceipt) // collect
 
         // Mock block number and graph node methods for collect
         network.networkProvider.getBlockNumber = jest.fn().mockResolvedValue(100)
@@ -773,35 +773,49 @@ describe('DipsManager', () => {
         const result = await dipsManager.cancelAgreement(mockAgreement.id, mockAgreement)
 
         expect(result).toBe(true)
-        // executeTransaction called twice: once for cancel, once for collect
+        // executeTransaction called twice: once for collect, once for cancel
         expect(network.transactionManager.executeTransaction).toHaveBeenCalledTimes(2)
         // Tracker should be cleaned up (untracked = ready)
         expect(
           dipsManager.collectionTracker.isReadyForCollection(mockAgreement.id, 0),
         ).toBe(true)
       })
 
-      test('cancel fails returns false, no collect attempted', async () => {
-        // Mock cancel transaction failure
+      test('cancel fails returns false (after the pre-cancel collect attempt)', async () => {
+        const mockCollectReceipt = { hash: '0xcollect456' }
+
+        // Collect (before cancel) succeeds, then the on-chain cancel reverts.
         network.transactionManager.executeTransaction = jest
           .fn()
-          .mockRejectedValueOnce(new Error('cancel tx reverted'))
+          .mockResolvedValueOnce(mockCollectReceipt) // collect (before cancel)
+          .mockRejectedValueOnce(new Error('cancel tx reverted')) // cancel
+
+        network.networkProvider.getBlockNumber = jest.fn().mockResolvedValue(100)
+        graphNode.entityCount = jest.fn().mockResolvedValue([250000])
+        graphNode.subgraphFeatures = jest.fn().mockResolvedValue({ network: 'mainnet' })
+        graphNode.blockHashFromNumber = jest.fn().mockResolvedValue('0xblockhash')
+        graphNode.proofOfIndexing = jest
+          .fn()
+          .mockResolvedValue(
+            '0x0000000000000000000000000000000000000000000000000000000000000001',
+          )
 
         const result = await dipsManager.cancelAgreement(mockAgreement.id, mockAgreement)
 
         expect(result).toBe(false)
-        // executeTransaction called only once (for cancel)
-        expect(network.transactionManager.executeTransaction).toHaveBeenCalledTimes(1)
+        // collect attempted first, then the failing cancel
+        expect(network.transactionManager.executeTransaction).toHaveBeenCalledTimes(2)
       })
 
-      test('cancel succeeds but collect fails returns true, tracker still cleaned up', async () => {
+      test('pre-cancel collect fails but cancel succeeds returns true, tracker cleaned up', async () => {
         const mockReceipt = { hash: '0xcancel123' }
 
-        // Mock cancel succeeds
+        // Collect (before cancel) reverts — outside the collection window — then
+        // the SP cancel succeeds anyway.
         network.transactionManager.executeTransaction = jest
           .fn()
+          .mockRejectedValueOnce(new Error('collect failed')) // collect (before cancel)
           .mockResolvedValueOnce(mockReceipt) // cancel succeeds
-          .mockRejectedValueOnce(new Error('collect failed')) // collect fails
 
         // Mock block number and graph node methods
         network.networkProvider.getBlockNumber = jest.fn().mockResolvedValue(100)
@@ -817,7 +831,7 @@ describe('DipsManager', () => {
         const result = await dipsManager.cancelAgreement(mockAgreement.id, mockAgreement)
 
         expect(result).toBe(true)
-        // Tracker should be cleaned up even though collect failed
+        // Tracker should be cleaned up even though the pre-cancel collect failed
         expect(
           dipsManager.collectionTracker.isReadyForCollection(mockAgreement.id, 0),
         ).toBe(true)

packages/indexer-common/src/indexing-fees/dips.ts

@@ -748,13 +748,23 @@ export class DipsManager {
       agreementId,
     })
 
-    // Step 1: Cancel on-chain (skipped if payer already canceled — a second
-    // cancel reverts on `InvalidAgreementState` and would skip the final collect).
+    // Step 1: Best-effort final collection BEFORE cancelling.
+    try {
+      const blockNumber = await this.network.networkProvider.getBlockNumber()
+      await this.tryCollectAgreement(agreement, blockNumber, logger)
+      logger.info('Final collection succeeded before cancel')
+    } catch (err) {
+      const errorMsg = err instanceof Error ? err.message : String(err)
+      logger.warn(
+        'Final collection before cancel failed (likely outside the collection window); proceeding to cancel',
+        { deployment: agreement.subgraphDeploymentId, error: errorMsg },
+      )
+    }
+
+    // Step 2: Cancel on-chain, skipped if payer already canceled.
     const indexerAddress = this.network.specification.indexerOptions.address
     if (agreement.state === 'CanceledByPayer') {
-      logger.info(
-        'Payer already canceled on-chain; skipping cancel, proceeding to final collection',
-      )
+      logger.info('Payer already canceled on-chain; skipping service-provider cancel')
     } else {
       try {
         const receipt = await this.network.transactionManager.executeTransaction(
@@ -787,19 +797,6 @@ export class DipsManager {
       }
     }
 
-    // Step 2: Best-effort final collection
-    try {
-      const blockNumber = await this.network.networkProvider.getBlockNumber()
-      await this.tryCollectAgreement(agreement, blockNumber, logger)
-      logger.info('Final collection succeeded after cancel')
-    } catch (err) {
-      const errorMsg = err instanceof Error ? err.message : String(err)
-      logger.error('Final collection after cancel failed, fees may be lost', {
-        deployment: agreement.subgraphDeploymentId,
-        error: errorMsg,
-      })
-    }
-
     // Step 3: Cleanup
     this.collectionTracker.remove(agreementId)