feat: add hermes client #2931
feat: add hermes client #2931
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
There was a problem hiding this comment.
Looks like this client is focused specifically on the websockets endpoint. Should we rename it to pyth-hermes-websockets-client instead of pyth-hermes-client which implies that it's a client that implements the Hermes OpenAPI spec?
Also, i believe we want to push the public to use the SSE endpoint, not WS. If we release a client for the (undocumented) WS endpoint it could be confusing for end users. Maybe we keep it privately published?
Sidenote, I actually had Devin generate a rust client a while ago from the OpenAPI spec plus SSE support but never got around to merging and publishing it.
We might add more features to it in future depending on our/clients needs. I don't think we should deprecate the WS anymore as we have seen it has benefits over SSE; that's why we have changed it to ws in circuit breaker. |
| license = "Apache-2.0" | ||
|
|
||
| [dependencies] | ||
| pyth-sdk = { version = "0.8.0" } |
There was a problem hiding this comment.
Should we instead read the hermes structs since it seems it defines things (metadata) that isn't in pyth-sdk?
There was a problem hiding this comment.
i tried that, it doesn't work out of the box and require changes to the hermes server, which I don't think is worth doing.
| /// This struct encapsulates the parameters needed to configure exponential backoff | ||
| /// behavior and can be converted into the backoff crate's [`ExponentialBackoff`] type. | ||
| #[derive(Debug)] | ||
| pub struct HermesExponentialBackoff { |
There was a problem hiding this comment.
What's the value add of defining a wrapper struct around expo backoff? I don't seem to see any additional features or behavior?
There was a problem hiding this comment.
There is a feature that we don't support max_elapsed_time iirc.
| timeout: Duration, | ||
| channel_capacity: usize, | ||
| ) -> Result<Self> { | ||
| if endpoints.is_empty() { |
There was a problem hiding this comment.
We should add a check to ensure num_connections is also zero.
There was a problem hiding this comment.
this causes panic. num connections being 0 i think doesn't panic.
There was a problem hiding this comment.
It could cause strange invisible behavior such as the client running but doing nothing. Given this is a customer facing function, I figure it's good to communicate to them any behavior we don't expect.
| } | ||
|
|
||
| #[derive(Deserialize, Serialize, Debug, Clone, Hash)] | ||
| pub struct HermesPrice { |
There was a problem hiding this comment.
Can you provide doc comments there explaining that price and publish time can be 0 if the feed is disabled, and that the publish time can also be when a market closes, and doesn't change? It's useful behavior to document since it changes how the data can be handled.
There was a problem hiding this comment.
That's a hermes behavior and I think should be documented in hermes api docs.
Yeah true if we are relying on it for Lazer, we should undeprecate it. I'd wanna understand why we initially hid it from public use though and pushed people to use SSE. cc @ali-behjati |
Summary
This PR adds a resilient hermes client that will be published to cargo
Rationale
We use hermes in multiple services, including hermes publisher and circuit breaker in lazer. This crate allows us to easily get data from hermes while having resiliency.
How has this been tested?
Manually ran the code.