fix: use snapshotting library in bounded union source adapter #12

Reinis-FRP · 2024-05-16T10:49:09Z

This test demonstrated that Oval prices can flip backwards when using BoundedUnionSourceAdapter. The root cause of the issue is that stacking multiple source adapters ended up using the same storage slot for snapshotting historical price data.

This PR addresses the issue by converting SnapshotSource to stateless library, so that any source adapters that need snapshotting would instead pass their own storage pointers. With this fixed it now became possible to snapshot individual source oracle data that did not support native historical data.

Added test testLookbackDoesNotFlipBackward demonstrates that now its not possible to return earlier data than returned compared to a request made before.

We might also want to fix this in raw UnionSourceAdapter

Fixes: https://linear.app/uma/issue/UMA-2183/consider-snapshotted-data-in-union-adapters

Signed-off-by: Reinis Martinsons <reinis@umaproject.org>

linear · 2024-05-16T10:49:44Z

UMA-2183 Consider snapshotted data in Union adapters

chrismaree · 2024-05-16T11:40:16Z

src/adapters/lib/SnapshotSourceLib.sol

 */
-abstract contract SnapshotSource is DiamondRootOval {
+library SnapshotSourceLib {
    // Snapshot records the historical answer at a specific timestamp.
    struct Snapshot {


no reason really to snapshot round ID I guess? especially given any source that has the notion of a round ID also has history?

chrismaree

Did you run a before and after gas comparison?

chrismaree · 2024-05-16T11:49:26Z

src/adapters/source-adapters/OSMSourceAdapter.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 {


nice. this is more consistent now.

chrismaree · 2024-05-16T11:49:53Z

src/adapters/source-adapters/OSMSourceAdapter.sol

    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.


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.

chrismaree · 2024-05-16T11:50:22Z

test/mocks/MockPyth.sol

@@ -0,0 +1,26 @@
+// SPDX-License-Identifier: BUSL-1.1


why is this showing up as new? was this not here before?

we had them only in oval demo repo

chrismaree · 2024-05-16T11:56:09Z

src/adapters/source-adapters/BoundedUnionSourceAdapter.sol

-        // 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);


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?

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

that makes a lot more sence.

Signed-off-by: Reinis Martinsons <reinis@umaproject.org>

Reinis-FRP · 2024-05-16T16:20:37Z

src/adapters/source-adapters/BoundedUnionSourceAdapter.sol

-        (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);


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.

Signed-off-by: Reinis Martinsons <reinis@umaproject.org>

mrice32

LGTM!

mrice32 · 2024-05-20T09:04:02Z

src/adapters/source-adapters/BoundedUnionSourceAdapter.sol

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


+1 great comments throughout this file

md0x · 2024-05-20T09:17:35Z

src/adapters/source-adapters/BoundedUnionSourceAdapter.sol

+        );
+
+        // Return bounded data unless there is a newer snapshotted data that still satisfies time constraint.
+        if (boundedTimestamp >= snapshot.timestamp || snapshot.timestamp > timestamp) {


If I've followed the logic correctly, should we have if (boundedTimestamp > snapshot.timestamp) instead, so we give preference to the snapshot data in case there is one?

I understand that at this point (boundedAnswer, boundedTimestamp) and (snapshot.answer, snapshot.timestamp) could be different values if boundedTimestamp == snapshot.timestamp, as the snapshot could have a more complete information?

this makes sense, pushed the fix

md0x · 2024-05-20T10:00:49Z

src/adapters/source-adapters/BoundedUnionSourceAdapter.sol

-        (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(


Can we do _selectBoundedPrice(data)

and update _selectBoundedPrice like

function _selectBoundedPrice(AllSourceData memory data) internal view returns (int256, uint256) {

Wdyt?

great suggestion, pushing commit for this

md0x

Looks good!
I added one question and one suggestion. Overall, it looks like a solid solution 👍

chrismaree · 2024-05-20T11:39:40Z

src/adapters/source-adapters/BoundedUnionSourceAdapter.sol


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


the drop logic here I wanted to investigate a bit more:
consider chronicle or pyth have their second most recent values being newer than chainlink, but their newest value is too recent. the implementation below, due to setting these timestamps to zero, will result in this value not being returned, even though the snapshot for these sources might have a "better" value to return (it's newer than chainlink, but still old enough to not be filtered out by the lockWindow).

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?

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.

Signed-off-by: Reinis Martinsons <reinis@umaproject.org>

…a point (#14) Signed-off-by: Matt Rice <matthewcrice32@gmail.com> Signed-off-by: chrismaree <christopher.maree@gmail.com> Co-authored-by: chrismaree <christopher.maree@gmail.com>

Signed-off-by: Reinis Martinsons <reinis@umaproject.org>

daywiss

lgtm

Reinis-FRP added 4 commits May 16, 2024 09:51

test: bounded union source adapter not flipping backward

05b2ae5

Signed-off-by: Reinis Martinsons <reinis@umaproject.org>

Merge branch 'master' into reinis-frp/fix-bounded-union

0fa0033

fix: convert snapshot source to lib

c19424b

Signed-off-by: Reinis Martinsons <reinis@umaproject.org>

fix: use snapshotting in bounded union source adapter

8c2485b

Signed-off-by: Reinis Martinsons <reinis@umaproject.org>

Reinis-FRP changed the title ~~Reinis frp/fix bounded union~~ fix: use snapshotting library in bounded union source adapter May 16, 2024

Reinis-FRP requested review from daywiss, mrice32, md0x and chrismaree May 16, 2024 10:50

chrismaree reviewed May 16, 2024

View reviewed changes

fix: snapshot aggregated union data

978a9e1

Signed-off-by: Reinis Martinsons <reinis@umaproject.org>

Reinis-FRP commented May 16, 2024

View reviewed changes

Reinis-FRP requested a review from chrismaree May 16, 2024 16:21

Reinis-FRP added 2 commits May 20, 2024 06:05

Merge branch 'master' into reinis-frp/fix-bounded-union

5eb6aea

Signed-off-by: Reinis Martinsons <reinis@umaproject.org>

fix: round id in tests

89bad8b

Signed-off-by: Reinis Martinsons <reinis@umaproject.org>

mrice32 approved these changes May 20, 2024

View reviewed changes

md0x reviewed May 20, 2024

View reviewed changes

md0x approved these changes May 20, 2024

View reviewed changes

chrismaree reviewed May 20, 2024

View reviewed changes

Reinis-FRP and others added 4 commits May 20, 2024 16:06

fix: pass data struct in internal call

77b162c

Signed-off-by: Reinis Martinsons <reinis@umaproject.org>

fix: give preference to the snapshot data for same timestamp

d425284

Signed-off-by: Reinis Martinsons <reinis@umaproject.org>

feat: add maxAge parameter to limit the staleness of a historical dat…

424d94e

…a point (#14) Signed-off-by: Matt Rice <matthewcrice32@gmail.com> Signed-off-by: chrismaree <christopher.maree@gmail.com> Co-authored-by: chrismaree <christopher.maree@gmail.com>

Merge branch 'master' into reinis-frp/fix-bounded-union

9cb7ed6

Signed-off-by: Reinis Martinsons <reinis@umaproject.org>

Merge branch 'master' into reinis-frp/fix-bounded-union

9193e32

Signed-off-by: Reinis Martinsons <reinis@umaproject.org>

daywiss approved these changes Jun 20, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: use snapshotting library in bounded union source adapter #12

fix: use snapshotting library in bounded union source adapter #12

Reinis-FRP commented May 16, 2024 •

edited

Loading

linear bot commented May 16, 2024

chrismaree May 16, 2024

chrismaree left a comment

chrismaree May 16, 2024

chrismaree May 16, 2024

chrismaree May 16, 2024

Reinis-FRP May 16, 2024

chrismaree May 16, 2024 •

edited

Loading

Reinis-FRP May 16, 2024

chrismaree May 16, 2024

Reinis-FRP May 16, 2024

mrice32 left a comment

mrice32 May 20, 2024

md0x May 20, 2024

Reinis-FRP May 20, 2024

md0x May 20, 2024

Reinis-FRP May 20, 2024

md0x left a comment

chrismaree May 20, 2024

Reinis-FRP May 20, 2024

daywiss left a comment


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

fix: use snapshotting library in bounded union source adapter #12

Are you sure you want to change the base?

fix: use snapshotting library in bounded union source adapter #12

Conversation

Reinis-FRP commented May 16, 2024 • edited Loading

linear bot commented May 16, 2024

Choose a reason for hiding this comment

chrismaree left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrismaree May 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrice32 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

md0x left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daywiss left a comment

Choose a reason for hiding this comment

Reinis-FRP commented May 16, 2024 •

edited

Loading

chrismaree May 16, 2024 •

edited

Loading