Feat/multiple oracles by xrave110 · Pull Request #14 · Byte-Masons/stability-pool-compounder

xrave110 · 2024-02-04T13:49:29Z

No description provided.

xrave110 · 2024-02-05T15:07:33Z

src/ReaperStrategyStabilityPool.sol

        pools[0] = address(uniV3UsdcErnPool);
        uint256 usdcAmount =
-            uniV3TWAP.quoteSpecificPoolsWithTimePeriod(ernAmount, want, address(usdc), pools, uniV3TWAPPeriod);
+            uniV3TWAP.quoteSpecificPoolsWithTimePeriod(ernAmount, want, address(usdc), pools, twapPeriod);


Could be adjusted with new interface "getErnAmountForUsdcAll"

.gitmodules

remappings.txt

src/ReaperStrategyStabilityPool.sol

0xulver · 2024-02-05T15:01:19Z

src/ReaperStrategyStabilityPool.sol

+    }
+
+    /**
+     * @dev Returns the {ernAmount} for the specified {_baseAmount} of USDC over a given {_period} (in seconds)


Are the comments correct for WETH?

0xulver · 2024-02-05T15:03:48Z

src/ReaperStrategyStabilityPool.sol

+        return ((usdcAmountForWethPriceFeeds * 1 ether * 10 ** usdc.decimals()) / veloAmountForWethVelo);
+    }
+
+    function getInfoAboutTwapOracles(uint256[] memory prices, uint32 idx, uint32 tolerance)


Needs comments, Info is very general. What is info exactly? Why is this needed?

0xulver · 2024-02-05T15:07:17Z

src/ReaperStrategyStabilityPool.sol

+     * @dev Returns the {ernAmount} for the specified {_baseAmount} of USDC over a given {_period} (in seconds)
+     * using the all possible oracles.
+     */
+    function getErnAmountForUsdcAll(uint256[] memory _prices, uint128 _baseAmount, uint32 _period, uint32 _tolerance)


Why are _baseAmount and _period unused?

0xulver · 2024-02-05T15:08:16Z

src/ReaperStrategyStabilityPool.sol

