Skip to content
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

Add aggregate_price_status which takes care of becoming stale #21

Merged
merged 7 commits into from
Feb 16, 2022

Conversation

ali-bahjati
Copy link
Contributor

@ali-bahjati ali-bahjati commented Feb 15, 2022

Addsaggregate_price_status property in Price Account which returns the aggregate price status considering the latest fetch slot to make sure price is not stale. get_aggregate_price_status_with_slot is also added so users can give latest solana slot for checking that price is not stale.

Additional Changes:

  • Price Info slot field is renamed to pub_slot: slot is used in other objects within the pyth client with a different meaning (fetch slot). pub_slot is also consistent with other clients.
  • aggregate_price and aggregate_price_confidence_interval will use status and return None if price is not available (status != trading). Comments have been added to guide how to get these values if needed regardless of availability. This is more consistent with our Rust client api and will prevent incautious users to rely on price if it's not available.
  • In solana module get_commitment_slot is renamed to get_slot to be more consistent with the rest of its interface. Also the return type is updated.
  • Also fixes a small bug in dump example by indenting back the break on ws update handling logic.

@@ -501,6 +502,18 @@ def aggregate_price_confidence_interval(self) -> Optional[float]:
"""the aggregate price confidence interval"""
return self.aggregate_price_info and self.aggregate_price_info.confidence_interval

async def get_aggregate_price_status(self, commitment: str = SolanaCommitment.CONFIRMED) -> Optional[PythPriceStatus]:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be better to pass in the current slot as an argument. If someone has a list of accounts and they want to get the status of each one, it will be expensive to query the API for the current slot multiple times. Also, it means that they can't get the status of all the accounts at a single instant in time, because the API might return different slots each time it is queried.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, but on the other hand it'll become a bit harder for people to use it.
I might create another normal (not async) function for it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think it would be reasonable to have a non-async version where the caller passes in the slot, and also an async version that calls the non-async version.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed async entirely as I realized using the provided slot when object is created is more efficient.

@ali-bahjati
Copy link
Contributor Author

ali-bahjati commented Feb 16, 2022

Please read the updated description above.

Copy link
Contributor

@jayantk jayantk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm. this is really well done

@ali-bahjati ali-bahjati merged commit 1e43edb into main Feb 16, 2022
@ali-bahjati ali-bahjati deleted the feat/add-stale-price branch February 16, 2022 16:25
@ali-bahjati ali-bahjati changed the title Add get_aggregate_price_status which takes care of becoming stale Add aggregate_price_status which takes care of becoming stale Feb 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants