Fix naming bug so ens names are for graph accounts. Not subgraphs (#258)

davekay100 · web-flow · commit e7ddbbee83c0 · 2020-07-23T15:22:13.000-04:00
* Fix default name to be on account, not subgraph. Subgraph metadata no longer triggers version update

* Fix tests for naming changes
diff --git a/contracts/GNS.sol b/contracts/GNS.sol
@@ -59,6 +59,22 @@ contract GNS is Governed, BancorFormula {
 
     // -- Events --
 
+    /**
+     * @dev TODO
+     */
+    event SetDefaultName(
+        address graphAccount,
+        uint256 nameSystem, // only ENS for now
+        bytes32 nameIdentifier,
+        string name
+    );
+
+    event SubgraphMetadataUpdated(
+        address graphAccount,
+        uint256 subgraphNumber,
+        bytes32 subgraphMetadata
+    );
+
     /**
      * @dev Emitted when a `graph account` publishes a `subgraph` with a `version`.
      * Every time this event is emitted, indicates a new version has been created.
@@ -68,10 +84,7 @@ contract GNS is Governed, BancorFormula {
         address graphAccount,
         uint256 subgraphNumber,
         bytes32 subgraphDeploymentID,
-        uint256 nameSystem, // only ENS for now
-        bytes32 nameIdentifier,
-        string name,
-        bytes32 metadataHash
+        bytes32 versionMetadata
     );
 
     /**
@@ -183,90 +196,76 @@ contract GNS is Governed, BancorFormula {
         emit ParameterUpdated("minimumVSignalStake");
     }
 
+    function setDefaultName(
+        address _graphAccount,
+        uint8 _nameSystem,
+        bytes32 _nameIdentifier,
+        string calldata _name
+    ) external onlyGraphAccountOwner(_graphAccount) {
+        emit SetDefaultName(_graphAccount, _nameSystem, _nameIdentifier, _name);
+    }
+
+    function updateSubgraphMetadata(
+        address _graphAccount,
+        uint256 _subgraphNumber,
+        bytes32 _metadataHash
+    ) public onlyGraphAccountOwner(_graphAccount) {
+        emit SubgraphMetadataUpdated(_graphAccount, _subgraphNumber, _metadataHash);
+    }
+
     /**
      * @dev Allows a graph account to publish a new subgraph, which means a new subgraph number
-     * will be used. It then will call publish version. Subsequent versions can be created
-     * by calling publishVersion() directly.
+     * will be used.
      * @param _graphAccount Account that is publishing the subgraph
      * @param _subgraphDeploymentID Subgraph deployment ID of the version, linked to the name
-     * @param _nameIdentifier The value used to look up ownership in the naming system
-     * @param _name Name of the subgraph, from any valid system
-     * @param _metadataHash IPFS hash for the subgraph, and subgraph version metadata
+     * @param _versionMetadata IPFS hash for the subgraph version metadata
      */
     function publishNewSubgraph(
         address _graphAccount,
         bytes32 _subgraphDeploymentID,
-        bytes32 _nameIdentifier,
-        string calldata _name,
-        bytes32 _metadataHash
+        bytes32 _versionMetadata
     ) external onlyGraphAccountOwner(_graphAccount) {
         uint256 subgraphNumber = graphAccountSubgraphNumbers[_graphAccount];
-        publishVersion(
-            _graphAccount,
-            subgraphNumber,
-            _subgraphDeploymentID,
-            _nameIdentifier,
-            _name,
-            _metadataHash
-        );
+        publishVersion(_graphAccount, subgraphNumber, _subgraphDeploymentID, _versionMetadata);
         graphAccountSubgraphNumbers[_graphAccount]++;
     }
 
     /**
-     * @dev Allows a graph account to publish a subgraph, with a version, a name, and metadata
-     * Graph account must own the name of the name system they are linking to the subgraph
-     * Version is derived from the occurance of SubgraphPublish being emitted. i.e. version 0
-     * is the first time the event is emitted for the graph account and subgraph number
-     * combination.
+     * @dev Allows a graph account to publish a new version of their subgraph.
+     * Version is derived from the occurance of SubgraphPublished being emitted.
+     * The first time SubgraphPublished is called would be Version 0
      * @param _graphAccount Account that is publishing the subgraph
      * @param _subgraphNumber Subgraph number for the account
      * @param _subgraphDeploymentID Subgraph deployment ID of the version, linked to the name
-     * @param _nameIdentifier The value used to look up ownership in the naming system
-     * @param _name Name of the subgraph, from any valid system
-     * @param _metadataHash IPFS hash for the subgraph, and subgraph version metadata
+     * @param _versionMetadata IPFS hash for the subgraph version metadata
      */
     function publishNewVersion(
         address _graphAccount,
         uint256 _subgraphNumber,
         bytes32 _subgraphDeploymentID,
-        bytes32 _nameIdentifier,
-        string calldata _name,
-        bytes32 _metadataHash
+        bytes32 _versionMetadata
     ) external onlyGraphAccountOwner(_graphAccount) {
         require(
-            isPublished(_graphAccount, _subgraphNumber) || // Hasn't been created yet
-                _subgraphNumber < graphAccountSubgraphNumbers[_graphAccount], // Was created, but deprecated
-            "GNS: Cant publish a version directly for a subgraph that wasnt created yet"
-        );
-
-        publishVersion(
-            _graphAccount,
-            _subgraphNumber,
-            _subgraphDeploymentID,
-            _nameIdentifier,
-            _name,
-            _metadataHash
+            isPublished(_graphAccount, _subgraphNumber),
+            "GNS: Cannot update version if not published, or has been deprecated"
         );
+        publishVersion(_graphAccount, _subgraphNumber, _subgraphDeploymentID, _versionMetadata);
     }
 
     /**
      * @dev Internal function used by both external publishing functions
      * @param _graphAccount Account that is publishing the subgraph
      * @param _subgraphNumber Subgraph number for the account
      * @param _subgraphDeploymentID Subgraph deployment ID of the version, linked to the name
-     * @param _nameIdentifier The value used to look up ownership in the naming system
-     * @param _name Name of the subgraph, from any valid system
-     * @param _metadataHash IPFS hash for the subgraph, and subgraph version metadata
+     * @param _versionMetadata IPFS hash for the subgraph version metadata
      */
     function publishVersion(
         address _graphAccount,
         uint256 _subgraphNumber,
         bytes32 _subgraphDeploymentID,
-        bytes32 _nameIdentifier,
-        string memory _name,
-        bytes32 _metadataHash
+        bytes32 _versionMetadata
     ) internal {
-        require(_subgraphDeploymentID != 0, "GNS: Cannot set to 0 in publish");
+        require(_subgraphDeploymentID != 0, "GNS: Cannot set deploymentID to 0 in publish");
 
         // Stores a subgraph deployment ID, which indicates a version has been created
         subgraphs[_graphAccount][_subgraphNumber] = _subgraphDeploymentID;
@@ -275,10 +274,7 @@ contract GNS is Governed, BancorFormula {
             _graphAccount,
             _subgraphNumber,
             _subgraphDeploymentID,
-            0,
-            _nameIdentifier,
-            _name,
-            _metadataHash
+            _versionMetadata
         );
     }
 
@@ -353,6 +349,7 @@ contract GNS is Governed, BancorFormula {
         uint256 _subgraphNumber,
         bytes32 _newSubgraphDeploymentID
     ) external onlyGraphAccountOwner(_graphAccount) {
+        require(_newSubgraphDeploymentID != 0, "GNS: Deployment ID cannot be 0");
         bytes32 subgraphDeploymentID = subgraphs[_graphAccount][_subgraphNumber];
         // Subgraph owner must first update the numbered subgraph to point to this deploymentID
         // Then they can direct the name curators vSignal to this new name curation curve
@@ -486,7 +483,7 @@ contract GNS is Governed, BancorFormula {
         bytes32 subgraphDeploymentID = subgraphs[_graphAccount][_subgraphNumber];
         NameCurationPool storage namePool = nameSignals[_graphAccount][_subgraphNumber];
         require(
-            namePool.subgraphDeploymentID == subgraphDeploymentID,
+            namePool.subgraphDeploymentID == subgraphDeploymentID, // TODO EDGE CASE - when both subgraph ids are 0, this will fail, leading something to be deprecated before it exists
             "GNS: Name owner updated version without updating name signal"
         );
         require(namePool.disabled == false, "GNS: Cannot be disabled twice");
diff --git a/test/gns.test.ts b/test/gns.test.ts
@@ -14,7 +14,8 @@ interface Subgraph {
   subgraphDeploymentID: string
   name: string
   nameIdentifier: string
-  metadataHash: string
+  versionMetadata: string
+  subgraphMetadata: string
 }
 
 describe('GNS', () => {
@@ -40,7 +41,8 @@ describe('GNS', () => {
       subgraphDeploymentID: randomHexBytes(),
       name: name,
       nameIdentifier: ethers.utils.namehash(name),
-      metadataHash: randomHexBytes(),
+      versionMetadata: randomHexBytes(),
+      subgraphMetadata: randomHexBytes(),
     }
   }
 
@@ -62,20 +64,15 @@ describe('GNS', () => {
       .publishNewSubgraph(
         graphAccount,
         subgraphToPublish.subgraphDeploymentID,
-        subgraphToPublish.nameIdentifier,
-        subgraphToPublish.name,
-        subgraphToPublish.metadataHash,
+        subgraphToPublish.versionMetadata,
       )
     await expect(tx)
       .emit(gns, 'SubgraphPublished')
       .withArgs(
         subgraphToPublish.graphAccount.address,
         subgraphNumber0,
         subgraphToPublish.subgraphDeploymentID,
-        0,
-        subgraphToPublish.nameIdentifier,
-        subgraphToPublish.name,
-        subgraphToPublish.metadataHash,
+        subgraphToPublish.versionMetadata,
       )
     return tx
   }
@@ -91,9 +88,7 @@ describe('GNS', () => {
         graphAccount,
         subgraphNumber0,
         subgraphToPublish.subgraphDeploymentID,
-        subgraphToPublish.nameIdentifier,
-        subgraphToPublish.name,
-        subgraphToPublish.metadataHash,
+        subgraphToPublish.versionMetadata,
       )
 
   const deprecateSubgraph = async (
@@ -419,7 +414,39 @@ describe('GNS', () => {
     await fixture.tearDown()
   })
 
-  describe('Publishing names', function () {
+  describe('Publishing names and versions', function () {
+    describe('setDefaultName', function () {
+      it('setDefaultName emits the event', async function () {
+        const tx = gns
+          .connect(me.signer)
+          .setDefaultName(me.address, 0, subgraph1.nameIdentifier, subgraph1.name)
+        await expect(tx)
+          .emit(gns, 'SetDefaultName')
+          .withArgs(subgraph1.graphAccount.address, 0, subgraph1.nameIdentifier, subgraph1.name)
+      })
+      it('setDefaultName fails if not owner', async function () {
+        const tx = gns
+          .connect(other.signer)
+          .setDefaultName(me.address, 0, subgraph1.nameIdentifier, subgraph1.name)
+        await expect(tx).revertedWith('GNS: Only graph account owner can call')
+      })
+    })
+    describe('updateSubgraphMetadata', function () {
+      it('updateSubgraphMetadata emits the event', async function () {
+        const tx = gns
+          .connect(me.signer)
+          .updateSubgraphMetadata(me.address, 0, subgraph1.subgraphMetadata)
+        await expect(tx)
+          .emit(gns, 'SubgraphMetadataUpdated')
+          .withArgs(subgraph1.graphAccount.address, 0, subgraph1.subgraphMetadata)
+      })
+      it('updateSubgraphMetadata fails if not owner', async function () {
+        const tx = gns
+          .connect(other.signer)
+          .updateSubgraphMetadata(me.address, 0, subgraph1.subgraphMetadata)
+        await expect(tx).revertedWith('GNS: Only graph account owner can call')
+      })
+    })
     describe('isPublished', function () {
       it('should return if the subgraph is published', async function () {
         expect(await gns.isPublished(subgraph1.graphAccount.address, 0)).eq(false)
@@ -455,11 +482,9 @@ describe('GNS', () => {
           .publishNewSubgraph(
             subgraph1.graphAccount.address,
             ethers.constants.HashZero,
-            subgraph1.nameIdentifier,
-            subgraph1.name,
-            subgraph1.metadataHash,
+            subgraph1.versionMetadata,
           )
-        await expect(tx).revertedWith('GNS: Cannot set to 0 in publish')
+        await expect(tx).revertedWith('GNS: Cannot set deploymentID to 0 in publish')
       })
     })
 
@@ -475,18 +500,13 @@ describe('GNS', () => {
             subgraph1.graphAccount.address,
             0,
             subgraph1.subgraphDeploymentID,
-            0,
-            subgraph1.nameIdentifier,
-            subgraph1.name,
-            subgraph1.metadataHash,
+            subgraph1.versionMetadata,
           )
       })
 
       it('should reject publishing a version to a numbered subgraph that does not exist', async function () {
-        const tx = publishNewVersion(me, me.address, 0)
-        await expect(tx).revertedWith(
-          'GNS: Cant publish a version directly for a subgraph that wasnt created yet',
-        )
+        const tx = publishNewVersion(me, me.address, 9999)
+        await expect(tx).revertedWith('GNS: Cannot update version if not published')
       })
 
       it('reject if not the owner', async function () {
@@ -507,23 +527,13 @@ describe('GNS', () => {
         expect(ethers.constants.HashZero).eq(deploymentID)
       })
 
-      it('should allow a deprecated subgraph to be republished', async function () {
+      it('should prevent a deprecated subgraph from being republished', async function () {
         await publishNewSubgraph(me, me.address, 0)
         await deprecateSubgraph(me, me.address, 0)
         const tx = publishNewVersion(me, me.address, 0)
-
-        // Event being emitted indicates version has been updated
-        await expect(tx)
-          .emit(gns, 'SubgraphPublished')
-          .withArgs(
-            subgraph1.graphAccount.address,
-            0,
-            subgraph1.subgraphDeploymentID,
-            0,
-            subgraph1.nameIdentifier,
-            subgraph1.name,
-            subgraph1.metadataHash,
-          )
+        await expect(tx).revertedWith(
+          'Cannot update version if not published, or has been deprecated',
+        )
       })
 
       it('reject if the subgraph does not exist', async function () {
