From 44a1be10a3a836156f980abd89af31fa0a27f4b3 Mon Sep 17 00:00:00 2001 From: MoonBoi9001 Date: Thu, 14 May 2026 20:20:16 +0800 Subject: [PATCH 1/4] fix(dips): collect after payer-side cancel instead of double-canceling When the payer cancels first via the on-chain contract, the agent's own opt-out rule from closing the allocation routes the agreement back through a second cancel. That cancel reverts, the function returns early, and the indexer is never paid for the active period. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../src/indexing-fees/__tests__/dips.test.ts | 86 +++++++++++++++++++ .../indexer-common/src/indexing-fees/dips.ts | 71 +++++++++------ 2 files changed, 132 insertions(+), 25 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 54abc9733..8ab88b5fb 100644 --- a/packages/indexer-common/src/indexing-fees/__tests__/dips.test.ts +++ b/packages/indexer-common/src/indexing-fees/__tests__/dips.test.ts @@ -803,6 +803,92 @@ describe('DipsManager', () => { expect(cancelSpy).not.toHaveBeenCalled() }) + + test('skips CanceledByPayer agreements even when a NEVER rule exists', async () => { + // Reproduces the bug where the payer canceled on-chain first + // (via dipper) and the agent's own NEVER rule for the closed + // allocation would otherwise route the agreement back through + // cancelAgreement, which would attempt a redundant on-chain + // cancel and skip the final collection. + await managementModels.IndexingRule.create({ + identifier: testDeploymentId, + identifierType: SubgraphIdentifierType.DEPLOYMENT, + decisionBasis: IndexingDecisionBasis.NEVER, + requireSupported: true, + safety: true, + protocolNetwork: 'eip155:421614', + allocationAmount: '0', + }) + + const cancelSpy = jest + .spyOn(dipsManager, 'cancelAgreement') + .mockResolvedValue(true) + + const canceledByPayer: SubgraphIndexingAgreement = { + ...mockAgreement, + state: 'CanceledByPayer', + } + + await dipsManager.cancelBlocklistedAgreements([canceledByPayer]) + + expect(cancelSpy).not.toHaveBeenCalled() + }) + }) + + describe('cancelAgreement', () => { + const mockAgreement: SubgraphIndexingAgreement = { + id: '0x123e4567e89b12d3a456426614174000', + allocationId: '0xabcd47df40c29949a75a6693c77834c00b8ad626', + subgraphDeploymentId: 'QmTZ8ejXJxRo7vDBS4uwqBeGoxLSWbhaA7oXa1RvxunLy7', + state: 'Accepted', + lastCollectionAt: '0', + endsAt: '9999999999', + maxInitialTokens: '1000', + maxOngoingTokensPerSecond: '100', + tokensPerSecond: '10', + tokensPerEntityPerSecond: '1', + minSecondsPerCollection: 60, + maxSecondsPerCollection: 300, + canceledAt: '0', + } + + test('skips on-chain cancel for CanceledByPayer agreements and still calls final collect', async () => { + // Defense-in-depth for the case where any caller passes an + // already-canceled agreement: skip the doomed on-chain cancel, + // proceed to the final collect so the indexer gets paid for the + // period the agreement was active. + const cancelCallSpy = jest.fn() + ;( + dipsManager as unknown as { + network: { contracts: { SubgraphService: { cancelIndexingAgreement: jest.Mock } } } + } + ).network.contracts.SubgraphService = { + cancelIndexingAgreement: cancelCallSpy, + } as never + + const tryCollectSpy = jest + .spyOn(dipsManager as unknown as { tryCollectAgreement: jest.Mock }, 'tryCollectAgreement') + .mockResolvedValue('collected') + + ;( + dipsManager as unknown as { + network: { networkProvider: { getBlockNumber: jest.Mock } } + } + ).network.networkProvider = { + getBlockNumber: jest.fn().mockResolvedValue(123), + } as never + + const canceledByPayer: SubgraphIndexingAgreement = { + ...mockAgreement, + state: 'CanceledByPayer', + } + + const result = await dipsManager.cancelAgreement(canceledByPayer.id, canceledByPayer) + + expect(cancelCallSpy).not.toHaveBeenCalled() + expect(tryCollectSpy).toHaveBeenCalledTimes(1) + expect(result).toBe(true) + }) }) }) }) diff --git a/packages/indexer-common/src/indexing-fees/dips.ts b/packages/indexer-common/src/indexing-fees/dips.ts index c9e4837d5..a35c90e08 100644 --- a/packages/indexer-common/src/indexing-fees/dips.ts +++ b/packages/indexer-common/src/indexing-fees/dips.ts @@ -472,36 +472,47 @@ export class DipsManager { agreementId, }) - // Step 1: Cancel on-chain + // Step 1: Cancel on-chain — but only if the agreement is not already + // canceled. The payer can cancel via RecurringCollector before we get + // here (e.g., when Studio shrinks the indexer set), in which case + // re-canceling reverts with `InvalidAgreementState` and we'd lose the + // chance to collect for the period the indexer was active. Skip + // straight to Step 2 in that case. const indexerAddress = this.network.specification.indexerOptions.address - try { - const receipt = await this.network.transactionManager.executeTransaction( - async () => - this.network.contracts.SubgraphService.cancelIndexingAgreement.estimateGas( - indexerAddress, - agreementId, - ), - async (gasLimit) => - this.network.contracts.SubgraphService.cancelIndexingAgreement( - indexerAddress, - agreementId, - { gasLimit }, - ), - logger.child({ function: 'SubgraphService.cancelIndexingAgreement' }), + if (agreement.state === 'CanceledByPayer') { + logger.info( + 'Agreement already canceled on-chain by payer; skipping cancel call, proceeding to final collection', ) + } else { + try { + const receipt = await this.network.transactionManager.executeTransaction( + async () => + this.network.contracts.SubgraphService.cancelIndexingAgreement.estimateGas( + indexerAddress, + agreementId, + ), + async (gasLimit) => + this.network.contracts.SubgraphService.cancelIndexingAgreement( + indexerAddress, + agreementId, + { gasLimit }, + ), + logger.child({ function: 'SubgraphService.cancelIndexingAgreement' }), + ) - if (receipt === 'paused' || receipt === 'unauthorized') { - logger.warn('Cannot cancel: network paused or unauthorized') + if (receipt === 'paused' || receipt === 'unauthorized') { + logger.warn('Cannot cancel: network paused or unauthorized') + return false + } + + logger.info('Successfully cancelled agreement on-chain', { + txHash: receipt.hash, + }) + } catch (err) { + const errorMsg = err instanceof Error ? err.message : String(err) + logger.error('Failed to cancel agreement on-chain', { error: errorMsg }) return false } - - logger.info('Successfully cancelled agreement on-chain', { - txHash: receipt.hash, - }) - } catch (err) { - const errorMsg = err instanceof Error ? err.message : String(err) - logger.error('Failed to cancel agreement on-chain', { error: errorMsg }) - return false } // Step 2: Best-effort final collection @@ -536,6 +547,16 @@ export class DipsManager { }) for (const agreement of agreements) { + // Skip agreements the payer has already canceled on-chain. They no + // longer need a "cancel" — they need a final collect, which the + // regular collection loop handles. Routing them through + // cancelAgreement instead would attempt a redundant on-chain cancel + // (reverting on `InvalidAgreementState`) and never reach the + // best-effort collect step inside cancelAgreement, leaving the + // indexer unpaid for the active period. + if (agreement.state === 'CanceledByPayer') { + continue + } const subgraphDeploymentID = new SubgraphDeploymentID( agreement.subgraphDeploymentId, ) From 2cd130dab09e0bbe660bbf5e0ae6be62ae306a5e Mon Sep 17 00:00:00 2001 From: MoonBoi9001 Date: Thu, 14 May 2026 20:24:54 +0800 Subject: [PATCH 2/4] style(dips): apply prettier formatting to new test cases CI's format check rejected the previous commit because the new test assertions exceeded the line-length budget. No behaviour change. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../src/indexing-fees/__tests__/dips.test.ts | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 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 8ab88b5fb..2b6d40564 100644 --- a/packages/indexer-common/src/indexing-fees/__tests__/dips.test.ts +++ b/packages/indexer-common/src/indexing-fees/__tests__/dips.test.ts @@ -860,14 +860,19 @@ describe('DipsManager', () => { const cancelCallSpy = jest.fn() ;( dipsManager as unknown as { - network: { contracts: { SubgraphService: { cancelIndexingAgreement: jest.Mock } } } + network: { + contracts: { SubgraphService: { cancelIndexingAgreement: jest.Mock } } + } } ).network.contracts.SubgraphService = { cancelIndexingAgreement: cancelCallSpy, } as never const tryCollectSpy = jest - .spyOn(dipsManager as unknown as { tryCollectAgreement: jest.Mock }, 'tryCollectAgreement') + .spyOn( + dipsManager as unknown as { tryCollectAgreement: jest.Mock }, + 'tryCollectAgreement', + ) .mockResolvedValue('collected') ;( @@ -883,7 +888,10 @@ describe('DipsManager', () => { state: 'CanceledByPayer', } - const result = await dipsManager.cancelAgreement(canceledByPayer.id, canceledByPayer) + const result = await dipsManager.cancelAgreement( + canceledByPayer.id, + canceledByPayer, + ) expect(cancelCallSpy).not.toHaveBeenCalled() expect(tryCollectSpy).toHaveBeenCalledTimes(1) From cf15313a82e20fe8260da0138569be6b2c72862c Mon Sep 17 00:00:00 2001 From: MoonBoi9001 Date: Thu, 14 May 2026 20:27:09 +0800 Subject: [PATCH 3/4] style(dips): tighten new comments to match this file's convention Existing comments in the file cap at 4 lines; the additions ran to 6-7. Trims both to two lines while keeping the why intact. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../indexer-common/src/indexing-fees/dips.ts | 19 +++++-------------- 1 file changed, 5 insertions(+), 14 deletions(-) diff --git a/packages/indexer-common/src/indexing-fees/dips.ts b/packages/indexer-common/src/indexing-fees/dips.ts index a35c90e08..42b0550a8 100644 --- a/packages/indexer-common/src/indexing-fees/dips.ts +++ b/packages/indexer-common/src/indexing-fees/dips.ts @@ -472,16 +472,12 @@ export class DipsManager { agreementId, }) - // Step 1: Cancel on-chain — but only if the agreement is not already - // canceled. The payer can cancel via RecurringCollector before we get - // here (e.g., when Studio shrinks the indexer set), in which case - // re-canceling reverts with `InvalidAgreementState` and we'd lose the - // chance to collect for the period the indexer was active. Skip - // straight to Step 2 in that case. + // Step 1: Cancel on-chain (skipped if payer already canceled — a second + // cancel reverts on `InvalidAgreementState` and would skip the final collect). const indexerAddress = this.network.specification.indexerOptions.address if (agreement.state === 'CanceledByPayer') { logger.info( - 'Agreement already canceled on-chain by payer; skipping cancel call, proceeding to final collection', + 'Payer already canceled on-chain; skipping cancel, proceeding to final collection', ) } else { try { @@ -547,13 +543,8 @@ export class DipsManager { }) for (const agreement of agreements) { - // Skip agreements the payer has already canceled on-chain. They no - // longer need a "cancel" — they need a final collect, which the - // regular collection loop handles. Routing them through - // cancelAgreement instead would attempt a redundant on-chain cancel - // (reverting on `InvalidAgreementState`) and never reach the - // best-effort collect step inside cancelAgreement, leaving the - // indexer unpaid for the active period. + // Already-canceled agreements need a final collect, not another cancel — + // the regular collection loop handles them. if (agreement.state === 'CanceledByPayer') { continue } From 7d9272d8ccb2a5c6bc74ed8fde4c72d27820b764 Mon Sep 17 00:00:00 2001 From: MoonBoi9001 Date: Thu, 14 May 2026 23:29:23 +0800 Subject: [PATCH 4/4] refactor(dips): tighten the payer-cancel collect path after review Cross-link the two CanceledByPayer guards, document the return contract of cancelAgreement, and raise the lost-fees log to error with the deployment id attached. Consolidate the duplicate test block and add the failure-path coverage that was missing. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../src/indexing-fees/__tests__/dips.test.ts | 120 ++++++++---------- .../indexer-common/src/indexing-fees/dips.ts | 8 +- 2 files changed, 62 insertions(+), 66 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 2b6d40564..5af2b0f64 100644 --- a/packages/indexer-common/src/indexing-fees/__tests__/dips.test.ts +++ b/packages/indexer-common/src/indexing-fees/__tests__/dips.test.ts @@ -670,6 +670,62 @@ describe('DipsManager', () => { dipsManager.collectionTracker.isReadyForCollection(mockAgreement.id, 0), ).toBe(true) }) + + test('CanceledByPayer skips on-chain cancel and only runs final collect', async () => { + const canceledByPayer = { ...mockAgreement, state: 'CanceledByPayer' as const } + const mockCollectReceipt = { hash: '0xcollect456' } + + network.transactionManager.executeTransaction = jest + .fn() + .mockResolvedValueOnce(mockCollectReceipt) // only collect, no 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( + canceledByPayer.id, + canceledByPayer, + ) + + expect(result).toBe(true) + expect(network.transactionManager.executeTransaction).toHaveBeenCalledTimes(1) + }) + + test('CanceledByPayer with failing final collect still returns true', async () => { + const canceledByPayer = { ...mockAgreement, state: 'CanceledByPayer' as const } + + network.transactionManager.executeTransaction = jest + .fn() + .mockRejectedValueOnce(new Error('collect failed')) + + 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( + canceledByPayer.id, + canceledByPayer, + ) + + expect(result).toBe(true) + expect(network.transactionManager.executeTransaction).toHaveBeenCalledTimes(1) + expect( + dipsManager.collectionTracker.isReadyForCollection(canceledByPayer.id, 0), + ).toBe(true) + }) }) describe('cleanupFinishedAgreement', () => { @@ -834,69 +890,5 @@ describe('DipsManager', () => { expect(cancelSpy).not.toHaveBeenCalled() }) }) - - describe('cancelAgreement', () => { - const mockAgreement: SubgraphIndexingAgreement = { - id: '0x123e4567e89b12d3a456426614174000', - allocationId: '0xabcd47df40c29949a75a6693c77834c00b8ad626', - subgraphDeploymentId: 'QmTZ8ejXJxRo7vDBS4uwqBeGoxLSWbhaA7oXa1RvxunLy7', - state: 'Accepted', - lastCollectionAt: '0', - endsAt: '9999999999', - maxInitialTokens: '1000', - maxOngoingTokensPerSecond: '100', - tokensPerSecond: '10', - tokensPerEntityPerSecond: '1', - minSecondsPerCollection: 60, - maxSecondsPerCollection: 300, - canceledAt: '0', - } - - test('skips on-chain cancel for CanceledByPayer agreements and still calls final collect', async () => { - // Defense-in-depth for the case where any caller passes an - // already-canceled agreement: skip the doomed on-chain cancel, - // proceed to the final collect so the indexer gets paid for the - // period the agreement was active. - const cancelCallSpy = jest.fn() - ;( - dipsManager as unknown as { - network: { - contracts: { SubgraphService: { cancelIndexingAgreement: jest.Mock } } - } - } - ).network.contracts.SubgraphService = { - cancelIndexingAgreement: cancelCallSpy, - } as never - - const tryCollectSpy = jest - .spyOn( - dipsManager as unknown as { tryCollectAgreement: jest.Mock }, - 'tryCollectAgreement', - ) - .mockResolvedValue('collected') - - ;( - dipsManager as unknown as { - network: { networkProvider: { getBlockNumber: jest.Mock } } - } - ).network.networkProvider = { - getBlockNumber: jest.fn().mockResolvedValue(123), - } as never - - const canceledByPayer: SubgraphIndexingAgreement = { - ...mockAgreement, - state: 'CanceledByPayer', - } - - const result = await dipsManager.cancelAgreement( - canceledByPayer.id, - canceledByPayer, - ) - - expect(cancelCallSpy).not.toHaveBeenCalled() - expect(tryCollectSpy).toHaveBeenCalledTimes(1) - expect(result).toBe(true) - }) - }) }) }) diff --git a/packages/indexer-common/src/indexing-fees/dips.ts b/packages/indexer-common/src/indexing-fees/dips.ts index 42b0550a8..a5470b43b 100644 --- a/packages/indexer-common/src/indexing-fees/dips.ts +++ b/packages/indexer-common/src/indexing-fees/dips.ts @@ -463,6 +463,8 @@ export class DipsManager { } } + // Returns true once the final-collect step ran (whether we canceled on-chain + // or the payer already had); false only when our own on-chain cancel failed. async cancelAgreement( agreementId: string, agreement: SubgraphIndexingAgreement, @@ -518,7 +520,8 @@ export class DipsManager { logger.info('Final collection succeeded after cancel') } catch (err) { const errorMsg = err instanceof Error ? err.message : String(err) - logger.warn('Final collection after cancel failed, fees may be lost', { + logger.error('Final collection after cancel failed, fees may be lost', { + deployment: agreement.subgraphDeploymentId, error: errorMsg, }) } @@ -544,7 +547,8 @@ export class DipsManager { for (const agreement of agreements) { // Already-canceled agreements need a final collect, not another cancel — - // the regular collection loop handles them. + // the regular collection loop handles them. cancelAgreement also guards + // this state internally as defense-in-depth. if (agreement.state === 'CanceledByPayer') { continue }