-
Notifications
You must be signed in to change notification settings - Fork 231
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
feat(pyth-lazer-sdk): add funding rate #2423
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
3 Skipped Deployments
|
Exponent(i16), | ||
Confidence(Option<Price>), | ||
FundingRate(Option<i64>), | ||
FundingTimestamp(Option<u64>), |
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 is a funding timestamp? Why is the main timestamp of the update not sufficient?
I also suggest creating a newtype called Rate
which works similar to Price
instead of using raw i64
. We can also use TimestampUs
for the timestamp here.
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.
funding timestamp is the timestamp of the funding rate which is different from the timestamp we have in the aggregated update (which is the time we created the aggregate). Also the timestamp needs to be different per feed as each funding rate has different intervals.
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.
But do the users care about that timestamp? Why can't they use the timestamp of the aggregate?
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 it's important to have the timestamp. It needs to be verified too, for example if publishers don't see the latest data that just came out we will send an update with a timestamp for the next period but with the data of the previous rate.
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.
It will be hard to explain to consumers why there are two different timestamps and how they should use them. Consumers shouldn't be required to even be aware of funding periods, and it's our responsibility to make sure that our data does not contain stale values. To protect against stale values, it makes more sense to require publishers to provide the expiration timestamp for their update. Then we can clear expired updates on aggregation. This way consumers wouldn't need to worry about that at all so they wouldn't need to check a separate timestamp. Actually, if we're choosing to add a timestamp field, the rate expiration timestamp would be more useful to users than the start of the period.
Is it possible to fetch the funding rate for a new period ahead of time? Ideally we should be serving the new rate immediately when the new period starts. If it's not possible, I guess the best action would be to return None until we fetch the new rate.
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 it's better to keep the data similar to the funding rate apis of the exchanges. It will be easier to explain it to users that the data coming is exactly the same as binance api for example. The timestamp is the actual time that the funding happened, it makes sense to have it in the data. If we go with the expiration date instead, it will be more confusing to the consumers as we have to explain what this value exactly represents.
Getting the funding rate for next interval is impossible as it will change when users buy/see futures. I checked to see if there are any apis to get the instantaneous funding rate (i.e the next funding rate if nothing changes until next interval), but i couldn't find any.
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.
Some comments but otherwise, looks good.
Exponent(i16), | ||
Confidence(Option<Price>), | ||
FundingRate(Option<Rate>), | ||
FundingTimestamp(Option<TimestampUs>), |
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.
How is this different from the timestamp of the update itself?
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.
Never mind this is answered above.
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.
unding timestamp is the timestamp of the funding rate which is different from the timestamp we have in the aggregated update (which is the time we created the aggregate). Also the timestamp needs to be different per feed as each funding rate has different intervals.
} | ||
|
||
fn read_option_rate<BO: ByteOrder>(mut reader: impl Read) -> std::io::Result<Option<Rate>> { | ||
let present = reader.read_u8()? != 0; |
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.
This is what you were discussing about the onchain payload right? We need to implement this function on contract?
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.
Yes, the contract needs to update to support this.
pub best_ask_price: Option<Price>, | ||
/// Last known value of the funding rate of this feed. | ||
/// `None` if no value is currently available. | ||
pub funding_rate: Option<Rate>, |
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 about funding timestamp?
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 reused the source_timestamp as funding timestamp.
@@ -480,9 +522,11 @@ impl ParsedFeedPayload { | |||
price: data.price, | |||
best_bid_price: data.best_bid_price, | |||
best_ask_price: data.best_ask_price, | |||
publisher_count: data.publisher_count, | |||
publisher_count: Some(data.publisher_count), |
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.
Why does this need a Some() now if we changed this to no be an Option? I think the parsed payload type needs to also be updated.
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.
all the properties in parsedFeedPayload need to be option because it depends on the properties a consumer request.
@@ -12,7 +12,33 @@ use { | |||
/// from the publisher to the router. | |||
#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] | |||
#[serde(rename_all = "camelCase")] | |||
pub struct PriceFeedData { | |||
pub struct PriceFeedDataV2 { |
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.
Wait, how will V1/V2 be communicated in the binary? Do we need to add a magic?
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.
there are different urls in the relayers for these 2.
Summary
This PR adds the required stuff for funding rates in pyth lazer sdk.
This PR also fixes serde of publisher count which was done incorrectly.
Rationale
This changes are required to support funding rates.
How has this been tested?
Manually testes the code with funding rate publisher through both WS and Latest Price API.