Osmosis price oracle #155

uint · 2023-10-26T16:24:24Z

Maybe closes #99

check for outdated price data in consumer (store timestamp)
review how/when subscriptions are saved/removed in provider (IBC handshake vs explicit local endpoint?)
for twap, verify how sdk.Dec gotten from arithmetic_twap_to_now is stringified
tests

maurolacy

Initial review of the price provider. Will take a look at the consumer next.

contracts/consumer/remote-price-feed/Cargo.toml

contracts/consumer/remote-price-feed/README.md

contracts/osmosis-price-provider/Cargo.toml

maurolacy · 2023-11-01T05:31:35Z

contracts/osmosis-price-provider/Cargo.toml

+serde           = { workspace = true }
+sylvia          = { workspace = true }
+thiserror       = { workspace = true }
+osmosis-std = "0.20.1"


maurolacy · 2023-11-01T05:35:13Z

packages/apis/src/ibc/packet.rs

+#[cw_serde]
+pub enum PriceFeedProviderPacket {
+    Update { twap: Decimal },
+}


Good start. Some ideas / suggestions:

Listing the token pair the price is for, for robustness.

Adding a timestamp.

Adding a source / origin. It can be just a description. Good for logging / tracking.

contracts/osmosis-price-provider/src/state.rs

contracts/osmosis-price-provider/src/contract.rs

contracts/consumer/remote-price-feed/src/contract.rs

uint · 2023-11-01T08:42:34Z

Initial review of the price provider. Will take a look at the consumer next.

Hey @maurolacy! I appreciate the review, but there's no need to go so in-depth when this is still a draft, pending a cleanup. I feel like the energy is better spent taking a high level view and verifying if this is what we want. I'll address the comments on Monday. Till then!

maurolacy · 2023-11-02T07:16:35Z

Thanks for the pointer. Put some comments there as well.

codecov-commenter · 2023-11-08T18:02:13Z

Codecov Report

Attention: 321 lines in your changes are missing coverage. Please review.

Comparison is base (c20eecc) 88.40% compared to head (1dfb124) 87.18%.
Report is 16 commits behind head on main.

Files	Patch %	Lines
contracts/consumer/remote-price-feed/src/ibc.rs	0.00%	105 Missing ⚠️
contracts/osmosis-price-provider/src/ibc.rs	0.00%	92 Missing ⚠️
...ntracts/consumer/remote-price-feed/src/contract.rs	0.00%	60 Missing ⚠️
contracts/osmosis-price-provider/src/contract.rs	0.00%	30 Missing ⚠️
contracts/consumer/remote-price-feed/src/msg.rs	0.00%	14 Missing ⚠️
...racts/consumer/remote-price-feed/src/bin/schema.rs	0.00%	7 Missing ⚠️
...tracts/consumer/remote-price-feed/src/scheduler.rs	92.06%	0 Missing and 5 partials ⚠️
contracts/consumer/remote-price-feed/src/state.rs	0.00%	2 Missing ⚠️
packages/apis/src/ibc/packet.rs	0.00%	2 Missing ⚠️
contracts/consumer/remote-price-feed/src/error.rs	0.00%	1 Missing ⚠️
... and 3 more

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #155      +/-   ##
==========================================
- Coverage   88.40%   87.18%   -1.22%     
==========================================
  Files          69       80      +11     
  Lines       13151    14264    +1113     
  Branches    13151    14264    +1113     
==========================================
+ Hits        11626    12436     +810     
- Misses        992     1310     +318     
+ Partials      533      518      -15

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

uint · 2023-11-08T21:52:57Z

Alright, I won't stall more. I'll try to add some tests in a separate PR for what I can, although there's very little non-IBC logic here.

Co-authored-by: Mauro Lacy <[email protected]>

maurolacy

Pretty good. Looking forward to some tests, and we can merge.

packages/bindings/src/sudo.rs

contracts/consumer/remote-price-feed/README.md

contracts/osmosis-price-provider/src/error.rs

maurolacy · 2023-11-14T08:35:12Z

contracts/osmosis-price-provider/src/contract.rs

+        Decimal::from_str(
+            &querier
+                .arithmetic_twap_to_now(pool_id, base.into(), quote.into(), None)?
+                .arithmetic_twap,
+        )


Looks good. Let's confirm this with the Osmosis team at some point, and adjust if needed.

maurolacy · 2023-11-14T08:44:36Z

contracts/consumer/remote-price-feed/src/contract.rs

+        self.last_epoch
+            .save(ctx.deps.storage, &Timestamp::from_seconds(0))?;


Does this mean 'never'? You can as well save the current block time here.

I think this is better, as it would schedule a price update at the first en block handler call after instantiation, right?

No, this is the time of the last update. 0 stands for year 1970 and is pretty much guaranteed to mean "out of date", so that an update is scheduled after instantiation. If we change it to "now", the next update will be at now + epoch_in_secs, so it might introduce an unnecessary lag if anything.

TBH if instead of using "end block" we can configure an interval in the native Cosmos module, this whole "epoch length" dance becomes unnecessary here.

Yes, but it doesn't hurt either, and allows for different contracts to have different update periods while using the same SDK.

It's also slightly more robust.

maurolacy · 2023-11-14T08:47:38Z

