[BAC-119] Use Uniswap V2 and Curve in DAI-USD oracle #498
Conversation
Kenadia
changed the title
There was a problem hiding this comment.
I'm thinking that it might make more sense to use Uniswap ETH-USDC for the ETH price rather than a Maker Oracle. The Uniswap pools are quite deep these days, and the price should be off by no more than 0.3% due to arbitrage opportunities. This is tighter than most other on-chain oracles (Maker, Chainlink, etc) which are often updated if the price is off by more than 0.5% or 1.0%
| getCurvePrice().value, | ||
| getUniswapPrice(ethUsd).value |
There was a problem hiding this comment.
nit: should we maybe just return a uint256 instead of Monetary.Price from these functions? seems unnecessary to wrap and unwrap the values like this
| // Parameters used when getting the DAI-USD price from Curve. | ||
| int128 constant CURVE_DAI_ID = 0; | ||
| int128 constant CURVE_USDC_ID = 1; | ||
| uint256 constant CURVE_FEE_DENOMINATOR = 10000000000; |
There was a problem hiding this comment.
nit: prefer 10 ** 10 for readability
| view | ||
| returns (uint256); | ||
|
|
||
| function get_dy_underlying( |
There was a problem hiding this comment.
do you prefer get_dy_underlying over get_dy as get_dy_underlying works on more types of Curve pools? Are there any gas implications to that?
There was a problem hiding this comment.
From the source code I thought get_dy_underlying was going to be cheaper, but looks like that's not the case. 55572 vs 55037. So I'll use get_dy instead.
| uint256 uniswapPrice = UNISWAP_DECIMALS_BASE | ||
| .mul(usdcAmt) | ||
| .mul(poolOneEthAmt) |
There was a problem hiding this comment.
I believe there's potential for overflow here for some tokens.
I think it should be fine for USDC since it has only 6 decimals.
10^30 * 10^6 * 10^18 for 1 USDC and 1 ETH
Maybe something to keep in mind or at least comment
edit: I see you have a test for this which is great
| (uint256 daiAmt, uint256 poolOneEthAmt, ) = UNISWAP_DAI_ETH.getReserves(); | ||
| (uint256 usdcAmt, uint256 poolTwoEthAmt, ) = UNISWAP_USDC_ETH.getReserves(); |
There was a problem hiding this comment.
Maybe comment that the order is dependent on the pool. For a different address paired with ETH, the order might not be ETH-2nd
| * Another mock Uniswap V2 pair. | ||
| */ | ||
| contract TestUniswapV2Pair2 is | ||
| IUniswapV2Pair |
There was a problem hiding this comment.
nit: you can just have it inherit from TestUniswapV2Pair1 and add no new functions or variables
No description provided.