diff --git a/contracts/ExchangeRates.sol b/contracts/ExchangeRates.sol index ac0902db53..cdcbc89432 100644 --- a/contracts/ExchangeRates.sol +++ b/contracts/ExchangeRates.sol @@ -22,6 +22,7 @@ contract ExchangeRates is Owned, MixinSystemSettings, IExchangeRates { using SafeDecimalMath for uint; bytes32 public constant CONTRACT_NAME = "ExchangeRates"; + bytes32 public constant SUSD = "sUSD"; // Exchange rates and update times stored by currency code, e.g. 'SNX', or 'sUSD' mapping(bytes32 => mapping(uint => RateAndUpdatedTime)) private _rates; @@ -59,8 +60,12 @@ contract ExchangeRates is Owned, MixinSystemSettings, IExchangeRates { oracle = _oracle; - // The sUSD rate is always 1 and is never stale. - _setRate("sUSD", 0, SafeDecimalMath.unit(), now); + // The sUSD rate is always 1 on round 1 and is never stale. + currentRoundForRate[SUSD] = 1; + _rates[SUSD][currentRoundForRate[SUSD]] = RateAndUpdatedTime({ + rate: uint216(SafeDecimalMath.unit()), + time: uint40(now) + }); internalUpdateRates(_currencyKeys, _newRates, now); } @@ -341,7 +346,7 @@ contract ExchangeRates is Owned, MixinSystemSettings, IExchangeRates { function rateAndInvalid(bytes32 currencyKey) external view returns (uint rate, bool isInvalid) { RateAndUpdatedTime memory rateAndTime = _getRateAndUpdatedTime(currencyKey); - if (currencyKey == "sUSD") { + if (currencyKey == SUSD) { return (rateAndTime.rate, false); } return ( @@ -367,7 +372,7 @@ contract ExchangeRates is Owned, MixinSystemSettings, IExchangeRates { // do one lookup of the rate & time to minimize gas RateAndUpdatedTime memory rateEntry = _getRateAndUpdatedTime(currencyKeys[i]); rates[i] = rateEntry.rate; - if (!anyRateInvalid && currencyKeys[i] != "sUSD") { + if (!anyRateInvalid && currencyKeys[i] != SUSD) { anyRateInvalid = flagList[i] || _rateIsStaleWithTime(_rateStalePeriod, rateEntry.time); } } @@ -436,12 +441,12 @@ contract ExchangeRates is Owned, MixinSystemSettings, IExchangeRates { uint256 rate, uint256 time ) internal { - if (roundId > 0) { - currentRoundForRate[currencyKey] = roundId; - } else { - // Note: this will effectively start the rounds at 1, which matches Chainlink's Agggregators - currentRoundForRate[currencyKey]++; - } + require(roundId > 0, "Round id must be greater than 0."); + + // No caching for sUSD + if (currencyKey == SUSD) return; + + currentRoundForRate[currencyKey] = roundId; // skip writing if the rate is the same if (rate == _rates[currencyKey][currentRoundForRate[currencyKey]].rate) return; @@ -468,14 +473,14 @@ contract ExchangeRates is Owned, MixinSystemSettings, IExchangeRates { // truely worthless and still valid. In this scenario, we should // delete the rate and remove it from the system. require(newRates[i] != 0, "Zero is not a valid rate, please call deleteRate instead."); - require(currencyKey != "sUSD", "Rate of sUSD cannot be updated, it's always UNIT."); // We should only update the rate if it's at least the same age as the last rate we've got. if (timeSent < _getUpdatedTime(currencyKey)) { continue; } - _setRate(currencyKey, 0, newRates[i], timeSent); + currentRoundForRate[currencyKey]++; + _setRate(currencyKey, currentRoundForRate[currencyKey], newRates[i], timeSent); } emit RatesUpdated(currencyKeys, newRates); @@ -605,7 +610,7 @@ contract ExchangeRates is Owned, MixinSystemSettings, IExchangeRates { function _rateIsStale(bytes32 currencyKey, uint _rateStalePeriod) internal view returns (bool) { // sUSD is a special case and is never stale (check before an SLOAD of getRateAndUpdatedTime) - if (currencyKey == "sUSD") return false; + if (currencyKey == SUSD) return false; return _rateIsStaleWithTime(_rateStalePeriod, _getUpdatedTime(currencyKey)); } @@ -616,7 +621,7 @@ contract ExchangeRates is Owned, MixinSystemSettings, IExchangeRates { function _rateIsFlagged(bytes32 currencyKey, FlagsInterface flags) internal view returns (bool) { // sUSD is a special case and is never invalid - if (currencyKey == "sUSD") return false; + if (currencyKey == SUSD) return false; address aggregator = address(aggregators[currencyKey]); // when no aggregator or when the flags haven't been setup if (aggregator == address(0) || flags == FlagsInterface(0)) { diff --git a/contracts/Exchanger.sol b/contracts/Exchanger.sol index 6f308e219f..0471146e08 100644 --- a/contracts/Exchanger.sol +++ b/contracts/Exchanger.sol @@ -470,7 +470,12 @@ contract Exchanger is Owned, MixinSystemSettings, IExchanger { return (0, 0, IVirtualSynth(0)); } - entry.exchangeFeeRate = _feeRateForExchange(sourceCurrencyKey, destinationCurrencyKey); + entry.exchangeFeeRate = _feeRateForExchangeAtRound( + sourceCurrencyKey, + destinationCurrencyKey, + entry.roundIdForSrc, + entry.roundIdForDest + ); amountReceived = _deductFeesFromAmount(entry.destinationAmount, entry.exchangeFeeRate); // Note: `fee` is denominated in the destinationCurrencyKey. @@ -771,6 +776,31 @@ contract Exchanger is Owned, MixinSystemSettings, IExchanger { return _calculateFeeRateFromExchangeSynths(baseRate, sourceCurrencyKey, destinationCurrencyKey); } + /// @notice Calculate the exchange fee for a given source and destination currency key + /// @param sourceCurrencyKey The source currency key + /// @param destinationCurrencyKey The destination currency key + /// @param roundIdForSrc The round id of the source currency. + /// @param roundIdForDest The round id of the target currency. + /// @return The exchange fee rate + /// @return The exchange dynamic fee rate + function _feeRateForExchangeAtRound( + bytes32 sourceCurrencyKey, + bytes32 destinationCurrencyKey, + uint roundIdForSrc, + uint roundIdForDest + ) internal view returns (uint exchangeFeeRate) { + // Get the exchange fee rate as per destination currencyKey + uint baseRate = getExchangeFeeRate(destinationCurrencyKey); + return + _calculateFeeRateFromExchangeSynthsAtRound( + baseRate, + sourceCurrencyKey, + destinationCurrencyKey, + roundIdForSrc, + roundIdForDest + ); + } + function _calculateFeeRateFromExchangeSynths( uint exchangeFeeRate, bytes32 sourceCurrencyKey, @@ -786,19 +816,48 @@ contract Exchanger is Owned, MixinSystemSettings, IExchanger { return exchangeFeeRate; } + function _calculateFeeRateFromExchangeSynthsAtRound( + uint exchangeFeeRate, + bytes32 sourceCurrencyKey, + bytes32 destinationCurrencyKey, + uint roundIdForSrc, + uint roundIdForDest + ) internal view returns (uint) { + uint exchangeDynamicFeeRate = _getDynamicFeeForExchangeAtRound(destinationCurrencyKey, roundIdForDest); + exchangeDynamicFeeRate = exchangeDynamicFeeRate.add( + _getDynamicFeeForExchangeAtRound(sourceCurrencyKey, roundIdForSrc) + ); + uint maxDynamicFee = getExchangeMaxDynamicFee(); + + exchangeFeeRate = exchangeFeeRate.add(exchangeDynamicFeeRate); + // Cap to max exchange dynamic fee + exchangeFeeRate = exchangeFeeRate > maxDynamicFee ? maxDynamicFee : exchangeFeeRate; + return exchangeFeeRate; + } + /// @notice Get dynamic fee for a given currency key (SIP-184) /// @param currencyKey The given currency key /// @return The dyanmic fee function _getDynamicFeeForExchange(bytes32 currencyKey) internal view returns (uint dynamicFee) { // No dynamic fee for sUSD - if (currencyKey == sUSD) { - return 0; - } + if (currencyKey == sUSD) return 0; + (uint threshold, uint weightDecay, uint rounds) = getExchangeDynamicFeeData(); + uint[] memory prices; + uint roundId = exchangeRates().getCurrentRoundId(currencyKey); + (prices, ) = exchangeRates().ratesAndUpdatedTimeForCurrencyLastNRounds(currencyKey, rounds, roundId); + dynamicFee = DynamicFee.getDynamicFee(prices, threshold, weightDecay); + } + + /// @notice Get dynamic fee for a given currency key (SIP-184) + /// @param currencyKey The given currency key + /// @param roundId The round id + /// @return The dyanmic fee + function _getDynamicFeeForExchangeAtRound(bytes32 currencyKey, uint roundId) internal view returns (uint dynamicFee) { + // No dynamic fee for sUSD + if (currencyKey == sUSD) return 0; (uint threshold, uint weightDecay, uint rounds) = getExchangeDynamicFeeData(); uint[] memory prices; - // Note: We are using cache round ID here for cheap read - uint currentRoundId = exchangeRates().currentRoundForRate(currencyKey); - (prices, ) = exchangeRates().ratesAndUpdatedTimeForCurrencyLastNRounds(currencyKey, rounds, currentRoundId); + (prices, ) = exchangeRates().ratesAndUpdatedTimeForCurrencyLastNRounds(currencyKey, rounds, roundId); dynamicFee = DynamicFee.getDynamicFee(prices, threshold, weightDecay); } diff --git a/test/contracts/ExchangeRates.js b/test/contracts/ExchangeRates.js index 87ca3afd35..4128d66369 100644 --- a/test/contracts/ExchangeRates.js +++ b/test/contracts/ExchangeRates.js @@ -312,15 +312,14 @@ contract('Exchange Rates', async accounts => { assert.equal(lastUpdatedTimeLGHI.toNumber(), updatedTime); }); - it('should revert when trying to set sUSD price', async () => { + it('should skip when trying to set sUSD price', async () => { await fastForward(1); - await assert.revert( - instance.updateRates([sUSD], [web3.utils.toWei('1.0', 'ether')], timeSent, { - from: oracle, - }), - "Rate of sUSD cannot be updated, it's always UNIT" - ); + instance.updateRates([sUSD], [toUnit('2')], timeSent, { + from: oracle, + }); + + assert.bnEqual(await instance.rateForCurrency(toBytes32('sUSD')), toUnit('1')); }); it('should emit RatesUpdated event when rate updated', async () => { diff --git a/test/contracts/Exchanger.spec.js b/test/contracts/Exchanger.spec.js index 6a77044cfd..2abe0df9fb 100644 --- a/test/contracts/Exchanger.spec.js +++ b/test/contracts/Exchanger.spec.js @@ -724,7 +724,7 @@ contract('Exchanger (spec tests)', async accounts => { from: oracle, } ); - // Disable Dynamic Fee by setting rounds to 0 + // Disable Dynamic Fee to neglect it await systemSettings.setExchangeDynamicFeeRounds('0', { from: owner }); }); describe('and the exchange fee rate is 1% for easier human consumption', () => { @@ -2749,6 +2749,9 @@ contract('Exchanger (spec tests)', async accounts => { let exchangeFeeRate; beforeEach(async () => { + // Disable Dynamic Fee to neglect it + await systemSettings.setExchangeDynamicFeeRounds('0', { from: owner }); + await systemSettings.setAtomicExchangeFeeRate(sETH, feeRateOverride, { from: owner, });