Implement tests for subgraph owner fee

Author: davekay100

cli/commands/protocol/get.ts

@@ -41,7 +41,7 @@ export const gettersList = {
   // GNS
   'gns-minimum-signal': { contract: 'GNS', name: 'minimumVSignalStake' },
   'gns-bonding-curve': { contract: 'GNS', name: 'bondingCurve' },
-  'gns-deprecate-fee-percentage': { contract: 'GNS', name: 'changeFeePercentage' },
+  'gns-owner-fee-percentage': { contract: 'GNS', name: 'ownerFeePercentage' },
   // Token
   'token-governor': { contract: 'GraphToken', name: 'governor' },
   'token-supply': { contract: 'GraphToken', name: 'totalSupply' },

cli/commands/protocol/set.ts

@@ -43,7 +43,7 @@ export const settersList = {
   'rewards-issuance-rate': { contract: 'RewardsManager', name: 'setIssuanceRate' },
   // GNS
   'gns-minimum-signal': { contract: 'GNS', name: 'setMinimumVsignal' },
-  'gns-deprecate-fee-percentage': { contract: 'GNS', name: 'setChangeFeePercentage' },
+  'gns-owner-fee-percentage': { contract: 'GNS', name: 'setOwnerFeePercentage' },
   // Token
   'token-transfer-governor': { contract: 'GraphToken', name: 'transferOwnership' },
   'token-accept-governor': { contract: 'GraphToken', name: 'acceptOwnership' },

contracts/discovery/GNS.sol

@@ -34,7 +34,7 @@ contract GNS is Managed {
     uint32 private constant defaultReserveRatio = 1000000;
 
     // In parts per hundred
-    uint32 public changeFeePercentage = 50;
+    uint32 public ownerFeePercentage = 50;
 
     // Bonding curve formula
     address public bondingCurve;
@@ -207,14 +207,14 @@ contract GNS is Managed {
     }
 
     /**
-     * @dev Set the change fee percentage. This is used to prevent a subgraph owner to drain all
+     * @dev Set the owner fee percentage. This is used to prevent a subgraph owner to drain all
      * the name curators tokens while upgrading or deprecating and is configurable in parts per hundred.
-     * @param _changeFeePercentage Change fee percentage
+     * @param _ownerFeePercentage Owner fee percentage
      */
-    function setChangeFeePercentage(uint32 _changeFeePercentage) external onlyGovernor {
-        require(_changeFeePercentage <= 100, "Change fee must be 100 or less");
-        changeFeePercentage = _changeFeePercentage;
-        emit ParameterUpdated("changeFeePercentage");
+    function setOwnerFeePercentage(uint32 _ownerFeePercentage) external onlyGovernor {
+        require(_ownerFeePercentage <= 100, "Owner fee must be 100 or less");
+        ownerFeePercentage = _ownerFeePercentage;
+        emit ParameterUpdated("ownerFeePercentage");
     }
 
     /**
@@ -388,12 +388,11 @@ contract GNS is Managed {
         require(namePool.disabled == false, "GNS: Cannot be disabled");
 
         uint256 vSignalOld = nSignalToVSignal(_graphAccount, _subgraphNumber, namePool.nSignal);
-        (uint256 tokens, uint256 withdrawalFees) = _burnVSignal(
+        (uint256 tokens, , uint256 ownerFee) = _burnVSignal(
             _graphAccount,
             namePool.subgraphDeploymentID,
             vSignalOld
         );
-        uint256 ownerFee = _ownerFee(withdrawalFees, _graphAccount);
         namePool.vSignal = namePool.vSignal.sub(vSignalOld);
         // Update name signals deployment ID to match the subgraphs deployment ID
         namePool.subgraphDeploymentID = _newSubgraphDeploymentID;
@@ -405,7 +404,7 @@ contract GNS is Managed {
             _graphAccount,
             _subgraphNumber,
             vSignalNew,
-            tokens + withdrawalFees,
+            tokens + ownerFee,
             _newSubgraphDeploymentID
         );
     }
@@ -470,12 +469,11 @@ contract GNS is Managed {
         namePool.vSignal = 0;
         // If no nSignal, then no need to burn vSignal
         if (namePool.nSignal != 0) {
-            (uint256 tokens, uint256 withdrawalFees) = curation().burn(
-                _subgraphDeploymentID,
+            (uint256 tokens, , uint256 ownerFee) = _burnVSignal(
+                _graphAccount,
+                namePool.subgraphDeploymentID,
                 vSignal
             );
-
-            uint256 ownerFee = _ownerFee(withdrawalFees, _graphAccount);
             namePool.withdrawableGRT = tokens + ownerFee;
         }
         // Set the NameCurationPool fields to make it disabled
@@ -566,19 +564,24 @@ contract GNS is Managed {
      * @param _graphAccount Subgraph owner
      * @param _subgraphDeploymentID Subgraph deployment to burn all vSignal from
      * @param _vSignal vSignal being burnt
-     * @return Tokens returned to the gns contract, and withdrawal fees the owner transferred to the gns
+     * @return Tokens returned to the gns contract, withdrawal fees charged, and the owner fee
+     * that the owner reimbursed from the withdrawal fee
      */
     function _burnVSignal(
         address _graphAccount,
         bytes32 _subgraphDeploymentID,
         uint256 _vSignal
-    ) private returns (uint256, uint256) {
+    )
+        private
+        returns (
+            uint256,
+            uint256,
+            uint256
+        )
+    {
         (uint256 tokens, uint256 withdrawalFees) = curation().burn(_subgraphDeploymentID, _vSignal);
-        require(
-            graphToken().transferFrom(_graphAccount, address(this), withdrawalFees),
-            "GNS: Error reimbursing withdrawal fees"
-        );
-        return (tokens, withdrawalFees);
+        uint256 ownerFee = _ownerFee(withdrawalFees, _graphAccount);
+        return (tokens, withdrawalFees, ownerFee);
     }
 
     /**
@@ -588,7 +591,10 @@ contract GNS is Managed {
      * @return Amount the owner must pay by transferring GRT to the GNS
      */
     function _ownerFee(uint256 _withdrawalFees, address _owner) private returns (uint256) {
-        uint256 ownerFee = _withdrawalFees.mul(changeFeePercentage).div(100);
+        if (ownerFeePercentage == 0) {
+            return 0;
+        }
+        uint256 ownerFee = _withdrawalFees.mul(ownerFeePercentage).div(100);
         // Get the owner of the Name to reimburse the withdrawal fee
         require(
             graphToken().transferFrom(_owner, address(this), ownerFee),

test/gns.test.ts

@@ -176,9 +176,7 @@ describe('GNS', () => {
     subgraphToPublish = subgraph0, // Defaults to subgraph created in before()
   ) => {
     // Before stats for the old vSignal curve
-    const tokensBeforeVSigOldCuration = await getTokensAndVSig(subgraph0.subgraphDeploymentID)
-    const tokensBeforeOldCuration = tokensBeforeVSigOldCuration[0]
-    const vSigBeforeOldCuration = tokensBeforeVSigOldCuration[1]
+    const ownerFee = await gns.ownerFeePercentage()
 
     // Before stats for the name curve
     const poolBefore = await gns.nameSignals(graphAccount, subgraphNumber)
@@ -190,11 +188,10 @@ describe('GNS', () => {
       subgraphNumber,
       nSigBefore,
     )
-    const vSignalBurnEstimate = nSignalToTokensResult[0]
     const tokensReceivedEstimate = nSignalToTokensResult[1]
 
     // since in upgrade, owner must refund fees, we need to actually add this back in
-    const feesToAddBackEstimate = nSignalToTokensResult[2]
+    const feesToAddBackEstimate = nSignalToTokensResult[2].div(toBN(100 / ownerFee))
     const upgradeTokenReturn = tokensReceivedEstimate.add(feesToAddBackEstimate)
 
     // Get the value for new vSignal that should be created on the new curve
@@ -228,12 +225,12 @@ describe('GNS', () => {
         subgraphToPublish.subgraphDeploymentID,
       )
 
-    // Check curation vSignal old was lowered and tokens too
+    // Check curation vSignal old are set to zero
     const tokensVSigOldCuration = await getTokensAndVSig(subgraph0.subgraphDeploymentID)
     const tokensAfterOldCuration = tokensVSigOldCuration[0]
     const vSigAfterOldCuration = tokensVSigOldCuration[1]
-    expect(tokensAfterOldCuration).eq(tokensBeforeOldCuration.sub(upgradeTokenReturn))
-    expect(vSigAfterOldCuration).eq(vSigBeforeOldCuration.sub(vSignalBurnEstimate))
+    expect(tokensAfterOldCuration).eq(0)
+    expect(vSigAfterOldCuration).eq(0)
 
     // Check the vSignal of the new curation curve, amd tokens
     const tokensVSigNewCuration = await getTokensAndVSig(subgraphToPublish.subgraphDeploymentID)
@@ -260,15 +257,18 @@ describe('GNS', () => {
     subgraphNumber0: number,
   ) => {
     const curationBefore = await getTokensAndVSig(subgraph0.subgraphDeploymentID)
+    const ownerFee = await gns.ownerFeePercentage()
     // We can use the whole amount, since in this test suite all vSignal is used to be staked on nSignal
     const tokensBefore = curationBefore[0]
     const ownerBalanceBefore = await grt.balanceOf(account.address)
+    const expectedWithdrawalFee = tokensBefore.div(toBN(1000000 / withdrawalPercentage))
+    const expectedOwnerFee = expectedWithdrawalFee.div(toBN(100 / ownerFee))
 
     const tx = gns.connect(account.signer).deprecateSubgraph(graphAccount, subgraphNumber0)
     await expect(tx).emit(gns, 'SubgraphDeprecated').withArgs(subgraph0.graphAccount.address, 0)
     await expect(tx)
       .emit(gns, 'NameSignalDisabled')
-      .withArgs(graphAccount, subgraphNumber0, tokensBefore)
+      .withArgs(graphAccount, subgraphNumber0, tokensBefore.sub(expectedOwnerFee))
 
     const deploymentID = await gns.subgraphs(subgraph0.graphAccount.address, 0)
     expect(ethers.constants.HashZero).eq(deploymentID)
@@ -279,11 +279,11 @@ describe('GNS', () => {
     expect(poolVSignalAfter.eq(toBN('0')))
     // Check that the owner balance decreased by the withdrawal fee
     const ownerBalanceAfter = await grt.balanceOf(account.address)
-    expect(ownerBalanceBefore.sub(tokensBefore.div(toBN(1000000 / withdrawalPercentage)))).eq(
-      ownerBalanceAfter,
-    )
+    expect(ownerBalanceBefore.sub(expectedOwnerFee).eq(ownerBalanceAfter))
     // Should be equal since owner pays withdrawal fees
-    expect(poolAfter.withdrawableGRT).eq(tokensBefore)
+    expect(poolAfter.withdrawableGRT).eq(
+      tokensBefore.sub(expectedWithdrawalFee).add(expectedOwnerFee),
+    )
     // Check that deprecated is true
     expect(poolAfter.disabled).eq(true)
     // Check balance of gns increase by withdrawalFees from owner being added
@@ -901,21 +901,21 @@ describe('GNS', () => {
           // we compare 1:1 ratio. Its implied that vSignal is 1 as well (1:1:1)
           expect(tokensToDeposit).eq(nSignalCreated)
         }
-    describe('setDeprecateFeePercentage', function () {
+    describe('setOwnerFeePercentage', function () {
       const newValue = 10
-      it('should set `minimumVSignalStake`', async function () {
+      it('should set `ownerFeePercentage`', async function () {
         // Can set if allowed
-        await gns.connect(governor.signer).setDeprecateFeePercentage(newValue)
-        expect(await gns.deprecateFeePercentage()).eq(newValue)
+        await gns.connect(governor.signer).setOwnerFeePercentage(newValue)
+        expect(await gns.ownerFeePercentage()).eq(newValue)
       })
 
-      it('reject set `minimumVSignalStake` if out of bounds', async function () {
-        const tx = gns.connect(governor.signer).setDeprecateFeePercentage(101)
-        await expect(tx).revertedWith('Deprecate fee must be 100 or less')
+      it('reject set `ownerFeePercentage` if out of bounds', async function () {
+        const tx = gns.connect(governor.signer).setOwnerFeePercentage(101)
+        await expect(tx).revertedWith('Owner fee must be 100 or less')
       })
 
-      it('reject set `minimumVSignalStake` if not allowed', async function () {
-        const tx = gns.connect(me.signer).setDeprecateFeePercentage(newValue)
+      it('reject set `ownerFeePercentage` if not allowed', async function () {
+        const tx = gns.connect(me.signer).setOwnerFeePercentage(newValue)
         await expect(tx).revertedWith('Caller must be Controller governor')
       })
     })