fix: remove collector allowance feature from payments escrow (TRST-CL1)

Signed-off-by: Tomás Migone <tomas@edgeandnode.com>
graphprotocol · Nov 28, 2024 · ec69bf9 · ec69bf9
1 parent 71bf587
commit ec69bf9
Show file tree

Hide file tree

Showing 16 changed files with 10 additions and 384 deletions.
diff --git a/packages/horizon/contracts/interfaces/IPaymentsEscrow.sol b/packages/horizon/contracts/interfaces/IPaymentsEscrow.sol
@@ -25,50 +25,6 @@ interface IPaymentsEscrow {
         uint256 thawEndTimestamp;
     }
 
-    /// @notice Details for a payer-collector pair
-    /// @dev Collectors can be removed only after a thawing period
-    struct Collector {
-        // Amount of tokens the collector is allowed to collect
-        uint256 allowance;
-        // Timestamp at which the collector thawing period ends (zero if not thawing)
-        uint256 thawEndTimestamp;
-    }
-
-    /**
-     * @notice Emitted when a payer authorizes a collector to collect funds
-     * @param payer The address of the payer
-     * @param collector The address of the collector
-     * @param addedAllowance The amount of tokens added to the collector's allowance
-     * @param newTotalAllowance The new total allowance after addition
-     */
-    event AuthorizedCollector(
-        address indexed payer,
-        address indexed collector,
-        uint256 addedAllowance,
-        uint256 newTotalAllowance
-    );
-
-    /**
-     * @notice Emitted when a payer thaws a collector
-     * @param payer The address of the payer
-     * @param collector The address of the collector
-     */
-    event ThawCollector(address indexed payer, address indexed collector);
-
-    /**
-     * @notice Emitted when a payer cancels the thawing of a collector
-     * @param payer The address of the payer
-     * @param collector The address of the collector
-     */
-    event CancelThawCollector(address indexed payer, address indexed collector);
-
-    /**
-     * @notice Emitted when a payer revokes a collector authorization.
-     * @param payer The address of the payer
-     * @param collector The address of the collector
-     */
-    event RevokeCollector(address indexed payer, address indexed collector);
-
     /**
      * @notice Emitted when a payer deposits funds into the escrow for a payer-collector-receiver tuple
      * @param payer The address of the payer
@@ -152,13 +108,6 @@ interface IPaymentsEscrow {
      */
     error PaymentsEscrowThawingPeriodTooLong(uint256 thawingPeriod, uint256 maxWaitPeriod);
 
-    /**
-     * @notice Thrown when a collector has insufficient allowance to collect funds
-     * @param allowance The current allowance
-     * @param minAllowance The minimum required allowance
-     */
-    error PaymentsEscrowInsufficientAllowance(uint256 allowance, uint256 minAllowance);
-
     /**
      * @notice Thrown when the contract balance is not consistent with the collection amount
      * @param balanceBefore The balance before the collection
@@ -172,54 +121,6 @@ interface IPaymentsEscrow {
      */
     error PaymentsEscrowInvalidZeroTokens();
 
-    /**
-     * @notice Authorize a collector to collect funds from the payer's escrow
-     * @dev This function can only be used to increase the allowance of a collector.
-     * To reduce it the authorization must be revoked and a new one must be created.
-     *
-     * Requirements:
-     * - `allowance` must be greater than zero
-     *
-     * Emits an {AuthorizedCollector} event
-     *
-     * @param collector The address of the collector
-     * @param allowance The amount of tokens to add to the collector's allowance
-     */
-    function approveCollector(address collector, uint256 allowance) external;
-
-    /**
-     * @notice Thaw a collector's collector authorization
-     * @dev The thawing period is defined by the `REVOKE_COLLECTOR_THAWING_PERIOD` constant
-     *
-     * Emits a {ThawCollector} event
-     *
-     * @param collector The address of the collector
-     */
-    function thawCollector(address collector) external;
-
-    /**
-     * @notice Cancel a collector's authorization thawing
-     * @dev Requirements:
-     * - `collector` must be thawing
-     *
-     * Emits a {CancelThawCollector} event
-     *
-     * @param collector The address of the collector
-     */
-    function cancelThawCollector(address collector) external;
-
-    /**
-     * @notice Revoke a collector's authorization.
-     * Removes the collector from the list of authorized collectors.
-     * @dev Requirements:
-     * - `collector` must have thawed
-     *
-     * Emits a {RevokeCollector} event
-     *
-     * @param collector The address of the collector
-     */
-    function revokeCollector(address collector) external;
-
     /**
      * @notice Deposits funds into the escrow for a payer-collector-receiver tuple, where
      * the payer is the transaction caller.
@@ -277,8 +178,6 @@ interface IPaymentsEscrow {
      * @notice Collects funds from the payer-collector-receiver's escrow and sends them to {GraphPayments} for
      * distribution using the Graph Horizon Payments protocol.
      * The function will revert if there are not enough funds in the escrow.
-     * @dev Requirements:
-     * - `collector` needs to be authorized by the payer and have enough allowance
      *
      * Emits an {EscrowCollected} event
      *

diff --git a/packages/horizon/contracts/payments/PaymentsEscrow.sol b/packages/horizon/contracts/payments/PaymentsEscrow.sol
@@ -23,10 +23,6 @@ import { GraphDirectory } from "../utilities/GraphDirectory.sol";
 contract PaymentsEscrow is Initializable, MulticallUpgradeable, GraphDirectory, IPaymentsEscrow {
     using TokenUtils for IGraphToken;
 
-    /// @notice Authorization details for payer-collector pairs
-    mapping(address payer => mapping(address collector => IPaymentsEscrow.Collector collectorDetails))
-        public authorizedCollectors;
-
     /// @notice Escrow account details for payer-collector-receiver tuples
     mapping(address payer => mapping(address collector => mapping(address receiver => IPaymentsEscrow.EscrowAccount escrowAccount)))
         public escrowAccounts;
@@ -35,9 +31,6 @@ contract PaymentsEscrow is Initializable, MulticallUpgradeable, GraphDirectory,
     /// @dev This is a precautionary measure to avoid inadvertedly locking funds for too long
     uint256 public constant MAX_WAIT_PERIOD = 90 days;
 
-    /// @notice Thawing period in seconds for authorized collectors
-    uint256 public immutable REVOKE_COLLECTOR_THAWING_PERIOD;
-
     /// @notice Thawing period in seconds for escrow funds withdrawal
     uint256 public immutable WITHDRAW_ESCROW_THAWING_PERIOD;
 
@@ -49,24 +42,14 @@ contract PaymentsEscrow is Initializable, MulticallUpgradeable, GraphDirectory,
     /**
      * @notice Construct the PaymentsEscrow contract
      * @param controller The address of the controller
-     * @param revokeCollectorThawingPeriod Thawing period in seconds for authorized collectors
      * @param withdrawEscrowThawingPeriod Thawing period in seconds for escrow funds withdrawal
      */
-    constructor(
-        address controller,
-        uint256 revokeCollectorThawingPeriod,
-        uint256 withdrawEscrowThawingPeriod
-    ) GraphDirectory(controller) {
-        require(
-            revokeCollectorThawingPeriod <= MAX_WAIT_PERIOD,
-            PaymentsEscrowThawingPeriodTooLong(revokeCollectorThawingPeriod, MAX_WAIT_PERIOD)
-        );
+    constructor(address controller, uint256 withdrawEscrowThawingPeriod) GraphDirectory(controller) {
         require(
             withdrawEscrowThawingPeriod <= MAX_WAIT_PERIOD,
             PaymentsEscrowThawingPeriodTooLong(withdrawEscrowThawingPeriod, MAX_WAIT_PERIOD)
         );
 
-        REVOKE_COLLECTOR_THAWING_PERIOD = revokeCollectorThawingPeriod;
         WITHDRAW_ESCROW_THAWING_PERIOD = withdrawEscrowThawingPeriod;
     }
 
@@ -77,52 +60,6 @@ contract PaymentsEscrow is Initializable, MulticallUpgradeable, GraphDirectory,
         __Multicall_init();
     }
 
