Skip to content

SIP-184: Remove currentRound auto incrementation from ExchangeRates #1630

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
leckylao wants to merge 8 commits into from
Closed

SIP-184: Remove currentRound auto incrementation from ExchangeRates #1630

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
140ad99
fix(ExchangeRates): Remove currentRound auto incrementation
leckylao Dec 11, 2021
083dba9
skip cache setRate for sUSD
leckylao Dec 11, 2021
32ba730
Allow setRate for 0 so that it could be trigger by circuit breaker
leckylao Dec 11, 2021
6eda259
Fixed comment
leckylao Dec 13, 2021
15b5578
fix comment:
leckylao Dec 13, 2021
11c20f2
Fix test for skip setting sUSD
leckylao Dec 13, 2021
59f52b7
Remove using of currentRoundForRate
leckylao Dec 13, 2021
07807d5
Fix PR comment
leckylao Dec 14, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 19 additions & 14 deletions contracts/ExchangeRates.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
}
Expand Down Expand Up @@ -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 (
Expand All @@ -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);
}
}
Expand Down Expand Up @@ -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;
Copy link
Copy Markdown
Contributor

@artdgn artdgn Dec 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was under the impression that we've agreed that currentRoundForRate should be removed because it introduces more risks, potential for bugs, and too much coupling (between rates and exchanger contract).

Copy link
Copy Markdown
Contributor Author

@leckylao leckylao Dec 13, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the first iteration. And currentRoundForRate is more for improvement due to the tightly coupled code and timeframe.


// skip writing if the rate is the same
if (rate == _rates[currencyKey][currentRoundForRate[currencyKey]].rate) return;
Expand All @@ -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);
Expand Down Expand Up @@ -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));
}
Expand All @@ -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)) {
Expand Down
73 changes: 66 additions & 7 deletions contracts/Exchanger.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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,
Expand All @@ -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);
}

Expand Down
13 changes: 6 additions & 7 deletions test/contracts/ExchangeRates.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 () => {
Expand Down
5 changes: 4 additions & 1 deletion test/contracts/Exchanger.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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', () => {
Expand Down Expand Up @@ -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,
});
Expand Down