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

feat: wrap Pythnet/Solana accounts generically #107

Merged
merged 3 commits into from
Feb 19, 2024
Merged
Changes from 1 commit
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
Prev Previous commit
Next Next commit
fix: avoid the implicit copy
ali-bahjati committed Feb 13, 2024
commit a218ebcd18585c5de635b80555df6056690099f8
48 changes: 27 additions & 21 deletions src/agent/solana/oracle.rs
Original file line number Diff line number Diff line change
@@ -70,11 +70,11 @@ impl From<SolanaPriceAccount> for PriceEntry {
fn from(other: SolanaPriceAccount) -> PriceEntry {
unsafe {
// NOTE: We know the size is 32 because It's a Solana account. This is for tests only.
let comp_mem = std::slice::from_raw_parts(other.comp.as_ptr() as *const PriceComp, 32);
let comp_mem = std::slice::from_raw_parts(other.comp.as_ptr(), 32);
let account =
*(&other as *const SolanaPriceAccount as *const GenericPriceAccount<0, ()>);
let mut comp = [PriceComp::default(); 64];
(&mut comp[0..32]).copy_from_slice(comp_mem);
comp[0..32].copy_from_slice(comp_mem);
PriceEntry { account, comp }
}
}
@@ -90,12 +90,15 @@ impl PriceEntry {
_ => return None,
};
Copy link
Contributor

@guibescos guibescos Feb 9, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

@guibescos guibescos Feb 9, 2024

Choose a reason for hiding this comment

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

It's probably fine copying the data, you're copying it anyway


let account = *(acc.as_ptr() as *const GenericPriceAccount<0, ()>);
let comp_mem =
std::slice::from_raw_parts(account.comp.as_ptr() as *const PriceComp, size);
// Getting a pointer to avoid copying the account
let account_ptr = &*(acc.as_ptr() as *const GenericPriceAccount<0, ()>);
let comp_mem = std::slice::from_raw_parts(account_ptr.comp.as_ptr(), size);
let mut comp = [PriceComp::default(); 64];
(&mut comp[0..size]).copy_from_slice(comp_mem);
Some(Self { account, comp })
comp[0..size].copy_from_slice(comp_mem);
Some(Self {
account: *account_ptr,
comp,
})
}
}
}
@@ -321,10 +324,10 @@ impl Oracle {
.collect::<HashSet<_>>();
let new_publishers = data.publisher_permissions.keys().collect::<HashSet<_>>();
info!(
self.logger,
"updated publisher permissions";
"new_publishers" => format!("{:?}", new_publishers.difference(&previous_publishers).collect::<HashSet<_>>()),
"total_publishers" => new_publishers.len(),
self.logger,
"updated publisher permissions";
"new_publishers" => format!("{:?}", new_publishers.difference(&previous_publishers).collect::<HashSet<_>>()),
"total_publishers" => new_publishers.len(),
);

// Update the data with the new data structs
@@ -499,6 +502,10 @@ impl Poller {

for (price_key, price_entry) in price_accounts.iter() {
for component in price_entry.comp {
if component.publisher == Pubkey::default() {
continue;
}

let component_pub_entry = publisher_permissions
.entry(component.publisher)
.or_insert(HashMap::new());
@@ -607,16 +614,15 @@ impl Poller {
product.iter().find(|(k, _v)| *k == "weekly_schedule")
{
wsched_val.parse().unwrap_or_else(|err| {
warn!(
self.logger,
"Oracle: Product has weekly_schedule defined but it could not be parsed. Falling back to 24/7 publishing.";
"product_key" => product_key.to_string(),
"weekly_schedule" => wsched_val,
);
debug!(self.logger, "parsing error context"; "context" => format!("{:?}", err));
Default::default()
}
)
warn!(
self.logger,
"Oracle: Product has weekly_schedule defined but it could not be parsed. Falling back to 24/7 publishing.";
"product_key" => product_key.to_string(),
"weekly_schedule" => wsched_val,
);
debug!(self.logger, "parsing error context"; "context" => format!("{:?}", err));
Default::default()
})
} else {
Default::default() // No market hours specified, meaning 24/7 publishing
};