storage types vs usage types (#10)

.eslintrc.js

@@ -35,7 +35,7 @@ module.exports = {
               // misc
               "deployer", "http", "https", "github", "chai", "argv", "evm",
               "jsonrpc", "timestamp", "uint256", "erc20", "bignumber", "lodash",
-              "arg", "npm", "seedrandom", "eql", "sinon", "yaml", "promisify",
+              "arg", "npm", "seedrandom", "eql", "sinon", "yaml", "posix", "promisify",
               "passcode", "geth", "rpc", "rpcmsg","stdev",  "stochasm", "aggregator",
               "whitelist",
 

contracts/MarketOracle.sol

@@ -11,14 +11,11 @@ import "./MarketSource.sol";
  * @notice https://www.fragments.org/protocol/
  *
  * @dev This oracle provides price and volume data onchain using data from a whitelisted
-        set of market sources.
+ *      set of market sources.
  */
 contract MarketOracle is Ownable {
     using SafeMath for uint256;
 
-    // Maximum number of whitelisted sources
-    uint8 public constant MAX_SOURCES = 255;
-
     // Whitelist of sources
     MarketSource[] public whitelist;
 
@@ -31,33 +28,32 @@ contract MarketOracle is Ownable {
      *         The returned price is in an 18 decimal fixed point format.
      *         The returned volume parameter is in a 2 decimal fixed point format.
      */
-    function getPriceAndVolume() external returns (uint128, uint128) {
+    function getPriceAndVolume() external returns (uint256, uint256) {
         uint256 volumeWeightedSum = 0;
         uint256 volume = 0;
 
-        for (uint8 i = 0; i < whitelist.length; i++) {
+        for (uint256 i = 0; i < whitelist.length; i++) {
             if (!whitelist[i].isActive()) {
                 emit SourceExpired(whitelist[i]);
                 continue;
             }
 
             volumeWeightedSum = volumeWeightedSum.add(
-                whitelist[i].exchangeRate().mul(whitelist[i].volume())
+                whitelist[i].getExchangeRate().mul(whitelist[i].getVolume24hrs())
             );
 
-            volume = volume.add(whitelist[i].volume());
+            volume = volume.add(whitelist[i].getVolume24hrs());
         }
 
         uint256 exchangeRate = volumeWeightedSum.div(volume);
-        return (uint128(exchangeRate), uint128(volume));
+        return (exchangeRate, volume);
     }
 
     /**
      * @dev Adds a market source to the whitelist
      * @param source Reference to the MarketSource contract to be whitelisted.
      */
     function addSource(MarketSource source) external onlyOwner {
-        require(whitelist.length < MAX_SOURCES);
         whitelist.push(source);
         emit SourceAdded(source);
     }

contracts/MarketSource.sol

@@ -14,22 +14,19 @@ import "openzeppelin-solidity/contracts/math/SafeMath.sol";
 contract MarketSource is Destructible {
     using SafeMath for uint256;
 
+    event ExchangeRateReported(uint128 exchangeRate, uint128 volume24hrs, uint64 indexed posixTimestamp);
+
     // Name of the source reporting exchange rates
     string public name;
 
     // The amount of time after which the report must be deemed expired
     uint256 public constant REPORT_EXPIRATION_TIME = 1 hours;
 
-    struct Report {
-        uint256 exchangeRate;
-        uint256 volume24hrs;
-        uint256 timestamp;
-    }
-
-    // Most recent report from the source
-    Report public report;
-
-    event ExchangeRateReported(uint256 exchangeRate, uint256 volume24hrs, uint256 indexed timestamp);
+    // These are the three oracle values that are continuously updated.
+    // Smaller types are used here locally to save on storage gas.
+    uint128 private exchangeRate;
+    uint128 private volume24hrs;
+    uint64 private posixTimestamp;
 
     constructor(string _name) public {
         name = _name;
@@ -38,46 +35,44 @@ contract MarketSource is Destructible {
     /**
      * @dev The MarketSource receives offchain information about the state of the market and
      *      provides it to downstream onchain consumers.
-     * @param exchangeRate The average UFragments-USD exchange rate over 24-hours.
+     * @param _exchangeRate The average UFragments-USD exchange rate over 24-hours.
      *        Submitted as an fixed point number scaled by {1/10**18}.
      *        (eg) 1500000000000000000 (1.5e18) means the rate is [1.5 USD = 1 UFragments]
-     * @param volume24hrs The total trade volume of UFragments over 24-hours,
+     * @param _volume24hrs The total trade volume of UFragments over 24-hours,
      *        up to the time of observation. Submitted as a fixed point number scaled by {1/10**2}.
      *        (eg) 12350032 means 123500.32 UFragments were being traded.
-     * @param timestamp The date and time when the observation was made. Sumbitted
+     * @param _posixTimestamp The date and time when the observation was made. Sumbitted
      *        as a UNIX timestamp, (ie) number of seconds since Jan 01 1970(UTC).
      */
-    function reportRate(uint256 exchangeRate, uint256 volume24hrs, uint256 timestamp) public onlyOwner {
-        require(exchangeRate > 0);
-        require(volume24hrs > 0);
+    function reportRate(uint128 _exchangeRate, uint128 _volume24hrs, uint64 _posixTimestamp) external onlyOwner {
+        require(_exchangeRate > 0);
+        require(_volume24hrs > 0);
 
-        report = Report({
-            exchangeRate: exchangeRate,
-            volume24hrs: volume24hrs,
-            timestamp: timestamp
-        });
+        exchangeRate = _exchangeRate;
+        volume24hrs = _volume24hrs;
+        posixTimestamp = _posixTimestamp;
 
-        emit ExchangeRateReported(exchangeRate, volume24hrs, timestamp);
+        emit ExchangeRateReported(exchangeRate, volume24hrs, posixTimestamp);
     }
 
     /**
-     * @return Most recently reported exchange rate.
+     * @return Most recently reported exchange rate as a uint256.
      */
-    function exchangeRate() public view returns (uint256) {
-        return report.exchangeRate;
+    function getExchangeRate() public view returns (uint256) {
+        return uint256(exchangeRate);
     }
 
     /**
-     * @return Most recently reported trade volume.
+     * @return Most recently reported trade volume as a uint256.
      */
-    function volume() public view returns (uint256) {
-        return report.volume24hrs;
+    function getVolume24hrs() public view returns (uint256) {
+        return uint256(volume24hrs);
     }
 
     /**
      * @return If less than {REPORT_EXPIRATION_TIME} has passed since the most recent report.
      */
     function isActive() public view returns (bool) {
-        return (report.timestamp + REPORT_EXPIRATION_TIME > now);
+        return (REPORT_EXPIRATION_TIME.add(posixTimestamp) > now);
     }
 }

test/logs/gas-utilization.yaml

@@ -1,5 +1,5 @@
 # Code generated - DO NOT EDIT.
 # Any manual changes may be lost.
-'MarketSourceFactory:DEPLOYMENT': 998567
-'MarketOracle:DEPLOYMENT': 1133401
-'MarketOracle:getPriceAndVolume(10 sources)': 135809
+'MarketSourceFactory:DEPLOYMENT': 1106283
+'MarketOracle:DEPLOYMENT': 1085933
+'MarketOracle:getPriceAndVolume(10 sources)': 138069

test/unit/market_oracle.js

@@ -50,26 +50,6 @@ contract('MarketOracle', async function (accounts) {
         expect(await oracle.whitelist.call(0)).to.eq(s1.address);
       });
     });
-
-    describe('when more than MAX_SOURCES are added', function () {
-      let snapshot;
-      before(async function () {
-        snapshot = await chain.snapshotChain();
-        for (let i = 0; i < 254; i++) {
-          await oracle.addSource(s1.address);
-        }
-      });
-      after(async function () {
-        await chain.revertToSnapshot(snapshot);
-      });
-
-      it('should fail', async function () {
-        await oracle.addSource(s1.address);
-        await chain.expectEthException(
-          oracle.addSource(s1.address)
-        );
-      });
-    });
   });
 
   describe('removeSource', function () {

test/unit/market_source.js

@@ -52,15 +52,15 @@ contract('MarketSource', async function (accounts) {
           r = await source.reportRate(1050000000000000000, 3, timestamp, { from: A, gas: gasLimit });
         });
         it('should update the report', async function () {
-          expect((await source.exchangeRate.call()).toNumber()).to.eq(1050000000000000000);
-          expect((await source.volume.call()).toNumber()).to.eq(3);
+          expect((await source.getExchangeRate.call()).toNumber()).to.eq(1050000000000000000);
+          expect((await source.getVolume24hrs.call()).toNumber()).to.eq(3);
         });
         it('should emit ExchangeRateReported', async function () {
           const reportEvent = r.logs[0];
           expect(reportEvent.event).to.eq('ExchangeRateReported');
           expect(reportEvent.args.exchangeRate.toNumber()).to.eq(1050000000000000000);
           expect(reportEvent.args.volume24hrs.toNumber()).to.eq(3);
-          expect(reportEvent.args.timestamp.toNumber()).to.eq(timestamp);
+          expect(reportEvent.args.posixTimestamp.toNumber()).to.eq(timestamp);
         });
       });
     });