Skip to content

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

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions pyth-sdk-solana/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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.


[features]
default = []

[lib]
crate-type = ["cdylib", "lib"]
Expand Down
180 changes: 180 additions & 0 deletions pyth-sdk-solana/src/state.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,8 @@
//! Structures and functions for interacting with Solana on-chain account data.
//!
//! 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. 👍


use borsh::{
BorshDeserialize,
Expand Down Expand Up @@ -511,10 +515,16 @@ mod test {
use solana_program::pubkey::Pubkey;

use super::{
CorpAction,
MappingAccount,
PriceAccount,
PriceComp,
PriceInfo,
PriceStatus,
PriceType,
ProductAccount,
Rational,
PROD_ATTR_SIZE,
};


Expand Down Expand Up @@ -737,4 +747,174 @@ mod test {

assert_eq!(price_account.get_price_no_older_than(&clock, 1), None);
}

// Convert the pyth-client program definition to our local accounts. This uses
// destructuring to make sure that we cover all fields and gain some confidence that the
// types are at least isomoprhic.
#[test]
fn test_compare_upstream_definitions() {
impl From<pyth_oracle::PriceInfo> for PriceInfo {
fn from(other: pyth_oracle::PriceInfo) -> Self {
let pyth_oracle::PriceInfo {
price_,
conf_,
status_,
corp_act_status_,
pub_slot_,
} = other;

Self {
price: price_,
conf: conf_,
status: match status_ {
0 => PriceStatus::Unknown,
1 => PriceStatus::Trading,
2 => PriceStatus::Halted,
3 => PriceStatus::Auction,
4 => PriceStatus::Ignored,
_ => unreachable!(),
},
corp_act: match corp_act_status_ {
0 => CorpAction::NoCorpAct,
_ => unreachable!(),
},
pub_slot: pub_slot_,
}
}
}

impl From<pyth_oracle::PriceAccount> for PriceAccount {
fn from(other: pyth_oracle::PriceAccount) -> Self {
let pyth_oracle::PriceAccount {
header,
price_type,
exponent,
num_,
num_qt_,
last_slot_,
valid_slot_,
twap_,
twac_,
timestamp_,
min_pub_,
unused_1_,
unused_2_,
unused_3_,
product_account,
next_price_account,
prev_slot_,
prev_price_,
prev_conf_,
prev_timestamp_,
agg_,
comp_,
} = other;

Self {
magic: header.magic_number,
ver: header.version,
atype: header.account_type,
size: header.size,
ptype: match price_type {
0 => PriceType::Unknown,
1 => PriceType::Price,
_ => unreachable!(),
},
expo: exponent,
num: num_,
num_qt: num_qt_,
last_slot: last_slot_,
valid_slot: valid_slot_,
ema_price: Rational {
val: twap_.val_,
numer: twap_.numer_,
denom: twap_.denom_,
},
ema_conf: Rational {
val: twac_.val_,
numer: twac_.numer_,
denom: twac_.denom_,
},
timestamp: timestamp_,
min_pub: min_pub_,
drv2: unused_1_ as u8,
drv3: unused_2_ as u16,
drv4: unused_3_ as u32,
prod: product_account,
next: next_price_account,
prev_slot: prev_slot_,
prev_price: prev_price_,
prev_conf: prev_conf_,
prev_timestamp: prev_timestamp_,
agg: PriceInfo::from(agg_),
comp: comp_.map(|comp| PriceComp {
publisher: comp.pub_,
agg: PriceInfo::from(comp.agg_),
latest: PriceInfo::from(comp.latest_),
}),
}
}
}

impl From<pyth_oracle::MappingAccount> for MappingAccount {
fn from(other: pyth_oracle::MappingAccount) -> Self {
let pyth_oracle::MappingAccount {
header,
number_of_products,
unused_,
next_mapping_account,
products_list,
} = other;

Self {
magic: header.magic_number,
ver: header.version,
atype: header.account_type,
size: header.size,
num: number_of_products,
unused: unused_,
next: next_mapping_account,
products: products_list,
}
}
}

// 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.

assert_eq!(
std::mem::size_of::<PriceAccount>(),
std::mem::size_of::<pyth_oracle::PriceAccount>()
);

assert_eq!(
std::mem::size_of::<MappingAccount>(),
std::mem::size_of::<pyth_oracle::MappingAccount>()
);

assert_eq!(
std::mem::size_of::<PriceInfo>(),
std::mem::size_of::<pyth_oracle::PriceInfo>()
);

assert_eq!(
std::mem::size_of::<PriceComp>(),
std::mem::size_of::<pyth_oracle::PriceComponent>()
);

assert_eq!(
std::mem::size_of::<Rational>(),
std::mem::size_of::<pyth_oracle::PriceEma>()
);

// For ProductAccount, there are fields we expose from this library that are not exposed by
// pyth-oracle. So we need to ignore the remaining fields of the account. This is safe and
// is in fact an example of our local types being better than upstream, we don't worry
// about enforcing that. Assume a header.size of 0.
assert_eq!(
std::mem::size_of::<ProductAccount>(),
std::mem::size_of::<pyth_oracle::ProductAccount>()
+ std::mem::size_of::<[u8; PROD_ATTR_SIZE]>()
);
}
}