-    /**
-     * @notice See {IPaymentsEscrow-approveCollector}
-     */
-    function approveCollector(address collector_, uint256 allowance) external override notPaused {
-        require(allowance != 0, PaymentsEscrowInvalidZeroTokens());
-        Collector storage collector = authorizedCollectors[msg.sender][collector_];
-        collector.allowance += allowance;
-        emit AuthorizedCollector(msg.sender, collector_, allowance, collector.allowance);
-    }
-
-    /**
-     * @notice See {IPaymentsEscrow-thawCollector}
-     */
-    function thawCollector(address collector) external override notPaused {
-        authorizedCollectors[msg.sender][collector].thawEndTimestamp =
-            block.timestamp +
-            REVOKE_COLLECTOR_THAWING_PERIOD;
-        emit ThawCollector(msg.sender, collector);
-    }
-
-    /**
-     * @notice See {IPaymentsEscrow-cancelThawCollector}
-     */
-    function cancelThawCollector(address collector) external override notPaused {
-        require(authorizedCollectors[msg.sender][collector].thawEndTimestamp != 0, PaymentsEscrowNotThawing());
-
-        authorizedCollectors[msg.sender][collector].thawEndTimestamp = 0;
-        emit CancelThawCollector(msg.sender, collector);
-    }
-
-    /**
-     * @notice See {IPaymentsEscrow-revokeCollector}
-     */
-    function revokeCollector(address collector_) external override notPaused {
-        Collector storage collector = authorizedCollectors[msg.sender][collector_];
-
-        require(collector.thawEndTimestamp != 0, PaymentsEscrowNotThawing());
-        require(
-            collector.thawEndTimestamp < block.timestamp,
-            PaymentsEscrowStillThawing(block.timestamp, collector.thawEndTimestamp)
-        );
-
-        delete authorizedCollectors[msg.sender][collector_];
-        emit RevokeCollector(msg.sender, collector_);
-    }
-
     /**
      * @notice See {IPaymentsEscrow-deposit}
      */
