-
Notifications
You must be signed in to change notification settings - Fork 15
feat(lazer_exporter): Add lazer exporter #155
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
Conversation
best_ask_price: None, | ||
best_bid_price: None, |
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.
can we use conf as bid and ask?
cc: @ali-bahjati
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.
I don't know. Typically conf (although we ask for it) is wider than bid-ask by publishers and it makes me a bit worried. let me ask internally.
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.
i still prefer None here just because bid ask might not be symmetric. we can do it on a newer update protocol.
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.
Since we are calculating conf with bid/ask, this will be exactly what we are calculating in pythnet. But the bid and ask themselves might get skewed.
src/agent/services/lazer_exporter.rs
Outdated
None => { | ||
tracing::error!("relayer connection closed"); | ||
return; | ||
} |
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.
does the thread halt when we get an error?
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, was wondering if that should crash the agent altogether or have retries in-process.
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 have a few retries before crashing altogether. Also I'm not sure if we should crash at all because for example if lazer goes down the publishers will stop publishing to pythnet too.
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.
cc: @ali-bahjati
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.
the behaviour on agent is to aggressively reconnect upon failures for Pythnet without crashing at all and i think it should be the same for Lazer. It's like a software you run once and it works all the time and should have lower operational overhead.
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.
Nice! I'm happy that you left some works for future (retry/reliability and continuous state polling). I'll approve if things apart from that get resolved.
src/agent/services/lazer_exporter.rs
Outdated
#[derive(Deserialize)] | ||
struct SymbolResponse { | ||
pub pyth_lazer_id: u32, | ||
pub name: String, | ||
pub symbol: String, | ||
pub description: String, | ||
pub asset_type: String, | ||
pub exponent: i32, | ||
pub cmc_id: Option<u32>, | ||
pub interval: Option<String>, | ||
pub min_publishers: u16, | ||
pub min_channel: String, | ||
pub state: String, | ||
pub hermes_id: Option<String>, | ||
} |
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.
maybe a leave a todo to move it to Lazer's protocol SDK?
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.
the types inside are also not great. we have String for many things but they are more explicit.
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.
I copied from https://github.com/pyth-network/pyth-lazer/blob/main/history-service/src/api.rs#L91 ; sorry, not clear what they should be.
best_ask_price: None, | ||
best_bid_price: None, |
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.
I don't know. Typically conf (although we ask for it) is wider than bid-ask by publishers and it makes me a bit worried. let me ask internally.
loop { | ||
tokio::select! { | ||
_ = publish_interval.tick() => { | ||
for (identifier, price_info) in state.get_all_price_infos().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.
since we want to run this in high frequency i want to make sure this doesn't become expensive as it might copy the data.
best_ask_price: None, | ||
best_bid_price: None, |
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.
i still prefer None here just because bid ask might not be symmetric. we can do it on a newer update protocol.
ah another comment, please fix CI (formatting/..) and clippy warnings. |
I think we should roll out the new agent when we have publisher's signature and agent is working with signatures rather than access tokens. Giving a prod token to all publishers in pythnet might not be a good idea and we will have to ask them to change it soon. |
@keyvankhademi yeah sure thing. It's still better to make changes in multiple PRs and merge this one. we can merge the code and mark it as alpha (@merolish let's do it). no one can publish data until tokens are given out. |
for (identifier, price_info) in state.get_all_price_infos().await { | ||
if let Some(symbol) = lazer_symbols.get(&identifier.to_string()) { | ||
if let Err(e) = relayer_sender.send_price_update(&PriceFeedDataV1 { | ||
price: Some(Price(NonZeroI64::try_from(price_info.price).unwrap())), |
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.
what if the price is zero? are we sure it won't happen?
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.
Excellent question, no idea, and I was curious on this enforcement in the Lazer messages. In the general case there are instruments out there (e.g. futures spreads) that can have zero or negative prices, but in terms of anything that anyone will reasonably send to agent, perhaps just warning log and drop a zero price message?
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.
I think we support negative but just not zero. This came from Pythnet.
What i like you to do is at least not crash the whole process when it happens and gracefully handle it instead.
@merolish Btw, looking at this PR again. Can you update the config.toml reference in your future PRs. |
Oops, sorry about that. |
Add an exporter that periodically polls local state and publishes updates to Lazer relayers. Note that this depends on the symbols endpoint including the Hermes ID so we can map to Lazer ID.