Skip to content

Commit fcb563d

Browse files
RembrandtKRembrandtK
committed
feat: idempotent accept/update with allocation rebinding
Align re-accept and re-update semantics across RecurringCollector and SubgraphService so duplicate calls with the same signed terms are no-ops rather than reverts, and allow an accepted agreement to be moved to a different allocation. RecurringCollector - accept(): _registerNew short-circuits on matching activeTermsHash and _acceptRegistered short-circuits on Accepted state, so re-accepting the same signed RCA is a no-op. Cancelled agreements still revert — re-accept of a cancelled agreement is never valid. - update(): entry-point short-circuit when activeTermsHash already equals the RCAU hash, skipping deadline and authorization checks on the idempotent path. _validateAndStoreUpdate retains the inner short-circuit for safety. SubgraphService (IndexingAgreement library) - update(): defers state authority to the collector — _isValid replaces _isActive, and an activeTermsHash match short-circuits the SS-side event and terms re-write. - accept(): same-allocation re-accept is an idempotent no-op at the SS layer; different-allocation re-accept rebinds the agreement by clearing the old allocationToActiveAgreementId link and establishing the new one. Enables moving an active agreement to a new allocation when the original is closed. Not strictly required by the audit, but triggered by and aligned with the version-indexed view and state-authority semantics introduced in the preceding L-11 refactor: once the collector owns version identity via activeTermsHash, matching that hash on re-entry becomes the natural idempotence signal, and the SS layer follows suit.
1 parent 9d92690 commit fcb563d

7 files changed

Lines changed: 227 additions & 72 deletions

File tree

‎packages/horizon/contracts/payments/collectors/RecurringCollector.sol‎

