Allow batch transactions using multicall in the GNS (#485)

abarmat · web-flow · commit 329e92fb3c89 · 2021-09-07T15:32:00.000-03:00
Add multicall for batched calls to the GNS

By using a multicall a user can batch multiple operations into a single transaction.
An example of this is if the owner publishing a subgraph decides to mint some signal on that subgraph.

* Use convenience token utils for transferring tokens
* Reduce bytecode by using an inline function for access
* Introduce a multicall abstract contract for batch calls
* Add security tests to multicall
diff --git a/contracts/base/IMulticall.sol b/contracts/base/IMulticall.sol
@@ -0,0 +1,17 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+
+pragma solidity ^0.7.3;
+pragma experimental ABIEncoderV2;
+
+/**
+ * @title Multicall interface
+ * @notice Enables calling multiple methods in a single call to the contract
+ */
+interface IMulticall {
+    /**
+     * @notice Call multiple functions in the current contract and return the data from all of them if they all succeed
+     * @param data The encoded function data for each of the calls to make to this contract
+     * @return results The results from each of the calls passed in via data
+     */
+    function multicall(bytes[] calldata data) external returns (bytes[] memory results);
+}
diff --git a/contracts/base/Multicall.sol b/contracts/base/Multicall.sol
@@ -0,0 +1,34 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+
+pragma solidity ^0.7.3;
+pragma experimental ABIEncoderV2;
+
+import "./IMulticall.sol";
+
+// Inspired by https://github.com/Uniswap/uniswap-v3-periphery/blob/main/contracts/base/Multicall.sol
+// Note: Removed payable from the multicall
+
+/**
+ * @title Multicall
+ * @notice Enables calling multiple methods in a single call to the contract
+ */
+abstract contract Multicall is IMulticall {
+    /// @inheritdoc IMulticall
+    function multicall(bytes[] calldata data) external override returns (bytes[] memory results) {
+        results = new bytes[](data.length);
+        for (uint256 i = 0; i < data.length; i++) {
+            (bool success, bytes memory result) = address(this).delegatecall(data[i]);
+
+            if (!success) {
+                // Next 5 lines from https://ethereum.stackexchange.com/a/83577
+                if (result.length < 68) revert();
+                assembly {
+                    result := add(result, 0x04)
+                }
+                revert(abi.decode(result, (string)));
+            }
+
+            results[i] = result;
+        }
+    }
+}
diff --git a/contracts/discovery/GNS.sol b/contracts/discovery/GNS.sol
@@ -5,8 +5,10 @@ pragma experimental ABIEncoderV2;
 
 import "@openzeppelin/contracts/math/SafeMath.sol";
 
+import "../base/Multicall.sol";
 import "../bancor/BancorFormula.sol";
 import "../upgrades/GraphUpgradeable.sol";
+import "../utils/TokenUtils.sol";
 
 import "./IGNS.sol";
 import "./GNSStorage.sol";
@@ -17,10 +19,14 @@ import "./GNSStorage.sol";
  * used in the scope of the Graph Network. It translates subgraph names into subgraph versions.
  * Each version is associated with a Subgraph Deployment. The contract has no knowledge of
  * human-readable names. All human readable names emitted in events.
+ * The contract implements a multicall behaviour to support batching multiple calls in a single
+ * transaction.
  */
-contract GNS is GNSV1Storage, GraphUpgradeable, IGNS {
+contract GNS is GNSV1Storage, GraphUpgradeable, IGNS, Multicall {
     using SafeMath for uint256;
 
+    // -- Constants --
+
     uint256 private constant MAX_UINT256 = 2**256 - 1;
 
     // 100% in parts per million
@@ -136,16 +142,28 @@ contract GNS is GNSV1Storage, GraphUpgradeable, IGNS {
         uint256 withdrawnGRT
     );
 
+    // -- Modifiers --
+
     /**
-    @dev Modifier that allows a function to be called by owner of a graph account
-    @param _graphAccount Address of the graph account
-    */
-    modifier onlyGraphAccountOwner(address _graphAccount) {
+     * @dev Check if the owner is the graph account
+     * @param _graphAccount Address of the graph account
+     */
+    function _isGraphAccountOwner(address _graphAccount) private view {
         address graphAccountOwner = erc1056Registry.identityOwner(_graphAccount);
         require(graphAccountOwner == msg.sender, "GNS: Only graph account owner can call");
+    }
+
+    /**
+     * @dev Modifier that allows a function to be called by owner of a graph account
+     * @param _graphAccount Address of the graph account
+     */
+    modifier onlyGraphAccountOwner(address _graphAccount) {
+        _isGraphAccountOwner(_graphAccount);
         _;
     }
 
+    // -- Functions --
+
     /**
      * @dev Initialize this contract.
      */
@@ -408,10 +426,7 @@ contract GNS is GNSV1Storage, GraphUpgradeable, IGNS {
         );
 
         // Pull tokens from sender
-        require(
-            graphToken().transferFrom(msg.sender, address(this), _tokensIn),
-            "GNS: Cannot transfer tokens to mint n signal"
-        );
+        TokenUtils.pullTokens(graphToken(), msg.sender, _tokensIn);
 
         // Get name signal to mint for tokens deposited
         (uint256 vSignal, ) = curation().mint(namePool.subgraphDeploymentID, _tokensIn, 0);
@@ -461,11 +476,8 @@ contract GNS is GNSV1Storage, GraphUpgradeable, IGNS {
         namePool.nSignal = namePool.nSignal.sub(_nSignal);
         namePool.curatorNSignal[msg.sender] = namePool.curatorNSignal[msg.sender].sub(_nSignal);
 
-        // Return the tokens to the nameCurator
-        require(
-            graphToken().transfer(msg.sender, tokens),
-            "GNS: Error sending nameCurators tokens"
-        );
+        // Return the tokens to the curator
+        TokenUtils.pushTokens(graphToken(), msg.sender, tokens);
 
         emit NSignalBurned(_graphAccount, _subgraphNumber, msg.sender, _nSignal, vSignal, tokens);
     }
@@ -482,6 +494,7 @@ contract GNS is GNSV1Storage, GraphUpgradeable, IGNS {
 
         // If no nSignal, then no need to burn vSignal
         if (namePool.nSignal != 0) {
+            // Note: No slippage, burn at any cost
             namePool.withdrawableGRT = curation().burn(
                 namePool.subgraphDeploymentID,
                 namePool.vSignal,
@@ -522,10 +535,8 @@ contract GNS is GNSV1Storage, GraphUpgradeable, IGNS {
         namePool.nSignal = namePool.nSignal.sub(curatorNSignal);
         namePool.withdrawableGRT = namePool.withdrawableGRT.sub(tokensOut);
 
-        require(
-            graphToken().transfer(msg.sender, tokensOut),
-            "GNS: Error withdrawing tokens for nameCurator"
-        );
+        // Return tokens to the curator
+        TokenUtils.pushTokens(graphToken(), msg.sender, tokensOut);
 
         emit GRTWithdrawn(_graphAccount, _subgraphNumber, msg.sender, curatorNSignal, tokensOut);
     }
@@ -567,10 +578,8 @@ contract GNS is GNSV1Storage, GraphUpgradeable, IGNS {
         uint256 ownerTaxAdjustedUp = totalAdjustedUp.sub(_tokens);
 
         // Get the owner of the subgraph to reimburse the curation tax
-        require(
-            graphToken().transferFrom(_owner, address(this), ownerTaxAdjustedUp),
-            "GNS: Error reimbursing curation tax"
-        );
+        TokenUtils.pullTokens(graphToken(), _owner, ownerTaxAdjustedUp);
+
         return totalAdjustedUp;
     }
 
@@ -587,8 +596,8 @@ contract GNS is GNSV1Storage, GraphUpgradeable, IGNS {
         uint256 _tokensIn
     )
         public
-        override
         view
+        override
         returns (
             uint256,
             uint256,
@@ -615,7 +624,7 @@ contract GNS is GNSV1Storage, GraphUpgradeable, IGNS {
         address _graphAccount,
         uint256 _subgraphNumber,
         uint256 _nSignalIn
-    ) public override view returns (uint256, uint256) {
+    ) public view override returns (uint256, uint256) {
         NameCurationPool storage namePool = nameSignals[_graphAccount][_subgraphNumber];
         uint256 vSignal = nSignalToVSignal(_graphAccount, _subgraphNumber, _nSignalIn);
         uint256 tokensOut = curation().signalToTokens(namePool.subgraphDeploymentID, vSignal);
@@ -633,7 +642,7 @@ contract GNS is GNSV1Storage, GraphUpgradeable, IGNS {
         address _graphAccount,
         uint256 _subgraphNumber,
         uint256 _vSignalIn
-    ) public override view returns (uint256) {
+    ) public view override returns (uint256) {
         NameCurationPool storage namePool = nameSignals[_graphAccount][_subgraphNumber];
 
         // Handle initialization by using 1:1 version to name signal
@@ -661,7 +670,7 @@ contract GNS is GNSV1Storage, GraphUpgradeable, IGNS {
         address _graphAccount,
         uint256 _subgraphNumber,
         uint256 _nSignalIn
-    ) public override view returns (uint256) {
+    ) public view override returns (uint256) {
         NameCurationPool storage namePool = nameSignals[_graphAccount][_subgraphNumber];
         return
             BancorFormula(bondingCurve).calculateSaleReturn(
@@ -683,7 +692,7 @@ contract GNS is GNSV1Storage, GraphUpgradeable, IGNS {
         address _graphAccount,
         uint256 _subgraphNumber,
         address _curator
-    ) public override view returns (uint256) {
+    ) public view override returns (uint256) {
         return nameSignals[_graphAccount][_subgraphNumber].curatorNSignal[_curator];
     }
 
@@ -695,8 +704,8 @@ contract GNS is GNSV1Storage, GraphUpgradeable, IGNS {
      */
     function isPublished(address _graphAccount, uint256 _subgraphNumber)
         public
-        override
         view
+        override
         returns (bool)
     {
         return subgraphs[_graphAccount][_subgraphNumber] != 0;
diff --git a/contracts/discovery/GNSStorage.sol b/contracts/discovery/GNSStorage.sol
@@ -6,7 +6,6 @@ pragma experimental ABIEncoderV2;
 import "../governance/Managed.sol";
 
 import "./erc1056/IEthereumDIDRegistry.sol";
-
 import "./IGNS.sol";
 
 contract GNSV1Storage is Managed {
diff --git a/test/gns.test.ts b/test/gns.test.ts
@@ -2,11 +2,11 @@ import { expect } from 'chai'
 import { ethers, ContractTransaction, BigNumber, Event } from 'ethers'
 
 import { GNS } from '../build/types/GNS'
-import { getAccounts, randomHexBytes, Account, toGRT } from './lib/testHelpers'
-import { NetworkFixture } from './lib/fixtures'
 import { GraphToken } from '../build/types/GraphToken'
 import { Curation } from '../build/types/Curation'
 
+import { getAccounts, randomHexBytes, Account, toGRT } from './lib/testHelpers'
+import { NetworkFixture } from './lib/fixtures'
 import { toBN, formatGRT } from './lib/testHelpers'
 
 interface Subgraph {
@@ -983,4 +983,68 @@ describe('GNS', () => {
       await gns.connect(me.signer).mintNSignal(me.address, 1, toGRT('10'), 0)
     })
   })
+
+  describe('batch calls', function () {
+    it('should publish new subgraph and mint signal in single transaction', async function () {
+      // Create a subgraph
+      const tx1 = await gns.populateTransaction.publishNewSubgraph(
+        me.address,
+        subgraph0.subgraphDeploymentID,
+        subgraph0.versionMetadata,
+        subgraph0.subgraphMetadata,
+      )
+      // Curate on the subgraph
+      const subgraphNumber = await gns.graphAccountSubgraphNumbers(me.address)
+      const tx2 = await gns.populateTransaction.mintNSignal(
+        me.address,
+        subgraphNumber,
+        toGRT('90000'),
+        0,
+      )
+
+      // Batch send transaction
+      await gns.connect(me.signer).multicall([tx1.data, tx2.data])
+    })
+
+    it('should revert if batching a call to non-authorized function', async function () {
+      // Call a forbidden function
+      const tx1 = await gns.populateTransaction.setOwnerTaxPercentage(100)
+
+      // Create a subgraph
+      const tx2 = await gns.populateTransaction.publishNewSubgraph(
+        me.address,
+        subgraph0.subgraphDeploymentID,
+        subgraph0.versionMetadata,
+        subgraph0.subgraphMetadata,
+      )
+
+      // Batch send transaction
+      const tx = gns.connect(me.signer).multicall([tx1.data, tx2.data])
+      await expect(tx).revertedWith('Caller must be Controller governor')
+    })
+
+    it('should revert if trying to call a private function', async function () {
+      // Craft call a private function
+      const hash = ethers.utils.id('_setOwnerTaxPercentage(uint32)')
+      const functionHash = hash.slice(0, 10)
+      const calldata = ethers.utils.arrayify(
+        ethers.utils.defaultAbiCoder.encode(['uint32'], ['100']),
+      )
+      const bogusPayload = ethers.utils.concat([functionHash, calldata])
+
+      // Create a subgraph
+      const tx2 = await gns.populateTransaction.publishNewSubgraph(
+        me.address,
+        subgraph0.subgraphDeploymentID,
+        subgraph0.versionMetadata,
+        subgraph0.subgraphMetadata,
+      )
+
+      // Batch send transaction
+      const tx = gns.connect(me.signer).multicall([bogusPayload, tx2.data])
+      await expect(tx).revertedWith(
+        "function selector was not recognized and there's no fallback function",
+      )
+    })
+  })
 })
