Applied review comments from ben-review session OpenST#1

Author: Paruyr Gevorgyan

contracts/PriceOracleInterface.sol

@@ -46,8 +46,16 @@ interface PriceOracleInterface {
         returns (bytes3);
 
     /**
-     * @notice Returns an amount of the quote currency needed to purchase
-     *         one unit of the base currency.
+     * @notice Returns quote currency decimals.
+     */
+    function decimals()
+        external
+        view
+        returns (uint8);
+
+    /**
+     * @notice Returns an amount of the quote currency (see decimals()) needed
+     *         to purchase one unit of the base currency.
      *
      * @dev Function throws an exception if the price is invalid, for example,
      *      was not set, or became outdated, etc.

contracts/PricerRule.sol

@@ -70,6 +70,11 @@ contract PricerRule is Organized {
      */
     uint256 public conversionRateDecimalsFromBaseCurrencyToToken;
 
+    /**
+     * @dev Required decimals for price oracles.
+     */
+    uint8 public requiredPriceOracleDecimals;
+
     /**
      * @dev Token rules address of the economy.
      */
@@ -111,6 +116,7 @@ contract PricerRule is Organized {
      *                        to the token.
      * @param _conversionRateDecimals The conversion rate's decimals from the
      *                                economy base currency to the token.
+     * @param _requiredPriceOracleDecimals Required decimals for price oracles.
      * @param _tokenRules The economy token rules address.
      */
     constructor(
@@ -119,6 +125,7 @@ contract PricerRule is Organized {
         bytes3 _baseCurrencyCode,
         uint256 _conversionRate,
         uint8 _conversionRateDecimals,
+        uint8 _requiredPriceOracleDecimals,
         address _tokenRules
     )
         Organized(_organization)
@@ -149,6 +156,8 @@ contract PricerRule is Organized {
 
         conversionRateDecimalsFromBaseCurrencyToToken = _conversionRateDecimals;
 
+        requiredPriceOracleDecimals = _requiredPriceOracleDecimals;
+
         tokenRules = TokenRules(_tokenRules);
     }
 
@@ -189,10 +198,19 @@ contract PricerRule is Organized {
             "'to' and 'amount' transfer arrays' lengths are not equal."
         );
 
+        if (_toList.length == 0) {
+            return;
+        }
+
         uint256 baseCurrencyCurrentPrice = baseCurrencyPrice(
             _payCurrencyCode
         );
 
+        require(
+            baseCurrencyCurrentPrice != 0,
+            "Base currency price in pay currency is 0."
+        );
+
         require(
             isPriceInRange(
                 _baseCurrencyIntendedPrice,
@@ -206,7 +224,7 @@ contract PricerRule is Organized {
 
         for(uint256 i = 0; i < _amountList.length; ++i) {
             convertedAmounts[i] = convertPayCurrencyToToken(
-                _baseCurrencyIntendedPrice,
+                baseCurrencyCurrentPrice,
                 _amountList[i]
             );
         }
@@ -228,6 +246,8 @@ contract PricerRule is Organized {
      *            equal to the economy base currency code specified in this
      *            contract constructor.
      *          - The proposed price oracle does not exist.
+     *          - The proposed price oracle decimals number is equal to
+     *            the contract required price oracle decimals number.
      *
      * @param _priceOracle The proposed price oracle.
      */
@@ -242,6 +262,11 @@ contract PricerRule is Organized {
             "Price oracle address is null."
         );
 
+        require(
+            _priceOracle.decimals() == requiredPriceOracleDecimals,
+            "Price oracle decimals number is difference from the required one."
+        );
+
         bytes3 payCurrencyCode = _priceOracle.quoteCurrency();
 
         require(

contracts/TokenHolder.sol

@@ -45,21 +45,35 @@ contract TokenHolder {
         address _sessionKey
     );
 
+    /**
+     * @dev Rule's hash is calculated by:
+     *          keccak256(
+     *               abi.encodePacked(
+     *                   to, // rule's address
+     *                   keccak(data), // data is rule's payload
+     *                   nonce, // non-used nonce for the specific session key
+     *                   v, r, s // signature
+     *               )
+     *           )
+     */
     event RuleExecuted(
-        address indexed _to,
-        bytes4 _functionSelector,
-        address _sessionKey,
-        uint256 _nonce,
-        bytes32 _messageHash,
+        bytes32 _ruleHash,
         bool _status
     );
 
+    /**
+     * @dev Redemption's hash is calculated by:
+     *          keccak256(
+     *               abi.encodePacked(
+     *                   to, // cogateway's address
+     *                   keccak(data), // data is cogateway::redeem function's payload
+     *                   nonce, // non-used nonce for the specific session key
+     *                   v, r, s // signature
+     *               )
+     *           )
+     */
     event RedeemExecuted(
-        address indexed _to,
-        bytes4 _functionSelector,
-        address _sessionKey,
-        uint256 _nonce,
-        bytes32 _messageHash,
+        bytes32 _redemptionHash,
         bool _status
     );
 
@@ -349,14 +363,19 @@ contract TokenHolder {
 
         TokenRules(tokenRules).disallowTransfers();
 
-        bytes4 functionSelector = bytesToBytes4(_data);
+        bytes32 ruleHash = keccak256(
+            abi.encodePacked(
+                _to,
+                keccak256(_data),
+                _nonce,
+                _v,
+                _r,
+                _s
+            )
+        );
 
         emit RuleExecuted(
-            _to,
-            functionSelector,
-            sessionKey,
-            _nonce,
-            messageHash,
+            ruleHash,
             executionStatus_
         );
     }
@@ -406,15 +425,21 @@ contract TokenHolder {
         // solium-disable-next-line security/no-call-value
         (executionStatus_, returnData) = _to.call.value(msg.value)(_data);
 
-
         token.approve(_to, 0);
 
+        bytes32 redemptionHash = keccak256(
+            abi.encodePacked(
+                _to,
+                keccak256(_data),
+                _nonce,
+                _v,
+                _r,
+                _s
+            )
+        );
+
         emit RedeemExecuted(
-            _to,
-            functionSelector,
-            sessionKey,
-            _nonce,
-            messageHash,
+            redemptionHash,
             executionStatus_
         );
     }

contracts/test_doubles/unit_tests/pricer_rule/PriceOracleFake.sol

@@ -28,12 +28,15 @@ contract PriceOracleFake is PriceOracleInterface {
 
     uint256 expirationHeight;
 
+    uint8 quoteCurrencyDecimals;
+
 
     /* Special */
 
     constructor(
         bytes3 _baseCurrencyCode,
         bytes3 _quoteCurrencyCode,
+        uint8 _quoteCurrencyDecimals,
         uint256 _initialPrice,
         uint256 _expirationHeight
     )
@@ -53,6 +56,8 @@ contract PriceOracleFake is PriceOracleInterface {
 
         quoteCurrencyCode = _quoteCurrencyCode;
 
+        quoteCurrencyDecimals = _quoteCurrencyDecimals;
+
         setPrice(_initialPrice, _expirationHeight);
     }
 
@@ -85,6 +90,17 @@ contract PriceOracleFake is PriceOracleInterface {
         return quoteCurrencyCode;
     }
 
+    /**
+     * @notice Returns quote currency decimals.
+     */
+    function decimals()
+        external
+        view
+        returns(uint8)
+    {
+        return quoteCurrencyDecimals;
+    }
+
     /**
      * @notice Returns an amount of the quote currency needed to purchase
      *         one unit of the base currency.
@@ -117,8 +133,6 @@ contract PriceOracleFake is PriceOracleInterface {
     )
         public
     {
-        require(_price != 0, "Price is 0.");
-
         require(
             _expirationHeight > block.number,
             "Price expiration height is lte to the current block height."

test/pricer_rule/add_price_oracle.js

@@ -58,11 +58,38 @@ contract('PricerRule::add_price_oracle', async () => {
             );
         });
 
+        it('Reverts as the proposed price oracle decimals number is invalid.', async () => {
+            const {
+                organizationWorker,
+                baseCurrencyCode,
+                requiredPriceOracleDecimals,
+                pricerRule,
+            } = await PricerRuleUtils.createTokenEconomy(accountProvider);
+
+            const priceOracle = await PriceOracleFake.new(
+                web3.utils.stringToHex(baseCurrencyCode),
+                web3.utils.stringToHex('BTC'),
+                requiredPriceOracleDecimals + 1,
+                1, // price oracle initial price
+                (await web3.eth.getBlockNumber()) + 10000, // expiration height
+            );
+
+            await Utils.expectRevert(
+                pricerRule.addPriceOracle(
+                    priceOracle.address,
+                    { from: organizationWorker },
+                ),
+                'Should revert as the proposed price oracle decimals number is invalid.',
+                'Price oracle decimals number is difference from the required one.',
+            );
+        });
+
         it('Reverts as the proposed price oracle base currency code does not '
         + 'match with pricer base currency.', async () => {
             const {
                 organizationWorker,
                 baseCurrencyCode,
+                requiredPriceOracleDecimals,
                 pricerRule,
                 quoteCurrencyCode,
             } = await PricerRuleUtils.createTokenEconomy(accountProvider);
@@ -78,6 +105,7 @@ contract('PricerRule::add_price_oracle', async () => {
             const priceOracle = await PriceOracleFake.new(
                 web3.utils.stringToHex(anotherBaseCurrencyCode),
                 web3.utils.stringToHex(quoteCurrencyCode),
+                requiredPriceOracleDecimals,
                 100, // initial price
                 (await web3.eth.getBlockNumber()) + 10000, // expiration height
             );
@@ -97,6 +125,7 @@ contract('PricerRule::add_price_oracle', async () => {
             const {
                 organizationWorker,
                 baseCurrencyCode,
+                requiredPriceOracleDecimals,
                 pricerRule,
                 priceOracle,
                 quoteCurrencyCode,
@@ -110,6 +139,7 @@ contract('PricerRule::add_price_oracle', async () => {
             const priceOracle2 = await PriceOracleFake.new(
                 web3.utils.stringToHex(baseCurrencyCode),
                 web3.utils.stringToHex(quoteCurrencyCode),
+                requiredPriceOracleDecimals,
                 100, // initial price
                 (await web3.eth.getBlockNumber()) + 10000, // expiration height
             );
@@ -140,7 +170,6 @@ contract('PricerRule::add_price_oracle', async () => {
                 { from: organizationWorker },
             );
 
-
             const events = Event.decodeTransactionResponse(
                 response,
             );

test/pricer_rule/constructor.js

@@ -35,6 +35,7 @@ contract('PricerRule::constructor', async () => {
                     web3.utils.stringToHex('OST'), // base currency code
                     1, // conversion rate
                     0, // conversion rate decimals
+                    0, // price oracles required decimals number
                     accountProvider.get(), // token rules
                 ),
                 'Should revert as the economy token address is null.',
@@ -54,6 +55,7 @@ contract('PricerRule::constructor', async () => {
                     web3.utils.stringToHex(''), // base currency code
                     1, // conversion rate
                     0, // conversion rate decimals
+                    0, // price oracles required decimals number
                     accountProvider.get(), // token rules
                 ),
                 'Should revert as the base currency code is null.',
@@ -73,6 +75,7 @@ contract('PricerRule::constructor', async () => {
                     web3.utils.stringToHex('OST'), // base currency code
                     0, // conversion rate
                     0, // conversion rate decimals
+                    0, // price oracles required decimals number
                     accountProvider.get(), // token rules
                 ),
                 'Should revert as the conversion rate from the base currency to the token is 0.',
@@ -92,6 +95,7 @@ contract('PricerRule::constructor', async () => {
                     web3.utils.stringToHex('OST'), // base currency code
                     1, // conversion rate
                     0, // conversion rate decimals
+                    0, // price oracles required decimals number
                     Utils.NULL_ADDRESS, // token rules
                 ),
                 'Should revert as token rules is null.',
@@ -116,6 +120,7 @@ contract('PricerRule::constructor', async () => {
                 web3.utils.stringToHex('OST'), // base currency code
                 10, // conversion rate
                 1, // conversion rate decimals
+                2, // price oracles required decimals number
                 tokenRules, // token rules
             );
 
@@ -147,6 +152,10 @@ contract('PricerRule::constructor', async () => {
                 (await pricerRule.conversionRateDecimalsFromBaseCurrencyToToken.call()).eqn(1),
             );
 
+            assert.isOk(
+                (await pricerRule.requiredPriceOracleDecimals.call()).eqn(2),
+            );
+
             assert.strictEqual(
                 (await pricerRule.tokenRules.call()),
                 tokenRules,