feat(hip-3-pusher): Configurable price sources #3169
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Maybe its just me, but i get nervous when i see a config.toml committed to git, even though I know its an example config :P The convention in the rest of the codebase is to name it config.sample.toml to disambiguate, i think we should follow that here too
| value: str | ||
|
|
||
|
|
||
| PriceSourceConfig = Union[SingleSourceConfig, PairSourceConfig, ConstantSourceConfig] |
There was a problem hiding this comment.
FYI in python 3 you don't need to use the Union type, you can just express this as SingleSourceConfig | PairSourceConfig | ConstantSourceConfig for a more concise syntax.
There was a problem hiding this comment.
This is a "you're absolutely right" moment, cough.
| self.price_state.hermes_base_price = PriceUpdate(price, now) | ||
| if id == self.quote_feed_id: | ||
| self.price_state.hermes_quote_price = PriceUpdate(price, now) | ||
| self.price_state.state[self.SOURCE_NAME][id] = PriceUpdate(price, now) |
There was a problem hiding this comment.
It feels a bit strange to me that we are using a mono-state, but indexing into it with "source name" to distinguish between states of different modules. It would be safer and more explicit to have distinct state objects (which you can still nest within the mono-state, like how Hermes does it,) something like HermesState / LazerState / HLState. The state objects can be passed to the Listener objects during instantiation, which guarantees a listener is scoped to its specific state, and can only mutate the data within.
In the price consumers, you can also more safely reference price_state.hermes_state instead of price_state[self.SOURCE_NAME]. The state objects could also live inside of the Listener classes themselves to reduce indirection if desired.
There was a problem hiding this comment.
I like it, will change.
|
|
||
| def __str__(self): | ||
| return f"PriceUpdate(price={self.price}, timestamp={self.timestamp})" | ||
| price: float | str |
There was a problem hiding this comment.
Float or str? Should we conform to one or the other?
There was a problem hiding this comment.
HL messages have prices as a string-of-a-float; Lazer and Hermes messages have price as a string-of-an-int, and I was dividing by the exponent and keeping it as a float to not lose precision. We could keep the exponent in the PriceUpdate as well, perhaps.
| def __str__(self): | ||
| return f"PriceUpdate(price={self.price}, timestamp={self.timestamp})" | ||
| price: float | str | ||
| timestamp: float |
There was a problem hiding this comment.
Timestamps should probably be integer (or datetime)
There was a problem hiding this comment.
In this case I've been using time.time() which is indeed a float, I guess to allow differing precisions?
| def get_prices(self, symbol_configs: dict[str, list[PriceSourceConfig]], market_name: str): | ||
| pxs = {} | ||
| for symbol in symbol_configs: | ||
| for source_config in symbol_configs[symbol]: | ||
| # find first valid price in the waterfall | ||
| px = self.get_price(source_config) |
There was a problem hiding this comment.
Could you please check that our unit tests sufficiently test the new waterfall logic?
There was a problem hiding this comment.
The tests can definitely stand to be expanded.
| source: PriceSource | ||
|
|
||
|
|
||
| class PairSourceConfig(BaseModel): |
There was a problem hiding this comment.
I guess generally we would leave "combining feeds" up to SEDA? Or would we use this PairSourceConfig for it?
There was a problem hiding this comment.
Set this up specifcally for the case (as in our internal publishers) where we need to quote in e.g. USDT. For more complicated transformations yes, hopefully SEDA can abstract it out.
There was a problem hiding this comment.
It's a bit difficult to follow what's happening in these tests, do you mind adding comments that explain the setup , action, and expected result? Even better would be to also update the test names to encode this information, e.g. test_lazer_price_falls_back_to_hyperliquid_upon_stale_price.
Summary
Configurable price source waterfall for oracle/mark/external prices for a particular HIP-3 dex.
Rationale
Supporting multiple HIP-3 dex customers.
How has this been tested?