feat: wrap Pythnet/Solana accounts generically #107
Conversation
Reisen
force-pushed
the
push-qkqyyytqnrzp
branch
3 times, most recently
from
75933b4 to
6f0c20e
Compare
| .context(format!("Could not parse price account at {}", price_key))?; | ||
|
|
||
| let next_price = price.next; |
There was a problem hiding this comment.
Do we need this extra variable?
There was a problem hiding this comment.
Yeah unfortunately the price gets moved when it's inserted into the map, rather than clone the whole thing this just copies out the only useful data we need.
src/agent/solana/oracle.rs
Outdated
| @@ -288,14 +353,17 @@ impl Oracle { | |||
| account_key: &Pubkey, | |||
| account: &Account, | |||
| ) -> Result<()> { | |||
| let price_account = *load_price_account(&account.data) | |||
| // TODO: remove unwrap() | |||
There was a problem hiding this comment.
Ah yeah thank you, removed comment.
| /// and construct the right one regardless of which network we read. This will only work | ||
| /// as long as we don't care about any extended fields. | ||
| /// | ||
| /// TODO: Refactor the agent's network handling code. |
There was a problem hiding this comment.
FWIW I think it's fine to keep things like this, because we're going to deprecate the solana oracle anyway, and then we won't need both account representations.
Reisen
force-pushed
the
push-qkqyyytqnrzp
branch
from
6f0c20e to
7eca044
Compare
| n if n == std::mem::size_of::<SolanaPriceAccount>() => 32, | ||
| n if n == std::mem::size_of::<PythnetPriceAccount>() => 64, | ||
| _ => return None, | ||
| }; |
There was a problem hiding this comment.
Do you really need unsafe? Why don't you conditionally deserialize as SolanaPriceAccount or PythnetPriceAccount.
Then you use the resulting SolanaPriceAccount or PythnetPriceAccount to build a PriceEntry.
There was a problem hiding this comment.
Can you expand on how to do that parsing? You might be right and I missed it but as far as I can tell these types are used mostly with zero copy casts and we haven't properly defined a parser/wire format to do this. In order to construct the underlying array I'm inferring from size, and the slice constructor is inherently unsafe which is where unsafe comes from.
Definitely would prefer a solution without unsafe, can you expand a bit more?
There was a problem hiding this comment.
Bytemuck try_from_bytes it like here :
https://github.com/pyth-network/pyth-client/blob/f3b78939d768c525d29ed5fcebd767dc13e2fce8/program/rust/src/deserialize.rs#L33
There was a problem hiding this comment.
It's probably fine copying the data, you're copying it anyway
Reisen
force-pushed
the
push-qkqyyytqnrzp
branch
from
7eca044 to
23a0193
Compare
ali-bahjati
force-pushed
the
push-qkqyyytqnrzp
branch
from
7ddba33 to
a218ebc
Compare
| @@ -423,7 +426,7 @@ impl Adapter { | |||
| publisher_accounts: price_account | |||
| .comp | |||
| .iter() | |||
| .filter(|comp| **comp != PriceComp::default()) | |||
| .filter(|comp| *comp != &PriceComp::default()) | |||
There was a problem hiding this comment.
Lack of this check is not harmful but having a default publisher in logs is annoying.
See pyth-network/pyth-sdk-rs#113 for how and why the API changed.
The short version is the API now provides the ability to read price accounts on both Solana and Pythnet accounting for the differences in those accounts. That API intentionally does not expose a generic wrapper because the differences (especially as new fields are added) cannot be abstracted.
In pyth-agent, the design currently does not distinguish between which network a price account came from in various code paths, but because these account structures now differ this will require a large refactor that introduces the concept of network differences. Rather than do this now, this introduces a thin wrapper around the API that does act as a generic wrapper, which is possible because it currently doesn't care about extended fields. This is not a decent approach which is why it is not in the API, but is a good stopgap for pyth-agent as It's a small diff to unwind.
The wrapper works by copying the data into a fixed 64 sized account and ignoring the rest of the type entirely. It uses some hacks to do this unfortunately, but the alternative refactor for pyth-agent will be very large.
No new test is added to the integration tests because the tests use program admin to sync accounts and it doesn't work on newer oracle versions. We need to revamp the integration tests in the future to be in Rust and use new permissioning approaches.