curation: remove totalTokens accounting and use balanceOf()

Initially we decided to use our own tracking of tokens used for curation
to exclude tokens that might have been sent directly to the contract that could
distort the rewards calculation.

We had an issue in the totalTokens accounting that didn't account for the collected tokens.
This lead to a mismatch in rewards proportional to total collected funds, but a bigger issue
was that it could make the burn() function to fail as more tokens were in the contract than
the expected in our own accounting.

We find that there is no incentive to send tokens directly to the contract to distort
those calculations and it is both more gas-efficient and safer to use balanceOf to
get the signalled tokens from the Curation contract when querying from RewardsManager.

This commit implements the use of balanceOf(Curation) from the RewardsManager to get totalTokens used
for Curation.

Author: abarmat

contracts/curation/Curation.sol

@@ -183,14 +183,44 @@ contract Curation is CurationV1Storage, GraphUpgradeable, ICuration {
         // Need to deposit some funds
         require(_tokens > 0, "Cannot deposit zero tokens");
 
+        CurationPool storage curationPool = pools[_subgraphDeploymentID];
+
+        // If it hasn't been curated before then initialize the curve
+        if (!isCurated(_subgraphDeploymentID)) {
+            require(
+                _tokens >= minimumCurationDeposit,
+                "Curation deposit is below minimum required"
+            );
+
+            // Initialize
+            curationPool.reserveRatio = defaultReserveRatio;
+
+            // If no signal token for the pool - create one
+            if (address(curationPool.gst) == address(0)) {
+                // TODO: the gas cost of deploying the subgraph token can be greatly optimized
+                // by deploying a proxy each time, sharing the same implementation
+                curationPool.gst = IGraphSignalToken(
+                    address(new GraphSignalToken("GST", address(this)))
+                );
+            }
+        }
+
+        // Trigger update rewards calculation snapshot
+        _updateRewards(_subgraphDeploymentID);
+
         // Transfer tokens from the curator to this contract
+        // This needs to happen after _updateRewards snapshot as that function
+        // is using balanceOf(curation)
         require(
             graphToken().transferFrom(curator, address(this), _tokens),
             "Cannot transfer tokens to deposit"
         );
 
-        // Deposit tokens to a curation pool reserve
-        return _mint(curator, _subgraphDeploymentID, _tokens);
+        // Exchange GRT tokens for GST of the subgraph pool
+        uint256 signal = _mintSignal(curator, _subgraphDeploymentID, _tokens);
+
+        emit Signalled(curator, _subgraphDeploymentID, _tokens, signal);
+        return signal;
     }
 
     /**
@@ -396,9 +426,6 @@ contract Curation is CurationV1Storage, GraphUpgradeable, ICuration {
         // Mint signal to the curator
         curationPool.gst.mint(_curator, signal);
 
-        // Update the global reserve
-        totalTokens = totalTokens.add(_tokens);
-
         return signal;
     }
 
@@ -424,9 +451,6 @@ contract Curation is CurationV1Storage, GraphUpgradeable, ICuration {
         // Burn signal from curator
         curationPool.gst.burnFrom(_curator, _signal);
 
-        // Update the global reserve
-        totalTokens = totalTokens.sub(outTokens);
-
         return (tokens, withdrawalFees);
     }
 
@@ -448,50 +472,6 @@ contract Curation is CurationV1Storage, GraphUpgradeable, ICuration {
         emit Collected(_subgraphDeploymentID, _tokens);
     }
 
-    /**
-     * @dev Deposit Graph Tokens in exchange for signal of a curation pool.
-     * @param _curator Address of the staking party
-     * @param _subgraphDeploymentID Subgraph deployment from where the curator is minting
-     * @param _tokens Amount of Graph Tokens to deposit
-     * @return Signal minted
-     */
-    function _mint(
-        address _curator,
-        bytes32 _subgraphDeploymentID,
-        uint256 _tokens
-    ) internal returns (uint256) {
-        CurationPool storage curationPool = pools[_subgraphDeploymentID];
-
-        // If it hasn't been curated before then initialize the curve
-        if (!isCurated(_subgraphDeploymentID)) {
-            require(
-                _tokens >= minimumCurationDeposit,
-                "Curation deposit is below minimum required"
-            );
-
-            // Initialize
-            curationPool.reserveRatio = defaultReserveRatio;
-
-            // If no signal token for the pool - create one
-            if (address(curationPool.gst) == address(0)) {
-                // TODO: the gas cost of deploying the subgraph token can be greatly optimized
-                // by deploying a proxy each time, sharing the same implementation
-                curationPool.gst = IGraphSignalToken(
-                    address(new GraphSignalToken("GST", address(this)))
-                );
-            }
-        }
-
-        // Trigger update rewards calculation
-        _updateRewards(_subgraphDeploymentID);
-
-        // Exchange GRT tokens for GST of the subgraph pool
-        uint256 signal = _mintSignal(_curator, _subgraphDeploymentID, _tokens);
-
-        emit Signalled(_curator, _subgraphDeploymentID, _tokens, signal);
-        return signal;
-    }
-
     /**
      * @dev Triggers an update of rewards due to a change in signal.
      * @param _subgraphDeploymentID Subgrapy deployment updated
@@ -503,11 +483,4 @@ contract Curation is CurationV1Storage, GraphUpgradeable, ICuration {
         }
         return 0;
     }
-
-    /**
-     * @dev Exter
-     */
-    function getTotalTokens() external override view returns (uint256) {
-        return totalTokens;
-    }
 }

contracts/curation/CurationStorage.sol

@@ -18,9 +18,6 @@ contract CurationV1Storage is Managed {
     // This is the `startPoolBalance` for the bonding curve
     uint256 public minimumCurationDeposit;
 
-    // Total tokens in held as reserves received from curators deposits
-    uint256 internal totalTokens;
-
     // Bonding curve formula
     address public bondingCurve;
 

contracts/curation/ICuration.sol

@@ -13,8 +13,6 @@ interface ICuration {
 
     // -- Configuration --
 
-    function getTotalTokens() external view returns (uint256);
-
     function setDefaultReserveRatio(uint32 _defaultReserveRatio) external;
 
     function setMinimumCurationDeposit(uint256 _minimumCurationDeposit) external;

contracts/rewards/RewardsManager.sol

@@ -165,6 +165,8 @@ contract RewardsManager is RewardsManagerV1Storage, GraphUpgradeable, IRewardsMa
      * @return newly accrued rewards per signal since last update
      */
     function getNewRewardsPerSignal() public override view returns (uint256) {
+        IGraphToken graphToken = graphToken();
+
         // Calculate time steps
         uint256 t = block.number.sub(accRewardsPerSignalLastBlockUpdated);
         // Optimization to skip calculations if zero time steps elapsed
@@ -178,16 +180,16 @@ contract RewardsManager is RewardsManagerV1Storage, GraphUpgradeable, IRewardsMa
         }
 
         // Zero issuance if no signalled tokens
-        uint256 signalledTokens = curation().getTotalTokens();
+        uint256 signalledTokens = graphToken.balanceOf(address(curation()));
         if (signalledTokens == 0) {
             return 0;
         }
 
         uint256 r = issuanceRate;
-        uint256 p = graphToken().totalSupply();
+        uint256 p = graphToken.totalSupply();
         uint256 a = p.mul(_pow(r, t, TOKEN_DECIMALS)).div(TOKEN_DECIMALS);
 
-        // New issuance per signal during time steps
+        // New issuance per signalled token during time steps
         uint256 x = a.sub(p);
 
         // We multiply the decimals to keep the precision as fixed-point number

test/curation/curation.test.ts

@@ -324,6 +324,22 @@ describe('Curation', () => {
         await shouldCollect(toGRT('200'))
         await shouldCollect(toGRT('500.25'))
       })
+
+      it('should collect tokens and then unsignal all', async function () {
+        await controller
+          .connect(governor.signer)
+          .setContractProxy(utils.id('Staking'), stakingMock.address)
+
+        // Collect increase the pool reserves
+        await shouldCollect(toGRT('100'))
+
+        // When we burn signal we should get more tokens than initially curated
+        const signalToRedeem = await curation.getCuratorSignal(
+          curator.address,
+          subgraphDeploymentID,
+        )
+        await shouldRedeem(signalToRedeem, toGRT('1100'))
+      })
     })
   })
 

test/rewards/rewards.test.ts

@@ -78,7 +78,7 @@ describe('Rewards', () => {
     async snapshot() {
       this.accumulated = this.accumulated.add(await this.accruedGRT())
       this.totalSupply = await grt.totalSupply()
-      this.totalSignalled = await curation.getTotalTokens()
+      this.totalSignalled = await grt.balanceOf(curation.address)
       this.lastUpdatedBlock = await latestBlock()
       return this
     }