feat(lazer-protocol): add Metadata V3 response types #3111
Conversation
tejasbadadare
requested review from
darunrs,
Riateche and
ali-behjati
as code owners
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
| /// Commodity | ||
| Commodity, | ||
| /// Funding rate | ||
| FundingRate, |
There was a problem hiding this comment.
is this really an asset class?
There was a problem hiding this comment.
Hmmm.. we currently consider it an asset class in the V1 API, but i agree that conceptually it doesn't really fit. We can introduce an AssetClassV3 that excludes these.
But then the question is what we'd set for asset_class for funding rate feeds. Maybe it's reasonable to leave it as FundingRate? Or we can set it to the underlying asset's class. Will check with @YaserJazouane on this.
There was a problem hiding this comment.
Caught up, yes funding rate is not an asset class. It'll be a feed_kind (as it already is).
| /// High-level asset class. | ||
| #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, Serialize, Deserialize)] | ||
| #[serde(rename_all = "kebab-case")] | ||
| pub enum AssetClass { |
There was a problem hiding this comment.
One of the reasons for opting in a dynamic metadata was to not go through a code change when new asset classes are added to the system. I know that users (on both sides) rely on these, and that's probably why you opted for explicit definition here. But if that's the case, you might actually remove any dynamic field and make everything very explicit. Being in the middle (some explicit metadata, some implicit dynamic) is probably worse.
There was a problem hiding this comment.
I think we should stick with our decision and use fully dynamic metadata in the protocols. In Rust that would be BTreeMap<String, serde_value::Value>. We can revisit it later if we feel like the metadata structure is very stable and future-proof, but I doubt that it will happen soon.
There was a problem hiding this comment.
Yeah my main goal in adding these explicit types is to ensure end users can depend on a stable API contract across different versions. I.e. the different types like the existing SymbolResponse and the new FeedResponseV3 can handle the differences between v1 and v3 representations derived from the same internal dynamic metadata representation.
| /// Unique human-readable identifier for a feed. | ||
| /// Format: `source.instrument_type.base/quote` | ||
| /// Examples: `"pyth.spot.btc/usd"`, `"pyth.redemptionrate.alp/usd"`, `"binance.fundingrate.btc/usdt"`, `"pyth.future.emz5/usd"` | ||
| pub symbol: String, |
There was a problem hiding this comment.
i recommend opting for a specific type with validation here to ensure it's correct.
q: what if the asset has no quote asset? (such as rates)
There was a problem hiding this comment.
A type with validation to enforce the string format is a good idea.
Not sure about a feed with no quote asset. If such a feed is valid it poses a deeper problem with the data model, since it revolves around feed being an asset/quote pair. Fwiw currently funding rates are USD denominated.
There was a problem hiding this comment.
Okay caught up with @YaserJazouane on this and yep we should make the quote asset optional. There are cases like yields, indices, fundingrates that aren't quoted in any currency. Our existing funding rate example is denominated in USD which is incorrect.
tldr:
- We're amending the format to be
source.instrument_type.base[/quote]where the quote asset is optional - We're making
quote_asset_idoptional
| /// CoinMarketCap asset identifier. | ||
| /// Example: `"123"` | ||
| #[serde(skip_serializing_if = "Option::is_none")] | ||
| pub cmc_id: Option<String>, |
There was a problem hiding this comment.
i wonder whether you considered @jayantk suggestion on having a nested struct as "external mappings". It's probably more difficult on the DB layer.
There was a problem hiding this comment.
Yeah i did consider it, but i figured that a nested struct in the dynamic metadata opened the door to making its own table to ensure proper types in the DB. FIgured it was simpler to leave it flat.
| pub feed_expiry: Option<String>, | ||
| /// The nature of the data produced by the feed. | ||
| /// Examples: `"price"`, `"fundingRate"` | ||
| pub feed_kind: FeedKind, |
There was a problem hiding this comment.
Hmmm so we do have a feedkind and an instrument type. I wonder whether we can merge them.
There was a problem hiding this comment.
I think they're capturing slightly different things.
Afaik feed_kind is meant to be the coarsest discriminator of feeds, which we use to trigger different aggregation logic (price feeds, non price feeds like funding rates). Whereas instrument_type is a bit more specific, and describes the pricing context (spot price, averaged price, etc.).
There was a problem hiding this comment.
Consensus on this topic was that the two fields capture different things as mentioned above, so planning on keeping it this way.
…String to InstrumentType for string parsing, add tests
… make SymbolV3.quote optional
Summary
Add types for output representations of the Metadata v3 API.
FeedResponseV3: Returned by the/v3/feedsAPIsAssetResponseV3: Returned by the/v3/assetsAPIs/v1/symbolsAPI will continue to return thehistory_service::api::SymbolResponsefrom Lazer internals for the time being. In the future, this type should be unified with the duplicated one in the protocol crate (pyth_lazer_protocol::jrpc::SymbolMetadata, used by History client).How has this been tested?