+        indexes[idx] = true;
+        validAmount = 1;
+
+        for (uint32 cnt = (idx + 1) % uint32(prices.length); cnt != idx; cnt = (cnt + 1) % uint32(prices.length)) {


This reads kind of like a C program or something, it is not very readable

src/OracleAggregator.sol

0xulver · 2024-03-07T11:27:25Z

src/OracleAggregator.sol

+        uint256[] memory deviationsSq = new uint256[](nrValidPrices);
+        for (uint256 i = 0; i < nrValidPrices; i++) {
+            int256 deviation = int256(prices[i]) - int256(mean);
+            deviationsSq[i] = uint256(deviation * deviation);


What happens in case of negative number? I suppose when multiplied it will always be positive so there is no issue casting to uint?

martelskiy · 2024-03-07T11:50:16Z

test/ReaperStrategyStabilityPool.t.solTODO

        uint256 usdcInPool = IERC20Upgradeable(usdcAddress).balanceOf(uniV3UsdcErnPool);
        console.log("usdcInPool: ", usdcInPool);
-        uint256 usdcToDump = usdcInPool * 9999 / 10_000;
+        uint256 usdcToDump = (usdcInPool * 9999) / 10_000;


parentheses would not make the difference here really. Was it linter that added those on the save?

.gitmodules

martelskiy · 2024-03-07T11:55:24Z

remappings.txt

+ds-test/=lib/vault-v2/lib/forge-std/lib/ds-test/src/
+forge-std/=lib/vault-v2/lib/forge-std/src/
+oz-upgradeable/=lib/vault-v2/lib/openzeppelin-contracts-upgradeable/contracts/
+oz/=lib/vault-v2/lib/openzeppelin-contracts/contracts/


all remappings can actually be in foundry.toml file.
Like this:

remappings = [ "forge-std/=lib/vault-v2/lib/forge-std/src/", ... ]

martelskiy · 2024-03-07T11:59:41Z

src/OracleAggregator.sol

+        OracleKind kind;
+    }
+
+    function getTwapPrices(OracleRoute[] memory oracles, uint256 baseAmount) public returns (uint256[] memory prices) {


this function can be made external instead of public since it is not called within the contract itself

The OracleAggregator is supposed to be inherited by other contracts. It could also be turned into a library or, as you said, an external contract. It may be better to make it a library.

martelskiy · 2024-03-07T12:40:47Z

src/OracleAggregator.sol

+        return IPriceFeed(source).fetchPrice(target) * baseAmount;
+    }
+
+    uint256[50] private __gap;


what is this state variable?

This is to make the storage layout easier to update, in case the strategy is upgraded. Will be removed in case this is turned into an external contract.

martelskiy · 2024-03-07T13:18:37Z

src/OracleAggregator.sol

+    /// For example, a MAD higher than 10% of the median means the prices are too spread out,
+    /// and the whole list is considered unreliable.
+    /// @param maxScoreBPS If a price has a Z-score higher than this, it's considered an outlier and filtered out
+    function getMeanPrice(uint256[] memory prices, uint256 maxMadRelativeToMedianBPS, uint256 maxScoreBPS)


Who would be the caller of the getMeanPrice function? Other contracts? It is extremely compute-heavy. getMAD function runs quick sort algo(which is O(nlog n) average time complexity) 2 times over the entire collection of items. Then there are other places where we iterate over the array. It would be good to perform some gas usage tests. Also, on average, how many items do you think prices collection will store? I think it would work great with smaller collections, which I assume would be the case(the prices collection will contain prices from various oracles and there are not that many oracles out there. Do I get it right?). Otherwise, if the collections are large is there an option of putting this implementation off-chain so we do not pay high gas fees?
From what I understand it works this way(very roughly):
[5, 3513, 104331, 222] - will revert because the prices are too spread out
[313, 12, 10, 9, 3113] - will consider 313 and 3113 to be outliers and compute the mean(average) of [12, 10, 9] which is ~10.3 (the outliers will be calculated based on Z score value)
[133, 130, 129, 135] - won't consider any value to be invalid(outlier), won't revert, will calculate mean (sum(n)/count(n))
Questions:

why you decided to go with a quick sort algorithm in particular? I am not sorting algo experts, but some of the sorting algorithms space and time complexity heavily depends on the number of items in the collection. So the expected size of the collection should determine which algorithm to use. Although, if the price collection contains, let's say, 1 - 100 items, the algorithm probably does not matter much

were you implementing this from scratch? Have you worked with statistics/algorithms before? We need to pull Yuvi here XD

Yes, this function would be used on chain. The amount of oracles should be from 2 to a max of 5, from what I understand.
Quick sort seems to be the most efficient in solidity and even more so considering the small data. Merge sort is the only other considerable option, but it requires additional arrays so more memory, and merges aren't computationally efficient in solidity. Quick sort on the other hand just swaps values in the same memory array.
I haven't worked with statistics, thought this would be easier than I thought initially lol..

In some simple tests I made, it seems that a single TWAP calculation for some oracles uses more gas than this whole getMeanPrice algorithm.

src/OracleAggregator.sol

martelskiy · 2024-03-07T14:06:57Z

test/ReaperStrategyStabilityPool.t.solTODO

 import "forge-std/console.sol";
 import "src/ReaperStrategyStabilityPool.sol";
-import "vault-v2/ReaperSwapper.sol";
+import {ReaperSwapper, ISwapRouter, TransferHelper} from "vault-v2/ReaperSwapper.sol";


we can apply the same pattern to other imports to be consistent

martelskiy · 2024-03-07T14:17:33Z

src/oracles/VeloTwapMixin.sol

+        return (3 * x0 * ((y * y) / 1e18)) / 1e18 + ((((x0 * x0) / 1e18) * x0) / 1e18);
+    }
+
+    function _get_y(uint256 x0, uint256 xy, uint256 y, uint256 decimals0, uint256 decimals1, bool stable)