@@ -194,19 +131,11 @@ contract PaymentsEscrow is Initializable, MulticallUpgradeable, GraphDirectory,
         address dataService,
         uint256 tokensDataService
     ) external override notPaused {
-        // Check if collector is authorized and has enough funds
-        Collector storage collectorDetails = authorizedCollectors[payer][msg.sender];
-        require(
-            collectorDetails.allowance >= tokens,
-            PaymentsEscrowInsufficientAllowance(collectorDetails.allowance, tokens)
-        );
-
         // Check if there are enough funds in the escrow account
         EscrowAccount storage account = escrowAccounts[payer][msg.sender][receiver];
         require(account.balance >= tokens, PaymentsEscrowInsufficientBalance(account.balance, tokens));
 
-        // Reduce amount from approved collector and account balance
-        collectorDetails.allowance -= tokens;
+        // Reduce amount from account balance
         account.balance -= tokens;
 
         uint256 balanceBefore = _graphToken().balanceOf(address(this));

diff --git a/packages/horizon/ignition/configs/horizon.hardhat.json b/packages/horizon/ignition/configs/horizon.hardhat.json
@@ -36,7 +36,6 @@
     "protocolPaymentCut": 10000
   },
   "PaymentsEscrow": {
-    "revokeCollectorThawingPeriod": 10000,
     "withdrawEscrowThawingPeriod": 10000
   },
   "TAPCollector": {

diff --git a/packages/horizon/ignition/modules/core/PaymentsEscrow.ts b/packages/horizon/ignition/modules/core/PaymentsEscrow.ts
@@ -10,13 +10,12 @@ export default buildModule('PaymentsEscrow', (m) => {
   const { Controller, PeripheryRegistered } = m.useModule(GraphPeripheryModule)
   const { PaymentsEscrowProxyAdmin, PaymentsEscrowProxy, HorizonRegistered } = m.useModule(HorizonProxiesModule)
 
-  const revokeCollectorThawingPeriod = m.getParameter('revokeCollectorThawingPeriod')
   const withdrawEscrowThawingPeriod = m.getParameter('withdrawEscrowThawingPeriod')
 
   // Deploy PaymentsEscrow implementation
   const PaymentsEscrowImplementation = m.contract('PaymentsEscrow',
     PaymentsEscrowArtifact,
-    [Controller, revokeCollectorThawingPeriod, withdrawEscrowThawingPeriod],
+    [Controller, withdrawEscrowThawingPeriod],
     {
       after: [PeripheryRegistered, HorizonRegistered],
     },

diff --git a/packages/horizon/test/GraphBase.t.sol b/packages/horizon/test/GraphBase.t.sol
@@ -116,7 +116,7 @@ abstract contract GraphBaseTest is IHorizonStakingTypes, Utils, Constants {
         // PaymentsEscrow
         bytes memory escrowImplementationParameters = abi.encode(
             address(controller),
-            revokeCollectorThawingPeriod,withdrawEscrowThawingPeriod
+            withdrawEscrowThawingPeriod
         );
         bytes memory escrowImplementationBytecode = abi.encodePacked(
             type(PaymentsEscrow).creationCode,

diff --git a/packages/horizon/test/escrow/GraphEscrow.t.sol b/packages/horizon/test/escrow/GraphEscrow.t.sol
@@ -24,12 +24,6 @@ contract GraphEscrowTest is HorizonStakingSharedTest, PaymentsEscrowSharedTest {
         _;
     }
 
-    modifier useCollector(uint256 tokens) {
-        vm.assume(tokens > 0);
-        escrow.approveCollector(users.verifier, tokens);
-        _;
-    }
-
     modifier depositAndThawTokens(uint256 amount, uint256 thawAmount) {
         vm.assume(thawAmount > 0);
         vm.assume(amount > thawAmount);

diff --git a/packages/horizon/test/escrow/collect.t.sol b/packages/horizon/test/escrow/collect.t.sol
@@ -106,47 +106,17 @@ contract GraphEscrowCollectTest is GraphEscrowTest {
         _delegate(users.indexer, subgraphDataServiceAddress, delegationTokens, 0);
 
         resetPrank(users.gateway);
-        escrow.approveCollector(users.verifier, tokens);
         _depositTokens(users.verifier, users.indexer, tokens);
 
         resetPrank(users.verifier);
         _collect(IGraphPayments.PaymentTypes.QueryFee, users.gateway, users.indexer, tokens, subgraphDataServiceAddress, tokensDataService);
     }
 
-    function testCollect_RevertWhen_CollectorNotAuthorized(uint256 amount) public {
-        vm.assume(amount > 0);
-        vm.startPrank(users.verifier);
-        uint256 dataServiceCut = 30000; // 3%
-        bytes memory expectedError = abi.encodeWithSelector(
-            IPaymentsEscrow.PaymentsEscrowInsufficientAllowance.selector,
-            0,
-            amount
-        );
-        vm.expectRevert(expectedError);
-        escrow.collect(IGraphPayments.PaymentTypes.QueryFee, users.gateway, users.indexer, amount, subgraphDataServiceAddress, dataServiceCut);
-        vm.stopPrank();
-    }
-
-    function testCollect_RevertWhen_CollectorHasInsufficientAmount(
-        uint256 amount,
-        uint256 insufficientAmount
-    ) public useGateway useCollector(insufficientAmount) useDeposit(amount) {
-        vm.assume(insufficientAmount < amount);
-
-        vm.startPrank(users.verifier);
-        bytes memory expectedError = abi.encodeWithSignature(
-            "PaymentsEscrowInsufficientAllowance(uint256,uint256)", 
-            insufficientAmount, 
-            amount
-        );
-        vm.expectRevert(expectedError);
-        escrow.collect(IGraphPayments.PaymentTypes.QueryFee, users.gateway, users.indexer, amount, subgraphDataServiceAddress, 0);
-    }
-
     function testCollect_RevertWhen_SenderHasInsufficientAmountInEscrow(
         uint256 amount, 
         uint256 insufficientAmount
-    ) public useGateway useCollector(amount) useDeposit(insufficientAmount)  {
+    ) public useGateway useDeposit(insufficientAmount)  {
+        vm.assume(amount > 0);
         vm.assume(insufficientAmount < amount);
 
         vm.startPrank(users.verifier);
@@ -162,7 +132,6 @@ contract GraphEscrowCollectTest is GraphEscrowTest {
         vm.assume(amount > 1 ether);
 
         resetPrank(users.gateway);
-        escrow.approveCollector(users.verifier, amount);
         _depositTokens(users.verifier, users.indexer, amount);
 
         resetPrank(users.verifier);
@@ -181,7 +150,6 @@ contract GraphEscrowCollectTest is GraphEscrowTest {
         vm.assume(amount <= MAX_STAKING_TOKENS);
 
         resetPrank(users.gateway);
-        escrow.approveCollector(users.verifier, amount);
         _depositTokens(users.verifier, users.indexer, amount);
 
         resetPrank(users.verifier);