diff --git a/packages/horizon/contracts/interfaces/IGraphPayments.sol b/packages/horizon/contracts/interfaces/IGraphPayments.sol index f446d6f52..eaac08ae4 100644 --- a/packages/horizon/contracts/interfaces/IGraphPayments.sol +++ b/packages/horizon/contracts/interfaces/IGraphPayments.sol @@ -23,27 +23,30 @@ interface IGraphPayments { * @param payer The address of the payer * @param receiver The address of the receiver * @param dataService The address of the data service - * @param tokensReceiver Amount of tokens for the receiver - * @param tokensDelegationPool Amount of tokens for delegators - * @param tokensDataService Amount of tokens for the data service + * @param tokens The total amount of tokens being collected * @param tokensProtocol Amount of tokens charged as protocol tax + * @param tokensDataService Amount of tokens for the data service + * @param tokensDelegationPool Amount of tokens for delegators + * @param tokensReceiver Amount of tokens for the receiver */ - event PaymentCollected( + event GraphPaymentCollected( address indexed payer, address indexed receiver, address indexed dataService, - uint256 tokensReceiver, - uint256 tokensDelegationPool, + uint256 tokens, + uint256 tokensProtocol, uint256 tokensDataService, - uint256 tokensProtocol + uint256 tokensDelegationPool, + uint256 tokensReceiver ); /** - * @notice Thrown when there are insufficient tokens to pay the required amount - * @param tokens The amount of tokens available - * @param minTokens The amount of tokens being collected + * @notice Thrown when the calculated amount of tokens to be paid out to all parties is + * not the same as the amount of tokens being collected + * @param tokens The amount of tokens being collected + * @param tokensCalculated The sum of all the tokens to be paid out */ - error GraphPaymentsInsufficientTokens(uint256 tokens, uint256 minTokens); + error GraphPaymentsBadAccounting(uint256 tokens, uint256 tokensCalculated); /** * @notice Thrown when the protocol payment cut is invalid @@ -51,6 +54,12 @@ interface IGraphPayments { */ error GraphPaymentsInvalidProtocolPaymentCut(uint256 protocolPaymentCut); + /** + * @notice Thrown when trying to use a cut that is not expressed in PPM + * @param cut The cut + */ + error GraphPaymentsInvalidCut(uint256 cut); + /** * @notice Collects funds from a payer. * It will pay cuts to all relevant parties and forward the rest to the receiver. @@ -58,13 +67,13 @@ interface IGraphPayments { * @param receiver The address of the receiver * @param tokens The amount of tokens being collected * @param dataService The address of the data service - * @param tokensDataService The amount of tokens that should be sent to the data service + * @param dataServiceCut The data service cut in PPM */ function collect( PaymentTypes paymentType, address receiver, uint256 tokens, address dataService, - uint256 tokensDataService + uint256 dataServiceCut ) external; } diff --git a/packages/horizon/contracts/interfaces/IPaymentsCollector.sol b/packages/horizon/contracts/interfaces/IPaymentsCollector.sol index 85d09d59f..61854bb71 100644 --- a/packages/horizon/contracts/interfaces/IPaymentsCollector.sol +++ b/packages/horizon/contracts/interfaces/IPaymentsCollector.sol @@ -19,17 +19,15 @@ interface IPaymentsCollector { * @param paymentType The payment type collected as defined by {IGraphPayments} * @param payer The address of the payer * @param receiver The address of the receiver - * @param tokensReceiver The amount of tokens received by the receiver * @param dataService The address of the data service - * @param tokensDataService The amount of tokens received by the data service + * @param tokens The amount of tokens being collected */ event PaymentCollected( IGraphPayments.PaymentTypes indexed paymentType, address indexed payer, address receiver, - uint256 tokensReceiver, address indexed dataService, - uint256 tokensDataService + uint256 tokens ); /** diff --git a/packages/horizon/contracts/interfaces/IPaymentsEscrow.sol b/packages/horizon/contracts/interfaces/IPaymentsEscrow.sol index 32f83d597..760a086a7 100644 --- a/packages/horizon/contracts/interfaces/IPaymentsEscrow.sol +++ b/packages/horizon/contracts/interfaces/IPaymentsEscrow.sol @@ -186,7 +186,7 @@ interface IPaymentsEscrow { * @param receiver The address of the receiver * @param tokens The amount of tokens to collect * @param dataService The address of the data service - * @param tokensDataService The amount of tokens that {GraphPayments} should send to the data service + * @param dataServiceCut The data service cut in PPM that {GraphPayments} should send */ function collect( IGraphPayments.PaymentTypes paymentType, @@ -194,7 +194,7 @@ interface IPaymentsEscrow { address receiver, uint256 tokens, address dataService, - uint256 tokensDataService + uint256 dataServiceCut ) external; /** diff --git a/packages/horizon/contracts/payments/GraphPayments.sol b/packages/horizon/contracts/payments/GraphPayments.sol index 0ebe566a5..0dd06ef72 100644 --- a/packages/horizon/contracts/payments/GraphPayments.sol +++ b/packages/horizon/contracts/payments/GraphPayments.sol @@ -32,7 +32,7 @@ contract GraphPayments is Initializable, MulticallUpgradeable, GraphDirectory, I * @param protocolPaymentCut The protocol tax in PPM */ constructor(address controller, uint256 protocolPaymentCut) GraphDirectory(controller) { - require(PPMMath.isValidPPM(protocolPaymentCut), GraphPaymentsInvalidProtocolPaymentCut(protocolPaymentCut)); + require(PPMMath.isValidPPM(protocolPaymentCut), GraphPaymentsInvalidCut(protocolPaymentCut)); PROTOCOL_PAYMENT_CUT = protocolPaymentCut; _disableInitializers(); } @@ -52,41 +52,54 @@ contract GraphPayments is Initializable, MulticallUpgradeable, GraphDirectory, I address receiver, uint256 tokens, address dataService, - uint256 tokensDataService + uint256 dataServiceCut ) external { + require(PPMMath.isValidPPM(dataServiceCut), GraphPaymentsInvalidCut(dataServiceCut)); + + // Pull tokens from the sender _graphToken().pullTokens(msg.sender, tokens); - // Calculate cuts - uint256 tokensProtocol = tokens.mulPPM(PROTOCOL_PAYMENT_CUT); - uint256 delegationFeeCut = _graphStaking().getDelegationFeeCut(receiver, dataService, paymentType); - uint256 tokensDelegationPool = tokens.mulPPM(delegationFeeCut); - uint256 totalCut = tokensProtocol + tokensDataService + tokensDelegationPool; - require(tokens >= totalCut, GraphPaymentsInsufficientTokens(tokens, totalCut)); + // Calculate token amounts for each party + // Order matters: protocol -> data service -> delegators -> receiver + // Note the substractions should not underflow as we are only deducting a percentage of the remainder + uint256 tokensRemaining = tokens; + + uint256 tokensProtocol = tokensRemaining.mulPPMRoundUp(PROTOCOL_PAYMENT_CUT); + tokensRemaining = tokensRemaining - tokensProtocol; + + uint256 tokensDataService = tokensRemaining.mulPPMRoundUp(dataServiceCut); + tokensRemaining = tokensRemaining - tokensDataService; + + uint256 tokensDelegationPool = tokensRemaining.mulPPMRoundUp( + _graphStaking().getDelegationFeeCut(receiver, dataService, paymentType) + ); + tokensRemaining = tokensRemaining - tokensDelegationPool; + + // Ensure accounting is correct + uint256 tokensTotal = tokensProtocol + tokensDataService + tokensDelegationPool + tokensRemaining; + require(tokens == tokensTotal, GraphPaymentsBadAccounting(tokens, tokensTotal)); - // Pay protocol cut + // Pay all parties _graphToken().burnTokens(tokensProtocol); - // Pay data service cut _graphToken().pushTokens(dataService, tokensDataService); - // Pay delegators if (tokensDelegationPool > 0) { _graphToken().approve(address(_graphStaking()), tokensDelegationPool); _graphStaking().addToDelegationPool(receiver, dataService, tokensDelegationPool); } - // Pay receiver - uint256 tokensReceiverRemaining = tokens - totalCut; - _graphToken().pushTokens(receiver, tokensReceiverRemaining); + _graphToken().pushTokens(receiver, tokensRemaining); - emit PaymentCollected( + emit GraphPaymentCollected( msg.sender, receiver, dataService, - tokensReceiverRemaining, - tokensDelegationPool, + tokens, + tokensProtocol, tokensDataService, - tokensProtocol + tokensDelegationPool, + tokensRemaining ); } } diff --git a/packages/horizon/contracts/payments/PaymentsEscrow.sol b/packages/horizon/contracts/payments/PaymentsEscrow.sol index 108197fb3..6f4252873 100644 --- a/packages/horizon/contracts/payments/PaymentsEscrow.sol +++ b/packages/horizon/contracts/payments/PaymentsEscrow.sol @@ -129,7 +129,7 @@ contract PaymentsEscrow is Initializable, MulticallUpgradeable, GraphDirectory, address receiver, uint256 tokens, address dataService, - uint256 tokensDataService + uint256 dataServiceCut ) external override notPaused { // Check if there are enough funds in the escrow account EscrowAccount storage account = escrowAccounts[payer][msg.sender][receiver]; @@ -141,7 +141,7 @@ contract PaymentsEscrow is Initializable, MulticallUpgradeable, GraphDirectory, uint256 balanceBefore = _graphToken().balanceOf(address(this)); _graphToken().approve(address(_graphPayments()), tokens); - _graphPayments().collect(paymentType, receiver, tokens, dataService, tokensDataService); + _graphPayments().collect(paymentType, receiver, tokens, dataService, dataServiceCut); uint256 balanceAfter = _graphToken().balanceOf(address(this)); require( diff --git a/packages/horizon/contracts/payments/collectors/TAPCollector.sol b/packages/horizon/contracts/payments/collectors/TAPCollector.sol index e23d16486..8d15ff5c8 100644 --- a/packages/horizon/contracts/payments/collectors/TAPCollector.sol +++ b/packages/horizon/contracts/payments/collectors/TAPCollector.sol @@ -214,21 +214,12 @@ contract TAPCollector is EIP712, GraphDirectory, ITAPCollector { } } - uint256 tokensDataService = tokensToCollect.mulPPM(dataServiceCut); - if (tokensToCollect > 0) { tokensCollected[dataService][receiver][payer] += tokensToCollect; - _graphPaymentsEscrow().collect( - _paymentType, - payer, - receiver, - tokensToCollect, - dataService, - tokensDataService - ); + _graphPaymentsEscrow().collect(_paymentType, payer, receiver, tokensToCollect, dataService, dataServiceCut); } - emit PaymentCollected(_paymentType, payer, receiver, tokensToCollect, dataService, tokensDataService); + emit PaymentCollected(_paymentType, payer, receiver, dataService, tokensToCollect); emit RAVCollected( payer, dataService, diff --git a/packages/horizon/test/escrow/GraphEscrow.t.sol b/packages/horizon/test/escrow/GraphEscrow.t.sol index b2c4438bc..120713c6c 100644 --- a/packages/horizon/test/escrow/GraphEscrow.t.sol +++ b/packages/horizon/test/escrow/GraphEscrow.t.sol @@ -7,8 +7,11 @@ import { IGraphPayments } from "../../contracts/interfaces/IGraphPayments.sol"; import { HorizonStakingSharedTest } from "../shared/horizon-staking/HorizonStakingShared.t.sol"; import { PaymentsEscrowSharedTest } from "../shared/payments-escrow/PaymentsEscrowShared.t.sol"; +import { PPMMath } from "../../contracts/libraries/PPMMath.sol"; contract GraphEscrowTest is HorizonStakingSharedTest, PaymentsEscrowSharedTest { + using PPMMath for uint256; + /* * MODIFIERS */ @@ -59,6 +62,7 @@ contract GraphEscrowTest is HorizonStakingSharedTest, PaymentsEscrowSharedTest { uint256 receiverBalance; uint256 delegationPoolBalance; uint256 dataServiceBalance; + uint256 payerEscrowBalance; } function _collectEscrow( @@ -67,53 +71,62 @@ contract GraphEscrowTest is HorizonStakingSharedTest, PaymentsEscrowSharedTest { address _receiver, uint256 _tokens, address _dataService, - uint256 _tokensDataService + uint256 _dataServiceCut ) internal { (, address _collector, ) = vm.readCallers(); // Previous balances - (uint256 previousPayerEscrowBalance, , ) = escrow.escrowAccounts(_payer, _collector, _receiver); CollectPaymentData memory previousBalances = CollectPaymentData({ escrowBalance: token.balanceOf(address(escrow)), paymentsBalance: token.balanceOf(address(payments)), receiverBalance: token.balanceOf(_receiver), delegationPoolBalance: staking.getDelegatedTokensAvailable(_receiver, _dataService), - dataServiceBalance: token.balanceOf(_dataService) + dataServiceBalance: token.balanceOf(_dataService), + payerEscrowBalance: 0 }); + { + (uint256 payerEscrowBalance, , ) = escrow.escrowAccounts(_payer, _collector, _receiver); + previousBalances.payerEscrowBalance = payerEscrowBalance; + } + vm.expectEmit(address(escrow)); emit IPaymentsEscrow.EscrowCollected(_payer, _collector, _receiver, _tokens); - escrow.collect(_paymentType, _payer, _receiver, _tokens, _dataService, _tokensDataService); + escrow.collect(_paymentType, _payer, _receiver, _tokens, _dataService, _dataServiceCut); // Calculate cuts - uint256 protocolPaymentCut = payments.PROTOCOL_PAYMENT_CUT(); - uint256 delegatorCut = staking.getDelegationFeeCut(_receiver, _dataService, _paymentType); + // this is nasty but stack is indeed too deep + uint256 tokensDataService = (_tokens - _tokens.mulPPMRoundUp(payments.PROTOCOL_PAYMENT_CUT())).mulPPMRoundUp( + _dataServiceCut + ); + uint256 tokensDelegation = (_tokens - + _tokens.mulPPMRoundUp(payments.PROTOCOL_PAYMENT_CUT()) - + tokensDataService).mulPPMRoundUp(staking.getDelegationFeeCut(_receiver, _dataService, _paymentType)); + uint256 receiverExpectedPayment = _tokens - + _tokens.mulPPMRoundUp(payments.PROTOCOL_PAYMENT_CUT()) - + tokensDataService - + tokensDelegation; // After balances - (uint256 afterPayerEscrowBalance, , ) = escrow.escrowAccounts(_payer, _collector, _receiver); CollectPaymentData memory afterBalances = CollectPaymentData({ escrowBalance: token.balanceOf(address(escrow)), paymentsBalance: token.balanceOf(address(payments)), receiverBalance: token.balanceOf(_receiver), delegationPoolBalance: staking.getDelegatedTokensAvailable(_receiver, _dataService), - dataServiceBalance: token.balanceOf(_dataService) + dataServiceBalance: token.balanceOf(_dataService), + payerEscrowBalance: 0 }); + { + (uint256 afterPayerEscrowBalance, , ) = escrow.escrowAccounts(_payer, _collector, _receiver); + afterBalances.payerEscrowBalance = afterPayerEscrowBalance; + } // Check receiver balance after payment - uint256 receiverExpectedPayment = _tokens - - _tokensDataService - - (_tokens * protocolPaymentCut) / - MAX_PPM - - (_tokens * delegatorCut) / - MAX_PPM; assertEq(afterBalances.receiverBalance - previousBalances.receiverBalance, receiverExpectedPayment); assertEq(token.balanceOf(address(payments)), 0); // Check delegation pool balance after payment - assertEq( - afterBalances.delegationPoolBalance - previousBalances.delegationPoolBalance, - (_tokens * delegatorCut) / MAX_PPM - ); + assertEq(afterBalances.delegationPoolBalance - previousBalances.delegationPoolBalance, tokensDelegation); // Check that the escrow account has been updated assertEq(previousBalances.escrowBalance, afterBalances.escrowBalance + _tokens); @@ -122,9 +135,9 @@ contract GraphEscrowTest is HorizonStakingSharedTest, PaymentsEscrowSharedTest { assertEq(previousBalances.paymentsBalance, afterBalances.paymentsBalance); // Check data service balance after payment - assertEq(afterBalances.dataServiceBalance - previousBalances.dataServiceBalance, _tokensDataService); + assertEq(afterBalances.dataServiceBalance - previousBalances.dataServiceBalance, tokensDataService); // Check payers escrow balance after payment - assertEq(previousPayerEscrowBalance - _tokens, afterPayerEscrowBalance); + assertEq(previousBalances.payerEscrowBalance - _tokens, afterBalances.payerEscrowBalance); } } diff --git a/packages/horizon/test/escrow/collect.t.sol b/packages/horizon/test/escrow/collect.t.sol index 711dd7d19..106582beb 100644 --- a/packages/horizon/test/escrow/collect.t.sol +++ b/packages/horizon/test/escrow/collect.t.sol @@ -16,18 +16,16 @@ contract GraphEscrowCollectTest is GraphEscrowTest { function testCollect_Tokens( uint256 tokens, uint256 delegationTokens, - uint256 tokensDataService + uint256 dataServiceCut ) public useIndexer useProvision(tokens, 0, 0) useDelegationFeeCut(IGraphPayments.PaymentTypes.QueryFee, delegationFeeCut) { - uint256 tokensProtocol = (tokens * protocolPaymentCut) / MAX_PPM; - uint256 tokensDelegatoion = (tokens * delegationFeeCut) / MAX_PPM; - vm.assume(tokensDataService < tokens - tokensProtocol - tokensDelegatoion); - + dataServiceCut = bound(dataServiceCut, 0, MAX_PPM); delegationTokens = bound(delegationTokens, 1, MAX_STAKING_TOKENS); + resetPrank(users.delegator); _delegate(users.indexer, subgraphDataServiceAddress, delegationTokens, 0); @@ -41,7 +39,7 @@ contract GraphEscrowCollectTest is GraphEscrowTest { users.indexer, tokens, subgraphDataServiceAddress, - tokensDataService + dataServiceCut ); } diff --git a/packages/horizon/test/payments/GraphPayments.t.sol b/packages/horizon/test/payments/GraphPayments.t.sol index 19028bde1..559180bf1 100644 --- a/packages/horizon/test/payments/GraphPayments.t.sol +++ b/packages/horizon/test/payments/GraphPayments.t.sol @@ -8,8 +8,10 @@ import { IGraphPayments } from "../../contracts/interfaces/IGraphPayments.sol"; import { GraphPayments } from "../../contracts/payments/GraphPayments.sol"; import { HorizonStakingSharedTest } from "../shared/horizon-staking/HorizonStakingShared.t.sol"; +import { PPMMath } from "../../contracts/libraries/PPMMath.sol"; contract GraphPaymentsTest is HorizonStakingSharedTest { + using PPMMath for uint256; struct CollectPaymentData { uint256 escrowBalance; @@ -24,54 +26,52 @@ contract GraphPaymentsTest is HorizonStakingSharedTest { address _receiver, uint256 _tokens, address _dataService, - uint256 _tokensDataService + uint256 _dataServiceCut ) private { // Previous balances CollectPaymentData memory previousBalances = CollectPaymentData({ escrowBalance: token.balanceOf(address(escrow)), paymentsBalance: token.balanceOf(address(payments)), receiverBalance: token.balanceOf(_receiver), - delegationPoolBalance: staking.getDelegatedTokensAvailable( - _receiver, - _dataService - ), + delegationPoolBalance: staking.getDelegatedTokensAvailable(_receiver, _dataService), dataServiceBalance: token.balanceOf(_dataService) }); // Calculate cuts - uint256 protocolPaymentCut = payments.PROTOCOL_PAYMENT_CUT(); - uint256 delegatorCut = staking.getDelegationFeeCut( - _receiver, - _dataService, - _paymentType + uint256 tokensProtocol = _tokens.mulPPMRoundUp(payments.PROTOCOL_PAYMENT_CUT()); + uint256 tokensDataService = (_tokens - tokensProtocol).mulPPMRoundUp(_dataServiceCut); + uint256 tokensDelegation = (_tokens - tokensProtocol - tokensDataService).mulPPMRoundUp( + staking.getDelegationFeeCut(_receiver, _dataService, _paymentType) ); - uint256 tokensProtocol = _tokens * protocolPaymentCut / MAX_PPM; - uint256 tokensDelegation = _tokens * delegatorCut / MAX_PPM; - uint256 receiverExpectedPayment = _tokens - _tokensDataService - tokensProtocol - tokensDelegation; + uint256 receiverExpectedPayment = _tokens - tokensProtocol - tokensDataService - tokensDelegation; - (,address msgSender, ) = vm.readCallers(); + (, address msgSender, ) = vm.readCallers(); vm.expectEmit(address(payments)); - emit IGraphPayments.PaymentCollected( + emit IGraphPayments.GraphPaymentCollected( msgSender, _receiver, _dataService, - receiverExpectedPayment, + _tokens, + tokensProtocol, + tokensDataService, tokensDelegation, - _tokensDataService, - tokensProtocol + receiverExpectedPayment + ); + payments.collect( + _paymentType, + _receiver, + _tokens, + _dataService, + _dataServiceCut ); - payments.collect(_paymentType, _receiver, _tokens, _dataService, _tokensDataService); // After balances CollectPaymentData memory afterBalances = CollectPaymentData({ escrowBalance: token.balanceOf(address(escrow)), paymentsBalance: token.balanceOf(address(payments)), receiverBalance: token.balanceOf(_receiver), - delegationPoolBalance: staking.getDelegatedTokensAvailable( - _receiver, - _dataService - ), + delegationPoolBalance: staking.getDelegatedTokensAvailable(_receiver, _dataService), dataServiceBalance: token.balanceOf(_dataService) }); @@ -89,7 +89,7 @@ contract GraphPaymentsTest is HorizonStakingSharedTest { assertEq(previousBalances.paymentsBalance, afterBalances.paymentsBalance); // Check data service balance after payment - assertEq(afterBalances.dataServiceBalance - previousBalances.dataServiceBalance, _tokensDataService); + assertEq(afterBalances.dataServiceBalance - previousBalances.dataServiceBalance, tokensDataService); } /* @@ -97,11 +97,11 @@ contract GraphPaymentsTest is HorizonStakingSharedTest { */ function testConstructor_RevertIf_InvalidProtocolPaymentCut(uint256 protocolPaymentCut) public { - protocolPaymentCut = bound(protocolPaymentCut, MAX_PPM + 1, MAX_PPM + 100); + protocolPaymentCut = bound(protocolPaymentCut, MAX_PPM + 1, type(uint256).max); resetPrank(users.deployer); bytes memory expectedError = abi.encodeWithSelector( - IGraphPayments.GraphPaymentsInvalidProtocolPaymentCut.selector, + IGraphPayments.GraphPaymentsInvalidCut.selector, protocolPaymentCut ); vm.expectRevert(expectedError); @@ -110,12 +110,15 @@ contract GraphPaymentsTest is HorizonStakingSharedTest { function testCollect( uint256 amount, - uint256 tokensDataService, + uint256 dataServiceCut, uint256 tokensDelegate - ) public useIndexer useProvision(amount, 0, 0) useDelegationFeeCut(IGraphPayments.PaymentTypes.QueryFee, delegationFeeCut) { - uint256 tokensProtocol = amount * protocolPaymentCut / MAX_PPM; - uint256 tokensDelegation = amount * delegationFeeCut / MAX_PPM; - vm.assume(tokensDataService < amount - tokensProtocol - tokensDelegation); + ) + public + useIndexer + useProvision(amount, 0, 0) + useDelegationFeeCut(IGraphPayments.PaymentTypes.QueryFee, delegationFeeCut) + { + dataServiceCut = bound(dataServiceCut, 0, MAX_PPM); address escrowAddress = address(escrow); // Delegate tokens @@ -129,35 +132,50 @@ contract GraphPaymentsTest is HorizonStakingSharedTest { approve(address(payments), amount); // Collect payments through GraphPayments - _collect(IGraphPayments.PaymentTypes.QueryFee, users.indexer, amount, subgraphDataServiceAddress, tokensDataService); + _collect( + IGraphPayments.PaymentTypes.QueryFee, + users.indexer, + amount, + subgraphDataServiceAddress, + dataServiceCut + ); vm.stopPrank(); } - function testCollect_RevertWhen_InsufficientAmount( + function testCollect_RevertWhen_InvalidDataServiceCut( uint256 amount, - uint256 tokensDataService - ) public useIndexer useProvision(amount, 0, 0) useDelegationFeeCut(IGraphPayments.PaymentTypes.QueryFee, delegationFeeCut) { - tokensDataService = bound(tokensDataService, amount + 1, MAX_STAKING_TOKENS + 1); - - address escrowAddress = address(escrow); - mint(escrowAddress, amount); - vm.startPrank(escrowAddress); - approve(address(payments), amount); + uint256 dataServiceCut + ) + public + useIndexer + useProvision(amount, 0, 0) + useDelegationFeeCut(IGraphPayments.PaymentTypes.QueryFee, delegationFeeCut) + { + dataServiceCut = bound(dataServiceCut, MAX_PPM + 1, type(uint256).max); - bytes memory expectedError; - { - uint256 tokensProtocol = amount * protocolPaymentCut / MAX_PPM; - uint256 tokensDelegatoion = amount * delegationFeeCut / MAX_PPM; - uint256 requiredAmount = tokensDataService + tokensProtocol + tokensDelegatoion; - expectedError = abi.encodeWithSignature("GraphPaymentsInsufficientTokens(uint256,uint256)", amount, requiredAmount); - } + resetPrank(users.deployer); + bytes memory expectedError = abi.encodeWithSelector( + IGraphPayments.GraphPaymentsInvalidCut.selector, + dataServiceCut + ); vm.expectRevert(expectedError); - payments.collect(IGraphPayments.PaymentTypes.QueryFee, users.indexer, amount, subgraphDataServiceAddress, tokensDataService); + payments.collect( + IGraphPayments.PaymentTypes.QueryFee, + users.indexer, + amount, + subgraphDataServiceAddress, + dataServiceCut + ); } function testCollect_RevertWhen_InvalidPool( uint256 amount - ) public useIndexer useProvision(amount, 0, 0) useDelegationFeeCut(IGraphPayments.PaymentTypes.QueryFee, delegationFeeCut) { + ) + public + useIndexer + useProvision(amount, 0, 0) + useDelegationFeeCut(IGraphPayments.PaymentTypes.QueryFee, delegationFeeCut) + { vm.assume(amount > 1 ether); address escrowAddress = address(escrow); @@ -167,11 +185,13 @@ contract GraphPaymentsTest is HorizonStakingSharedTest { approve(address(payments), amount); // Collect payments through GraphPayments - vm.expectRevert(abi.encodeWithSelector( - IHorizonStakingMain.HorizonStakingInvalidDelegationPool.selector, - users.indexer, - subgraphDataServiceAddress - )); + vm.expectRevert( + abi.encodeWithSelector( + IHorizonStakingMain.HorizonStakingInvalidDelegationPool.selector, + users.indexer, + subgraphDataServiceAddress + ) + ); payments.collect(IGraphPayments.PaymentTypes.QueryFee, users.indexer, amount, subgraphDataServiceAddress, 1); } @@ -181,18 +201,20 @@ contract GraphPaymentsTest is HorizonStakingSharedTest { vm.assume(amount > 1 ether); vm.assume(amount <= MAX_STAKING_TOKENS); address escrowAddress = address(escrow); - + // Add tokens in escrow mint(escrowAddress, amount); vm.startPrank(escrowAddress); approve(address(payments), amount); // Collect payments through GraphPayments - vm.expectRevert(abi.encodeWithSelector( - IHorizonStakingMain.HorizonStakingInvalidProvision.selector, - users.indexer, - subgraphDataServiceAddress - )); + vm.expectRevert( + abi.encodeWithSelector( + IHorizonStakingMain.HorizonStakingInvalidProvision.selector, + users.indexer, + subgraphDataServiceAddress + ) + ); payments.collect(IGraphPayments.PaymentTypes.QueryFee, users.indexer, amount, subgraphDataServiceAddress, 1); } } diff --git a/packages/horizon/test/payments/tap-collector/TAPCollector.t.sol b/packages/horizon/test/payments/tap-collector/TAPCollector.t.sol index ac67d6552..362cff8af 100644 --- a/packages/horizon/test/payments/tap-collector/TAPCollector.t.sol +++ b/packages/horizon/test/payments/tap-collector/TAPCollector.t.sol @@ -60,12 +60,12 @@ contract TAPCollectorTest is HorizonStakingSharedTest, PaymentsEscrowSharedTest function _authorizeSigner(address _signer, uint256 _proofDeadline, bytes memory _proof) internal { (, address msgSender, ) = vm.readCallers(); - + vm.expectEmit(address(tapCollector)); emit ITAPCollector.SignerAuthorized(msgSender, _signer); - + tapCollector.authorizeSigner(_signer, _proofDeadline, _proof); - + (address _payer, uint256 thawEndTimestamp, bool revoked) = tapCollector.authorizedSigners(_signer); assertEq(_payer, msgSender); assertEq(thawEndTimestamp, 0); @@ -111,7 +111,9 @@ contract TAPCollectorTest is HorizonStakingSharedTest, PaymentsEscrowSharedTest tapCollector.revokeAuthorizedSigner(_signer); - (address afterPayer, uint256 afterThawEndTimestamp, bool afterRevoked) = tapCollector.authorizedSigners(_signer); + (address afterPayer, uint256 afterThawEndTimestamp, bool afterRevoked) = tapCollector.authorizedSigners( + _signer + ); assertEq(beforePayer, afterPayer); assertEq(beforeThawEndTimestamp, afterThawEndTimestamp); @@ -126,23 +128,34 @@ contract TAPCollectorTest is HorizonStakingSharedTest, PaymentsEscrowSharedTest __collect(_paymentType, _data, _tokensToCollect); } - function __collect(IGraphPayments.PaymentTypes _paymentType, bytes memory _data, uint256 _tokensToCollect) internal { - (ITAPCollector.SignedRAV memory signedRAV, uint256 dataServiceCut) = abi.decode(_data, (ITAPCollector.SignedRAV, uint256)); + function __collect( + IGraphPayments.PaymentTypes _paymentType, + bytes memory _data, + uint256 _tokensToCollect + ) internal { + (ITAPCollector.SignedRAV memory signedRAV, ) = abi.decode( + _data, + (ITAPCollector.SignedRAV, uint256) + ); bytes32 messageHash = tapCollector.encodeRAV(signedRAV.rav); address _signer = ECDSA.recover(messageHash, signedRAV.signature); (address _payer, , ) = tapCollector.authorizedSigners(_signer); - uint256 tokensAlreadyCollected = tapCollector.tokensCollected(signedRAV.rav.dataService, signedRAV.rav.serviceProvider, _payer); - uint256 tokensToCollect = _tokensToCollect == 0 ? signedRAV.rav.valueAggregate - tokensAlreadyCollected : _tokensToCollect; - uint256 tokensDataService = tokensToCollect.mulPPM(dataServiceCut); - + uint256 tokensAlreadyCollected = tapCollector.tokensCollected( + signedRAV.rav.dataService, + signedRAV.rav.serviceProvider, + _payer + ); + uint256 tokensToCollect = _tokensToCollect == 0 + ? signedRAV.rav.valueAggregate - tokensAlreadyCollected + : _tokensToCollect; + vm.expectEmit(address(tapCollector)); emit IPaymentsCollector.PaymentCollected( - _paymentType, + _paymentType, _payer, signedRAV.rav.serviceProvider, - tokensToCollect, signedRAV.rav.dataService, - tokensDataService + tokensToCollect ); vm.expectEmit(address(tapCollector)); emit ITAPCollector.RAVCollected( @@ -154,10 +167,19 @@ contract TAPCollectorTest is HorizonStakingSharedTest, PaymentsEscrowSharedTest signedRAV.rav.metadata, signedRAV.signature ); - uint256 tokensCollected = _tokensToCollect == 0 ? tapCollector.collect(_paymentType, _data) : tapCollector.collect(_paymentType, _data, _tokensToCollect); + uint256 tokensCollected = _tokensToCollect == 0 + ? tapCollector.collect(_paymentType, _data) + : tapCollector.collect(_paymentType, _data, _tokensToCollect); - uint256 tokensCollectedAfter = tapCollector.tokensCollected(signedRAV.rav.dataService, signedRAV.rav.serviceProvider, _payer); + uint256 tokensCollectedAfter = tapCollector.tokensCollected( + signedRAV.rav.dataService, + signedRAV.rav.serviceProvider, + _payer + ); assertEq(tokensCollected, tokensToCollect); - assertEq(tokensCollectedAfter, _tokensToCollect == 0 ? signedRAV.rav.valueAggregate : tokensAlreadyCollected + _tokensToCollect); + assertEq( + tokensCollectedAfter, + _tokensToCollect == 0 ? signedRAV.rav.valueAggregate : tokensAlreadyCollected + _tokensToCollect + ); } } diff --git a/packages/subgraph-service/contracts/SubgraphService.sol b/packages/subgraph-service/contracts/SubgraphService.sol index a07463a25..b1403124b 100644 --- a/packages/subgraph-service/contracts/SubgraphService.sol +++ b/packages/subgraph-service/contracts/SubgraphService.sol @@ -556,13 +556,10 @@ contract SubgraphService is IGraphPayments.PaymentTypes.QueryFee, abi.encode(_signedRav, curationCut) ); - uint256 tokensCurators = tokensCollected.mulPPM(curationCut); uint256 balanceAfter = _graphToken().balanceOf(address(this)); - require( - balanceBefore + tokensCurators == balanceAfter, - SubgraphServiceInconsistentCollection(balanceBefore, balanceAfter, tokensCurators) - ); + require(balanceAfter >= balanceBefore, SubgraphServiceInconsistentCollection(balanceBefore, balanceAfter)); + uint256 tokensCurators = balanceAfter - balanceBefore; if (tokensCollected > 0) { // lock stake as economic security for fees diff --git a/packages/subgraph-service/contracts/interfaces/ISubgraphService.sol b/packages/subgraph-service/contracts/interfaces/ISubgraphService.sol index 32ea9e8fb..44b9b3e6d 100644 --- a/packages/subgraph-service/contracts/interfaces/ISubgraphService.sol +++ b/packages/subgraph-service/contracts/interfaces/ISubgraphService.sol @@ -80,13 +80,11 @@ interface ISubgraphService is IDataServiceFees { error SubgraphServiceInvalidPaymentType(IGraphPayments.PaymentTypes paymentType); /** - * @notice Thrown when the contract GRT balance is inconsistent with the payment amount collected - * from Graph Payments + * @notice Thrown when the contract GRT balance is inconsistent after collecting from Graph Payments * @param balanceBefore The contract GRT balance before the collection * @param balanceAfter The contract GRT balance after the collection - * @param tokensDataService The amount of tokens sent to the subgraph service */ - error SubgraphServiceInconsistentCollection(uint256 balanceBefore, uint256 balanceAfter, uint256 tokensDataService); + error SubgraphServiceInconsistentCollection(uint256 balanceBefore, uint256 balanceAfter); /** * @notice @notice Thrown when the service provider in the RAV does not match the expected indexer. diff --git a/packages/subgraph-service/test/subgraphService/SubgraphService.t.sol b/packages/subgraph-service/test/subgraphService/SubgraphService.t.sol index 64842b1f2..619ad4bab 100644 --- a/packages/subgraph-service/test/subgraphService/SubgraphService.t.sol +++ b/packages/subgraph-service/test/subgraphService/SubgraphService.t.sol @@ -242,7 +242,8 @@ contract SubgraphServiceTest is SubgraphServiceSharedTest { // Calculate curation cut uint256 curationFeesCut = subgraphService.curationFeesCut(); queryFeeData.curationCut = curation.isCurated(allocation.subgraphDeploymentId) ? curationFeesCut : 0; - uint256 tokensCurators = paymentCollected.mulPPM(queryFeeData.curationCut); + uint256 tokensProtocol = paymentCollected.mulPPMRoundUp(queryFeeData.protocolPaymentCut); + uint256 tokensCurators = (paymentCollected - tokensProtocol).mulPPMRoundUp(queryFeeData.curationCut); vm.expectEmit(address(subgraphService)); emit ISubgraphService.QueryFeesCollected(_indexer, paymentCollected, tokensCurators); @@ -302,8 +303,8 @@ contract SubgraphServiceTest is SubgraphServiceSharedTest { if (_paymentType == IGraphPayments.PaymentTypes.QueryFee) { // Check indexer got paid the correct amount { - uint256 tokensProtocol = paymentCollected.mulPPM(protocolPaymentCut); - uint256 curationTokens = paymentCollected.mulPPM(queryFeeData.curationCut); + uint256 tokensProtocol = paymentCollected.mulPPMRoundUp(protocolPaymentCut); + uint256 curationTokens = (paymentCollected - tokensProtocol).mulPPMRoundUp(queryFeeData.curationCut); uint256 expectedIndexerTokensPayment = paymentCollected - tokensProtocol - curationTokens; assertEq( collectPaymentDataAfter.indexerBalance,