In general, we write code for humans, not for machines. There are not many humans out there that can comprehend this implementation. It could be refactored for the sake of improving readability :)

I agree, but this is just code from Velodrome's pair pricing functions, which unfortunately aren't public. So I adapted them to work with external variables. I added a comment to address this

Only changes done here are passing values that used to be storage as calldata

martelskiy · 2024-03-07T14:29:07Z

src/oracles/VeloTwapMixin.sol

+        revert("!y");
+    }
+
+    function safe112(uint256 n) private pure returns (uint112) {


There are other places in the codebase where do not have this check before casting(like here. Should we have these checks everywhere? Do you how EVM behaves when the integer has greater size than the type we cast them to?
PS: you can also try this instead - if (n > type(uint112).max){ }

martelskiy · 2024-03-07T14:39:15Z

I would recommend guys, next time, try to split your work into smaller pieces and submit small PRs so it is easier to review and comprehend the changes, otherwise, there is a very high chance of: a) missing something important due to very high number of changes b) PR not being reviewed and merged at all, due to high risks c) review takes a lot of time and it will block you, because you will be waiting for the review before you can continue adding more code, hence you will need to context switch, which will impact your productivity and cause unnecessary high cognitive load.
Also, put attention on function access modifiers (restrict them to the maximum) and think about improving code readability. Complex = bad, simple = cool (think about other engineers who would need to work with your code)

lookeey · 2024-03-20T19:17:56Z

Set via-ir to true in the toml. This makes the initialization of the new complex structs much easier.

xrave110

Left some suggestions, potential issues and questions to be answered.

src/OracleAggregator.sol

xrave110 · 2024-05-27T12:30:33Z

src/OracleAggregator.sol

+            for (uint256 i = 0; i < oracles.length; i++) {
+                delayedPrices[i] = _fetchMultiHopPrice(oracles[i], amountIn, true);
+            }
+            (uint256 delayedMean, ) = getMean(delayedPrices, new bool[](2));


Not initiated bool array passed and later (line 215), it is used in if statement

src/OracleAggregator.sol

src/oracles/BalancerTwapMixin.sol

src/OracleAggregator.sol

xrave110 · 2024-05-29T10:39:48Z

src/OracleAggregator.sol

