SPOT Appraiser update #223
Conversation
aalavandhan
commented
Sep 16, 2024
•
aalavandhan
commented
- BillBroker uses a new beta volatility measure to adjust the price of perps.
- Updated spot appraiser; crated a unified pricer/oracle used by all vaults and implements pricing logic.
aalavandhan
force-pushed
the
appraiser-fix
branch
from
c709013
to
feb40d8
Compare
SPOT Appraiser update
🚨 Report Summary
For more details view the full report in OpenZeppelin Code Inspector |
aalavandhan
force-pushed
the
appraiser-fix
branch
2 times, most recently
from
e57a50c
to
6792e2e
Compare
aalavandhan
force-pushed
the
appraiser-fix
branch
2 times, most recently
from
28d5a35
to
4442b00
Compare
aalavandhan
force-pushed
the
appraiser-fix
branch
from
4442b00
to
6cd9f05
Compare
aalavandhan
force-pushed
the
appraiser-fix
branch
from
6cd9f05
to
0fcd00d
Compare
aalavandhan
force-pushed
the
appraiser-fix
branch
from
0fcd00d
to
21ed063
Compare
| /// @return Number of decimals representing the prices returned. | ||
| function decimals() external pure returns (uint8); | ||
|
|
||
| /// @return price The price of reference USD tokens. |
There was a problem hiding this comment.
| /// @return price The price of reference USD tokens. | |
| /// @return price The price of reference USD tokens in dollars. |
| Line memory fn, | ||
| uint256 xL, | ||
| uint256 xU, | ||
| uint256 yMin, |
There was a problem hiding this comment.
It seems weird to have a clip operation in a function that calculates an average. Would it be a cleaner interface to have this outside?
Same comment for computeY
| /// @return Number of decimals representing the prices returned. | ||
| function decimals() external pure returns (uint8); | ||
|
|
||
| /// @return price The price of USDC tokens in dollars. |
There was a problem hiding this comment.
This is the first time we've mentioned usdc tokens specifically. Think we should keep it general, as "usd tokens"? Especially since this is an interface
There was a problem hiding this comment.
I think for this interface, its fine. The meta oracle interface basically returns the prices of various crypto tokens. (Wampl, eth, spot, ampl, usdc etc) ..
Maybe I can rename to usdcUsdPrice which is consistent with other methods?
If we had to expose the price of usdt for example, we'll add usdtUsdPrice and so on.
|
|
||
| // @dev Helper function to clip y between min and max values | ||
| function _clip(uint256 y, uint256 min, uint256 max) private pure returns (uint256) { | ||
| y = (y <= min) ? min : y; |
There was a problem hiding this comment.
I'd default to no-ops, rather than executing changes that are presumed no-ops.
If the linter allows it, single lines are fine.
If (y < min) {y = min;}
If (y > max) {y = max;}
| return _clip(avgY_, yMin, yMax); | ||
| } | ||
|
|
||
| /// @dev This function computes y for a given x on the line (fn), bounded by yMin and yMax. |
There was a problem hiding this comment.
As a caller, I'd want to know:
- Must x exist between x1 and x2?
- Must x1 < x2?
I think the code is no for both of those.
If so, I think this means we need 12 different test cases to ensure we cover all the possibilities?
- two ways for a positive slope
- two ways for a negative slope
- 3 ways x can fall wrt to x1 and x2 (less than, within, and greater than)
Do we gain enough meaningful precision from the extra computation and complexity vs the old one?
There was a problem hiding this comment.
Hmm. maybe not worth the effort. Can revert back
| bool posM = (fn.y2 > fn.y1 && fn.x2 > fn.x1) || (fn.y2 < fn.y1 && fn.x2 < fn.x1); | ||
|
|
||
| // Determine if (x - x1) is positive | ||
| bool posDelX1 = (x > fn.x1); |
There was a problem hiding this comment.
Line 50 implies that we don't know which of x1 or x2 is greater. But here we only check x against x1. Is that correct?
I would expect that if x2 < x1, we'd check if x > x2.
| uint256 private constant CL_USDC_ORACLE_STALENESS_THRESHOLD_SEC = 3600 * 48; // 2 day | ||
| uint256 private constant USDC_UPPER_BOUND = (101 * ONE) / 100; // 1.01$ | ||
| uint256 private constant USDC_LOWER_BOUND = (99 * ONE) / 100; // 0.99$ | ||
| uint32 private constant TWAP_DURATION = 3600; |
There was a problem hiding this comment.
This means we use a 60min twap for prices from uni?
