From 52b80bd9f15b27806185ee23886b04f76b229d7b Mon Sep 17 00:00:00 2001 From: Ariel Barmat Date: Thu, 23 Sep 2021 01:47:40 -0300 Subject: [PATCH 1/5] curation: use a minimal proxy to save gas when minting for first time --- contracts/curation/Curation.sol | 54 ++++++++++++---------- contracts/curation/CurationStorage.sol | 21 +++++++-- contracts/curation/GraphCurationToken.sol | 10 ++-- contracts/curation/ICuration.sol | 8 ---- contracts/curation/IGraphCurationToken.sol | 6 ++- package.json | 1 + test/lib/deployment.ts | 2 + yarn.lock | 5 ++ 8 files changed, 63 insertions(+), 44 deletions(-) diff --git a/contracts/curation/Curation.sol b/contracts/curation/Curation.sol index 883226905..7e97066fd 100644 --- a/contracts/curation/Curation.sol +++ b/contracts/curation/Curation.sol @@ -2,7 +2,9 @@ pragma solidity ^0.7.6; +import "@openzeppelin/contracts/utils/Address.sol"; import "@openzeppelin/contracts/math/SafeMath.sol"; +import "@openzeppelin/contracts/proxy/Clones.sol"; import "../bancor/BancorFormula.sol"; import "../upgrades/GraphUpgradeable.sol"; @@ -23,7 +25,7 @@ import "./GraphCurationToken.sol"; * Holders can burn GCS using this contract to get GRT tokens back according to the * bonding curve. */ -contract Curation is CurationV1Storage, GraphUpgradeable, ICuration { +contract Curation is CurationV1Storage, GraphUpgradeable { using SafeMath for uint256; // 100% in parts per million @@ -70,6 +72,7 @@ contract Curation is CurationV1Storage, GraphUpgradeable, ICuration { function initialize( address _controller, address _bondingCurve, + address _curationTokenMaster, uint32 _defaultReserveRatio, uint32 _curationTaxPercentage, uint256 _minimumCurationDeposit @@ -83,6 +86,7 @@ contract Curation is CurationV1Storage, GraphUpgradeable, ICuration { _setDefaultReserveRatio(_defaultReserveRatio); _setCurationTaxPercentage(_curationTaxPercentage); _setMinimumCurationDeposit(_minimumCurationDeposit); + _setCurationTokenMaster(_curationTokenMaster); } /** @@ -154,10 +158,23 @@ contract Curation is CurationV1Storage, GraphUpgradeable, ICuration { "Curation tax percentage must be below or equal to MAX_PPM" ); - _curationTaxPercentage = _percentage; + curationTaxPercentage = _percentage; emit ParameterUpdated("curationTaxPercentage"); } + // TODO: add public version of this + /** + * @dev Internal: Set the master copy to use as clones for the curation token. + * @param _curationTokenMaster Address of implementation contract to use for curation tokens + */ + function _setCurationTokenMaster(address _curationTokenMaster) private { + require(_curationTokenMaster != address(0), "Token master must be non-empty"); + require(Address.isContract(_curationTokenMaster), "Token master must be a contract"); + + curationTokenMaster = _curationTokenMaster; + emit ParameterUpdated("curationTokenMaster"); + } + /** * @dev Assign Graph Tokens collected as curation fees to the curation pool reserve. * This function can only be called by the Staking contract and will do the bookeeping of @@ -212,13 +229,12 @@ contract Curation is CurationV1Storage, GraphUpgradeable, ICuration { curationPool.reserveRatio = defaultReserveRatio; // If no signal token for the pool - create one + // TODO: review if we can avoid re-deploying if was previously created if (address(curationPool.gcs) == address(0)) { - // TODO: Use a minimal proxy to reduce gas cost - // https://github.com/graphprotocol/contracts/issues/405 - // --abarmat-- 20201113 - curationPool.gcs = IGraphCurationToken( - address(new GraphCurationToken(address(this))) - ); + // Use a minimal proxy to reduce gas cost + IGraphCurationToken gcs = IGraphCurationToken(Clones.clone(curationTokenMaster)); + gcs.initialize(address(this)); + curationPool.gcs = gcs; } } @@ -318,10 +334,8 @@ contract Curation is CurationV1Storage, GraphUpgradeable, ICuration { override returns (uint256) { - if (address(pools[_subgraphDeploymentID].gcs) == address(0)) { - return 0; - } - return pools[_subgraphDeploymentID].gcs.balanceOf(_curator); + IGraphCurationToken gcs = pools[_subgraphDeploymentID].gcs; + return (address(gcs) == address(0)) ? 0 : gcs.balanceOf(_curator); } /** @@ -335,10 +349,8 @@ contract Curation is CurationV1Storage, GraphUpgradeable, ICuration { override returns (uint256) { - if (address(pools[_subgraphDeploymentID].gcs) == address(0)) { - return 0; - } - return pools[_subgraphDeploymentID].gcs.totalSupply(); + IGraphCurationToken gcs = pools[_subgraphDeploymentID].gcs; + return (address(gcs) == address(0)) ? 0 : gcs.totalSupply(); } /** @@ -355,14 +367,6 @@ contract Curation is CurationV1Storage, GraphUpgradeable, ICuration { return pools[_subgraphDeploymentID].tokens; } - /** - * @dev Get curation tax percentage - * @return Amount the curation tax percentage in PPM - */ - function curationTaxPercentage() external view override returns (uint32) { - return _curationTaxPercentage; - } - /** * @dev Calculate amount of signal that can be bought with tokens in a curation pool. * This function considers and excludes the deposit tax. @@ -376,7 +380,7 @@ contract Curation is CurationV1Storage, GraphUpgradeable, ICuration { override returns (uint256, uint256) { - uint256 curationTax = _tokensIn.mul(uint256(_curationTaxPercentage)).div(MAX_PPM); + uint256 curationTax = _tokensIn.mul(uint256(curationTaxPercentage)).div(MAX_PPM); uint256 signalOut = _tokensToSignal(_subgraphDeploymentID, _tokensIn.sub(curationTax)); return (signalOut, curationTax); } diff --git a/contracts/curation/CurationStorage.sol b/contracts/curation/CurationStorage.sol index 69c601bc2..dd2edd18b 100644 --- a/contracts/curation/CurationStorage.sol +++ b/contracts/curation/CurationStorage.sol @@ -2,28 +2,39 @@ pragma solidity ^0.7.6; -import "./ICuration.sol"; import "../governance/Managed.sol"; -contract CurationV1Storage is Managed { +abstract contract CurationV1Storage is Managed, ICuration { + // -- Pool -- + + struct CurationPool { + uint256 tokens; // GRT Tokens stored as reserves for the subgraph deployment + uint32 reserveRatio; // Ratio for the bonding curve + IGraphCurationToken gcs; // Curation token contract for this curation pool + } + // -- State -- // Tax charged when curator deposit funds // Parts per million. (Allows for 4 decimal points, 999,999 = 99.9999%) - uint32 internal _curationTaxPercentage; + uint32 public override curationTaxPercentage; // Default reserve ratio to configure curator shares bonding curve // Parts per million. (Allows for 4 decimal points, 999,999 = 99.9999%) uint32 public defaultReserveRatio; + // Master copy address that holds implementation of curation token + // This is used as the target for GraphCurationToken clones + address public curationTokenMaster; + // Minimum amount allowed to be deposited by curators to initialize a pool // This is the `startPoolBalance` for the bonding curve uint256 public minimumCurationDeposit; - // Bonding curve formula + // Bonding curve library address public bondingCurve; // Mapping of subgraphDeploymentID => CurationPool // There is only one CurationPool per SubgraphDeploymentID - mapping(bytes32 => ICuration.CurationPool) public pools; + mapping(bytes32 => CurationPool) public pools; } diff --git a/contracts/curation/GraphCurationToken.sol b/contracts/curation/GraphCurationToken.sol index 1c0a82df6..8ab2a8c3d 100644 --- a/contracts/curation/GraphCurationToken.sol +++ b/contracts/curation/GraphCurationToken.sol @@ -2,11 +2,12 @@ pragma solidity ^0.7.6; -import "@openzeppelin/contracts/token/ERC20/ERC20.sol"; +import "@openzeppelin/contracts-upgradeable/token/ERC20/ERC20Upgradeable.sol"; import "../governance/Governed.sol"; /** + * TODO: update to reflect that it is now a cloneable * @title GraphCurationToken contract * @dev This is the implementation of the Curation ERC20 token (GCS). * GCS are created for each subgraph deployment curated in the Curation contract. @@ -14,13 +15,14 @@ import "../governance/Governed.sol"; * burn them. GCS tokens are transferrable and their holders can do any action allowed * in a standard ERC20 token implementation except for burning them. */ -contract GraphCurationToken is ERC20, Governed { +contract GraphCurationToken is ERC20Upgradeable, Governed { /** - * @dev Graph Curation Token Contract Constructor. + * @dev Graph Curation Token Contract initializer. * @param _owner Address of the contract issuing this token */ - constructor(address _owner) ERC20("Graph Curation Share", "GCS") { + function initialize(address _owner) external initializer { Governed._initialize(_owner); + ERC20Upgradeable.__ERC20_init("Graph Curation Share", "GCS"); } /** diff --git a/contracts/curation/ICuration.sol b/contracts/curation/ICuration.sol index 35bad13b0..ec9a81ae9 100644 --- a/contracts/curation/ICuration.sol +++ b/contracts/curation/ICuration.sol @@ -5,14 +5,6 @@ pragma solidity ^0.7.6; import "./IGraphCurationToken.sol"; interface ICuration { - // -- Pool -- - - struct CurationPool { - uint256 tokens; // GRT Tokens stored as reserves for the subgraph deployment - uint32 reserveRatio; // Ratio for the bonding curve - IGraphCurationToken gcs; // Curation token contract for this curation pool - } - // -- Configuration -- function setDefaultReserveRatio(uint32 _defaultReserveRatio) external; diff --git a/contracts/curation/IGraphCurationToken.sol b/contracts/curation/IGraphCurationToken.sol index c9040a4b3..43679aba6 100644 --- a/contracts/curation/IGraphCurationToken.sol +++ b/contracts/curation/IGraphCurationToken.sol @@ -2,9 +2,11 @@ pragma solidity ^0.7.6; -import "@openzeppelin/contracts/token/ERC20/IERC20.sol"; +import "@openzeppelin/contracts-upgradeable/token/ERC20/IERC20Upgradeable.sol"; + +interface IGraphCurationToken is IERC20Upgradeable { + function initialize(address _owner) external; -interface IGraphCurationToken is IERC20 { function burnFrom(address _account, uint256 _amount) external; function mint(address _to, uint256 _amount) external; diff --git a/package.json b/package.json index 40df0d1b0..dfadc7281 100644 --- a/package.json +++ b/package.json @@ -25,6 +25,7 @@ "@nomiclabs/hardhat-etherscan": "^2.1.1", "@nomiclabs/hardhat-waffle": "^2.0.1", "@openzeppelin/contracts": "^3.4.1", + "@openzeppelin/contracts-upgradeable": "3.4.2", "@openzeppelin/hardhat-upgrades": "^1.6.0", "@tenderly/hardhat-tenderly": "^1.0.11", "@typechain/ethers-v5": "^7.0.0", diff --git a/test/lib/deployment.ts b/test/lib/deployment.ts index f04ba5b81..eb28b35dd 100644 --- a/test/lib/deployment.ts +++ b/test/lib/deployment.ts @@ -117,6 +117,7 @@ export async function deployCuration( ): Promise { // Dependency const bondingCurve = (await deployContract('BancorFormula', deployer)) as unknown as BancorFormula + const curationTokenMaster = await deployContract('GraphCurationToken', deployer) // Deploy return network.deployContractWithProxy( @@ -125,6 +126,7 @@ export async function deployCuration( [ controller, bondingCurve.address, + curationTokenMaster.address, defaults.curation.reserveRatio, defaults.curation.curationTaxPercentage, defaults.curation.minimumCurationDeposit, diff --git a/yarn.lock b/yarn.lock index 92c7f6944..9c0b94745 100644 --- a/yarn.lock +++ b/yarn.lock @@ -905,6 +905,11 @@ "@types/sinon-chai" "^3.2.3" "@types/web3" "1.0.19" +"@openzeppelin/contracts-upgradeable@3.4.2": + version "3.4.2" + resolved "https://registry.yarnpkg.com/@openzeppelin/contracts-upgradeable/-/contracts-upgradeable-3.4.2.tgz#2c2a1b0fa748235a1f495b6489349776365c51b3" + integrity sha512-mDlBS17ymb2wpaLcrqRYdnBAmP1EwqhOXMvqWk2c5Q1N1pm5TkiCtXM9Xzznh4bYsQBq0aIWEkFFE2+iLSN1Tw== + "@openzeppelin/contracts@^3.4.1": version "3.4.2" resolved "https://registry.yarnpkg.com/@openzeppelin/contracts/-/contracts-3.4.2.tgz#d81f786fda2871d1eb8a8c5a73e455753ba53527" From 79cf012b94e3a99ffabe0ffcf979d10179062793 Mon Sep 17 00:00:00 2001 From: Ariel Barmat Date: Thu, 23 Sep 2021 01:59:05 -0300 Subject: [PATCH 2/5] curation: use token utils for transfers --- contracts/curation/Curation.sol | 19 +++++++------------ 1 file changed, 7 insertions(+), 12 deletions(-) diff --git a/contracts/curation/Curation.sol b/contracts/curation/Curation.sol index 7e97066fd..a9dbddabb 100644 --- a/contracts/curation/Curation.sol +++ b/contracts/curation/Curation.sol @@ -8,6 +8,7 @@ import "@openzeppelin/contracts/proxy/Clones.sol"; import "../bancor/BancorFormula.sol"; import "../upgrades/GraphUpgradeable.sol"; +import "../utils/TokenUtils.sol"; import "./CurationStorage.sol"; import "./ICuration.sol"; @@ -242,18 +243,12 @@ contract Curation is CurationV1Storage, GraphUpgradeable { _updateRewards(_subgraphDeploymentID); // Transfer tokens from the curator to this contract - // This needs to happen after _updateRewards snapshot as that function + // Burn the curation tax + // NOTE: This needs to happen after _updateRewards snapshot as that function // is using balanceOf(curation) - IGraphToken graphToken = graphToken(); - require( - graphToken.transferFrom(curator, address(this), _tokensIn), - "Cannot transfer tokens to deposit" - ); - - // Burn withdrawal fees - if (curationTax > 0) { - graphToken.burn(curationTax); - } + IGraphToken _graphToken = graphToken(); + TokenUtils.pullTokens(_graphToken, curator, _tokensIn); + TokenUtils.burnTokens(_graphToken, curationTax); // Update curation pool curationPool.tokens = curationPool.tokens.add(_tokensIn.sub(curationTax)); @@ -306,7 +301,7 @@ contract Curation is CurationV1Storage, GraphUpgradeable { } // Return the tokens to the curator - require(graphToken().transfer(curator, tokensOut), "Error sending curator tokens"); + TokenUtils.pushTokens(graphToken(), curator, tokensOut); emit Burned(curator, _subgraphDeploymentID, tokensOut, _signalIn); From 852b5382ffd75adcd714085ca20f5a2a74588bda Mon Sep 17 00:00:00 2001 From: Ariel Barmat Date: Thu, 23 Sep 2021 18:58:52 -0300 Subject: [PATCH 3/5] tests: remove unnecessary block advance in test --- test/staking/delegation.test.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/test/staking/delegation.test.ts b/test/staking/delegation.test.ts index 130a38b5a..441bc3996 100644 --- a/test/staking/delegation.test.ts +++ b/test/staking/delegation.test.ts @@ -465,7 +465,6 @@ describe('Staking::Delegation', () => { await staking.setDelegationUnbondingPeriod('2') await shouldDelegate(delegator, toGRT('100')) await shouldUndelegate(delegator, toGRT('50')) - await advanceBlock() await advanceToNextEpoch(epochManager) // epoch 1 await advanceToNextEpoch(epochManager) // epoch 2 await shouldUndelegate(delegator, toGRT('10')) From 1a2a89b56ad04658655a25f1c0361b4bfd97c91f Mon Sep 17 00:00:00 2001 From: Ariel Barmat Date: Thu, 23 Sep 2021 21:30:38 -0300 Subject: [PATCH 4/5] curation: add external function to set token master copy and avoid re-deploy the clone on minting reset --- contracts/curation/Curation.sol | 17 +++++++++---- contracts/curation/GraphCurationToken.sol | 5 +++- contracts/curation/ICuration.sol | 2 ++ test/curation/configuration.test.ts | 30 ++++++++++++++++++++++- test/curation/curation.test.ts | 16 ++++++++++++ 5 files changed, 63 insertions(+), 7 deletions(-) diff --git a/contracts/curation/Curation.sol b/contracts/curation/Curation.sol index a9dbddabb..fc59b33d1 100644 --- a/contracts/curation/Curation.sol +++ b/contracts/curation/Curation.sol @@ -163,7 +163,14 @@ contract Curation is CurationV1Storage, GraphUpgradeable { emit ParameterUpdated("curationTaxPercentage"); } - // TODO: add public version of this + /** + * @dev Set the master copy to use as clones for the curation token. + * @param _curationTokenMaster Address of implementation contract to use for curation tokens + */ + function setCurationTokenMaster(address _curationTokenMaster) external override onlyGovernor { + _setCurationTokenMaster(_curationTokenMaster); + } + /** * @dev Internal: Set the master copy to use as clones for the curation token. * @param _curationTokenMaster Address of implementation contract to use for curation tokens @@ -226,11 +233,9 @@ contract Curation is CurationV1Storage, GraphUpgradeable { // If it hasn't been curated before then initialize the curve if (!isCurated(_subgraphDeploymentID)) { - // Initialize curationPool.reserveRatio = defaultReserveRatio; // If no signal token for the pool - create one - // TODO: review if we can avoid re-deploying if was previously created if (address(curationPool.gcs) == address(0)) { // Use a minimal proxy to reduce gas cost IGraphCurationToken gcs = IGraphCurationToken(Clones.clone(curationTokenMaster)); @@ -295,9 +300,11 @@ contract Curation is CurationV1Storage, GraphUpgradeable { curationPool.tokens = curationPool.tokens.sub(tokensOut); curationPool.gcs.burnFrom(curator, _signalIn); - // If all signal burnt delete the curation pool + // If all signal burnt delete the curation pool except for the + // curation token contract to avoid recreating it on a new mint if (getCurationPoolSignal(_subgraphDeploymentID) == 0) { - delete pools[_subgraphDeploymentID]; + curationPool.tokens = 0; + curationPool.reserveRatio = 0; } // Return the tokens to the curator diff --git a/contracts/curation/GraphCurationToken.sol b/contracts/curation/GraphCurationToken.sol index 8ab2a8c3d..78b721e1b 100644 --- a/contracts/curation/GraphCurationToken.sol +++ b/contracts/curation/GraphCurationToken.sol @@ -7,13 +7,16 @@ import "@openzeppelin/contracts-upgradeable/token/ERC20/ERC20Upgradeable.sol"; import "../governance/Governed.sol"; /** - * TODO: update to reflect that it is now a cloneable * @title GraphCurationToken contract * @dev This is the implementation of the Curation ERC20 token (GCS). + * * GCS are created for each subgraph deployment curated in the Curation contract. * The Curation contract is the owner of GCS tokens and the only one allowed to mint or * burn them. GCS tokens are transferrable and their holders can do any action allowed * in a standard ERC20 token implementation except for burning them. + * + * This contract is meant to be used as the implementation for Minimal Proxy clones for + * gas-saving purposes. */ contract GraphCurationToken is ERC20Upgradeable, Governed { /** diff --git a/contracts/curation/ICuration.sol b/contracts/curation/ICuration.sol index ec9a81ae9..9e1701aaf 100644 --- a/contracts/curation/ICuration.sol +++ b/contracts/curation/ICuration.sol @@ -13,6 +13,8 @@ interface ICuration { function setCurationTaxPercentage(uint32 _percentage) external; + function setCurationTokenMaster(address _curationTokenMaster) external; + // -- Curation -- function mint( diff --git a/test/curation/configuration.test.ts b/test/curation/configuration.test.ts index bbebd23a7..b2424c784 100644 --- a/test/curation/configuration.test.ts +++ b/test/curation/configuration.test.ts @@ -1,10 +1,13 @@ import { expect } from 'chai' +import { constants } from 'ethers' import { Curation } from '../../build/types/Curation' import { defaults } from '../lib/deployment' import { NetworkFixture } from '../lib/fixtures' -import { getAccounts, toBN, Account } from '../lib/testHelpers' +import { getAccounts, toBN, Account, randomAddress } from '../lib/testHelpers' + +const { AddressZero } = constants const MAX_PPM = 1000000 @@ -99,4 +102,29 @@ describe('Curation:Config', () => { await expect(tx).revertedWith('Caller must be Controller governor') }) }) + + describe('curationTokenMaster', function () { + it('should set `curationTokenMaster`', async function () { + const newCurationTokenMaster = curation.address + await curation.connect(governor.signer).setCurationTokenMaster(newCurationTokenMaster) + }) + + it('reject set `curationTokenMaster` to empty value', async function () { + const newCurationTokenMaster = AddressZero + const tx = curation.connect(governor.signer).setCurationTokenMaster(newCurationTokenMaster) + await expect(tx).revertedWith('Token master must be non-empty') + }) + + it('reject set `curationTokenMaster` to non-contract', async function () { + const newCurationTokenMaster = randomAddress() + const tx = curation.connect(governor.signer).setCurationTokenMaster(newCurationTokenMaster) + await expect(tx).revertedWith('Token master must be a contract') + }) + + it('reject set `curationTokenMaster` if not allowed', async function () { + const newCurationTokenMaster = curation.address + const tx = curation.connect(me.signer).setCurationTokenMaster(newCurationTokenMaster) + await expect(tx).revertedWith('Caller must be Controller governor') + }) + }) }) diff --git a/test/curation/curation.test.ts b/test/curation/curation.test.ts index 4bc2e704a..fdbf8da6e 100644 --- a/test/curation/curation.test.ts +++ b/test/curation/curation.test.ts @@ -456,6 +456,22 @@ describe('Curation', () => { .burn(subgraphDeploymentID, signalToRedeem, expectedTokens.add(1)) await expect(tx).revertedWith('Slippage protection') }) + + it('should not re-deploy the curation token when signal is reset', async function () { + const beforeSubgraphPool = await curation.pools(subgraphDeploymentID) + + // Burn all the signal + const signalToRedeem = await curation.getCuratorSignal(curator.address, subgraphDeploymentID) + const expectedTokens = tokensToDeposit + await shouldBurn(signalToRedeem, expectedTokens) + + // Mint again on the same subgraph + await curation.connect(curator.signer).mint(subgraphDeploymentID, tokensToDeposit, 0) + + // Check state + const afterSubgraphPool = await curation.pools(subgraphDeploymentID) + expect(afterSubgraphPool.gcs).eq(beforeSubgraphPool.gcs) + }) }) describe('conservation', async function () { From d7b9f3505ac0e593c37cd8f9b174e2eb2ef5954d Mon Sep 17 00:00:00 2001 From: Ariel Barmat Date: Sun, 28 Nov 2021 21:06:36 -0300 Subject: [PATCH 5/5] chore: add graph curation master copy token to deployment config --- cli/commands/migrate.ts | 1 + graph.config.yml | 3 ++- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/cli/commands/migrate.ts b/cli/commands/migrate.ts index 9a77ca78c..b1bd155fa 100644 --- a/cli/commands/migrate.ts +++ b/cli/commands/migrate.ts @@ -22,6 +22,7 @@ const allContracts = [ 'Controller', 'EpochManager', 'GraphToken', + 'GraphCurationToken', 'ServiceRegistry', 'Curation', 'GNS', diff --git a/graph.config.yml b/graph.config.yml index 7eb2faccd..6b242e05d 100644 --- a/graph.config.yml +++ b/graph.config.yml @@ -39,12 +39,13 @@ contracts: initialSupply: "10000000000000000000000000000" # 10,000,000,000 GRT calls: - fn: "addMinter" - minter: "${{RewardsManager.address}}" + minter: "${{RewardsManager.address}}" Curation: proxy: true init: controller: "${{Controller.address}}" bondingCurve: "${{BancorFormula.address}}" + curationTokenMaster: "${{GraphCurationToken.address}}" reserveRatio: 500000 # 50% (parts per million) curationTaxPercentage: 25000 # 2.5% (parts per million) minimumCurationDeposit: "1000000000000000000" # 1 GRT