Skip to content

Commit f4ea27a

Browse files
matiasedgeandnodematiasedgeandnode
committed
f - allow for zero collections
1 parent 4102e9e commit f4ea27a

File tree

11 files changed

+170
-75
lines changed

11 files changed

+170
-75
lines changed

‎IndexingPaymentsTodo.md‎

Lines changed: 13 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1,26 +1,30 @@
11
# Still pending
22

3-
* Double-check it supports paying per byte instead of per entity and eventually a subgraph-gas metric. DONE: ~~Built-in upgrade path to indexing agreements v2~~
4-
* Update Arbitration Charter to support disputing Indexing Fees. DONE: ~~Support `DisputeManager`~~
3+
* Arbitration Charter: Update to support disputing IndexingFee.
54
* Economics
65
* If service wants to collect more than collector allows. Collector limits but doesn't tell the service?
7-
* Support for agreements that end up in `RecurringCollectorCollectionTooLate` or ways to avoid getting to that state.
8-
* Should we deal with zero entities declared as a special case?
96
* Since an allocation is required for collecting, do we want to expect that the allocation is not stale? Do we want to add code to collect rewards as part of the collection of fees? Make sure allocation is more than one epoch old if we attempt this.
10-
* Reject Zero POIs?
117
* What happens if the escrow doesn't have enough funds? Since you can't collect that means you lose out forever?
12-
* Don't pay for entities on initial collection?
8+
* Don't pay for entities on initial collection? Where did we land in terms of payment terms?
139
* Should we set a different param for initial collection time max? Some subgraphs take a lot to catch up.
1410
* How do we solve for the case where an indexer has reached their max expected payout for the initial sync but haven't reached the current epoch (thus their POI is incorrect)?
1511
* Double check cancelation policy. Who can cancel when? Right now is either party at any time.
1612
* Expose a function that indexers can use to calculate the tokens to be collected and other collection params?
1713
* Support a way for gateway to shop an agreement around? Deadline + dedup key? So only one agreement with the dedupe key can be accepted?
18-
* Maybe check that the epoch the indexer is sending is the one the transaction will be run in?
19-
* Check upgrade conditions. Support indexing agreement upgadeability, so that there is a mechanism to adjust the rates without having to cancel and start over.
2014
* If an indexer closes an allocation, what should happen to the accepeted agreement?
2115
* test_SubgraphService_CollectIndexingFee_Integration fails with PaymentsEscrowInconsistentCollection
2216
* Reduce the number of errors declared and returned
17+
* Missing events for accept, cancel, upgrade RCAs.
18+
* Switch `duration` for `endsAt`?
19+
20+
# Done
21+
22+
* DONE: ~~Support `DisputeManager`~~
23+
* DONE: ~~Check upgrade conditions. Support indexing agreement upgradability, so that there is a mechanism to adjust the rates without having to cancel and start over.~~
24+
* DONE: ~~Maybe check that the epoch the indexer is sending is the one the transaction will be run in?~~
25+
* DONE: ~~Should we deal with zero entities declared as a special case?~~
26+
* DONE: ~~Support for agreements that end up in `RecurringCollectorCollectionTooLate` or ways to avoid getting to that state.~~
2327
* DONE: ~~Make `agreementId` unique globally so that we don't need the full tuple (`payer`+`indexer`+`agreementId`) as key?~~
2428
* DONE: ~~Maybe IRecurringCollector.cancel(address payer, address serviceProvider, bytes16 agreementId) should only take in agreementId?~~
2529
* DONE: ~~Unify to one error in Decoder.sol~~
26-
* Missing events for accept, cancel, upgrade RCAs.
30+
* DONE: ~~Built-in upgrade path to indexing agreements v2~~

