From 671ca979c914d9fe86cad4f28095632dc540ffc1 Mon Sep 17 00:00:00 2001 From: Miguel de Elias Date: Tue, 2 Jun 2026 18:09:09 -0300 Subject: [PATCH] fix: collect before cancelling an agreement --- .../src/indexing-fees/__tests__/dips.test.ts | 42 ++++++++++++------- .../indexer-common/src/indexing-fees/dips.ts | 33 +++++++-------- 2 files changed, 43 insertions(+), 32 deletions(-) diff --git a/packages/indexer-common/src/indexing-fees/__tests__/dips.test.ts b/packages/indexer-common/src/indexing-fees/__tests__/dips.test.ts index 3b5808a3d..6d9ca690e 100644 --- a/packages/indexer-common/src/indexing-fees/__tests__/dips.test.ts +++ b/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,7 +773,7 @@ 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( @@ -781,27 +781,41 @@ describe('DipsManager', () => { ).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) diff --git a/packages/indexer-common/src/indexing-fees/dips.ts b/packages/indexer-common/src/indexing-fees/dips.ts index c554ea66a..615a37655 100644 --- a/packages/indexer-common/src/indexing-fees/dips.ts +++ b/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)