+    /// @param route List of oracles for multihop price
+    /// @param amountIn Input amount of the base token
+    function fetchMultiHopPrice(OracleRoute memory route, uint256 amountIn) external view returns (uint256 price) {
+        for (uint256 i = 0; i < route.oracles.length; i++) {


Didn't see any test for multihop price with route.oracles.length > 1

src/oracles/VeloTwapMixin.sol

xrave110 · 2024-05-29T13:05:55Z

src/ReaperStrategyStabilityPool.sol

+    // constants
+
+    uint256 public constant SPREAD_TOLERANCE = 500; // 5%
+    uint256 public constant MAX_SCORE_BPS = 25_000; // / 2.5X MAD for price outlier detection


Why this exact value ? Do we want to have it configurable ?

BastienGuyonnet · 2024-06-30T21:01:17Z

src/ReaperStrategyStabilityPool.sol

    uint256 acceptableTWAPLowerBound; // The normal lower price for the , reverts harvest if below

+    OracleRoute[] internal ernForUsdcOracles;
+    OracleRoute[] internal ernForUsdcViewOracles;


Is this values used?

BastienGuyonnet · 2024-06-30T21:02:37Z

src/ReaperStrategyStabilityPool.sol


-        uniV3TWAP = IStaticOracle(_uniV3TWAP);
-        uniV3UsdcErnPool = IUniswapV3Pool(_pools.uniV3UsdcErnPool);
        compoundingFeeMarginBPS = 9950;


Could use the updateCompoundingFeeMarginBPS function here if made public

BastienGuyonnet · 2024-06-30T21:06:52Z

src/ReaperStrategyStabilityPool.sol

-            uint256 difference = newErnAmount > oldErnAmount ? newErnAmount - oldErnAmount : oldErnAmount - newErnAmount;
-            uint256 relativeChange = difference * PERCENT_DIVISOR / oldErnAmount;
-            require(relativeChange < 300, "TWAP duration change would change price");
+    function updateOracles(OracleRoute[] calldata newRoutes) public {


Function does not check the validity of the newRoutes array
Not sure if that is important to note though, as the default admin should be trustworthy

BastienGuyonnet · 2024-08-27T17:35:24Z

src/OracleAggregator.sol

+    /// @param route List of oracles for multihop price
+    /// @param amountIn Input amount of the base token
+    function fetchMultiHopPrice(OracleRoute memory route, uint256 amountIn) external view returns (uint256 price) {
+        _fetchMultiHopPrice(route, amountIn, false);


Add a return value here

BastienGuyonnet · 2024-08-27T17:42:04Z

src/oracles/VeloTwapMixin.sol

+        return (3 * x0 * ((y * y) / 1e18)) / 1e18 + ((((x0 * x0) / 1e18) * x0) / 1e18);
+    }
+
+    function _get_y(uint256 x0, uint256 xy, uint256 y, uint256 decimals0, uint256 decimals1, bool stable)


Only changes done here are passing values that used to be storage as calldata

src/OracleAggregator.sol

BastienGuyonnet · 2024-08-27T18:10:58Z

src/OracleAggregator.sol

+    // @param tokenIn address(0) for price, address(1) for inverted price
+    // @param decimalOffset Difference between tokenIn and tokenOut decimals
+    // @param amountIn Input amount of the base token
+    function getChainlinkPrice(address source, uint256 decimalOffset, address tokenIn, uint256 amountIn) internal view returns (uint256 price) {


Export to mixin for cleaner organisation

xrave110 added 2 commits February 4, 2024 13:53

handling multiple prices and velo twaps

ed039cf

Tests updates and adjustments

c7872de

xrave110 requested a review from 0xulver February 4, 2024 13:49

xrave110 commented Feb 5, 2024

View reviewed changes

0xulver reviewed Feb 5, 2024

View reviewed changes

xrave110 and others added 7 commits February 11, 2024 12:29

New velo twap added and improved interfaces

0fb2f31

Disabled ReaperStrategyStabilityPool.t tests

47e8696

Cleaning

59cbf93

Name refactor

fccc79f

Create OracleAggregator

4b68bc0

Use MAD instead of standard deviation

affe3f8

add natspec

ceed2cf

0xulver reviewed Mar 7, 2024

View reviewed changes

martelskiy reviewed Mar 7, 2024

View reviewed changes

.gitmodules Show resolved Hide resolved

martelskiy reviewed Mar 7, 2024

View reviewed changes

src/OracleAggregator.sol Show resolved Hide resolved

martelskiy reviewed Mar 7, 2024

View reviewed changes

Integrate OracleAggregator into Strategy

0ee142e

lookeey added 4 commits April 16, 2024 19:34

improve tests

94ac0b7

minor fixes

7872c09

standardize return variable names

6875246

change number of good prices required

51b3d3e

rework oracle + refactor

4e64860

xrave110 commented May 29, 2024

View reviewed changes

review fixes

2d35091

BastienGuyonnet reviewed Jun 30, 2024

View reviewed changes

review fixes

fd8433c

BastienGuyonnet reviewed Aug 27, 2024

View reviewed changes

review fixes

7225b09

Comments

Conversation

xrave110 commented Feb 4, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

martelskiy Mar 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

martelskiy commented Mar 7, 2024

Uh oh!

lookeey commented Mar 20, 2024

Uh oh!

xrave110 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

martelskiy Mar 7, 2024 •

edited

Loading