Cache address resolution from the Controller (#430)

Having a Controller contract is great in that we can sync addresses across the protocol from a single place, but it involves a roundtrip of CALL opcodes to get the addresses each time they are used.

This change creates a local cache within each contract to store the list of contract addresses and refresh it when the Controller changes any contract address.

This mechanism relies on an external party calling the `syncAllContracts` function.

Author: abarmat

contracts/governance/Managed.sol

@@ -2,8 +2,8 @@
 
 pragma solidity ^0.7.3;
 
-import "./IManaged.sol";
 import "./IController.sol";
+
 import "../curation/ICuration.sol";
 import "../epochs/IEpochManager.sol";
 import "../rewards/IRewardsManager.sol";
@@ -12,19 +12,33 @@ import "../token/IGraphToken.sol";
 
 /**
  * @title Graph Managed contract
- * @dev The Managed contract provides an interface for contracts to interact with the Controller
+ * @dev The Managed contract provides an interface to interact with the Controller.
+ * It also provides local caching for contract addresses. This mechanism relies on calling the
+ * public `syncAllContracts()` function whenever a contract changes in the controller.
+ *
  * Inspired by Livepeer:
  * https://github.com/livepeer/protocol/blob/streamflow/contracts/Controller.sol
  */
 contract Managed {
+    // -- State --
+
     // Controller that contract is registered with
     IController public controller;
-    mapping(bytes32 => address) public addressCache;
+    mapping(bytes32 => address) private addressCache;
     uint256[10] private __gap;
 
+    // -- Events --
+
     event ParameterUpdated(string param);
     event SetController(address controller);
 
+    /**
+     * @dev Emitted when contract with `nameHash` is synced to `contractAddress`.
+     */
+    event ContractSynced(bytes32 indexed nameHash, address contractAddress);
+
+    // -- Modifiers --
+
     function _notPartialPaused() internal view {
         require(!controller.paused(), "Paused");
         require(!controller.partialPaused(), "Partial-paused");
@@ -38,6 +52,10 @@ contract Managed {
         require(msg.sender == controller.getGovernor(), "Caller must be Controller governor");
     }
 
+    function _onlyController() internal view {
+        require(msg.sender == address(controller), "Caller must be Controller");
+    }
+
     modifier notPartialPaused {
         _notPartialPaused();
         _;
@@ -48,26 +66,29 @@ contract Managed {
         _;
     }
 
-    // Check if sender is controller
+    // Check if sender is controller.
     modifier onlyController() {
-        require(msg.sender == address(controller), "Caller must be Controller");
+        _onlyController();
         _;
     }
 
+    // Check if sender is the governor.
     modifier onlyGovernor() {
         _onlyGovernor();
         _;
     }
 
+    // -- Functions --
+
     /**
-     * @dev Initialize the controller
+     * @dev Initialize the controller.
      */
     function _initialize(address _controller) internal {
         _setController(_controller);
     }
 
     /**
-     * @notice Set Controller. Only callable by current controller
+     * @notice Set Controller. Only callable by current controller.
      * @param _controller Controller contract address
      */
     function setController(address _controller) external onlyController {
@@ -85,42 +106,81 @@ contract Managed {
     }
 
     /**
-     * @dev Return Curation interface
+     * @dev Return Curation interface.
      * @return Curation contract registered with Controller
      */
     function curation() internal view returns (ICuration) {
-        return ICuration(controller.getContractProxy(keccak256("Curation")));
+        return ICuration(_resolveContract(keccak256("Curation")));
     }
 
     /**
-     * @dev Return EpochManager interface
+     * @dev Return EpochManager interface.
      * @return Epoch manager contract registered with Controller
      */
     function epochManager() internal view returns (IEpochManager) {
-        return IEpochManager(controller.getContractProxy(keccak256("EpochManager")));
+        return IEpochManager(_resolveContract(keccak256("EpochManager")));
     }
 
     /**
-     * @dev Return RewardsManager interface
+     * @dev Return RewardsManager interface.
      * @return Rewards manager contract registered with Controller
      */
     function rewardsManager() internal view returns (IRewardsManager) {
-        return IRewardsManager(controller.getContractProxy(keccak256("RewardsManager")));
+        return IRewardsManager(_resolveContract(keccak256("RewardsManager")));
     }
 
     /**
-     * @dev Return Staking interface
+     * @dev Return Staking interface.
      * @return Staking contract registered with Controller
      */
     function staking() internal view returns (IStaking) {
-        return IStaking(controller.getContractProxy(keccak256("Staking")));
+        return IStaking(_resolveContract(keccak256("Staking")));
     }
 
     /**
-     * @dev Return GraphToken interface
+     * @dev Return GraphToken interface.
      * @return Graph token contract registered with Controller
      */
     function graphToken() internal view returns (IGraphToken) {
-        return IGraphToken(controller.getContractProxy(keccak256("GraphToken")));
+        return IGraphToken(_resolveContract(keccak256("GraphToken")));
+    }
+
+    /**
+     * @dev Resolve a contract address from the cache or the Controller if not found.
+     * @return Address of the contract
+     */
+    function _resolveContract(bytes32 _nameHash) internal view returns (address) {
+        address contractAddress = addressCache[_nameHash];
+        if (contractAddress == address(0)) {
+            contractAddress = controller.getContractProxy(_nameHash);
+        }
+        return contractAddress;
+    }
+
+    /**
+     * @dev Cache a contract address from the Controller registry.
+     * @param _name Name of the contract to sync into the cache
+     */
+    function _syncContract(string memory _name) internal {
+        bytes32 nameHash = keccak256(abi.encodePacked(_name));
+        address contractAddress = controller.getContractProxy(nameHash);
+        if (addressCache[nameHash] != contractAddress) {
+            addressCache[nameHash] = contractAddress;
+            emit ContractSynced(nameHash, contractAddress);
+        }
+    }
+
+    /**
+     * @dev Sync protocol contract addresses from the Controller registry.
+     * This function will cache all the contracts using the latest addresses
+     * Anyone can call the function whenever a Proxy contract change in the
+     * controller to ensure the protocol is using the latest version
+     */
+    function syncAllContracts() external {
+        _syncContract("Curation");
+        _syncContract("EpochManager");
+        _syncContract("RewardsManager");
+        _syncContract("Staking");
+        _syncContract("GraphToken");
     }
 }

test/curation/curation.test.ts

@@ -311,6 +311,8 @@ describe('Curation', () => {
         await controller
           .connect(governor.signer)
           .setContractProxy(utils.id('Staking'), stakingMock.address)
+        await curation.syncAllContracts()
+
         const tx = curation
           .connect(stakingMock.signer)
           .collect(subgraphDeploymentID, tokensToCollect)
@@ -332,6 +334,8 @@ describe('Curation', () => {
         await controller
           .connect(governor.signer)
           .setContractProxy(utils.id('Staking'), stakingMock.address)
+        await curation.syncAllContracts()
+
         await shouldCollect(toGRT('1'))
         await shouldCollect(toGRT('10'))
         await shouldCollect(toGRT('100'))
@@ -343,6 +347,7 @@ describe('Curation', () => {
         await controller
           .connect(governor.signer)
           .setContractProxy(utils.id('Staking'), stakingMock.address)
+        await curation.syncAllContracts()
 
         // Collect increase the pool reserves
         await shouldCollect(toGRT('100'))

test/lib/fixtures.ts

@@ -58,6 +58,13 @@ export class NetworkFixture {
     await controller.setContractProxy(utils.id('ServiceRegistry'), serviceRegistry.address)
 
     // Setup contracts
+    await curation.connect(deployer).syncAllContracts()
+    await gns.connect(deployer).syncAllContracts()
+    await serviceRegistry.connect(deployer).syncAllContracts()
+    await disputeManager.connect(deployer).syncAllContracts()
+    await rewardsManager.connect(deployer).syncAllContracts()
+    await staking.connect(deployer).syncAllContracts()
+
     await staking.connect(deployer).setSlasher(slasherAddress, true)
     await grt.connect(deployer).addMinter(rewardsManager.address)
     await gns.connect(deployer).approveAll()

test/staking/delegation.test.ts

@@ -15,6 +15,7 @@ import {
   toGRT,
   toBN,
   Account,
+  advanceBlock,
 } from '../lib/testHelpers'
 
 const { AddressZero, HashZero } = constants
@@ -445,6 +446,9 @@ describe('Staking::Delegation', () => {
       })
 
       it('should undelegate properly when multiple delegations', async function () {
+        // Use long enough epochs to avoid jumping to the next epoch involuntarily on our test
+        await epochManager.setEpochLength(toBN((60 * 60) / 15))
+
         await shouldDelegate(delegator, toGRT('1234'))
         await shouldDelegate(delegator, toGRT('100'))
         await shouldDelegate(delegator, toGRT('50'))
@@ -460,6 +464,7 @@ 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'))