Orchestrator Updates

contracts/Orchestrator.sol

@@ -1,15 +1,24 @@
-pragma solidity 0.7.6;
+// SPDX-License-Identifier: GPL-3.0-or-later
+pragma solidity 0.8.2;
 
-import "./_external/Ownable.sol";
+import {Ownable} from "@openzeppelin/contracts/access/Ownable.sol";
+import {StringUtils} from "./_external/StringUtils.sol";
 
-import "./UFragmentsPolicy.sol";
+interface IUFragmentsPolicy {
+    function rebase() external;
+}
 
 /**
  * @title Orchestrator
  * @notice The orchestrator is the main entry point for rebase operations. It coordinates the policy
  * actions with external consumers.
  */
 contract Orchestrator is Ownable {
+    using StringUtils for uint16;
+
+    // Reference to the Ampleforth Policy
+    address public policy;
+
     struct Transaction {
         bool enabled;
         address destination;
@@ -19,35 +28,32 @@ contract Orchestrator is Ownable {
     // Stable ordering is not guaranteed.
     Transaction[] public transactions;
 
-    UFragmentsPolicy public policy;
-
     /**
      * @param policy_ Address of the UFragments policy.
      */
-    constructor(address policy_) public {
-        Ownable.initialize(msg.sender);
-        policy = UFragmentsPolicy(policy_);
+    constructor(address policy_) Ownable() {
+        policy = policy_;
     }
 
     /**
      * @notice Main entry point to initiate a rebase operation.
      *         The Orchestrator calls rebase on the policy and notifies downstream applications.
      *         Contracts are guarded from calling, to avoid flash loan attacks on liquidity
      *         providers.
-     *         If a transaction in the transaction list fails, Orchestrator will stop execution
-     *         and revert to prevent a gas underprice attack.
      */
     function rebase() external {
         require(msg.sender == tx.origin); // solhint-disable-line avoid-tx-origin
 
-        policy.rebase();
+        IUFragmentsPolicy(policy).rebase();
 
-        for (uint256 i = 0; i < transactions.length; i++) {
+        for (uint16 i = 0; i < transactions.length; i++) {
             Transaction storage t = transactions[i];
             if (t.enabled) {
-                (bool result, ) = t.destination.call(t.data);
-                if (!result) {
-                    revert("Transaction Failed");
+                (bool success, ) = t.destination.call(t.data);
+
+                // Transaction failed, revert with message
+                if (!success) {
+                    revert(buildRevertReason(i));
                 }
             }
         }
@@ -59,15 +65,16 @@ contract Orchestrator is Ownable {
      * @param data Transaction data payload
      */
     function addTransaction(address destination, bytes memory data) external onlyOwner {
+        // require(transactions.length < type(uint16).max);
         transactions.push(Transaction({enabled: true, destination: destination, data: data}));
     }
 
     /**
      * @param index Index of transaction to remove.
      *              Transaction ordering may have changed since adding.
      */
-    function removeTransaction(uint256 index) external onlyOwner {
-        require(index < transactions.length, "index out of bounds");
+    function removeTransaction(uint16 index) external onlyOwner {
+        require(index < transactions.length, "Orchestrator: index out of bounds");
 
         if (index < transactions.length - 1) {
             transactions[index] = transactions[transactions.length - 1];
@@ -80,8 +87,11 @@ contract Orchestrator is Ownable {
      * @param index Index of transaction. Transaction ordering may have changed since adding.
      * @param enabled True for enabled, false for disabled.
      */
-    function setTransactionEnabled(uint256 index, bool enabled) external onlyOwner {
-        require(index < transactions.length, "index must be in range of stored tx list");
+    function setTransactionEnabled(uint16 index, bool enabled) external onlyOwner {
+        require(
+            index < transactions.length,
+            "Orchestrator: index must be in range of stored tx list"
+        );
         transactions[index].enabled = enabled;
     }
 
@@ -91,4 +101,18 @@ contract Orchestrator is Ownable {
     function transactionsSize() external view returns (uint256) {
         return transactions.length;
     }
+
+    /**
+     * @param txIndex The index of the failing transaction in the transaction array.
+     * @return The revert reason.
+     */
+    function buildRevertReason(uint16 txIndex) internal pure returns (string memory) {
+        return
+            string(
+                abi.encodePacked(
+                    "Orchestrator: transaction:{index} reverted:",
+                    txIndex.uintToBytes()
+                )
+            );
+    }
 }

contracts/UFragments.sol

@@ -1,3 +1,4 @@
+// SPDX-License-Identifier: GPL-3.0-or-later
 pragma solidity 0.7.6;
 
 import "./_external/SafeMath.sol";

contracts/UFragmentsPolicy.sol

@@ -1,3 +1,4 @@
+// SPDX-License-Identifier: GPL-3.0-or-later
 pragma solidity 0.7.6;
 
 import "./_external/SafeMath.sol";

contracts/_external/StringUtils.sol

@@ -0,0 +1,23 @@
+// SPDX-License-Identifier: Unlicensed
+pragma solidity ^0.8.0;
+
+// https://github.com/pipermerriam/ethereum-string-utils/blob/master/contracts/StringLib.sol
+// String Utils v0.1
+/// @title String Utils - String utility functions
+/// @author Piper Merriam - <[email protected]>
+library StringUtils {
+    /// @dev Converts an unsigned integert to its string representation.
+    /// @param v The number to be converted.
+    function uintToBytes(uint256 v) internal pure returns (bytes32 ret) {
+        if (v == 0) {
+            ret = "0";
+        } else {
+            while (v > 0) {
+                ret = bytes32(uint256(ret) / (2**8));
+                ret |= bytes32(((v % 10) + 48) * 2**(8 * 31));
+                v /= 10;
+            }
+        }
+        return ret;
+    }
+}

contracts/mocks/ConstructorRebaseCallerContract.sol

@@ -1,12 +1,14 @@
 pragma solidity 0.7.6;
 
-import "../Orchestrator.sol";
+interface IOrchestrator {
+    function rebase() external;
+}
 
 contract ConstructorRebaseCallerContract {
     constructor(address orchestrator) public {
         // Take out a flash loan.
         // Do something funky...
-        Orchestrator(orchestrator).rebase(); // should fail
+        IOrchestrator(orchestrator).rebase(); // should fail
         // pay back flash loan.
     }
 }

contracts/mocks/MockDownstream.sol

@@ -39,4 +39,14 @@ contract MockDownstream is Mock {
 
         require(false, "reverted");
     }
+
+    function revertsWithoutMessage() external {
+        emit FunctionCalled("MockDownstream", "reverts", msg.sender);
+
+        uint256[] memory uintVals = new uint256[](0);
+        int256[] memory intVals = new int256[](0);
+        emit FunctionArguments(uintVals, intVals);
+
+        revert();
+    }
 }

contracts/mocks/RebaseCallerContract.sol

@@ -1,12 +1,14 @@
 pragma solidity 0.7.6;
 
-import "../Orchestrator.sol";
+interface IOrchestrator {
+    function rebase() external;
+}
 
 contract RebaseCallerContract {
     function callRebase(address orchestrator) public returns (bool) {
         // Take out a flash loan.
         // Do something funky...
-        Orchestrator(orchestrator).rebase(); // should fail
+        IOrchestrator(orchestrator).rebase(); // should fail
         // pay back flash loan.
         return true;
     }

hardhat.config.ts

@@ -3,26 +3,26 @@ import { HardhatUserConfig } from 'hardhat/config'
 import '@nomiclabs/hardhat-ethers'
 import '@nomiclabs/hardhat-waffle'
 import '@openzeppelin/hardhat-upgrades'
-import "@nomiclabs/hardhat-etherscan";
+import '@nomiclabs/hardhat-etherscan'
 import 'solidity-coverage'
 import 'hardhat-gas-reporter'
 
 require('./scripts/deploy')
 
 export default {
   etherscan: {
-    apiKey: process.env.ETHERSCAN_API_KEY
+    apiKey: process.env.ETHERSCAN_API_KEY,
   },
   networks: {
     rinkeby: {
-      url: `https://rinkeby.infura.io/v3/${process.env.INFURA_SECRET}`
+      url: `https://rinkeby.infura.io/v3/${process.env.INFURA_SECRET}`,
     },
     kovan: {
-      url: `https://kovan.infura.io/v3/${process.env.INFURA_SECRET}`
+      url: `https://kovan.infura.io/v3/${process.env.INFURA_SECRET}`,
     },
     mainnet: {
-      url: `https://mainnet.infura.io/v3/${process.env.INFURA_SECRET}`
-    }
+      url: `https://mainnet.infura.io/v3/${process.env.INFURA_SECRET}`,
+    },
   },
   solidity: {
     compilers: [
@@ -36,7 +36,13 @@ export default {
         },
       },
       {
-        version: '0.4.24',
+        version: '0.8.2',
+        settings: {
+          optimizer: {
+            enabled: true,
+            runs: 200,
+          },
+        },
       },
     ],
   },

package.json

@@ -55,6 +55,7 @@
     "typescript": "^4.0.2"
   },
   "dependencies": {
+    "@openzeppelin/contracts": "^4.0.0",
     "hardhat-gas-reporter": "^1.0.4"
   }
 }

test/unit/Orchestrator.ts

@@ -261,7 +261,7 @@ describe('Orchestrator', function () {
     })
   })
 
-  describe('when a transaction reverts', async function () {
+  describe('when a transaction reverts with message', async function () {
     before('adding 3 transactions', async function () {
       const updateOneArgEncoded = await mockDownstream.populateTransaction.updateOneArg(
         123,
@@ -282,12 +282,75 @@ describe('Orchestrator', function () {
       await orchestrator
         .connect(deployer)
         .addTransaction(mockDownstream.address, updateTwoArgsEncoded.data)
-      await expect(orchestrator.connect(deployer).rebase()).to.be.reverted
+
+      let exp
+      try {
+        await orchestrator.connect(deployer).rebase()
+      } catch (e) {
+        exp = e
+      }
+      expect(!exp).to.be.false
+      expect(exp.message.replace(/\0/g, '')).to.eq(
+        'VM Exception while processing transaction: revert Orchestrator: transaction:{index} reverted:1',
+      )
     })
 
     it('should have 3 transactions', async function () {
       expect(await orchestrator.transactionsSize()).to.eq(3)
     })
+
+    after('removing 3 transactions', async function () {
+      await orchestrator.connect(deployer).removeTransaction(2)
+      await orchestrator.connect(deployer).removeTransaction(1)
+      await orchestrator.connect(deployer).removeTransaction(0)
+      expect(await orchestrator.transactionsSize()).to.eq(0)
+    })
+  })
+
+  describe('when a transaction reverts without message', async function () {
+    before('adding 3 transactions', async function () {
+      const updateOneArgEncoded = await mockDownstream.populateTransaction.updateOneArg(
+        555,
+      )
+      await orchestrator
+        .connect(deployer)
+        .addTransaction(mockDownstream.address, updateOneArgEncoded.data)
+
+      const updateTwoArgsEncoded = await mockDownstream.populateTransaction.updateTwoArgs(
+        72,
+        33,
+      )
+      await orchestrator
+        .connect(deployer)
+        .addTransaction(mockDownstream.address, updateTwoArgsEncoded.data)
+
+      const revertsEncoded = await mockDownstream.populateTransaction.revertsWithoutMessage()
+      await orchestrator
+        .connect(deployer)
+        .addTransaction(mockDownstream.address, revertsEncoded.data)
+
+      let exp
+      try {
+        await orchestrator.connect(deployer).rebase()
+      } catch (e) {
+        exp = e
+      }
+      expect(!exp).to.be.false
+      expect(exp.message.replace(/\0/g, '')).to.eq(
+        'VM Exception while processing transaction: revert Orchestrator: transaction:{index} reverted:2',
+      )
+    })
+
+    it('should have 3 transactions', async function () {
+      expect(await orchestrator.transactionsSize()).to.eq(3)
+    })
+
+    after('removing 3 transactions', async function () {
+      await orchestrator.connect(deployer).removeTransaction(2)
+      await orchestrator.connect(deployer).removeTransaction(1)
+      await orchestrator.connect(deployer).removeTransaction(0)
+      expect(await orchestrator.transactionsSize()).to.eq(0)
+    })
   })
 
   describe('Access Control', function () {
@@ -320,9 +383,9 @@ describe('Orchestrator', function () {
       })
 
       it('should revert if index out of bounds', async function () {
-        expect(await orchestrator.transactionsSize()).to.lt(5)
+        expect(await orchestrator.transactionsSize()).to.lt(2)
         await expect(
-          orchestrator.connect(deployer).setTransactionEnabled(5, true),
+          orchestrator.connect(deployer).setTransactionEnabled(2, true),
         ).to.be.reverted
       })
 
@@ -341,8 +404,8 @@ describe('Orchestrator', function () {
       })
 
       it('should revert if index out of bounds', async function () {
-        expect(await orchestrator.transactionsSize()).to.lt(5)
-        await expect(orchestrator.connect(deployer).removeTransaction(5)).to.be
+        expect(await orchestrator.transactionsSize()).to.lt(2)
+        await expect(orchestrator.connect(deployer).removeTransaction(2)).to.be
           .reverted
       })