-
Notifications
You must be signed in to change notification settings - Fork 2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: use snapshotting library in bounded union source adapter #12
base: master
Are you sure you want to change the base?
Changes from 5 commits
05b2ae5
0fa0033
c19424b
8c2485b
978a9e1
5eb6aea
89bad8b
77b162c
d425284
424d94e
9cb7ed6
9193e32
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,10 +6,10 @@ import {SignedMath} from "openzeppelin-contracts/contracts/utils/math/SignedMath | |
import {IAggregatorV3Source} from "../../interfaces/chainlink/IAggregatorV3Source.sol"; | ||
import {IMedian} from "../../interfaces/chronicle/IMedian.sol"; | ||
import {IPyth} from "../../interfaces/pyth/IPyth.sol"; | ||
import {SnapshotSourceLib} from "../lib/SnapshotSourceLib.sol"; | ||
import {ChainlinkSourceAdapter} from "./ChainlinkSourceAdapter.sol"; | ||
import {ChronicleMedianSourceAdapter} from "./ChronicleMedianSourceAdapter.sol"; | ||
import {PythSourceAdapter} from "./PythSourceAdapter.sol"; | ||
import {SnapshotSource} from "./SnapshotSource.sol"; | ||
|
||
/** | ||
* @title BoundedUnionSourceAdapter contract to read data from multiple sources and return the newest, contingent on it | ||
|
@@ -25,8 +25,20 @@ abstract contract BoundedUnionSourceAdapter is | |
ChronicleMedianSourceAdapter, | ||
PythSourceAdapter | ||
{ | ||
// Pack all source data into a struct to avoid stack too deep errors. | ||
struct AllSourceData { | ||
int256 clAnswer; | ||
uint256 clTimestamp; | ||
int256 crAnswer; | ||
uint256 crTimestamp; | ||
int256 pyAnswer; | ||
uint256 pyTimestamp; | ||
} | ||
|
||
uint256 public immutable BOUNDING_TOLERANCE; | ||
|
||
SnapshotSourceLib.Snapshot[] public boundedUnionSnapshots; // Historical answer and timestamp snapshots. | ||
|
||
constructor( | ||
IAggregatorV3Source chainlink, | ||
IMedian chronicle, | ||
|
@@ -48,21 +60,22 @@ abstract contract BoundedUnionSourceAdapter is | |
override(ChainlinkSourceAdapter, ChronicleMedianSourceAdapter, PythSourceAdapter) | ||
returns (int256 answer, uint256 timestamp) | ||
{ | ||
(int256 clAnswer, uint256 clTimestamp) = ChainlinkSourceAdapter.getLatestSourceData(); | ||
(int256 crAnswer, uint256 crTimestamp) = ChronicleMedianSourceAdapter.getLatestSourceData(); | ||
(int256 pyAnswer, uint256 pyTimestamp) = PythSourceAdapter.getLatestSourceData(); | ||
|
||
return _selectBoundedPrice(clAnswer, clTimestamp, crAnswer, crTimestamp, pyAnswer, pyTimestamp); | ||
AllSourceData memory data = _getAllLatestSourceData(); | ||
return _selectBoundedPrice( | ||
data.clAnswer, data.clTimestamp, data.crAnswer, data.crTimestamp, data.pyAnswer, data.pyTimestamp | ||
); | ||
} | ||
|
||
/** | ||
* @notice Snapshots is a no-op for this adapter as its never used. | ||
* @notice Snapshot the current bounded union source data. | ||
*/ | ||
function snapshotData() public override(ChainlinkSourceAdapter, SnapshotSource) {} | ||
function snapshotData() public override(ChainlinkSourceAdapter, ChronicleMedianSourceAdapter, PythSourceAdapter) { | ||
(int256 latestAnswer, uint256 latestTimestamp) = BoundedUnionSourceAdapter.getLatestSourceData(); | ||
SnapshotSourceLib.snapshotData(boundedUnionSnapshots, latestAnswer, latestTimestamp); | ||
} | ||
|
||
/** | ||
* @notice Tries getting latest data as of requested timestamp. Note that for all historic lookups we simply return | ||
* the Chainlink data as this is the only supported source that has historical data. | ||
* @notice Tries getting latest data as of requested timestamp. | ||
* @param timestamp The timestamp to try getting latest data at. | ||
* @param maxTraversal The maximum number of rounds to traverse when looking for historical data. | ||
* @return answer The answer as of requested timestamp, or earliest available data if not available, in 18 decimals. | ||
|
@@ -74,19 +87,46 @@ abstract contract BoundedUnionSourceAdapter is | |
override(ChainlinkSourceAdapter, ChronicleMedianSourceAdapter, PythSourceAdapter) | ||
returns (int256, uint256) | ||
{ | ||
// Chainlink has price history, so use tryLatestDataAt to pull the most recent price that satisfies the timestamp constraint. | ||
(int256 clAnswer, uint256 clTimestamp) = ChainlinkSourceAdapter.tryLatestDataAt(timestamp, maxTraversal); | ||
// In the happy path there have been no source updates since requested time, so we can return the latest data. | ||
AllSourceData memory data = _getAllLatestSourceData(); | ||
(int256 boundedAnswer, uint256 boundedTimestamp) = _selectBoundedPrice( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we do and update _selectBoundedPrice like
Wdyt? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. great suggestion, pushing commit for this |
||
data.clAnswer, data.clTimestamp, data.crAnswer, data.crTimestamp, data.pyAnswer, data.pyTimestamp | ||
); | ||
if (boundedTimestamp <= timestamp) return (boundedAnswer, boundedTimestamp); | ||
|
||
// For Chronicle and Pyth, just pull the most recent prices and drop them if they don't satisfy the constraint. | ||
(int256 crAnswer, uint256 crTimestamp) = ChronicleMedianSourceAdapter.getLatestSourceData(); | ||
(int256 pyAnswer, uint256 pyTimestamp) = PythSourceAdapter.getLatestSourceData(); | ||
// Chainlink has price history, so use tryLatestDataAt to pull the most recent price that satisfies the timestamp constraint. | ||
(data.clAnswer, data.clTimestamp) = ChainlinkSourceAdapter.tryLatestDataAt(timestamp, maxTraversal); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This could still be potentially optimized to avoid duplicate calls, but would require introducing version of tryLatestDataAt that accepts already fetched latest data as inputs. |
||
|
||
// To "drop" Chronicle and Pyth, we set their timestamps to 0 (as old as possible) if they are too recent. | ||
// "Drop" Chronicle and/or Pyth by setting their timestamps to 0 (as old as possible) if they are too recent. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the drop logic here I wanted to investigate a bit more: I might be missing something here on how we want to compare these kinds of sources, but given we now have the ability to snapshot & query all parts of the BoundedUnion can we not consider them equally and look within all sources to find the newest source that is old enough? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we only drop the latest pyth and chronicle values if they are too recent. but we might still end up using any of these sources if they had been selected as part of snapshoted values. |
||
// This means that they will never be used if either or both are 0. | ||
if (crTimestamp > timestamp) crTimestamp = 0; | ||
if (pyTimestamp > timestamp) pyTimestamp = 0; | ||
if (data.crTimestamp > timestamp) data.crTimestamp = 0; | ||
if (data.pyTimestamp > timestamp) data.pyTimestamp = 0; | ||
|
||
// Bounded union prices could have been captured at snapshot that satisfies time constraint. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1 great comments throughout this file |
||
SnapshotSourceLib.Snapshot memory snapshot = SnapshotSourceLib._tryLatestDataAt( | ||
boundedUnionSnapshots, boundedAnswer, boundedTimestamp, timestamp, maxTraversal | ||
); | ||
|
||
// Update bounded data with constrained source data. | ||
(boundedAnswer, boundedTimestamp) = _selectBoundedPrice( | ||
data.clAnswer, data.clTimestamp, data.crAnswer, data.crTimestamp, data.pyAnswer, data.pyTimestamp | ||
); | ||
|
||
// Return bounded data unless there is a newer snapshotted data that still satisfies time constraint. | ||
if (boundedTimestamp >= snapshot.timestamp || snapshot.timestamp > timestamp) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If I've followed the logic correctly, should we have I understand that at this point (boundedAnswer, boundedTimestamp) and (snapshot.answer, snapshot.timestamp) could be different values if There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this makes sense, pushed the fix |
||
return (boundedAnswer, boundedTimestamp); | ||
} | ||
return (snapshot.answer, snapshot.timestamp); | ||
} | ||
|
||
// Internal helper to get the latest data from all sources. | ||
function _getAllLatestSourceData() internal view returns (AllSourceData memory) { | ||
AllSourceData memory data; | ||
(data.clAnswer, data.clTimestamp) = ChainlinkSourceAdapter.getLatestSourceData(); | ||
(data.crAnswer, data.crTimestamp) = ChronicleMedianSourceAdapter.getLatestSourceData(); | ||
(data.pyAnswer, data.pyTimestamp) = PythSourceAdapter.getLatestSourceData(); | ||
|
||
return _selectBoundedPrice(clAnswer, clTimestamp, crAnswer, crTimestamp, pyAnswer, pyTimestamp); | ||
return data; | ||
} | ||
|
||
// Selects the appropriate price from the three sources based on the bounding tolerance and logic. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,19 +1,22 @@ | ||
// SPDX-License-Identifier: BUSL-1.1 | ||
pragma solidity 0.8.17; | ||
|
||
import {SnapshotSource} from "./SnapshotSource.sol"; | ||
import {DiamondRootOval} from "../../DiamondRootOval.sol"; | ||
import {SnapshotSourceLib} from "../lib/SnapshotSourceLib.sol"; | ||
import {IOSM} from "../../interfaces/makerdao/IOSM.sol"; | ||
|
||
/** | ||
* @title OSMSourceAdapter contract to read data from MakerDAO OSM and standardize it for Oval. | ||
*/ | ||
abstract contract OSMSourceAdapter is SnapshotSource { | ||
abstract contract OSMSourceAdapter is DiamondRootOval { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nice. this is more consistent now. |
||
IOSM public immutable osmSource; | ||
|
||
// MakerDAO performs decimal conversion in collateral adapter contracts, so all oracle prices are expected to have | ||
// 18 decimals and we can skip decimal conversion. | ||
uint8 public constant decimals = 18; | ||
|
||
SnapshotSourceLib.Snapshot[] public osmSnapshots; // Historical answer and timestamp snapshots. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. also this works well I think. when you need the snapshot within the adapter you simple: a) import the lib b) have a local snapshots array. |
||
|
||
event SourceSet(address indexed sourceOracle); | ||
|
||
constructor(IOSM source) { | ||
|
@@ -22,6 +25,14 @@ abstract contract OSMSourceAdapter is SnapshotSource { | |
emit SourceSet(address(source)); | ||
} | ||
|
||
/** | ||
* @notice Snapshot the current source data. | ||
*/ | ||
function snapshotData() public virtual override { | ||
(int256 latestAnswer, uint256 latestTimestamp) = OSMSourceAdapter.getLatestSourceData(); | ||
SnapshotSourceLib.snapshotData(osmSnapshots, latestAnswer, latestTimestamp); | ||
} | ||
|
||
/** | ||
* @notice Returns the latest data from the source. | ||
* @return answer The latest answer in 18 decimals. | ||
|
@@ -34,14 +45,16 @@ abstract contract OSMSourceAdapter is SnapshotSource { | |
/** | ||
* @notice Tries getting latest data as of requested timestamp. If this is not possible, returns the earliest data | ||
* available past the requested timestamp within provided traversal limitations. | ||
* @dev OSM does not support historical lookups so this uses SnapshotSource to get historic data. | ||
* @dev OSM does not support historical lookups so this uses SnapshotSourceLib to get historic data. | ||
* @param timestamp The timestamp to try getting latest data at. | ||
* @param maxTraversal The maximum number of rounds to traverse when looking for historical data. | ||
* @return answer The answer as of requested timestamp, or earliest available data if not available, in 18 decimals. | ||
* @return updatedAt The timestamp of the answer. | ||
*/ | ||
function tryLatestDataAt(uint256 timestamp, uint256 maxTraversal) public view override returns (int256, uint256) { | ||
Snapshot memory snapshot = _tryLatestDataAt(timestamp, maxTraversal); | ||
(int256 latestAnswer, uint256 latestTimestamp) = OSMSourceAdapter.getLatestSourceData(); | ||
SnapshotSourceLib.Snapshot memory snapshot = | ||
SnapshotSourceLib._tryLatestDataAt(osmSnapshots, latestAnswer, latestTimestamp, timestamp, maxTraversal); | ||
return (snapshot.answer, snapshot.timestamp); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no reason really to snapshot round ID I guess? especially given any source that has the notion of a round ID also has history?