feat: service registry v1 (#193)

* service-registry: draft v1
* service-registry: add validations and tests

Author: abarmat

contracts/ServiceRegistry.sol

@@ -1,63 +1,57 @@
 pragma solidity ^0.6.4;
 pragma experimental ABIEncoderV2;
 
-import "./Governed.sol";
-
-
-contract ServiceRegistry is Governed {
-    /*
-     * @title Graph Protocol Service Registry contract
-     *
-     * @author Bryant Eisenbach
-     * @author Reuven Etzion
-     *
-     * @notice Contract Specification:
-     *
-     * Requirements ("Service Registry" contract):
-     * req 01 Maps Ethereum Addresses to URLs
-     * req 02 No other contracts depend on this, rather is consumed by users of The Graph.
-     * ...
-     * @question - Who sets registeredUrls? Staking? (need interface)
-     */
 
-    /* EVENTS */
-    event ServiceUrlSet(address indexed serviceProvider, string urlString);
+/**
+ * @title ServiceRegistry contract
+ * @dev This contract supports the service discovery process by allowing indexers to
+ * register their service url and any other relevant information.
+ */
+contract ServiceRegistry {
+    // -- State --
+
+    struct IndexerService {
+        string url;
+        string geohash;
+    }
+
+    mapping(address => IndexerService) public services;
 
-    /* Contract Constructor */
-    /* @param _governor <address> - Address of the multisig contract as Governor of this contract */
-    constructor(address _governor) public Governed(_governor) {}
+    // -- Events --
 
-    /*  A dynamic array of URLs that bootstrap the graph subgraph
-        Note: The graph subgraph bootstraps the network. It has no way to retrieve
-        the list of all indexers at the start of indexing. Therefore a single
-        dynamic array bootstrapIndexerURLs is used to store the URLS of the Graph
-        Network indexing nodes for the query node to obtain */
+    event ServiceRegistered(address indexed indexer, string url, string geohash);
+    event ServiceUnregistered(address indexed indexer);
 
-    // TODO - Who should be able to set this? Right now it is only governance. It needed to more robust, and we need to consider the out of protocol coordination
-    mapping(address => bytes) public bootstrapIndexerURLs;
+    /**
+     * @dev Register an indexer service
+     * @param _url URL of the indexer service
+     * @param _geohash Geohash of the indexer service location
+     */
+    function register(string calldata _url, string calldata _geohash) external {
+        address indexer = msg.sender;
+        require(bytes(_url).length > 0, "Service must specify a URL");
 
-    /* Graph Protocol Functions */
+        services[indexer] = IndexerService(_url, _geohash);
 
-    /*
-     * @notice Set graph network subgraph indexer URL
-     * @dev Only governance can do this. Indexers added are arranged out of protocol
-     *
-     * @param _indexer <address> - Address of the indexer
-     * @param _url <string> - URL of the service provider
+        emit ServiceRegistered(indexer, _url, _geohash);
+    }
+
+    /**
+     * @dev Unregister an indexer service
      */
-    function setBootstrapIndexerURL(address _indexer, string calldata _url) external onlyGovernor {
-        bytes memory url = bytes(_url);
-        bootstrapIndexerURLs[_indexer] = url;
-        emit ServiceUrlSet(_indexer, _url);
+    function unregister() external {
+        address indexer = msg.sender;
+        require(isRegistered(indexer), "Service already unregistered");
+
+        delete services[indexer];
+        emit ServiceUnregistered(indexer);
     }
 
-    /*
-     * @notice Set service provider url from their address
-     * @dev Only msg.sender may do this
-     *
-     * @param _url <string> - URL of the service provider
+    /**
+     * @dev Return the registration status of an indexer service
+     * @return True if the indexer service is registered
      */
-    function setUrl(string calldata _url) external {
-        emit ServiceUrlSet(msg.sender, _url);
+    function isRegistered(address _indexer) public view returns (bool) {
+        return bytes(services[_indexer].url).length > 0;
     }
 }

test/lib/deployment.js

@@ -3,6 +3,7 @@ const Curation = artifacts.require('./Curation.sol')
 const DisputeManager = artifacts.require('./DisputeManager')
 const EpochManager = artifacts.require('./EpochManager')
 const GraphToken = artifacts.require('./GraphToken.sol')
+const ServiceRegisty = artifacts.require('./ServiceRegistry.sol')
 const Staking = artifacts.require('./Staking.sol')
 
 // helpers
@@ -39,6 +40,10 @@ function deployEpochManagerContract(owner, params) {
   return EpochManager.new(owner, defaults.epochs.lengthInBlocks, params)
 }
 
+function deployServiceRegistry(owner) {
+  return ServiceRegisty.new({ from: owner })
+}
+
 async function deployStakingContract(owner, graphToken, epochManager, curation, params) {
   const contract = await Staking.new(owner, graphToken, epochManager, curation, params)
   await contract.setChannelDisputeEpochs(defaults.staking.channelDisputeEpochs, { from: owner })
@@ -52,5 +57,6 @@ module.exports = {
   deployDisputeManagerContract,
   deployEpochManagerContract,
   deployGraphToken,
+  deployServiceRegistry,
   deployStakingContract,
 }

test/serviceRegisty.test.js

@@ -1,70 +1,84 @@
-const { expectEvent } = require('@openzeppelin/test-helpers')
+const { expectEvent, expectRevert } = require('@openzeppelin/test-helpers')
 
 // contracts
-const ServiceRegisty = artifacts.require('./ServiceRegistry.sol')
+const deployment = require('./lib/deployment')
 
-contract('Service Registry', accounts => {
-  let deployedServiceRegistry
-  const governor = accounts[0]
-  const indexer = accounts[1]
+contract('Service Registry', ([governor, indexer]) => {
+  beforeEach(async function() {
+    this.serviceRegisty = await deployment.deployServiceRegistry(governor)
+    this.geohash = '69y7hdrhm6mp'
 
-  before(async () => {
-    // deploy ServiceRegistry contract
-    deployedServiceRegistry = await ServiceRegisty.new(
-      governor, // governor
-      { from: governor },
-    )
-  })
+    this.shouldRegister = async function(url, geohash) {
+      // Register the indexer service
+      const { logs } = await this.serviceRegisty.register(url, geohash, {
+        from: indexer,
+      })
 
-  it('...should allow setting URL with arbitrary length string', async () => {
-    const url = 'https://192.168.2.1/'
+      // Updated state
+      const indexerService = await this.serviceRegisty.services(indexer)
+      expect(indexerService.url).to.be.eq(url)
+      expect(indexerService.geohash).to.be.eq(geohash)
 
-    // Set the url
-    const { logs } = await deployedServiceRegistry.setUrl(url, {
-      from: indexer,
-    })
+      // Event emitted
+      expectEvent.inLogs(logs, 'ServiceRegistered', {
+        indexer: indexer,
+        url: url,
+        geohash: geohash,
+      })
+    }
+  })
 
-    expectEvent.inLogs(logs, 'ServiceUrlSet', {
-      serviceProvider: indexer,
-      urlString: url,
+  describe('register()', function() {
+    it('should allow registering', async function() {
+      const url = 'https://192.168.2.1/'
+      await this.shouldRegister(url, this.geohash)
     })
-  })
 
-  it('...should allow setting URL with a very long string', async () => {
-    const url = 'https://' + 'a'.repeat(125) + '.com'
+    it('should allow registering with a very long string', async function() {
+      const url = 'https://' + 'a'.repeat(125) + '.com'
+      await this.shouldRegister(url, this.geohash)
+    })
 
-    // Set the url
-    const { logs } = await deployedServiceRegistry.setUrl(url, {
-      from: indexer,
+    it('should allow updating a registration', async function() {
+      const [url1, geohash1] = ['https://thegraph.com', '69y7hdrhm6mp']
+      const [url2, geohash2] = ['https://192.168.0.1', 'dr5regw2z6y']
+      await this.shouldRegister(url1, geohash1)
+      await this.shouldRegister(url2, geohash2)
     })
 
-    expectEvent.inLogs(logs, 'ServiceUrlSet', {
-      serviceProvider: indexer,
-      urlString: url,
+    it('reject registering empty URL', async function() {
+      await expectRevert(
+        this.serviceRegisty.register('', '', {
+          from: indexer,
+        }),
+        'Service must specify a URL',
+      )
     })
   })
 
-  it('...should allow setting multiple graph bootstrap indexer URLs', async () => {
-    const url = 'https://192.168.2.1/'
-    const urlBytes = web3.utils.utf8ToHex(url)
-
-    const indexers = accounts.slice(5, 8)
+  describe('unregister()', function() {
+    it('should unregister existing registration', async function() {
+      const url = 'https://thegraph.com'
 
-    for (let i = 0; i < 3; i++) {
-      // Set the url, only governor can
-      const { logs } = await deployedServiceRegistry.setBootstrapIndexerURL(indexers[i], url, {
-        from: governor,
+      // Register the indexer service
+      await this.serviceRegisty.register(url, this.geohash, {
+        from: indexer,
       })
 
-      // Verify that the the ServiceUrlSet event is emitted
-      expectEvent.inLogs(logs, 'ServiceUrlSet', {
-        serviceProvider: indexers[i],
-        urlString: url,
+      // Unregister the indexer service
+      const { logs } = await this.serviceRegisty.unregister({ from: indexer })
+
+      // Event emitted
+      expectEvent.inLogs(logs, 'ServiceUnregistered', {
+        indexer: indexer,
       })
+    })
 
-      // Verify that the indexer URL has been updated
-      const indexerUrlBytes = await deployedServiceRegistry.bootstrapIndexerURLs(indexers[i])
-      assert(indexerUrlBytes === urlBytes, `Indexer ${i} URL was not set to ${url}`)
-    }
+    it('reject unregister non-existing registration', async function() {
+      await expectRevert(
+        this.serviceRegisty.unregister({ from: indexer }),
+        'Service already unregistered',
+      )
+    })
   })
 })