diff --git a/packages/horizon/contracts/interfaces/IRecurringCollector.sol b/packages/horizon/contracts/interfaces/IRecurringCollector.sol index 5bf597090..c4930a954 100644 --- a/packages/horizon/contracts/interfaces/IRecurringCollector.sol +++ b/packages/horizon/contracts/interfaces/IRecurringCollector.sol @@ -28,6 +28,14 @@ interface IRecurringCollector is IAuthorizable, IPaymentsCollector { ThirdParty } + /// @notice Reasons why an agreement is not collectable + enum AgreementNotCollectableReason { + None, + InvalidAgreementState, + ZeroCollectionSeconds, + InvalidTemporalWindow + } + /** * @notice A representation of a signed Recurring Collection Agreement (RCA) * @param rca The Recurring Collection Agreement to be signed @@ -303,6 +311,13 @@ interface IRecurringCollector is IAuthorizable, IPaymentsCollector { */ error RecurringCollectorAgreementIncorrectState(bytes16 agreementId, AgreementState incorrectState); + /** + * @notice Thrown when an agreement is not collectable + * @param agreementId The agreement ID + * @param reason The reason why the agreement is not collectable + */ + error RecurringCollectorAgreementNotCollectable(bytes16 agreementId, AgreementNotCollectableReason reason); + /** * @notice Thrown when accepting an agreement with an address that is not set */ @@ -440,10 +455,11 @@ interface IRecurringCollector is IAuthorizable, IPaymentsCollector { * @return isCollectable Whether the agreement is in a valid state that allows collection attempts, * not that there are necessarily funds available to collect. * @return collectionSeconds The valid collection duration in seconds (0 if not collectable) + * @return reason The reason why the agreement is not collectable (None if collectable) */ function getCollectionInfo( AgreementData memory agreement - ) external view returns (bool isCollectable, uint256 collectionSeconds); + ) external view returns (bool isCollectable, uint256 collectionSeconds, AgreementNotCollectableReason reason); /** * @notice Generate a deterministic agreement ID from agreement parameters diff --git a/packages/horizon/contracts/payments/collectors/RecurringCollector.sol b/packages/horizon/contracts/payments/collectors/RecurringCollector.sol index a24e2aac9..99bf5d3fb 100644 --- a/packages/horizon/contracts/payments/collectors/RecurringCollector.sol +++ b/packages/horizon/contracts/payments/collectors/RecurringCollector.sol @@ -269,7 +269,7 @@ contract RecurringCollector is EIP712, GraphDirectory, Authorizable, IRecurringC /// @inheritdoc IRecurringCollector function getCollectionInfo( AgreementData memory agreement - ) external view returns (bool isCollectable, uint256 collectionSeconds) { + ) external view returns (bool isCollectable, uint256 collectionSeconds, AgreementNotCollectableReason reason) { return _getCollectionInfo(agreement); } @@ -309,14 +309,11 @@ contract RecurringCollector is EIP712, GraphDirectory, Authorizable, IRecurringC ) private returns (uint256) { AgreementData storage agreement = _getAgreementStorage(_params.agreementId); - // Check if agreement exists first (for unknown agreements) - (bool isCollectable, uint256 collectionSeconds) = _getCollectionInfo(agreement); - require(isCollectable, RecurringCollectorAgreementIncorrectState(_params.agreementId, agreement.state)); - - require( - collectionSeconds > 0, - RecurringCollectorZeroCollectionSeconds(_params.agreementId, block.timestamp, agreement.lastCollectionAt) + // Check if agreement is collectable first + (bool isCollectable, uint256 collectionSeconds, AgreementNotCollectableReason reason) = _getCollectionInfo( + agreement ); + require(isCollectable, RecurringCollectorAgreementNotCollectable(_params.agreementId, reason)); require( msg.sender == agreement.dataService, @@ -583,17 +580,17 @@ contract RecurringCollector is EIP712, GraphDirectory, Authorizable, IRecurringC * @param _agreement The agreement data * @return isCollectable Whether the agreement can be collected from * @return collectionSeconds The valid collection duration in seconds (0 if not collectable) + * @return reason The reason why the agreement is not collectable (None if collectable) */ function _getCollectionInfo( AgreementData memory _agreement - ) private view returns (bool isCollectable, uint256 collectionSeconds) { + ) private view returns (bool, uint256, AgreementNotCollectableReason) { // Check if agreement is in collectable state - isCollectable = - _agreement.state == AgreementState.Accepted || + bool hasValidState = _agreement.state == AgreementState.Accepted || _agreement.state == AgreementState.CanceledByPayer; - if (!isCollectable) { - return (false, 0); + if (!hasValidState) { + return (false, 0, AgreementNotCollectableReason.InvalidAgreementState); } bool canceledOrElapsed = _agreement.state == AgreementState.CanceledByPayer || @@ -606,11 +603,14 @@ contract RecurringCollector is EIP712, GraphDirectory, Authorizable, IRecurringC uint256 collectionStart = _agreementCollectionStartAt(_agreement); if (collectionEnd < collectionStart) { - return (false, 0); + return (false, 0, AgreementNotCollectableReason.InvalidTemporalWindow); + } + + if (collectionStart == collectionEnd) { + return (false, 0, AgreementNotCollectableReason.ZeroCollectionSeconds); } - collectionSeconds = collectionEnd - collectionStart; - return (isCollectable, collectionSeconds); + return (true, collectionEnd - collectionStart, 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 index d44284e9f..738a0415c 100644 --- a/packages/horizon/test/unit/payments/recurring-collector/collect.t.sol +++ b/packages/horizon/test/unit/payments/recurring-collector/collect.t.sol @@ -90,9 +90,9 @@ contract RecurringCollectorCollectTest is RecurringCollectorSharedTest { bytes memory data = _generateCollectData(fuzzy.collectParams); bytes memory expectedErr = abi.encodeWithSelector( - IRecurringCollector.RecurringCollectorAgreementIncorrectState.selector, + IRecurringCollector.RecurringCollectorAgreementNotCollectable.selector, fuzzy.collectParams.agreementId, - IRecurringCollector.AgreementState.NotAccepted + IRecurringCollector.AgreementNotCollectableReason.InvalidAgreementState ); vm.expectRevert(expectedErr); vm.prank(dataService); @@ -116,9 +116,9 @@ contract RecurringCollectorCollectTest is RecurringCollectorSharedTest { bytes memory data = _generateCollectData(collectParams); bytes memory expectedErr = abi.encodeWithSelector( - IRecurringCollector.RecurringCollectorAgreementIncorrectState.selector, + IRecurringCollector.RecurringCollectorAgreementNotCollectable.selector, collectParams.agreementId, - IRecurringCollector.AgreementState.CanceledByServiceProvider + IRecurringCollector.AgreementNotCollectableReason.InvalidAgreementState ); vm.expectRevert(expectedErr); vm.prank(accepted.rca.dataService); diff --git a/packages/subgraph-service/contracts/libraries/IndexingAgreement.sol b/packages/subgraph-service/contracts/libraries/IndexingAgreement.sol index f9648e4fb..02f99be88 100644 --- a/packages/subgraph-service/contracts/libraries/IndexingAgreement.sol +++ b/packages/subgraph-service/contracts/libraries/IndexingAgreement.sol @@ -554,7 +554,7 @@ library IndexingAgreement { IndexingAgreementNotAuthorized(params.agreementId, params.indexer) ); // Get collection info from RecurringCollector (single source of truth for temporal logic) - (bool isCollectable, uint256 collectionSeconds) = _directory().recurringCollector().getCollectionInfo( + (bool isCollectable, uint256 collectionSeconds, ) = _directory().recurringCollector().getCollectionInfo( wrapper.collectorAgreement ); require(_isValid(wrapper) && isCollectable, IndexingAgreementNotCollectable(params.agreementId)); diff --git a/packages/subgraph-service/test/unit/subgraphService/indexing-agreement/collect.t.sol b/packages/subgraph-service/test/unit/subgraphService/indexing-agreement/collect.t.sol index f41bdf976..3f7a5657c 100644 --- a/packages/subgraph-service/test/unit/subgraphService/indexing-agreement/collect.t.sol +++ b/packages/subgraph-service/test/unit/subgraphService/indexing-agreement/collect.t.sol @@ -58,6 +58,8 @@ contract SubgraphServiceIndexingAgreementCollectTest is SubgraphServiceIndexingA ); _expectCollectCallAndEmit(data, indexerState, accepted, acceptedAgreementId, tokensCollected, entities, poi); + skip(1); // To make agreement collectable + subgraphService.collect( indexerState.addr, IGraphPayments.PaymentTypes.IndexingFee, @@ -215,6 +217,8 @@ contract SubgraphServiceIndexingAgreementCollectTest is SubgraphServiceIndexingA ); vm.expectRevert(expectedErr); + skip(1); // To make agreement collectable + subgraphService.collect( indexerState.addr, IGraphPayments.PaymentTypes.IndexingFee,