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

pyth-sdk-solana: Add constraints between SDK and oracle program types. #104

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Reisen
Copy link
Contributor

@Reisen Reisen commented Apr 6, 2023

The types defined here in theory should be byte-equivalent to those defined in the pyth-oracle program. It is difficult to check this without comparing the two sources together. This PR adds the oracle contract as a dependency during testing and attempts to convert between the prices and compare byte sizes to add some confidence that these don't go out of sync. It's also possible then to have this library directly depend on a specific version of the oracle program so that we're forced to identify which version the types depend on for better release numbering.

I did this because I have need for this library within pythnet and this was a pain point.

@@ -24,6 +24,10 @@ pyth-sdk = { path = "../pyth-sdk", version = "0.7.0" }
[dev-dependencies]
solana-client = ">= 1.9, < 1.15"
solana-sdk = ">= 1.9, < 1.15"
pyth-oracle = { path = "../../pyth-client/program/rust", features = ["library"] }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Relative path just for now because it relies on the upstream PR being merged first.

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.

see inline comment

//!
//! The definitions here should be in sync with the definitinos provided within the pyth-oracle
//! Solana contract. To enforce this the tests at the bottom of this file attempt to compare the
//! definitions here with the definitions in the contract.
Copy link
Contributor

Choose a reason for hiding this comment

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

why bother having the definitions here if you want them to be equivalent to the ones in pyth-oracle? seems like redundant code and we should just import from pyth-oracle.

Copy link
Contributor Author

@Reisen Reisen Apr 6, 2023

Choose a reason for hiding this comment

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

I'll fix the comment as I think that wasn't clear, they aren't identical, just isomorphic and it is difficult to tell that's the case which is exactly why I wanted to add this. Even if they were identical, having the API exposed by an SDK library and not the contract I would say is a good thing. If anything I would argue that this library should expose the types and pyth-oracle should depend on them but that's a large shift.

In other words the byte layout is the same, but the API exposed isn't the same. I didn't like that it wasn't easy to tell at a glance if those conversions were in sync. This is just adding some conversions that will error out/fail to type check if pyth-oracle were to change and this library does not.

Copy link
Contributor

Choose a reason for hiding this comment

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

ah makes sense. 👍


// We then compare the type sizes to get some guarantee that the layout is likely the same.
// Combined with the isomorphism above we should be able to catch layout changes that were
// not propogated from the pyth-client program.
Copy link
Contributor

Choose a reason for hiding this comment

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

propagated

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, will fix, along with definitinos in the above comment as well after I clarify it a bit better.

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

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