feat(pyth-lazer-protocol)!: enable subscribing by either price_feed_ids or symbols #3049
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 7 Skipped Deployments
|
| return Err("no price feed ids specified"); | ||
| if value.price_feed_ids.is_none() && value.symbols.is_none() { | ||
| return Err("either price feed ids or symbols must be specified"); | ||
| } |
There was a problem hiding this comment.
I think we should also forbid passing both feed ids and symbols at the same time.
There was a problem hiding this comment.
Yeah makes sense. I was debating between this and union-ing them in the server. But yeah i'll go ahead forbid passing both feed ids and symbols at the same time.
| // "ignoreInvalidFeedIds" was renamed to "ignoreInvalidFeeds". "ignoreInvalidFeedIds" is still supported for compatibility. | ||
| #[serde(default, alias = "ignoreInvalidFeedIds")] | ||
| pub ignore_invalid_feeds: bool, |
There was a problem hiding this comment.
So this can be omitted altogether (defaulting to false) or one of ignoreInvalidFeedIds or ignoreInvalidFeeds can be specified? What about both?
There was a problem hiding this comment.
Yeah this applies to the deserializer (e.g., from a JSON payload). Fields named ignoreInvalidFeedIds (the old name) will still be accepted, and will be aliased/renamed to the new ignoreInvalidFeeds. I'm actually not sure what happens if both are provided -- i assume serde would fail to deserialize the payload with some kind of "duplicate keys" error.
|
No objections. Guess there's no way around being messy with an API change that needs to retain compatibility. |
| pub price_feed_ids: Vec<PriceFeedId>, | ||
| // Either price feed ids or symbols must be specified. | ||
| pub price_feed_ids: Option<Vec<PriceFeedId>>, | ||
| pub symbols: Option<Vec<String>>, |
There was a problem hiding this comment.
I think this is a new field here. Shouldn't we add default value to None, to make sure api is backward compatible?
Maybe it's okay by default but I just wanna make sure if that's the case
There was a problem hiding this comment.
serde_json already defaults to None for an Option field if it's absent, so there is no need for #[serde(default)] in this case.
tejasbadadare
dismissed stale reviews from danimhr and Riateche
via
5ad7be9
tejasbadadare
force-pushed
the
tb/pyth-lazer-protocol/subscribe-by-symbols
branch
from
d2de4ca to
54cfac6
Compare
tejasbadadare
deleted the
tb/pyth-lazer-protocol/subscribe-by-symbols
branch
Summary
symbols: Option<Vec<String>>optional request parameter toSubscriptionParams,LatestPriceRequest,PriceRequestand makesprice_feed_idsan Option instead of being required. This enables specifying feeds by symbol rather than ID if desired.ignore_price_feed_idsis backward-compatibly renamed toignore_price_feedsso that it can be used regardless of whether the feeds are specified bysymbolsorprice_feed_ids.symbolsinto IDs in the router.Rationale
We want to support fetching latest price/subscribing to feeds via symbols, not just IDs. This is more user friendly for people trying out the system, as it eliminates the need to look up the mapping from symbol to ID. Paired with the new symbology, users should be able to discover feeds predictably.
How has this been tested?