SIP-120: TWAP Exchange Function #1127
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,170 @@ | ||
| pragma solidity ^0.5.16; | ||
|
|
||
| // Inheritance | ||
| import "./ExchangeRates.sol"; | ||
| import "./interfaces/IDexPriceAggregator.sol"; | ||
|
|
||
| // https://docs.synthetix.io/contracts/source/contracts/exchangerateswithdexpricing | ||
| contract ExchangeRatesWithDexPricing is ExchangeRates { | ||
| bytes32 public constant CONTRACT_NAME = "ExchangeRatesWithDexPricing"; | ||
|
|
||
| bytes32 internal constant SETTING_DEX_PRICE_AGGREGATOR = "dexPriceAggregator"; | ||
|
|
||
| constructor( | ||
| address _owner, | ||
| address _oracle, | ||
| address _resolver, | ||
| bytes32[] memory _currencyKeys, | ||
| uint[] memory _newRates | ||
| ) public ExchangeRates(_owner, _oracle, _resolver, _currencyKeys, _newRates) {} | ||
|
|
||
| /* ========== SETTERS ========== */ | ||
|
|
||
| function setDexPriceAggregator(IDexPriceAggregator _dexPriceAggregator) external onlyOwner { | ||
| flexibleStorage().setAddressValue( | ||
| ExchangeRates.CONTRACT_NAME, | ||
| SETTING_DEX_PRICE_AGGREGATOR, | ||
| address(_dexPriceAggregator) | ||
| ); | ||
| emit DexPriceAggregatorUpdated(address(_dexPriceAggregator)); | ||
| } | ||
|
|
||
| /* ========== VIEWS ========== */ | ||
|
|
||
| function dexPriceAggregator() public view returns (IDexPriceAggregator) { | ||
| return | ||
| IDexPriceAggregator( | ||
| flexibleStorage().getAddressValue(ExchangeRates.CONTRACT_NAME, SETTING_DEX_PRICE_AGGREGATOR) | ||
| ); | ||
| } | ||
|
|
||
| function atomicTwapWindow() external view returns (uint) { | ||
| return getAtomicTwapWindow(); | ||
| } | ||
|
|
||
| function atomicEquivalentForDexPricing(bytes32 currencyKey) external view returns (address) { | ||
| return getAtomicEquivalentForDexPricing(currencyKey); | ||
| } | ||
|
|
||
| function atomicPriceBuffer(bytes32 currencyKey) external view returns (uint) { | ||
| return getAtomicPriceBuffer(currencyKey); | ||
| } | ||
|
|
||
| function atomicVolatilityConsiderationWindow(bytes32 currencyKey) external view returns (uint) { | ||
| return getAtomicVolatilityConsiderationWindow(currencyKey); | ||
| } | ||
|
|
||
| function atomicVolatilityUpdateThreshold(bytes32 currencyKey) external view returns (uint) { | ||
| return getAtomicVolatilityUpdateThreshold(currencyKey); | ||
| } | ||
|
|
||
| // SIP-120 Atomic exchanges | ||
| // Note that the returned systemValue, systemSourceRate, and systemDestinationRate are based on | ||
| // the current system rate, which may not be the atomic rate derived from value / sourceAmount | ||
| function effectiveAtomicValueAndRates( | ||
| bytes32 sourceCurrencyKey, | ||
| uint sourceAmount, | ||
| bytes32 destinationCurrencyKey | ||
| ) | ||
| external | ||
| view | ||
| returns ( | ||
| uint value, | ||
| uint systemValue, | ||
| uint systemSourceRate, | ||
| uint systemDestinationRate | ||
| ) | ||
| { | ||
| IERC20 sourceEquivalent = IERC20(getAtomicEquivalentForDexPricing(sourceCurrencyKey)); | ||
|
Contributor
There was a problem hiding this comment. np consider it would be kind of slick to split majority this function into 2 internal functions, leaving the function body like: |
||
| require(address(sourceEquivalent) != address(0), "No atomic equivalent for src"); | ||
|
|
||
| IERC20 destEquivalent = IERC20(getAtomicEquivalentForDexPricing(destinationCurrencyKey)); | ||
|
Contributor
There was a problem hiding this comment. equivalent seems like misleading terminology here, maybe better to just say
Contributor
There was a problem hiding this comment. Agree that |
||
| require(address(destEquivalent) != address(0), "No atomic equivalent for dest"); | ||
|
|
||
| (systemValue, systemSourceRate, systemDestinationRate) = _effectiveValueAndRates( | ||
|
Contributor
There was a problem hiding this comment. Can the rate be incorrect if one of the currencies in Perhaps some extra checks / price normalisations are needed for |
||
| sourceCurrencyKey, | ||
| sourceAmount, | ||
| destinationCurrencyKey | ||
| ); | ||
| // Derive P_CLBUF from highest configured buffer between source and destination synth | ||
| uint sourceBuffer = getAtomicPriceBuffer(sourceCurrencyKey); | ||
| uint destBuffer = getAtomicPriceBuffer(destinationCurrencyKey); | ||
| uint priceBuffer = sourceBuffer > destBuffer ? sourceBuffer : destBuffer; // max | ||
| uint pClbufValue = systemValue.multiplyDecimal(SafeDecimalMath.unit().sub(priceBuffer)); | ||
|
|
||
| // refactired due to stack too deep | ||
| uint pDexValue = _dexPriceDestinationValue(sourceEquivalent, destEquivalent, sourceAmount); | ||
|
|
||
| // Final value is minimum output between P_CLBUF and P_TWAP | ||
| value = pClbufValue < pDexValue ? pClbufValue : pDexValue; // min | ||
| } | ||
|
|
||
| function _dexPriceDestinationValue( | ||
| IERC20 sourceEquivalent, | ||
| IERC20 destEquivalent, | ||
| uint sourceAmount | ||
| ) internal view returns (uint) { | ||
| // Normalize decimals in case equivalent asset uses different decimals from internal unit | ||
| uint sourceAmountInEquivalent = | ||
| (sourceAmount.mul(10**uint(sourceEquivalent.decimals()))).div(SafeDecimalMath.unit()); | ||
|
|
||
| uint twapWindow = getAtomicTwapWindow(); | ||
| require(twapWindow != 0, "Uninitialized atomic twap window"); | ||
|
|
||
| uint twapValueInEquivalent = | ||
| dexPriceAggregator().assetToAsset( | ||
|
Contributor
There was a problem hiding this comment. do we need to sanity check the return value here? |
||
| address(sourceEquivalent), | ||
| sourceAmountInEquivalent, | ||
| address(destEquivalent), | ||
| twapWindow | ||
| ); | ||
| require(twapValueInEquivalent > 0, "dex price returned 0"); | ||
|
|
||
| // Similar to source amount, normalize decimals back to internal unit for output amount | ||
| return (twapValueInEquivalent.mul(SafeDecimalMath.unit())).div(10**uint(destEquivalent.decimals())); | ||
| } | ||
|
|
||
| function synthTooVolatileForAtomicExchange(bytes32 currencyKey) external view returns (bool) { | ||
| // sUSD is a special case and is never volatile | ||
| if (currencyKey == "sUSD") return false; | ||
|
|
||
| uint considerationWindow = getAtomicVolatilityConsiderationWindow(currencyKey); | ||
| uint updateThreshold = getAtomicVolatilityUpdateThreshold(currencyKey); | ||
|
|
||
| if (considerationWindow == 0 || updateThreshold == 0) { | ||
| // If either volatility setting is not set, never judge an asset to be volatile | ||
| return false; | ||
| } | ||
|
|
||
| // Go back through the historical oracle update rounds to see if there have been more | ||
| // updates in the consideration window than the allowed threshold. | ||
| // If there have, consider the asset volatile--by assumption that many close-by oracle | ||
| // updates is a good proxy for price volatility. | ||
| uint considerationWindowStart = block.timestamp.sub(considerationWindow); | ||
|
Contributor
There was a problem hiding this comment. Is this heuristic too coupled to current CL implementation details? Would be easily broken improvements or differences between L1 & L2, requiring extra configuration overhead. It might be better to have a volatility heuristic that doesn't rely on an implementation quirk, but on the price values themselves. Volatility itself can be aggregated quite cheaply by aggregating the average price and the deviation from average price (https://www.investopedia.com/terms/v/volatility.asp) as EMA with L1 metrics (to avoid square and sqrt). This would also require just one call per round / per tx on average, and not Something like: // system settings
uint decayParam; // moving average decay
uint volatilityTreshold; // threshold for checking volatility
...
// persistent state variables
mapping(bytes32 => uint) lastRoundsSeen;
mapping(bytes32 => uint) priceEMAs; // mean aggregator
mapping(bytes32 => uint) deviationEMAs; // herustic volatility aggregator
...
// in the main method, check the volatility value vs. some threshold after updateing the aggregators
return _updatedVolatilityEMA(currencyKey) > volatilityTreshold;
...
function _updatedVolatilityEMA(currencyKey) internal returns (uint) {
uint roundId = _getCurrentRoundId(currencyKey);
uint lastRoundSeen = lastRoundsSeen[currencyKey];
if (roundId > lastRoundSeen) { // if round seen we can just use the up to date value
uint priceEMA = priceEMAs[currencyKey];
uint deviationEMA = deviationEMAs[currencyKey];
// for each round that was not see yet
for (lastRoundSeen; lastRoundSeen <= roundId; lastRoundSeen++) {
// the simple EMAs below don't use time, but can use it for more robustness
(uint price, uint time) = _getRateAndTimestampAtRound(currencyKey, roundId);
// update mean: priceEMA = priceEMA * decayParam + (1 - decayParam) * price
priceEMA = priceEMA.multiplyDecimal(decayParam).add(SafeDecimal.unit().sub(decayParam).multiplyDecimal(price));
// current deviation: curDeviation = abs(price - priceEMA)
// we may use abs (L1) difference to avoid squaring and squarerooting because
// we just need a value that can be used as threshold for heuristic and
// we don't need precise definition of volatility
uint curDeviation = price > priceEMA ? price - priceEMA : priceEMA - price;
deviationEMA = deviationEMA.multiplyDecimal(decayParam).add(SafeDecimal.unit().sub(decayParam).multiplyDecimal(curDeviation));
}
lastRoundsSeen[currencyKey] = lastRoundSeen;
priceEMAs[currencyKey] = priceEMA;
deviationEMAs[currencyKey] = deviationEMA;
}
return deviationEMAs[currencyKey];
}
|
||
| uint roundId = _getCurrentRoundId(currencyKey); | ||
| for (updateThreshold; updateThreshold > 0; updateThreshold--) { | ||
| (uint rate, uint time) = _getRateAndTimestampAtRound(currencyKey, roundId); | ||
|
Contributor
There was a problem hiding this comment. this loop looks expensive in terms of external calls to oracle, are we sure that this volatility check is the best we can do here? perhaps some options that will only perform one external call:
|
||
| if (time != 0 && time < considerationWindowStart) { | ||
| // Round was outside consideration window so we can stop querying further rounds | ||
| return false; | ||
| } else if (rate == 0 || time == 0) { | ||
| // Either entire round or a rate inside consideration window was not available | ||
| // Consider the asset volatile | ||
| break; | ||
| } | ||
|
|
||
| if (roundId == 0) { | ||
| // Not enough historical data to continue further | ||
| // Consider the asset volatile | ||
| break; | ||
| } | ||
| roundId--; | ||
| } | ||
|
|
||
| return true; | ||
| } | ||
|
|
||
| /* ========== EVENTS ========== */ | ||
|
|
||
| event DexPriceAggregatorUpdated(address newDexPriceAggregator); | ||
| } | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: perhaps worth adding detailed (natspec?) comments at least to all public / external methods. e.g. something
atomicEquivalentForDexPricing/atomicPriceBufferwithout a comment assumes detailed understanding of a lot of context that not every future reader may reasonably be expected to have.edit: I see there are some comments for those in
SystemSettings. I think it's worth to either duplicate them here as well, or at least add comments that mention where the actual detailed comments are. Also it's worth to expand the comments a bit more wherever they are.