Feat(DynamicFee): SIP-184 dynamic fees#1563
Conversation
- created libraries folder - moving SafeDecimalMath into libraries
- adding decimals into SafeDecimalMath
Codecov Report
@@ Coverage Diff @@
## develop #1563 +/- ##
===========================================
+ Coverage 95.66% 95.68% +0.01%
===========================================
Files 82 83 +1
Lines 1916 1946 +30
Branches 598 610 +12
===========================================
+ Hits 1833 1862 +29
- Misses 83 84 +1
Continue to review full report at Codecov.
|
- Adding dynamic fee constants;
- Adding require check for price; - Adding setup for test;
- Adding more rounds of price feeds for test; - Adding dynamic fee in systemSettings in test
contracts/DynamicFee.sol
Outdated
| ) public pure returns (uint) { | ||
| require(price > 0, "Price cannot be 0"); | ||
| require(previousPrice > 0, "Previous price cannot be 0"); | ||
| int(price.divideDecimal(previousPrice)) - 1; |
There was a problem hiding this comment.
does this line do anything?
contracts/DynamicFee.sol
Outdated
| /// @notice Calculate price differential | ||
| /// @param price Current round price | ||
| /// @param previousPrice Previous round price | ||
| /// @param threshold Threshold constant |
There was a problem hiding this comment.
I think a bit more explanations might be nice here:
- maybe a quick explanation what "price differential" means here
- threshold is expected as ratio extrpressed as decimal with 1e18 units?
- differential is returned also as 1e18 decimal right?
- non-zero differential is only returned if above threshold value is a fact worth noting
contracts/DynamicFee.sol
Outdated
| require(size >= 2, "Not enough prices"); | ||
| for (uint i = 0; i < size - 1; i++) { | ||
| uint priceDifferential = getPriceDifferential(prices[i], prices[i + 1], threshold); | ||
| uint priceWeight = getPriceWeight(i, weightDecay); |
There was a problem hiding this comment.
It seems that this might be inefficient to calculate the exponent every time for the weights. If there are 10 prices this will have to calculate 0.9^1, 0.9^2, .., 0.9^10 every time.
Why not instead just calculate the formula as in the SIP:
uint dynamicFee = 0;
for (uint i = 0; i < size - 1; i++) {
uint priceDifferential = getPriceDifferential(prices[i], prices[i + 1], threshold);
dynamicFee = ( dynamicFee.multiplyDecimal(weightDecay) ).add(priceDifferential);
}There was a problem hiding this comment.
Yeah I think the loop needs to be reversed, because the prices are in reversed order. So maybe:
uint dynamicFee = 0;
for (uint i = prices.length - 1; i > 0; i--) {
uint priceDifferential = getPriceDifferential(prices[i - 1], prices[i], threshold);
dynamicFee = ( dynamicFee.multiplyDecimal(weightDecay) ).add(priceDifferential);
}There was a problem hiding this comment.
I've tried with your code and the test passed! I see there's two differences: One being prices order from current to go back 10 rounds and roundDecay on the priceDifferential. The other being prices go from previous 10 rounds to current. And roundDecay on the previous dynamicFee.
So without the lastDynamicFee and lastRoundSeen cacheing. Both ways complexity should be the same that both need to calculate the decay 10 times right?
I do see the benefit of using roundDecay on lastDynamicFee so that we could just calculate the missing rounds. Nice found! So next question would be where to put the cache storage? As DynamicFee is a library so looks like best place to store lastDynamicFee and lastRoundSeen would be on Exchanger the place that it get call. As it's an internal caching value, so losing it across contract upgrade should be fine.
Will implement it as improvement after I have fixing all the broken tests.
There was a problem hiding this comment.
Both ways complexity should be the same that both need to calculate the decay 10 times right?
Not exactly. The previous way there is added complexity due to .pow() being calculated in every iteration inside the getPriceWeight(). That calculation runs it's own loop (doing multiplications). So that adds some complexity which can be removed. It's logarithmic, but still no need to run those extra internal loops.
As DynamicFee is a library so looks like best place to store lastDynamicFee and lastRoundSeen would be on Exchanger the place that it get call.
I think something along these lines, and probably the library interface would change a little. It would be something like updateDynamicFee(uint lastDynamicFee, uint lastRoundSeen, address exchangeRates) returns (uint dynamicFee, uint lastRound) or something. And inside there it would query ExchangeRates for the last round, and get the price data or something.
There was a problem hiding this comment.
Both ways complexity should be the same that both need to calculate the decay 10 times right?
Not exactly. The previous way there is added complexity due to
.pow()being calculated in every iteration inside thegetPriceWeight(). That calculation runs it's own loop (doing multiplications). So that adds some complexity which can be removed. It's logarithmic, but still no need to run those extra internal loops.
Can run through the roundDecay with Afif on the Monday brief session.
As DynamicFee is a library so looks like best place to store lastDynamicFee and lastRoundSeen would be on Exchanger the place that it get call.
I think something along these lines, and probably the library interface would change a little. It would be something like
updateDynamicFee(uint lastDynamicFee, uint lastRoundSeen, address exchangeRates) returns (uint dynamicFee, uint lastRound)or something. And inside there it would queryExchangeRatesfor the last round, and get the price data or something.
Then the library would have dependancy on the ExchangeRates and need to get it from AddressResolver which is another dependancy. The idea of using it as library is less dependancy and can be import anywhere else to use it.
There was a problem hiding this comment.
Then the library would have dependancy on the ExchangeRates and need to get it from AddressResolver which is another dependancy.
That's not what I meant. You pass in the address in updateDynamicFee(uint lastDynamicFee, uint lastRoundSeen, address exchangeRates) and then there is no dependency on resolver, just need to import the interface.
contracts/DynamicFee.sol
Outdated
| /// @param weightDecay A weight decay constant | ||
| /// @return uint dynamic fee | ||
| function getDynamicFee( | ||
| uint[] memory prices, |
There was a problem hiding this comment.
It seems that using a fixed price array (of size 10) is not what is described in the SIP. This means that in sustained volatility, the dynamic fee will be calculated as if 10 prices ago there was no volatility (dynamic fee was 0).
Also it will be highly inefficient to get the last 10 prices from oracle every time, because it's one external call per price. So it adds 10 external calls per transaction.
I think a more efficient implementation might be to keep track of the latest dynamic fee calculated and round index, and apply the SIP's formula to it with new prices only. Hopefully it will be either 0 or 1 price updates per tx (instead of 10).
This should be at many times more gas efficient, and match what the SIP describes more closely (for history that is longer than 10 prices).
There was a problem hiding this comment.
I think dynamic fee is independent and reset in each Epoch. Can't calculate from the previous dynamic fee as it could be 0 as shown in the spreadsheet. Would only depends on the recent history prices.
Maybe the spreadsheet from Afif could help clear this.
https://cdn.discordapp.com/attachments/892648844817473586/898374691402711090/dynamic-fee-calc.xlsx
There was a problem hiding this comment.
Given that we need the last 10 prices from the oracle, could we not store the last 10 rounds in our own ExchangeRates contract for example (as an improvement over making 10x external calls to the oracle).
Then only fetch the latest prices if they are not stored in the ExchangeRates and persist them to storage as well? This might be more expensive gas wise given we are writing to storage.
Also has CL looked into improving their interface to allow retrieving a list of roundID / prices with one call?
There was a problem hiding this comment.
Given that we need the last 10 prices from the oracle, could we not store the last 10 rounds in our own ExchangeRates contract for example (as an improvement over making 10x external calls to the oracle).
This was somewhat discussed and suggested on a past revision #1563 (comment) . I think a cheaper way (in gas and code changes) would be to update and store the dynamicFee with each round's price only once so will not need to cache (because if the round was already processed it will not request its info again).
(The problem with caching on exchanger - which is an idea @leckylao has suggested before - is that it will require additional code, and state on exchanger (which will be more expensive), that will also require substantial code and testing changes there (for the new caching). And these are not really needed, because no other exchanger caller uses past rounds data except this one, and this code can just update dynamicFee with any new information, and only needs to see each round's data once)
contracts/Exchanger.sol
Outdated
| uint weightDecay = getExchangeDynamicFeeWeightDecay(); | ||
| uint rounds = getExchangeDynamicFeeRounds(); | ||
| uint[] memory prices; | ||
| (prices, ) = exchangeRates().ratesAndUpdatedTimeForCurrencyLastNRounds(currencyKey, rounds); |
There was a problem hiding this comment.
It looks like this makes number of rounds of external calls to oracle.
I've described a proposal for a more efficient implementation in a comment above (in the library itself). Basically it's storing lastDynamicFee and lastRoundSeen, and only recalculating the fee for the rounds that happened since lastRoundSeen. If lastRoundSeen is the latest round - nothing to update. This should make only one external call in most cases - price and latest round.
There was a problem hiding this comment.
Is that means if lastRoundSeen is more than 10 rounds to the current round then it would be the same? But if lastRoundSeen to the current round is 5 rounds then could just get recent 5 rounds of feeds, saving another 5 rounds?
There was a problem hiding this comment.
Yeah if currentRound - lastRoundSeen:
0- use current storeddynamicFee. Someone traded after last round already.0< <=10- updatedynamicFee. No activity for a long time.>10- updatedynamicFeefor last 10 rounds. No activity for a long long time - is this thing even on?
If the platform is being used actively, we should mostly be in the first case.
There was a problem hiding this comment.
I like the cacheing idea, and I see we sort of caching the _rates
https://github.com/Synthetixio/synthetix/blob/develop/contracts/ExchangeRates.sol#L610. How about creating a separate PR to caching the feed round data probably. That way the dynamicFee logic doesn't need to change and the whole system can also benefits from it?
- removed fastForward as startTime and Now is not equal
test/contracts/setup.js
Outdated
| await artifact.link(safeDecimalMath); | ||
| // eslint-disable-next-line no-constant-condition | ||
| if (contract === 'Exchanger' || 'ExchangerWithVirtualSynth') { | ||
| if (contract === ('Exchanger' || 'ExchangerWithVirtualSynth')) { |
There was a problem hiding this comment.
I think 'Exchanger' || 'ExchangerWithVirtualSynth' will just evaluate to 'Exchanger' and 'ExchangerWithVirtualSynth' is not compared to.
There was a problem hiding this comment.
Right, forgot how stupid JS is .......
- Rename priceWeight to RoundDecay - adding cap 100% for max exchangeFeeRate - Added return for invalid rate - linking libraries in setup
| /// @param weightDecay A weight decay constant | ||
| /// @return uint dynamic fee | ||
| function getDynamicFee( | ||
| uint[] calldata prices, |
There was a problem hiding this comment.
Optimization: Instead of calldata here, should we use memory? Because this is called by delegatecall, it may be better to pass the memory address.
There was a problem hiding this comment.
Isn't calldata is more optimal than using memory so that it's non-modifiable and the cheapest place?
This is the cheapest location to use. In particular, that means that functions may be limited in their number of arguments.1 Notable implementation details about calldata are as follows:
It can only be used for function declaration parameters (and not function logic)
It is immutable (it can't be overwritten and changed)
It must be used for dynamic parameters of an external function
It is non-persistent (the value does not persist after the transaction has completed)
contracts/ExchangeRates.sol
Outdated
| uint private constant ORACLE_FUTURE_LIMIT = 10 minutes; | ||
|
|
||
| /// @notice ID for the current round | ||
| /// @param currencyKey is the currency key of the synth to be exchanged |
There was a problem hiding this comment.
[np] comments out of scope, thought this was a new variable
There was a problem hiding this comment.
It's to indicate that the bytes32 being used here is the currencyKey.
contracts/ExchangeRates.sol
Outdated
| } | ||
|
|
||
| function _getRateAndTimestampAtRound(bytes32 currencyKey, uint roundId) internal view returns (uint rate, uint time) { | ||
| function _getRateAndUpdatedTimeAtRound(bytes32 currencyKey, uint roundId) internal view returns (uint rate, uint time) { |
There was a problem hiding this comment.
[np] Renaming this doesn't significantly increase the understanding of what the function does
There was a problem hiding this comment.
The propose here is to make it consistent with the function _getRateAndUpdatedTime as the code are pretty much the same and with the AtRound to indicate accept roundId.
| uint considerationWindowStart = block.timestamp.sub(considerationWindow); | ||
| uint roundId = _getCurrentRoundId(currencyKey); | ||
| for (updateThreshold; updateThreshold > 0; updateThreshold--) { | ||
| (uint rate, uint time) = _getRateAndTimestampAtRound(currencyKey, roundId); |
There was a problem hiding this comment.
[np] There is no material benefit to update this file for a nomenclature change
There was a problem hiding this comment.
Right, good point. Will rollback so don't need to deploy this contract
| }); | ||
|
|
||
| it('exchanging between synths updates sUSD debt total due to fees', async () => { | ||
| // Disable Dynamic fee so that we can neglect it. |
There was a problem hiding this comment.
Can we also do this for the other tests? What if we disable dynamic fee by default and then only enable it for the tests that need it.
There was a problem hiding this comment.
Through this way so that we know what impact it could brings in and only disable if it's not relevant and in intention.
| new web3.utils.BN(1), | ||
| new web3.utils.BN(2), | ||
| ], | ||
| bnCloseVariance, |
There was a problem hiding this comment.
Is there a specific reason why this needed to be added here?
There was a problem hiding this comment.
Good point, should disable dynamic fee instead to test for ExchangeEntryAppended match.
There was a problem hiding this comment.
Oh, actually it's not caused by dynamic fee. It's actually caused by introducing of the cache rate so the actual exchange rate might not be the same from the view function getAmountsForExchange. As it's not enquiring the same round data. exchanger.getAmountsForExchange is getting the current round rate but ExchangeEntry is from the historical rate.
| }); | ||
|
|
||
| it('should disallow closing the current fee period too early', async () => { | ||
| const feePeriodDuration = await feePool.feePeriodDuration(); |
There was a problem hiding this comment.
Why were these lines removed?
There was a problem hiding this comment.
As dynamic fee involves more calculations and turns out it cannot fast forward anymore. But the test still valid after removal.
test/contracts/TestableDynamicFee.js
Outdated
| const DynamicFee = artifacts.require('DynamicFee'); | ||
| const TestableDynamicFee = artifacts.require('TestableDynamicFee'); | ||
|
|
||
| contract('TestableDynamicFee', () => { |
There was a problem hiding this comment.
[np] This file could be renamed DynamicFee since it's testing DynamicFee and not TestableDynamicFee.
test/contracts/TestableDynamicFee.js
Outdated
| assert.bnEqual(priceWeight2, toUnit('0.81')); | ||
| }); | ||
|
|
||
| it('Can get dynamic fee from dynamic-fee-calc.csv round 13-22, all below threshold', async () => { |
There was a problem hiding this comment.
Saying this is from dynamic-fee-calc.csv won't make sense since this file is not included in the repo.
Either check that csv file into the repo, or don't mention it.
There was a problem hiding this comment.
Made sense, changing it to "according to SIP feasibility spreadsheet"
- remove unnecesary comments - remove unnecesary changes
There was a problem hiding this comment.
- Still have some concerns about setRate caching w.r.t to roundId == 0 case.
- Would prefer to see the reference spreadsheet comitted as without it the tests for dynamic fee aren't maintainable.
- Some other small issues.
- Can you provide updated gas measurements please?
contracts/ExchangeRates.sol
Outdated
| } | ||
|
|
||
| // skip writing if the rate is the same | ||
| if (rate == _rates[currencyKey][currentRoundForRate[currencyKey]].rate) return; |
There was a problem hiding this comment.
nit: inline returns don't seem to be the usual style in the solidity in this codebase, and can be confusing to keep track when reasoning about where the method returns
There was a problem hiding this comment.
Well it works and that's why I leave a comment to explain there.
There was a problem hiding this comment.
It works but it's easy to miss so hurts readability IMO. Still a nit though.
contracts/ExchangeRates.sol
Outdated
There was a problem hiding this comment.
what happens when user passes 0 as round id in mutativeEffectiveValueAndRatesAtRound, what will oracle return and what will be cached in _setRate()? What will be the effect of that causing an increment in currentRoundForRate[currencyKey] because of the roundId == 0 flow?
contracts/ExchangeRates.sol
Outdated
There was a problem hiding this comment.
Still not sure if this flow needs to be in this contract if it's only used for testing. Shouldn't it be extended in a Testable... contract instead adding complexity and surface area in this contract?
I'm reading this again and again and it still doesn't sit right with me.
It seems that maybe anyone can increment currentRoundForRate[currencyKey] at will as much as they want by passing a 0 as round id in the mutativeEffectiveValueAndRatesAtRound. Can that cause some cache poisoning or some accounting bugs?
|
|
||
| // if no last exchange for this synth, then we need to look up last 3 rates (+1 for current rate) | ||
| (uint[] memory rates, ) = exchangeRates().ratesAndUpdatedTimeForCurrencyLastNRounds(currencyKey, 4); | ||
| (uint[] memory rates, ) = exchangeRates().ratesAndUpdatedTimeForCurrencyLastNRounds(currencyKey, 4, 0); |
There was a problem hiding this comment.
nit: mysterious 4 magic number should probably be a named constant (I know it's not new code, but still might be nice to fix since the line is touched).
There was a problem hiding this comment.
From the comment there I guess 4 is formed by the last 3 rates +1. Not sure how to fix.
There was a problem hiding this comment.
just a named constant in the contract that is set to 4, or set to 3 if used in some other place, and +1 here.
contracts/Exchanger.sol
Outdated
| (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); |
There was a problem hiding this comment.
can this be manipulated externally through mutativeEffectiveValueAndRatesAtRound and the ability to pass 0 roundId to increment currentRoundForRate?
There was a problem hiding this comment.
Yes as I commented above. But it's not total under control as any exchange would reset it, and no incentive as can't control the rate.
let me know if there's better way.
contracts/SystemSettings.sol
Outdated
| } | ||
| } | ||
|
|
||
| /// @notice Set exchange dynamic fee threshold constant default 40bps |
There was a problem hiding this comment.
nit: can probably mention that value is not in bips but is decimal ratio
| /// @param rounds The exchange dynamic fee last N rounds | ||
| /// @return uint dynamic fee last N rounds | ||
| function setExchangeDynamicFeeRounds(uint rounds) external onlyOwner { | ||
| flexibleStorage().setUIntValue(SETTING_CONTRACT_NAME, SETTING_EXCHANGE_DYNAMIC_FEE_ROUNDS, rounds); |
There was a problem hiding this comment.
Yes, minimal start from 2 rounds so can set it to either 0 or 1 to disable it.
There was a problem hiding this comment.
👍 maybe worth to add that into the comment
| /// @param maxFee The max exchange dynamic fee | ||
| /// @return uint dynamic fee last N rounds | ||
| function setExchangeMaxDynamicFee(uint maxFee) external onlyOwner { | ||
| flexibleStorage().setUIntValue(SETTING_CONTRACT_NAME, SETTING_EXCHANGE_MAX_DYNAMIC_FEE, maxFee); |
There was a problem hiding this comment.
Is there a use case for setting this to 0? aren't you disabling dynamic fee by setting rounds to 0 or 1?
There was a problem hiding this comment.
This is to cap max dynamic fee to 1 (100%), otherwise it could go negative/overflow.
test/contracts/CollateralShort.js
Outdated
| let timestamp; | ||
| for (let i = 0; i < EXCHANGE_DYNAMIC_FEE_ROUNDS; i++) { | ||
| timestamp = await currentTime(); | ||
| await exchangeRates.updateRates([sETH], ['100'].map(toUnit), timestamp, { |
There was a problem hiding this comment.
Is there a test for fewer than 10 rounds available?
| assert.bnEqual(priceWeight2, toUnit('0.81')); | ||
| }); | ||
|
|
||
| it('Can get dynamic fee according to SIP feasibility spreadsheet round 13-22, all below threshold', async () => { |
There was a problem hiding this comment.
can you please commit the spreadsheet as a csv with the formulas as text so that future maintainers can reference that if needed to update / check the tests?
- Adding dynamic-fee-calc.csv - Refactor getExchangeDynamicFeeData to be one flexibleStorage call - Remove default value in comment - Adding require non zero value check
- Adding default values for getUIntValues in flexibleStorage
* Merge branch 'develop' into feat/sip-184-dynamic-fees * fix ExchangeRates test - apply AtRound changes - Adding cacheRates on ExchangeRates * fixing ExchangerWithFeeRecAlternatives.unit.js - Adding anyRateIsInvalidAtRound mock * Fix RewardsIntegrationTests.js - Add updateRatesWithDefaults to invoid invalid rate * Fix Exchanger.spec.js - move setPriceAggregator out of the updateRates loop * Fix Exchanger.spec.js - Fix set price with no volatility on exchangeAtomically * Fix test/publish - remove format from cacheRate as it's already been formatted * Using roundId 1 as last round to be consistent * Fix integration test - Having the rounds rate loop inside the aggregator * Fix ExchangeRates.js - Using roundId 1 for sUSD to be consistent - Fix interface wrong compiler version warning * Try to fix CI unit test - adding --max-old-space-size
|
work continued in #1642 , closing this one |

lastDynamicFeeandlastRoundSeenfor caching