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

WIP: state.rs: Support Pythnet flavor prices (incl. 64 pubs) #112

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

drozdziak1
Copy link

Motivation

Pyth-agent relies on the pyth-sdk-solana package from this repo to parse Pyth price accounts. With the recent introduction of 64-publisher v2 price account on-chain, the parsing code needs to understand the bigger publisher cap in order to do permissioned symbol filtering.

Summary of changes

  • **/Cargo.toml - bump version to 0.10.0 in pyth-sdk-solana and references to it in other Cargo.toml's
  • pyth-sdk-solana/src/state.rs - Add a PriceAccountPythnet struct near-identical to PriceAccount. The new struct contains expanded v2 price account schema with more price components and PriceCumulative; Rename PriceAccount to PriceAccountSolana, create a typedef: PriceAccount = PriceAccountSolana. The typedef makes this change opaque to existing code using the library without breaking anything. In case of pyth-agent, after bumping to this version, it will attempt to parse raw bytes as PriceAccountPythnet and PriceAccountSolana, in that order. This allows us to avoid stuff like marking "this primary network is pythnet-like and contains v2 prices" in agent config.

Review Highlights

  • state.rs - I had a rare occasion to use a macro for PriceAccountSolana and PriceAccountPythnet impl blocks. Please help me confirm that it was the right decision. Without a macro, the code would be identical with different struct name. This feels justified as none of the existing methods touch the components array or price cumulative data that make the only difference in the v2 price struct. Some alternatives include:
    • A trait implemented by both of the structs - While this makes the interface well-defined, there would still be a lot of doubled code implementing the non-provided methods. Additionally user code would break due to not importing the trait (simple to fix but still annoying).
    • Chucking legacy v1 price accounts - This would suck bad for any public Solana chain users that use the Rust sdk.

Remaining items

  • Unit test PriceAccountPythnet
  • Verify that the code does the right thing with pyth-agent

@drozdziak1 drozdziak1 changed the title state.rs: Support Pythnet flavor prices (incl. 64 pubs) WIP: state.rs: Support Pythnet flavor prices (incl. 64 pubs) Dec 13, 2023
pub comp: [PriceComp; 32],
/// Rationale (2023-12-12): Rust is currently unable to derive Default for [PriceComp; 64]
pub comp2: [PriceComp; 32],
/// Cumulative sums of aggregative price and confidence used to compute arithmetic moving
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 this is wrong, there is room for 128 publishers afaik

@@ -456,8 +552,8 @@ pub fn load_product_account(data: &[u8]) -> Result<&ProductAccount, PythError> {
}

/// Get a `Price` account from the raw byte value of a Solana account.
pub fn load_price_account(data: &[u8]) -> Result<&PriceAccount, PythError> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it ok that this functions loads the account is PriceAccountSolana ? I think it's used in pyth-agent

Copy link
Contributor

@guibescos guibescos left a comment

Choose a reason for hiding this comment

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

Thanks! I like the macro.

I think it's weird that you implemented things like get_price_no_older_than for PriceAccountPythnet, but there's no function to load PriceAccountPythnet. pyth-agent will probably need to load PriceAccountPythnet.

Also I think there's a bug related to PriceAccountPythnet having space for 64 publishers but the real account has space for 128 publishers.

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.

2 participants