-
Notifications
You must be signed in to change notification settings - Fork 255
feat(argus): ControllerService update loop, ChainPriceService poll loop #2693
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
base: main
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
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.
Looks very good! I left some comments, please address them before merging the code.
.update_prices(subscription_id, prices_map); | ||
} | ||
Err(e) => { | ||
// If we failed to get prices for a subscription, we'll retry on the next poll interval. |
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.
this implicitly enforces having a high poll interval
let subscription_id = item.key().clone(); | ||
let subscription_params = item.value().clone(); | ||
|
||
// TODO: do this in parallel using tokio tasks? |
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.
yeah let's do it.
todo!() | ||
subscription_id: SubscriptionId, | ||
) -> Result<Vec<Price>> { | ||
let price_ids = self.get_prices_unsafe(subscription_id, vec![]).await?; |
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.
vec![]
seems wrong.
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.
actually it seems that it's part of the api. maybe document it in the IScheduler? or Ipulse
@@ -64,7 +115,13 @@ impl Service for ChainPriceService { | |||
loop { | |||
tokio::select! { | |||
_ = interval.tick() => { | |||
self.poll_prices(self.chain_price_state.clone()).await; | |||
if let Err(e) = self.poll_prices().await { |
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.
poll_prices doesn't seem to ever fail (maybe change its signature?)
service = self.name, | ||
error = %e, | ||
"Failed to perform price update" | ||
); |
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 retry here?
// If there's no price currently on the chain for this feed, an update is always needed. | ||
let chain_price = match chain_price_opt { | ||
None => { | ||
tracing::debug!("Update criteria met: No chain price available."); |
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.
in price pusher sometimes i found these logs helpful, might not be bad to have them at info level, or maybe expose as metrics or trace
// the chain price plus `heartbeat_seconds`. | ||
if update_criteria.update_on_heartbeat { | ||
if pyth_price.publish_time | ||
>= chain_price.publish_time + (update_criteria.heartbeat_seconds as i64) |
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.
let's never use + , instead use saturated_add
to make sure we never overflow (otherwise it might result in some security issues)
let threshold_val = (chain_price.price.abs() as u64 | ||
* update_criteria.deviation_threshold_bps as u64) | ||
/ 10000; | ||
|
||
if price_diff > threshold_val { |
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.
let's double check this matches the onchain logic 100% (it looked the same when i looked)
if pyth_price.price != 0 { | ||
tracing::debug!( | ||
"Deviation criteria met: Chain price is 0, Pyth price is non-zero." | ||
); | ||
return true; |
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.
this is not in the on-chain logic, however the on-chain logic accepts any update if there's no update, maybe it is this one?
Summary
How has this been tested?
needs_update
, which tests scenarios around update criteria. Given a pyth price, chain price, and update criteria, the function returns a boolean indicating whether the subscription should be updated or not.