‎packages/horizon/contracts/interfaces/IRecurringCollector.sol‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -125,6 +125,7 @@ interface IRecurringCollector is IAuthorizable, IPaymentsCollector {
125125
address indexed dataService,
126126
address indexed payer,
127127
address indexed serviceProvider,
128+
bytes16 agreementId,
128129
bytes32 collectionId,
129130
uint256 tokens,
130131
uint256 dataServiceCut
@@ -192,6 +193,12 @@ interface IRecurringCollector is IAuthorizable, IPaymentsCollector {
192193
*/
193194
error RecurringCollectorAgreementInvalid(bytes16 agreementId, uint256 acceptedAt);
194195

196+
/**
197+
* Thrown when accepting or upgrading an agreement with invalid params
198+
* @param agreementId The agreement ID
199+
*/
200+
error RecurringCollectorAgreementInvalidParams(bytes16 agreementId);
201+
195202
/**
196203
* Thrown when calling collect() on an elapsed agreement
197204
* @param agreementId The agreement ID

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

Lines changed: 60 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -96,12 +96,12 @@ contract RecurringCollector is EIP712, GraphDirectory, Authorizable, IRecurringC
9696
agreement.dataService = signedRCA.rca.dataService;
9797
agreement.payer = signedRCA.rca.payer;
9898
agreement.serviceProvider = signedRCA.rca.serviceProvider;
99-
// FIX-ME: These need to be validated to something that makes sense for the contract
10099
agreement.duration = signedRCA.rca.duration;
101100
agreement.maxInitialTokens = signedRCA.rca.maxInitialTokens;
102101
agreement.maxOngoingTokensPerSecond = signedRCA.rca.maxOngoingTokensPerSecond;
103102
agreement.minSecondsPerCollection = signedRCA.rca.minSecondsPerCollection;
104103
agreement.maxSecondsPerCollection = signedRCA.rca.maxSecondsPerCollection;
104+
_requireValidAgreement(agreement, signedRCA.rca.agreementId);
105105
}
106106

107107
/**
@@ -141,12 +141,12 @@ contract RecurringCollector is EIP712, GraphDirectory, Authorizable, IRecurringC
141141
_requireAuthorizedRCAUSigner(signedRCAU, agreement.payer);
142142

143143
// upgrade the agreement
144-
// FIX-ME: These need to be validated to something that makes sense for the contract
145144
agreement.duration = signedRCAU.rcau.duration;
146145
agreement.maxInitialTokens = signedRCAU.rcau.maxInitialTokens;
147146
agreement.maxOngoingTokensPerSecond = signedRCAU.rcau.maxOngoingTokensPerSecond;
148147
agreement.minSecondsPerCollection = signedRCAU.rcau.minSecondsPerCollection;
149148
agreement.maxSecondsPerCollection = signedRCAU.rcau.maxSecondsPerCollection;
149+
_requireValidAgreement(agreement, signedRCAU.rcau.agreementId);
150150
}
151151

152152
/**
@@ -211,17 +211,21 @@ contract RecurringCollector is EIP712, GraphDirectory, Authorizable, IRecurringC
211211
RecurringCollectorDataServiceNotAuthorized(_params.agreementId, msg.sender)
212212
);
213213

214-
_requireValidCollect(agreement, _params.agreementId, _params.tokens);
215-
agreement.lastCollectionAt = block.timestamp;
214+
_requireCollectableAgreement(agreement, _params.agreementId);
216215

217-
_graphPaymentsEscrow().collect(
218-
IGraphPayments.PaymentTypes.IndexingFee,
219-
agreement.payer,
220-
agreement.serviceProvider,
221-
_params.tokens,
222-
agreement.dataService,
223-
_params.dataServiceCut
224-
);
216+
if (_params.tokens != 0) {
217+
_requireValidCollect(agreement, _params.agreementId, _params.tokens);
218+
219+
_graphPaymentsEscrow().collect(
220+
IGraphPayments.PaymentTypes.IndexingFee,
221+
agreement.payer,
222+
agreement.serviceProvider,
223+
_params.tokens,
224+
agreement.dataService,
225+
_params.dataServiceCut
226+
);
227+
}
228+
agreement.lastCollectionAt = block.timestamp;
225229

226230
emit PaymentCollected(
227231
IGraphPayments.PaymentTypes.IndexingFee,
@@ -236,6 +240,7 @@ contract RecurringCollector is EIP712, GraphDirectory, Authorizable, IRecurringC
236240
agreement.dataService,
237241
agreement.payer,
238242
agreement.serviceProvider,
243+
_params.agreementId,
239244
_params.collectionId,
240245
_params.tokens,
241246
_params.dataServiceCut
@@ -244,10 +249,32 @@ contract RecurringCollector is EIP712, GraphDirectory, Authorizable, IRecurringC
244249
return _params.tokens;
245250
}
246251

247-
/**
248-
* @notice Requires that the agreement is valid for collection.
249-
*/
250-
function _requireValidCollect(AgreementData memory _agreement, bytes16 _agreementId, uint256 _tokens) private view {
252+
function _requireValidAgreement(AgreementData memory _agreement, bytes16 _agreementId) private view {
253+
require(
254+
_agreement.dataService != address(0) &&
255+
_agreement.payer != address(0) &&
256+
_agreement.serviceProvider != address(0),
257+
RecurringCollectorAgreementInvalidParams(_agreementId)
258+
);
259+
260+
// Agreement needs to end in the future
261+
require(_agreementEndsAt(_agreement) > block.timestamp, RecurringCollectorAgreementInvalidParams(_agreementId));
262+
263+
// Collection window needs to be at least 2 hours
264+
require(
265+
_agreement.maxSecondsPerCollection > _agreement.minSecondsPerCollection &&
266+
(_agreement.maxSecondsPerCollection - _agreement.minSecondsPerCollection >= 7200),
267+
RecurringCollectorAgreementInvalidParams(_agreementId)
268+
);
269+
270+
// Agreement needs to last at least one min collection window
271+
require(
272+
_agreement.duration >= _agreement.minSecondsPerCollection + 7200,
273+
RecurringCollectorAgreementInvalidParams(_agreementId)
274+
);
275+
}
276+
277+
function _requireCollectableAgreement(AgreementData memory _agreement, bytes16 _agreementId) private view {
251278
require(
252279
_agreement.acceptedAt > 0 && _agreement.acceptedAt != CANCELED,
253280
RecurringCollectorAgreementInvalid(_agreementId, _agreement.acceptedAt)
@@ -257,9 +284,14 @@ contract RecurringCollector is EIP712, GraphDirectory, Authorizable, IRecurringC
257284
? _agreement.acceptedAt + _agreement.duration
258285
: type(uint256).max;
259286
require(agreementEnd >= block.timestamp, RecurringCollectorAgreementElapsed(_agreementId, agreementEnd));
287+
}
260288

289+
/**
290+
* @notice Requires that the collection params are valid.
291+
*/
292+
function _requireValidCollect(AgreementData memory _agreement, bytes16 _agreementId, uint256 _tokens) private view {
261293
uint256 collectionSeconds = block.timestamp;
262-
collectionSeconds -= _agreement.lastCollectionAt > 0 ? _agreement.lastCollectionAt : _agreement.acceptedAt;
294+
collectionSeconds -= _agreementCollectionStartAt(_agreement);
263295
require(
264296
collectionSeconds >= _agreement.minSecondsPerCollection,
265297
RecurringCollectorCollectionTooSoon(_agreementId, collectionSeconds, _agreement.minSecondsPerCollection)
@@ -376,4 +408,15 @@ contract RecurringCollector is EIP712, GraphDirectory, Authorizable, IRecurringC
376408
function _getAgreement(bytes16 _agreementId) private view returns (AgreementData memory) {
377409
return agreements[_agreementId];
378410
}
411+
412+
function _agreementCollectionStartAt(AgreementData memory _agreement) private pure returns (uint256) {
413+
return _agreement.lastCollectionAt > 0 ? _agreement.lastCollectionAt : _agreement.acceptedAt;
414+
}
415+
416+
function _agreementEndsAt(AgreementData memory _agreement) private pure returns (uint256) {
417+
return
418+
_agreement.duration < type(uint256).max - _agreement.acceptedAt
419+
? _agreement.acceptedAt + _agreement.duration
420+
: type(uint256).max;
421+
}
379422
}

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

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,7 @@ contract RecurringCollectorCollectTest is RecurringCollectorSharedTest {
5353
address notDataService
5454
) public {
5555
vm.assume(rca.dataService != notDataService);
56+
rca = _sensibleRCA(rca);
5657
params.agreementId = rca.agreementId;
5758
bytes memory data = _generateCollectData(params);
5859

@@ -85,9 +86,11 @@ contract RecurringCollectorCollectTest is RecurringCollectorSharedTest {
8586
TestCollectParams memory testCollectParams,
8687
uint256 unboundedKey
8788
) public {
88-
IRecurringCollector.CollectParams memory fuzzyParams = testCollectParams.collectData;
89+
rca = _sensibleRCA(rca);
8990
_authorizeAndAccept(rca, boundKey(unboundedKey));
9091
_cancel(rca);
92+
IRecurringCollector.CollectParams memory fuzzyParams = testCollectParams.collectData;
93+
fuzzyParams.tokens = bound(fuzzyParams.tokens, 1, type(uint256).max);
9194
IRecurringCollector.CollectParams memory collectParams = _generateCollectParams(
9295
rca,
9396
fuzzyParams.collectionId,
@@ -162,6 +165,7 @@ contract RecurringCollectorCollectTest is RecurringCollectorSharedTest {
162165

163166
uint256 collectionSeconds = boundSkipCeil(unboundedSkip, rca.minSecondsPerCollection - 1);
164167
skip(collectionSeconds);
168+
fuzzyParams.tokens = bound(fuzzyParams.tokens, 1, type(uint256).max);
165169
IRecurringCollector.CollectParams memory collectParams = _generateCollectParams(
166170
rca,
167171
fuzzyParams.collectionId,
@@ -207,6 +211,7 @@ contract RecurringCollectorCollectTest is RecurringCollectorSharedTest {
207211
uint256 collectionSeconds = boundSkip(unboundedSkip, rca.maxSecondsPerCollection + 1, durationLeft);
208212
skip(collectionSeconds);
209213

214+
fuzzyParams.tokens = bound(fuzzyParams.tokens, 1, type(uint256).max);
210215
IRecurringCollector.CollectParams memory collectParams = _generateCollectParams(
211216
rca,
212217
fuzzyParams.collectionId,

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

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,7 @@ contract RecurringCollectorSharedTest is Test, Bounder {
4242
function _fuzzyAuthorizeAndAccept(
4343
FuzzyAcceptableRCA memory _fuzzyAcceptableRCA
4444
) internal returns (IRecurringCollector.SignedRCA memory) {
45-
vm.assume(_fuzzyAcceptableRCA.rca.payer != address(0));
45+
_fuzzyAcceptableRCA.rca = _sensibleRCA(_fuzzyAcceptableRCA.rca);
4646
_fuzzyAcceptableRCA.rca = _recurringCollectorHelper.withOKAcceptDeadline(_fuzzyAcceptableRCA.rca);
4747
return _authorizeAndAcceptV2(_fuzzyAcceptableRCA.rca, boundKey(_fuzzyAcceptableRCA.unboundedSignerKey));
4848
}
@@ -108,6 +108,7 @@ contract RecurringCollectorSharedTest is Test, Bounder {
108108
_rca.dataService,
109109
_rca.payer,
110110
_rca.serviceProvider,
111+
_rca.agreementId,
111112
_fuzzyParams.collectionId,
112113
_tokens,
113114
_fuzzyParams.dataServiceCut
@@ -136,9 +137,12 @@ contract RecurringCollectorSharedTest is Test, Bounder {
136137
function _sensibleRCA(
137138
IRecurringCollector.RecurringCollectionAgreement memory _rca
138139
) internal pure returns (IRecurringCollector.RecurringCollectionAgreement memory) {
140+
vm.assume(_rca.dataService != address(0));
141+
vm.assume(_rca.payer != address(0));
142+
vm.assume(_rca.serviceProvider != address(0));
139143
_rca.minSecondsPerCollection = uint32(bound(_rca.minSecondsPerCollection, 60, 60 * 60 * 24));
140144
_rca.maxSecondsPerCollection = uint32(
141-
bound(_rca.maxSecondsPerCollection, _rca.minSecondsPerCollection * 2, 60 * 60 * 24 * 30)
145+
bound(_rca.maxSecondsPerCollection, _rca.minSecondsPerCollection + 7200, 60 * 60 * 24 * 30)
142146
);
143147
_rca.duration = bound(_rca.duration, _rca.maxSecondsPerCollection * 10, type(uint256).max);
144148
_rca.maxInitialTokens = bound(_rca.maxInitialTokens, 0, 1e18 * 100_000_000);

‎packages/subgraph-service/contracts/Decoder.sol‎

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -37,8 +37,10 @@ contract Decoder {
3737
*
3838
* @param data The data to decode.
3939
*/
40-
function decodeCollectIndexingFeeDataV1(bytes memory data) external pure returns (uint256 entities, bytes32 poi) {
41-
return abi.decode(data, (uint256, bytes32));
40+
function decodeCollectIndexingFeeDataV1(
41+
bytes memory data
42+
) external pure returns (uint256 entities, bytes32 poi, uint256 epoch) {
43+
return abi.decode(data, (uint256, bytes32, uint256));
4244
}
4345

4446
/**
@@ -81,9 +83,9 @@ contract Decoder {
8183
}
8284
}
8385

84-
function _decodeCollectIndexingFeeDataV1(bytes memory _data) internal view returns (uint256, bytes32) {
85-
try this.decodeCollectIndexingFeeDataV1(_data) returns (uint256 entities, bytes32 poi) {
86-
return (entities, poi);
86+
function _decodeCollectIndexingFeeDataV1(bytes memory _data) internal view returns (uint256, bytes32, uint256) {
87+
try this.decodeCollectIndexingFeeDataV1(_data) returns (uint256 entities, bytes32 poi, uint256 epoch) {
88+
return (entities, poi, epoch);
8789
} catch {
8890
revert ISubgraphService.SubgraphServiceDecoderInvalidData("_decodeCollectIndexingFeeDataV1", _data);
8991
}

‎packages/subgraph-service/contracts/SubgraphService.sol‎

Lines changed: 29 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -739,12 +739,24 @@ contract SubgraphService is
739739
agreement.version == IndexingAgreementVersion.V1,
740740
SubgraphServiceInvalidIndexingAgreementVersion(agreement.version)
741741
);
742-
(uint256 entities, bytes32 poi) = _decodeCollectIndexingFeeDataV1(_data);
742+
(uint256 entities, bytes32 poi, uint256 epoch) = _decodeCollectIndexingFeeDataV1(_data);
743+
uint256 currentEpoch = _graphEpochManager().currentEpoch();
744+
if (epoch != currentEpoch) {
745+
revert SubgraphServiceInvalidEpoch(currentEpoch, epoch);
746+
}
747+
748+
uint256 tokensToCollect;
749+
if (entities == 0 && poi == bytes32(0)) {
750+
tokensToCollect = 0;
751+
} else {
752+
tokensToCollect = _indexingAgreementTokensToCollect(_agreementId, agreement, entities);
753+
}
754+
agreement.lastCollectionAt = block.timestamp;
743755

744756
uint256 tokensCollected = _indexingAgreementCollect(
745757
_agreementId,
746758
bytes32(uint256(uint160(agreement.allocationId))),
747-
_indexingAgreementTokensToCollect(_agreementId, agreement, entities)
759+
tokensToCollect
748760
);
749761

750762
_releaseAndLockStake(agreement.indexer, tokensCollected);
@@ -755,7 +767,7 @@ contract SubgraphService is
755767
_agreementId,
756768
agreement.allocationId,
757769
allocation.subgraphDeploymentId,
758-
_graphEpochManager().currentEpoch(),
770+
currentEpoch,
759771
tokensCollected,
760772
entities,
761773
poi
@@ -809,21 +821,6 @@ contract SubgraphService is
809821
storedTerms.tokensPerEntityPerSecond = newTerms.tokensPerEntityPerSecond;
810822
}
811823

812-
function _indexingAgreementTokensToCollect(
813-
bytes16 _agreementId,
814-
IndexingAgreementData storage _agreement,
815-
uint256 _entities
816-
) private returns (uint256) {
817-
IndexingAgreementTermsV1 memory termsV1 = _getIndexingAgreementTermsV1(_agreementId);
818-
819-
uint256 collectionSeconds = block.timestamp;
820-
collectionSeconds -= _agreement.lastCollectionAt > 0 ? _agreement.lastCollectionAt : _agreement.acceptedAt;
821-
_agreement.lastCollectionAt = block.timestamp;
822-
823-
// FIX-ME: this is bad because it encourages indexers to collect at max seconds allowed to maximize collection.
824-
return collectionSeconds * (termsV1.tokensPerSecond + termsV1.tokensPerEntityPerSecond * _entities);
825-
}
826-
827824
function _indexingAgreementCollect(
828825
bytes16 _agreementId,
829826
bytes32 _collectionId,
@@ -859,6 +856,20 @@ contract SubgraphService is
859856
_recurringCollector().cancel(_agreementId);
860857
}
861858

859+
function _indexingAgreementTokensToCollect(
860+
bytes16 _agreementId,
861+
IndexingAgreementData memory _agreement,
862+
uint256 _entities
863+
) private view returns (uint256) {
864+
IndexingAgreementTermsV1 memory termsV1 = _getIndexingAgreementTermsV1(_agreementId);
865+
866+
uint256 collectionSeconds = block.timestamp;
867+
collectionSeconds -= _agreement.lastCollectionAt > 0 ? _agreement.lastCollectionAt : _agreement.acceptedAt;
868+
869+
// FIX-ME: this is bad because it encourages indexers to collect at max seconds allowed to maximize collection.
870+
return collectionSeconds * (termsV1.tokensPerSecond + termsV1.tokensPerEntityPerSecond * _entities);
871+
}
872+
862873
function _getForUpdateIndexingAgreement(bytes16 _agreementId) private view returns (IndexingAgreementData storage) {
863874
return indexingAgreements[_agreementId];
864875
}

‎packages/subgraph-service/contracts/interfaces/ISubgraphService.sol‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -351,6 +351,13 @@ interface ISubgraphService is IDataServiceFees {
351351
uint256 tokensPerEntityPerSecond;
352352
}
353353

354+
/**
355+
* @notice Thrown when collecting on an invalid epoch
356+
* @param current The current epoch
357+
* @param invalid The invalid epoch
358+
*/
359+
error SubgraphServiceInvalidEpoch(uint256 current, uint256 invalid);
360+
354361
/**
355362
* @notice Thrown when the data can't be decoded as expected
356363
* @param t The type of data that was expected

0 commit comments

Comments
 (0)