chore(lazer) Added Feed struct #3135
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
| function hasPrice( | ||
| PythLazerStructs.Feed memory feed | ||
| ) public pure returns (bool) { | ||
| return (feed.existsFlags & PythLazerStructs.PRICE_EXISTS) != 0; | ||
| } |
There was a problem hiding this comment.
Should we create getters for the properties that enforce checking existence? It would ensure users dont assume a property like price always exists. Something like getPrice(feed) -> (bool, int64), it could return (exists, value). It's an alternative to storing the type as Option<Option<PropertyType>>.
There was a problem hiding this comment.
There are chances that user can ignore the exist flag. Plus returning a bool will consume more gas.
There was a problem hiding this comment.
As it stands they would have to first call hasPrice before using the price, which to me sounds like a greater chance of being missed by implementors since they would have to know about that method and intentionally use it. On the other hand, the exists flag more clearly signals the possibility that the field may not exist, and one would have to acknowledge this to ignore it. But that's just my opinion
There was a problem hiding this comment.
They can just call getPrice(). It will return an error if price is zero.
| bytes calldata payload, | ||
| address signer, |
There was a problem hiding this comment.
is there a need to return these?
There was a problem hiding this comment.
not sure. following the same pattern as verifyUpdate.
| int64 price; | ||
| int64 bestBidPrice; | ||
| int64 bestAskPrice; | ||
| // Slot 2: 8 + 8 + 8 + 8 = 32 bytes (fully packed) | ||
| int64 confidence; | ||
| int64 fundingRate; | ||
| uint64 fundingTimestamp; | ||
| uint64 fundingRateInterval; | ||
| // Slot 3: 2 + 2 = 4 bytes (28 bytes wasted, but unavoidable) | ||
| uint16 publisherCount; | ||
| int16 exponent; |
There was a problem hiding this comment.
people can directly use these right? i recommend you rename it to maybe<field> to make it clear for people that this is an optional field (and they should use a getter)
There was a problem hiding this comment.
I am replacing them like _price. I believe that are better than maybePrice.
| if (property == PythLazerStructs.PriceFeedProperty.Price) { | ||
| (feed.price, pos) = parseFeedValueInt64(payload, pos); | ||
| if (feed.price != 0) | ||
| feed.existsFlags |= PythLazerStructs.PRICE_EXISTS; |
There was a problem hiding this comment.
Just to be clear, it seems you do not distinguish between lack of a property vs the property being None (as in Lazer doesn't know the price). Is that intentional?
There was a problem hiding this comment.
Yeah, that was intentional. From a user standpoint, if the price is unknown by the lazer, we shouldn't even mention that property exist for timestamp.
I don't have a strong opinion on this. I took the easier approach from both development and user standpoint.
There was a problem hiding this comment.
I think it's important to keep the distinction. A consumer contract may require the presence of a field so that it always uses the value if it's available in Lazer, but it can also have an alternative code path that's only used if Lazer doesn't have a value for this timestamp (which the payload guarantees in that case).
|
|
||
| // Safe getter functions (revert if property doesn't exist) | ||
|
|
||
| /// @notice Get price (reverts if not exists) |
There was a problem hiding this comment.
please update the comments on the failure modes. what does "not exists" mean?
| FundingRateInterval | ||
| } | ||
|
|
||
| struct Feed { |
There was a problem hiding this comment.
add comments either here on what it is and how it should be used.
| property == PythLazerStructs.PriceFeedProperty.Confidence | ||
| ) { | ||
| (feed.confidence, pos) = parseFeedValueInt64(payload, pos); | ||
| if (feed.confidence != 0) |
There was a problem hiding this comment.
If the price doesn't exist, confidence doesn't exist. Which means the None is parsed as 0. Should we just make confidence throw error when price is zero?
There was a problem hiding this comment.
i recommend adding testing via some fixtures from Lazer as well to make sure everything works properly cross-environment.
4 participants
Summary
Rationale
How has this been tested?