add functions to parse vaa to price info #37
Conversation
pythclient/price_feeds.py
Outdated
| @@ -0,0 +1,430 @@ | |||
| # Classes and functions here are referenced from pyth-crosschain repo | |||
There was a problem hiding this comment.
suggest linking to the specific file that has these definitions
There was a problem hiding this comment.
was apprehensive doing this since the original source files could change but yeah we could do it
pythclient/price_feeds.py
Outdated
| return json.dumps(result) | ||
|
|
||
|
|
||
| def encode_vaa_for_chain(vaa, target_chain): |
There was a problem hiding this comment.
Let's think through how we want to expose the encoding/target_chain in the API. I think the current "target_chain" parameters are a little questionable (yes, I know I made them in the first place, but I'm not sure it was a good idea.)
| if offset != len(bytes_): | ||
| raise ValueError(f"Invalid length: {len(bytes_)}, expected: {offset}") | ||
|
|
||
| return { |
There was a problem hiding this comment.
why does this return a dictionary of one element?
There was a problem hiding this comment.
I followed the implementation here: https://github.com/pyth-network/pyth-crosschain/blob/main/wormhole_attester/sdk/js/src/index.ts#L122-L188
There was a problem hiding this comment.
oh... maybe leave a comment to that effect? I'm not sure why that function does it that way either.
There was a problem hiding this comment.
This is because that the returned struct was BatchPriceAttestation
pythclient/price_feeds.py
Outdated
| return None | ||
|
|
||
| for price_attestation in batch_attestation["price_attestations"]: | ||
| if price_attestation["price_id"] == price_feed_id: |
There was a problem hiding this comment.
(optional) you may want to make classes for all of these things. it's less error prone to access class members than keys of a dictionary like this. ofc it's also more code so there is a tradeoff there and i'm not sure which way it leans in this case.
pythclient/price_feeds.py
Outdated
| } | ||
|
|
||
|
|
||
| def vaa_to_price_info(price_feed_id, vaa) -> PriceInfo: |
There was a problem hiding this comment.
this function signature seems questionable to me, as it's doing too much. when you parse a vaa, you should get an object that has a collection of PriceInfos in it, and then the caller can search within that list for a specific feed id. those should be separate pieces of functionality that can be composed, not conflated together in this one function signature.
There was a problem hiding this comment.
making classes per the comment below would actually help you separate out the logic into composable bits.
There was a problem hiding this comment.
one more note here for the future: we need to come up with a term for "a binary blob of data that you can post on-chain to update the pyth price". We are currently calling these "vaas" but I don't think that's right anymore in the accumulator world.
There was a problem hiding this comment.
do you mean creating a separate function like vaa_to_price_infos which returns a List[PriceInfo] and then make vaa_to_price_info call vaa_to_price_infos and return the PriceInfo from the list where it has the specified id?
There was a problem hiding this comment.
it depends on how often people want to use vaa_to_price_info. If that's a common function that people want, then yes, you can have it. Otherwise, people can call find on the list returned by vaa_to_price_infos.
classes and functions here are referenced from pyth-crosschain repo and wormhole js sdk