exporter: Use market hours information in price filtering #95

drozdziak1 · 2023-11-24T16:51:03Z

Motivation

This change builds on the previously introduced market hours format and applies it to the agent's publishing logic. The extraction, parsing and distribution of the market hours info piggy-backs the existing publisher permissions flow. This felt appropriate because of a shared idea: "Does on-chain data say this is okay to publish?". You'll quickly see that a big part of the changeset is a series of additions to the publisher permissions processing logic. In the same spirit, the changes to state simply add market hours to the detected permissioned symbols that are checked when filtering prices.

Summary of changes

oracle.rs - Attempt to parse market hours when looking up products, fall back to 24/7 if failing po parse. Specifically, print a warning if the market hours field is defined but could not be understood. Additionally, the market hours are passed together with permissioned prices in the existing channel going to exporter.
exporter.rs - Use market hours data in price filtering.
market_hours.rs - Add missing traits to please the compiler; make can_publish_at infallible (turns out the weekday extraction exists in chrono but it required some digging)
test_integration.py - Add all-closed market hours to AAPL_USD and expect publishing to it to fail because of the market hours.

Review Highlights

The new metadata field is named "market_hours"
The fallback behavior for market hours parsing problems is to use 24/7 market hours. In case we fatfinger the field on a symbol, it feels more appropriate to risk publishing it at a wrong time than to bring it down for all pubs when it's supposed to be up. I think it's important for us to agree about this.
The point of reference for market hours is the OS system clock - I decided against using the solana clock since it is known to drift from time to time (there were incidents of sizable lag in the past on Solana's public networks)
test_integration.py - the test is very rudimentary, using an all-closed market hours schedule, expecting the symbol to be unchanged. This is mostly due to agent using system clock to determine the current date/time. Fixturing the system clock feels unnecessary when dedicated edge case testing is available in the unit testsuite. The goal of the integration test is to verify that the code path successfully results in no publishing for the all-closed market hours. In case the symbol's hours data were to be misunderstood/lost, it would presumably fall back to a 24/7 schedule, exposing the problems along the code path. If you have good suggestions for making the test less trivial, it would be nice to discuss that.

jayantk

seems like a solid solution to me. I left a couple requests for testing but feel free to do in follow up PRs.

jayantk · 2023-11-24T21:53:04Z

src/agent/market_hours.rs

@@ -53,13 +54,11 @@ impl MarketHours {
        }
    }