contracts/consumer/remote-price-feed/src/contract.rs

+    pub epoch_in_secs: Item<'static, u64>,
+    pub price_info_ttl_in_secs: Item<'static, u64>,


Makes sense. Setting a TTL that is slightly larger than the epoch will help with handling delays in the price update process.

Perhaps all this can be derived from a single parameter. Including the IBC timeout as well. But, let's leave that for another iteration.

contracts/consumer/remote-price-feed/src/contract.rs

Co-authored-by: Mauro Lacy <[email protected]>

uint requested a review from maurolacy October 26, 2023 16:24

uint force-pushed the 99-osmosis-price-oracle branch 2 times, most recently from 2585e06 to 305ee37 Compare October 26, 2023 16:27

maurolacy reviewed Nov 1, 2023

View reviewed changes

contracts/consumer/remote-price-feed/src/contract.rs Outdated Show resolved Hide resolved

contracts/consumer/remote-price-feed/src/contract.rs Outdated Show resolved Hide resolved

uint force-pushed the 99-osmosis-price-oracle branch 2 times, most recently from 18e9c34 to e21e090 Compare November 8, 2023 17:57

uint requested a review from maurolacy November 8, 2023 21:53

uint marked this pull request as ready for review November 8, 2023 21:53

uint and others added 11 commits November 13, 2023 12:54

scaffold osmosis-price-provider

017f2ad

osmosis-price-provider: implement subscriptions

76f1dd0

osmosis-price-provider: send out updates

b8a3abd

implement a basic remote-price-feed

67f569b

osmosis-oracle: better subscription process

e2c7078

redesign remote price oracle

653418f

remote-price-feed: handle outdated price data

de1dd23

cleanup

736e4f8

Update contracts/osmosis-price-provider/Cargo.toml

3bf001b

Co-authored-by: Mauro Lacy <[email protected]>

remote-price-feed: make epoch and ttl configurable

b27aa9b

fix after rebase

bac07f7

uint force-pushed the 99-osmosis-price-oracle branch from c45ac2c to bac07f7 Compare November 13, 2023 12:55

maurolacy approved these changes Nov 14, 2023

View reviewed changes

uint and others added 4 commits November 14, 2023 19:08

Merge branch 'main' into 99-osmosis-price-oracle

82f5c1e

address code review comments

14dfe9b

Update contracts/consumer/remote-price-feed/README.md

36841f2

Co-authored-by: Mauro Lacy <[email protected]>

refactor and test epoch scheduling

1dfb124

lints

3f5759f

uint force-pushed the 99-osmosis-price-oracle branch from f2367a2 to 3f5759f Compare November 15, 2023 16:22

uint added 2 commits November 15, 2023 17:35

cosmetic

ead36e4

refactor and test price keeping logic

112693b

uint merged commit 10c4a02 into main Nov 16, 2023
4 checks passed

uint deleted the 99-osmosis-price-oracle branch November 16, 2023 15:43

uint restored the 99-osmosis-price-oracle branch November 16, 2023 15:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Osmosis price oracle #155

Osmosis price oracle #155

uint commented Oct 26, 2023 •

edited

Loading

maurolacy left a comment

maurolacy Nov 1, 2023

maurolacy Nov 1, 2023

uint commented Nov 1, 2023

maurolacy commented Nov 2, 2023

codecov-commenter commented Nov 8, 2023 •

edited

Loading

uint commented Nov 8, 2023

maurolacy left a comment

maurolacy Nov 14, 2023

maurolacy Nov 14, 2023 •

edited

Loading

uint Nov 14, 2023 •

edited

Loading

uint Nov 14, 2023

maurolacy Nov 15, 2023 •

edited

Loading

maurolacy Nov 14, 2023

		self.last_epoch
		.save(ctx.deps.storage, &Timestamp::from_seconds(0))?;

		pub epoch_in_secs: Item<'static, u64>,
		pub price_info_ttl_in_secs: Item<'static, u64>,

Osmosis price oracle #155

Osmosis price oracle #155

Conversation

uint commented Oct 26, 2023 • edited Loading

maurolacy left a comment

Choose a reason for hiding this comment

maurolacy Nov 1, 2023

Choose a reason for hiding this comment

maurolacy Nov 1, 2023

Choose a reason for hiding this comment

uint commented Nov 1, 2023

maurolacy commented Nov 2, 2023

codecov-commenter commented Nov 8, 2023 • edited Loading

Codecov Report

uint commented Nov 8, 2023

maurolacy left a comment

Choose a reason for hiding this comment

maurolacy Nov 14, 2023

Choose a reason for hiding this comment

maurolacy Nov 14, 2023 • edited Loading

Choose a reason for hiding this comment

uint Nov 14, 2023 • edited Loading

Choose a reason for hiding this comment

uint Nov 14, 2023

Choose a reason for hiding this comment

maurolacy Nov 15, 2023 • edited Loading

Choose a reason for hiding this comment

maurolacy Nov 14, 2023

Choose a reason for hiding this comment

uint commented Oct 26, 2023 •

edited

Loading

codecov-commenter commented Nov 8, 2023 •

edited

Loading

maurolacy Nov 14, 2023 •

edited

Loading

uint Nov 14, 2023 •

edited

Loading

maurolacy Nov 15, 2023 •

edited

Loading