-
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 4 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 |
---|---|---|
|
@@ -9,7 +9,6 @@ import {IPyth} from "../../interfaces/pyth/IPyth.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 | ||
|
@@ -56,9 +55,12 @@ abstract contract BoundedUnionSourceAdapter is | |
} | ||
|
||
/** | ||
* @notice Snapshots is a no-op for this adapter as its never used. | ||
* @notice Snapshots data from all sources that require it. | ||
*/ | ||
function snapshotData() public override(ChainlinkSourceAdapter, SnapshotSource) {} | ||
function snapshotData() public override(ChainlinkSourceAdapter, ChronicleMedianSourceAdapter, PythSourceAdapter) { | ||
ChronicleMedianSourceAdapter.snapshotData(); | ||
PythSourceAdapter.snapshotData(); | ||
} | ||
|
||
/** | ||
* @notice Tries getting latest data as of requested timestamp. Note that for all historic lookups we simply return | ||
|
@@ -74,12 +76,14 @@ 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. | ||
// Chainlink has native price history, so use tryLatestDataAt to pull the most recent price that satisfies the | ||
// timestamp constraint. | ||
(int256 clAnswer, uint256 clTimestamp) = ChainlinkSourceAdapter.tryLatestDataAt(timestamp, maxTraversal); | ||
|
||
// 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(); | ||
// For Chronicle and Pyth, tryLatestDataAt would attempt to get price from snapshots, but we can drop them if | ||
// they don't satisfy the timestamp constraint. | ||
(int256 crAnswer, uint256 crTimestamp) = ChronicleMedianSourceAdapter.tryLatestDataAt(timestamp, maxTraversal); | ||
(int256 pyAnswer, uint256 pyTimestamp) = PythSourceAdapter.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 change is unrelated. why was it added? it does make sense but given we want the most recent data can we not continue to use those methods? 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 attempted to fix the original bug. But now I realize this unnecessarily snapshots source data both in Pyth and Chronicle adapters. Instead, we should snapshot the aggregated union value and use that here - I just refactored this in the latest commit 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. that makes a lot more sence. |
||
|
||
// To "drop" Chronicle and Pyth, we set their timestamps to 0 (as old as possible) if they are too recent. | ||
// This means that they will never be used if either or both are 0. | ||
|
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?