-    pub fn can_publish_at<Tz: TimeZone>(&self, when: &DateTime<Tz>) -> Result<bool> {


is the default timezone on line 46 the same no matter where you run this code? (May be better to explicitly specify UTC or something. I don't think it matters for the always open default market hours, but could be confusing later on)

sorry one more request not related to this PR. Can you add some tests for can_publish_at that specifically focus on the timezone. like the time X in tz A is ok, but X in tz B is not.

and maybe also worth testing days with daylight savings time shifts explicitly

Yeah, I think UTC default could make sense and simplify this part. My intuition was to make sure we are always conscious of the timezone, but that is already guaranteed by using DateTime<Utc> since it won't accept a DateTime<Tz> at compile-time with any other type argument than UTC.

Re: testing - sure, I didn't want to go all out on the edge cases before knowing that we want this

sorry the UTC comment was about line 46 above where you're using Default::default() to populate the all_closed object. I get that the Tz type parameter here makes timezone errors impossible (which is pretty cool actually)

jayantk · 2023-11-24T21:57:09Z

src/agent/solana/oracle.rs

@@ -525,10 +538,29 @@ impl Poller {
                let product = load_product_account(prod_acc.data.as_slice())
                    .context(format!("Could not parse product account {}", product_key))?;

+                let market_hours: MarketHours = if let Some((_mh_key, mh_val)) =
+                    product.iter().find(|(k, _v)| *k == "market_hours")


nit: maybe we can call this field something like "standard_hours" to indicate that it doesn't include holidays etc? Or maybe "weekly_schedule" ?

I think I like weekly_schedule the best, even my code comment for the MarketHours struct kind of points in that direction. I think renaming MarketHours to WeeklySchedule is also appropriate here. As we tackle holidays in the future, we may choose to create an umbrella MarketHours struct with weekly_schedule: WeeklySchedule and e.g. next_holidays: Vec<Holiday>.

jayantk · 2023-11-24T21:58:49Z

integration-tests/tests/test_integration.py

+        await client.update_price(price_account, 81, 1, "trading")
+        time.sleep(2)
+
+        # Confirm that the price account has been updated with the values from the first "update_price" request


this comment seems wrong

jayantk · 2023-11-24T22:00:04Z

integration-tests/tests/test_integration.py

+        Similar to test_update_price_simple, but using AAPL_USD and
+        asserting that nothing is published due to the symbol's
+        all-closed market hours.
+        '''


Ideally you would be able to write a test that had nontrivial hours (so closed sometimes and open others) and then test both cases where it should / shouldn't publish. I'm not sure how easy that is to do though given that you would need to mock the time for the agent.

ali-bahjati · 2023-11-27T14:57:08Z

src/agent/solana/oracle.rs

+                    product.iter().find(|(k, _v)| *k == "market_hours")
+                {
+                    mh_val.parse().unwrap_or_else(|err| {
+			warn!(


The formatting here seems messed up. Can you fix it?

ali-bahjati · 2023-11-27T14:59:11Z

src/agent/solana/oracle.rs

-    /// publisher => {their permissioned price accounts}
-    pub publisher_permissions: HashMap<Pubkey, HashSet<Pubkey>>,
+    /// publisher => {their permissioned price accounts => market hours}
+    pub publisher_permissions: HashMap<Pubkey, HashMap<Pubkey, MarketHours>>,


I am not sure that the market hours fits in a publisher permissions domain. Couldn't it become something separate?

I think I see your point. Maybe instead, we could consider renaming this field from publisher_permissions to publishing_rules or any other name that points generally at the publish/don't publish decision process, and not specifically the publisher permissioning?

ali-bahjati · 2023-11-27T15:03:56Z

src/agent/solana/exporter.rs

+                    let ret = market_hours.can_publish_at(&now);
+
+		    if !ret {
+			debug!(self.logger, "Exporter: Attempted to publish price outside market hours";


The formatting is bad here again because of mixed tab and spaces

Guilty as charged, I had a fairly old nightly locally while our CI grabs a fresh one with each run.

there's a way to pin the version used for formatting which solves this problem. I forget exactly how to do it but we're doing it in pyth-crosschain.

ali-bahjati

In general I recommend the flow to be from oracle to global store and from global store to the exporter to follow the design of agent.

drozdziak1 · 2023-11-28T18:54:13Z

Re: proposed oracle -> global store -> exporter flow.

I'm a bit on the fence about this. I think the proposed flow makes sense, but the existing permissions data flow is also a decent candidate, because of the similar "can I publish this?" concern. Perhaps the solution lies somewhere in the middle. Maybe both permissions and market hours could go through the global store.

exporter: Use market hours information in price filtering

c5bce62

jayantk approved these changes Nov 24, 2023

View reviewed changes

ali-bahjati reviewed Nov 27, 2023

View reviewed changes

drozdziak1 added 2 commits November 28, 2023 17:06

Rename MarketHours->WeeklySchedule, market_hours->weekly_schedule

5ab9e94

market_hours: verify DST behavior using EU and US lag example

9b10d54

market_hours.rs: true positives where markets agree on market hours

a874fe1

jayantk approved these changes Nov 28, 2023

View reviewed changes

drozdziak1 merged commit 3bc2d5a into main Nov 29, 2023
2 checks passed

drozdziak1 deleted the drozdziak1/market-hours-logic branch November 29, 2023 14:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

exporter: Use market hours information in price filtering #95

exporter: Use market hours information in price filtering #95

drozdziak1 commented Nov 24, 2023 •

edited

Loading

jayantk left a comment

jayantk Nov 24, 2023

jayantk Nov 24, 2023

drozdziak1 Nov 27, 2023

jayantk Nov 28, 2023

jayantk Nov 24, 2023

drozdziak1 Nov 27, 2023

jayantk Nov 24, 2023

jayantk Nov 24, 2023

ali-bahjati Nov 27, 2023

ali-bahjati Nov 27, 2023

drozdziak1 Nov 28, 2023

ali-bahjati Nov 27, 2023

drozdziak1 Nov 28, 2023

jayantk Nov 28, 2023

ali-bahjati left a comment

drozdziak1 commented Nov 28, 2023

exporter: Use market hours information in price filtering #95

exporter: Use market hours information in price filtering #95

Conversation

drozdziak1 commented Nov 24, 2023 • edited Loading

Motivation

Summary of changes

Review Highlights

jayantk 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ali-bahjati left a comment

Choose a reason for hiding this comment

drozdziak1 commented Nov 28, 2023

drozdziak1 commented Nov 24, 2023 •

edited

Loading