Lines changed: 13 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -308,6 +308,11 @@ contract RecurringCollector is
308308
*/
309309
function _acceptRegistered(bytes16 agreementId) private {
310310
AgreementData storage agreement = _getAgreementStorage(agreementId);
311+
312+
// Idempotent: already accepted.
313+
if (agreement.state == AgreementState.Accepted) return;
314+
315+
// Canceled state (or any non-NotAccepted state) blocks acceptance.
311316
require(
312317
agreement.state == AgreementState.NotAccepted,
313318
RecurringCollectorAgreementIncorrectState(agreementId, agreement.state)
@@ -364,15 +369,19 @@ contract RecurringCollector is
364369
function update(RecurringCollectionAgreementUpdate calldata rcau, bytes calldata signature) external whenNotPaused {
365370
AgreementData storage agreement = _requireValidUpdateTarget(rcau.agreementId);
366371

372+
bytes32 rcauHash = _hashRCAU(rcau);
373+
374+
// Idempotent: already at this version (state is Accepted per _requireValidUpdateTarget).
375+
// Skip deadline + auth since no state change happens.
376+
if (agreement.activeTermsHash == rcauHash) return;
377+
367378
/* solhint-disable gas-strict-inequalities */
368379
require(
369380
rcau.deadline >= block.timestamp,
370381
RecurringCollectorAgreementDeadlineElapsed(block.timestamp, rcau.deadline)
371382
);
372383
/* solhint-enable gas-strict-inequalities */
373384

374-
bytes32 rcauHash = _hashRCAU(rcau);
375-
376385
_requireAuthorization(agreement.payer, rcauHash, signature, rcau.agreementId, OFFER_TYPE_UPDATE);
377386

378387
_validateAndStoreUpdate(agreement, rcau, rcauHash);
@@ -1105,9 +1114,9 @@ contract RecurringCollector is
11051114
_rcau.maxOngoingTokensPerSecond * _rcau.maxSecondsPerCollection * 1024;
11061115

11071116
// Store the RCAU offer if it isn't already stored (signed update path with no prior offer).
1108-
if ($.offers[_rcauHash].data.length == 0)
1117+
if ($.offers[_rcauHash].data.length == 0) {
11091118
$.offers[_rcauHash] = StoredOffer({ offerType: OFFER_TYPE_UPDATE, data: abi.encode(_rcau) });
1110-
1119+
}
11111120
// Clean up the replaced active offer — oldHash is always non-zero for accepted agreements.
11121121
delete $.offers[_agreement.activeTermsHash];
11131122
// If an another pending update existed, drop it too — the nonce has been consumed.

‎packages/horizon/test/unit/payments/recurring-collector/accept.t.sol‎

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,8 @@ contract RecurringCollectorAcceptTest is RecurringCollectorSharedTest {
2525
) public {
2626
// Ensure non-empty signature so the signed path is taken (which checks deadline first)
2727
vm.assume(fuzzySignature.length > 0);
28+
// Pranking as the proxy admin hits ProxyDeniedAdminAccess before the deadline check.
29+
vm.assume(fuzzyRCA.dataService != _proxyAdmin);
2830
// Generate deterministic agreement ID for validation
2931
bytes16 agreementId = _recurringCollector.generateAgreementId(
3032
fuzzyRCA.payer,
@@ -47,22 +49,20 @@ contract RecurringCollectorAcceptTest is RecurringCollectorSharedTest {
4749
_recurringCollector.accept(fuzzyRCA, fuzzySignature);
4850
}
4951

50-
function test_Accept_Revert_WhenAlreadyAccepted(FuzzyTestAccept calldata fuzzyTestAccept) public {
52+
function test_Accept_Idempotent_WhenAlreadyAccepted(FuzzyTestAccept calldata fuzzyTestAccept) public {
5153
(
5254
IRecurringCollector.RecurringCollectionAgreement memory acceptedRca,
5355
bytes memory signature,
5456
,
5557
bytes16 agreementId
5658
) = _sensibleAuthorizeAndAccept(fuzzyTestAccept);
5759

58-
bytes memory expectedErr = abi.encodeWithSelector(
59-
IRecurringCollector.RecurringCollectorAgreementIncorrectState.selector,
60-
agreementId,
61-
IRecurringCollector.AgreementState.Accepted
62-
);
63-
vm.expectRevert(expectedErr);
60+
// Re-accepting the same RCA is a no-op — succeeds without reverting or re-emitting.
61+
vm.recordLogs();
6462
vm.prank(acceptedRca.dataService);
65-
_recurringCollector.accept(acceptedRca, signature);
63+
bytes16 returnedId = _recurringCollector.accept(acceptedRca, signature);
64+
assertEq(returnedId, agreementId);
65+
assertEq(vm.getRecordedLogs().length, 0, "no event emitted on idempotent re-accept");
6666
}
6767

6868
/* solhint-enable graph/func-name-mixedcase */

‎packages/horizon/test/unit/payments/recurring-collector/acceptUnsigned.t.sol‎

Lines changed: 4 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -128,7 +128,7 @@ contract RecurringCollectorAcceptUnsignedTest is RecurringCollectorSharedTest {
128128
_recurringCollector.accept(rca, "");
129129
}
130130

131-
function test_AcceptUnsigned_Revert_WhenAlreadyAccepted(FuzzyTestAccept calldata fuzzyTestAccept) public {
131+
function test_AcceptUnsigned_Idempotent_WhenAlreadyAccepted(FuzzyTestAccept calldata fuzzyTestAccept) public {
132132
MockAgreementOwner approver = _newApprover();
133133
IRecurringCollector.RecurringCollectionAgreement memory rca = _recurringCollectorHelper.sensibleRCA(
134134
fuzzyTestAccept.rca
@@ -143,16 +143,10 @@ contract RecurringCollectorAcceptUnsignedTest is RecurringCollectorSharedTest {
143143
vm.prank(rca.dataService);
144144
bytes16 agreementId = _recurringCollector.accept(rca, "");
145145

146-
// Stored offer persists, so authorization passes but state check fails
147-
vm.expectRevert(
148-
abi.encodeWithSelector(
149-
IRecurringCollector.RecurringCollectorAgreementIncorrectState.selector,
150-
agreementId,
151-
IRecurringCollector.AgreementState.Accepted
152-
)
153-
);
146+
// Re-accepting the same RCA is a no-op — succeeds without reverting.
154147
vm.prank(rca.dataService);
155-
_recurringCollector.accept(rca, "");
148+
bytes16 returnedId = _recurringCollector.accept(rca, "");
149+
assertEq(returnedId, agreementId);
156150
}
157151

158152
function test_AcceptUnsigned_Revert_WhenDeadlineElapsed() public {

‎packages/horizon/test/unit/payments/recurring-collector/update.t.sol‎

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -313,5 +313,38 @@ contract RecurringCollectorUpdateTest is RecurringCollectorSharedTest {
313313
assertEq(updatedAgreement2.updateNonce, 2);
314314
}
315315

316+
function test_Update_Idempotent_WhenAlreadyAtActiveHash(FuzzyTestUpdate calldata fuzzyTestUpdate) public {
317+
(
318+
IRecurringCollector.RecurringCollectionAgreement memory acceptedRca,
319+
,
320+
uint256 signerKey,
321+
bytes16 agreementId
322+
) = _sensibleAuthorizeAndAccept(fuzzyTestUpdate.fuzzyTestAccept);
323+
324+
IRecurringCollector.RecurringCollectionAgreementUpdate memory rcau = _recurringCollectorHelper.sensibleRCAU(
325+
fuzzyTestUpdate.rcau
326+
);
327+
rcau.agreementId = agreementId;
328+
rcau.nonce = 1;
329+
(, bytes memory signature) = _recurringCollectorHelper.generateSignedRCAU(rcau, signerKey);
330+
331+
// First update consumes nonce 1 and sets activeTermsHash = hash(rcau).
332+
vm.prank(acceptedRca.dataService);
333+
_recurringCollector.update(rcau, signature);
334+
335+
IRecurringCollector.AgreementData memory afterFirst = _recurringCollector.getAgreement(agreementId);
336+
assertEq(afterFirst.updateNonce, 1, "nonce advanced to 1 after first update");
337+
338+
// Re-submitting the same RCAU is a no-op — nonce does NOT advance, no event, no revert.
339+
vm.recordLogs();
340+
vm.prank(acceptedRca.dataService);
341+
_recurringCollector.update(rcau, signature);
342+
assertEq(vm.getRecordedLogs().length, 0, "no event emitted on idempotent re-update");
343+
344+
IRecurringCollector.AgreementData memory afterSecond = _recurringCollector.getAgreement(agreementId);
345+
assertEq(afterSecond.updateNonce, 1, "nonce unchanged on idempotent re-update");
346+
assertEq(afterSecond.activeTermsHash, afterFirst.activeTermsHash, "activeTermsHash unchanged");
347+
}
348+
316349
/* solhint-enable graph/func-name-mixedcase */
317350
}

‎packages/subgraph-service/contracts/libraries/IndexingAgreement.sol‎

Lines changed: 48 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -218,12 +218,6 @@ library IndexingAgreement {
218218
bytes32 allocationDeploymentId
219219
);
220220

221-
/**
222-
* @notice Thrown when the agreement is already accepted
223-
* @param agreementId The agreement ID
224-
*/
225-
error IndexingAgreementAlreadyAccepted(bytes16 agreementId);
226-
227221
/**
228222
* @notice Thrown when an allocation already has an active agreement
229223
* @param allocationId The allocation ID
@@ -310,42 +304,48 @@ library IndexingAgreement {
310304

311305
IIndexingAgreement.State storage agreement = self.agreements[agreementId];
312306

313-
require(agreement.allocationId == address(0), IndexingAgreementAlreadyAccepted(agreementId));
314-
315-
require(
316-
allocation.subgraphDeploymentId == metadata.subgraphDeploymentId,
317-
IndexingAgreementDeploymentIdMismatch(
318-
metadata.subgraphDeploymentId,
307+
// Accept is idempotent for the same allocation, and supports moving
308+
// the agreement to a different allocation. The collector's accept handles state
309+
// validity (reverts if the agreement is cancelled, no-ops if already accepted).
310+
if (agreement.allocationId != allocationId) {
311+
require(
312+
allocation.subgraphDeploymentId == metadata.subgraphDeploymentId,
313+
IndexingAgreementDeploymentIdMismatch(
314+
metadata.subgraphDeploymentId,
315+
allocationId,
316+
allocation.subgraphDeploymentId
317+
)
318+
);
319+
320+
// Ensure that an allocation can only have one active indexing agreement
321+
require(
322+
self.allocationToActiveAgreementId[allocationId] == bytes16(0),
323+
AllocationAlreadyHasIndexingAgreement(allocationId)
324+
);
325+
326+
if (agreement.allocationId != address(0)) delete self.allocationToActiveAgreementId[agreement.allocationId];
327+
agreement.allocationId = allocationId;
328+
329+
self.allocationToActiveAgreementId[allocationId] = agreementId;
330+
331+
agreement.version = metadata.version;
332+
333+
require(
334+
metadata.version == IIndexingAgreement.IndexingAgreementVersion.V1,
335+
IndexingAgreementInvalidVersion(metadata.version)
336+
);
337+
_setTermsV1(self, agreementId, metadata.terms, rca.maxOngoingTokensPerSecond);
338+
339+
emit IndexingAgreementAccepted(
340+
rca.serviceProvider,
341+
rca.payer,
342+
agreementId,
319343
allocationId,
320-
allocation.subgraphDeploymentId
321-
)
322-
);
323-
324-
// Ensure that an allocation can only have one active indexing agreement
325-
require(
326-
self.allocationToActiveAgreementId[allocationId] == bytes16(0),
327-
AllocationAlreadyHasIndexingAgreement(allocationId)
328-
);
329-
self.allocationToActiveAgreementId[allocationId] = agreementId;
330-
331-
agreement.version = metadata.version;
332-
agreement.allocationId = allocationId;
333-
334-
require(
335-
metadata.version == IIndexingAgreement.IndexingAgreementVersion.V1,
336-
IndexingAgreementInvalidVersion(metadata.version)
337-
);
338-
_setTermsV1(self, agreementId, metadata.terms, rca.maxOngoingTokensPerSecond);
339-
340-
emit IndexingAgreementAccepted(
341-
rca.serviceProvider,
342-
rca.payer,
343-
agreementId,
344-
allocationId,
345-
metadata.subgraphDeploymentId,
346-
metadata.version,
347-
metadata.terms
348-
);
344+
metadata.subgraphDeploymentId,
345+
metadata.version,
346+
metadata.terms
347+
);
348+
}
349349

350350
require(
351351
_directory().recurringCollector().accept(rca, authData) == agreementId,
@@ -380,12 +380,18 @@ library IndexingAgreement {
380380
bytes calldata authData
381381
) external {
382382
IIndexingAgreement.AgreementWrapper memory wrapper = _get(self, rcau.agreementId);
383-
require(_isActive(wrapper), IndexingAgreementNotActive(rcau.agreementId));
383+
// SS gate: only checks that this is an SS-managed, tracked agreement. Collector is the
384+
// state authority — it reverts if the agreement cannot actually accept an update.
385+
require(_isValid(wrapper), IndexingAgreementNotActive(rcau.agreementId));
384386
require(
385387
wrapper.collectorAgreement.serviceProvider == indexer,
386388
IndexingAgreementNotAuthorized(rcau.agreementId, indexer)
387389
);
388390

391+
// Idempotent: this RCAU is already the active version — both SS terms and collector state
392+
// are in sync because both are written together on the original update.
393+
if (wrapper.collectorAgreement.activeTermsHash == _directory().recurringCollector().hashRCAU(rcau)) return;
394+
389395
UpdateIndexingAgreementMetadata memory metadata = IndexingAgreementDecoder.decodeRCAUMetadata(rcau.metadata);
390396

391397
require(

0 commit comments

Comments
 (0)