Skip to content
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

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
Open
Prev Previous commit
Next Next commit
fix: use snapshotting in bounded union source adapter
Signed-off-by: Reinis Martinsons <reinis@umaproject.org>
Reinis-FRP committed May 16, 2024
commit 8c2485b50fc264f0a353103c5a61e09800c7657b
17 changes: 11 additions & 6 deletions src/adapters/source-adapters/BoundedUnionSourceAdapter.sol
Original file line number Diff line number Diff line change
@@ -55,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, ChronicleMedianSourceAdapter, PythSourceAdapter) {}
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
@@ -73,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);
Copy link
Member

@chrismaree chrismaree May 16, 2024

Choose a reason for hiding this comment

The 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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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

Copy link
Member

Choose a reason for hiding this comment

The 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.