poke once a day, use struct, more changes #4
Conversation
| uint32 public age; // time of last poke | ||
| struct Accumulator { | ||
| uint256 val; | ||
| uint32 ts; |
There was a problem hiding this comment.
I'm not sure tracking the exact poke ts is worth the added poke cost (vs just using the eod ts). Generally if we expect the window to be much larger than 1 day (e.g. 30 days) the gain in precision is likely to be negligible (I did some quick approximation math to convince myself of it but might be good to test with actual values).
There was a problem hiding this comment.
Well the poke is only done once a day, so even if it's a matter of a few storage operations (probably less) I think it's negligible.
I think storing the timestamps is the more simple and standard way (for example Uniswap have this Observation struct - https://github.com/Uniswap/v2-periphery/blob/master/contracts/examples/ExampleSlidingWindowOracle.sol#L19).
There was a problem hiding this comment.
Ok, though note that in the Uniswap example the timestamp is necessary as the accumulators are not indexed by days. That said I don't mind storing the ts here given the frequency of poke calls. I guess we don't need it to be packed as a uint32 though, uint256 would work fine and save a tiny bit of gas.
Co-authored-by: sunbreak1211 <[email protected]>
| // days_ == N will fill up a window corresponding to [lo == N, hi == 1] along with the current day | ||
| // days_ should be selected carefully as too many iterations can cause the transaction to run out of gas | ||
| // if the initiated timespan is shorter than the [lo, hi] window the initial cap will just be used for longer | ||
| function init(uint256 days_) external auth returns(bool) { |
There was a problem hiding this comment.
| function init(uint256 days_) external auth returns(bool) { | |
| function init(uint256 days_) external auth returns (bool) { |
| @@ -17,29 +17,39 @@ | |||
| pragma solidity ^0.8.16; | |||
|
|
|||
| interface PipLike { | |||
| function read() external view returns (uint128); | |||
| function peek() external view returns (uint128, bool); | |||
| function read() external view returns (uint128); // TODO: shouldn't this (and our function) return bytes32? https://github.com/makerdao/osm/blob/e36c874b4e14fba860e48c0cf99cd600c0c59efa/src/osm.sol#L150C49-L150C56 | |||
There was a problem hiding this comment.
The main reason for using a uint128 type was to be able to pack val and age in the same slot (and avoid an explicit cast for a value which under the hood is anyway a uint128 in the pip/osm). But since these variables have been removed, feel free to use a bytes32 (like in the osm) or a uint256 (like in the pip).
| uint96 public slope = uint96(RAY); // maximum allowable price growth factor from center of TWAP window to now (in RAY such that slope = (1 + {max growth rate}) * RAY) | ||
| uint8 public lo; // how many days ago should the TWAP window start (exclusive) | ||
| uint8 public hi; // how many days ago should the TWAP window end (inclusive) | ||
| uint96 public slope = uint96(RAY); // maximum allowable price growth factor from the average value of a TWAP window (in RAY such that slope = (1 + {max growth rate}) * RAY) |
There was a problem hiding this comment.
What I was trying to say here is that, given that the TWAP is an unbiased smoothed estimate of the price at the time in the middle of the TWAP window, the slope can be interpreted as the allowable growth between that particular time and now. But maybe it's simpler to just describe it as a TWAP multiplier.
| uint96 public slope = uint96(RAY); // maximum allowable price growth factor from the average value of a TWAP window (in RAY such that slope = (1 + {max growth rate}) * RAY) | |
| uint96 public slope = uint96(RAY); // maximum allowable price growth factor in reference to the TWAP (in RAY such that slope = (1 + {max growth rate}) * RAY) |
There was a problem hiding this comment.
Ahh ok got it. Although I think it is easier to describe it as the multiplier of the TWAP.
Your correction looks good to me.
No description provided.