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.
| properties[3] = buildProperty(3, encodeUint16(5)); // publisherCount | ||
| properties[4] = buildProperty(4, encodeInt16(-8)); // exponent | ||
| properties[5] = buildProperty(5, encodeInt64(50000)); // confidence | ||
| properties[6] = buildProperty(6, encodeInt64(123456)); // fundingRate |
There was a problem hiding this comment.
This is not correct because there is a presence byte (as noted above).
It would be good to fetch real price and funding rate updates from Lazer staging and parse them in the tests.
| bytes calldata payload, | ||
| address signer, |
There was a problem hiding this comment.
is there a need to return these?
| struct Feed { | ||
| // Slot 1: 4 + 4 + 8 + 8 + 8 = 32 bytes (fully packed) | ||
| uint32 feedId; | ||
| uint32 existsFlags; // Bitmap: bit 0-31 for up to 32 properties |
There was a problem hiding this comment.
just want to raise that exceeding this might break our upgradability for Feed struct.
| 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)
| 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?
|
|
||
| // 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.
| int64 bestBidPrice; | ||
| int64 bestAskPrice; | ||
| // Slot 2: 8 + 8 + 8 + 8 = 32 bytes (fully packed) | ||
| int64 confidence; |
There was a problem hiding this comment.
confidence is always positive, uint64
| property == PythLazerStructs.PriceFeedProperty.Confidence | ||
| ) { | ||
| (feed.confidence, pos) = parseFeedValueInt64(payload, pos); | ||
| if (feed.confidence != 0) |
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?