Skip to content
Merged
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
6 changes: 6 additions & 0 deletions program/c/src/oracle/oracle.h
Original file line number Diff line number Diff line change
Expand Up @@ -275,6 +275,12 @@ typedef enum {
// key[1] product account [signer writable]
// key[2] price account [signer writable]
e_cmd_del_price,

// deletes a product account
// key[0] funding account [signer writable]
// key[1] mapping account [signer writable]
// key[2] product account [signer writable]
e_cmd_del_product,
} command_t;

typedef struct cmd_hdr
Expand Down
3 changes: 3 additions & 0 deletions program/rust/src/processor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ use crate::c_oracle_header::{
command_t_e_cmd_add_publisher,
command_t_e_cmd_agg_price,
command_t_e_cmd_del_price,
command_t_e_cmd_del_product,
command_t_e_cmd_del_publisher,
command_t_e_cmd_init_mapping,
command_t_e_cmd_init_price,
Expand All @@ -29,6 +30,7 @@ use crate::rust_oracle::{
add_product,
add_publisher,
del_price,
del_product,
del_publisher,
init_mapping,
init_price,
Expand Down Expand Up @@ -75,6 +77,7 @@ pub fn process_instruction(
command_t_e_cmd_upd_product => upd_product(program_id, accounts, instruction_data),
command_t_e_cmd_set_min_pub => set_min_pub(program_id, accounts, instruction_data),
command_t_e_cmd_del_price => del_price(program_id, accounts, instruction_data),
command_t_e_cmd_del_product => del_product(program_id, accounts, instruction_data),
_ => Err(OracleError::UnrecognizedInstruction.into()),
}
}
70 changes: 70 additions & 0 deletions program/rust/src/rust_oracle.rs
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ use crate::utils::{
check_valid_writable_account,
is_component_update,
pubkey_assign,
pubkey_clear,
pubkey_equal,
pubkey_is_zero,
pyth_assert,
Expand Down Expand Up @@ -705,3 +706,72 @@ pub fn set_min_pub(

Ok(())
}

/// Delete a product account and remove it from the product list of its associated mapping account.
/// The deleted product account must not have any price accounts.
///
/// This function allows you to delete products from non-tail mapping accounts. This ability is a
/// little weird, as it allows you to construct a list of multiple mapping accounts where non-tail
/// accounts have empty space. This is fine however; users should simply add new products to the
/// first available spot.
pub fn del_product(
program_id: &Pubkey,
accounts: &[AccountInfo],
instruction_data: &[u8],
) -> ProgramResult {
let [funding_account, mapping_account, product_account] = match accounts {
[w, x, y] => Ok([w, x, y]),
_ => Err(ProgramError::InvalidArgument),
}?;

check_valid_funding_account(funding_account)?;
check_valid_signable_account(program_id, mapping_account, size_of::<pc_map_table_t>())?;
check_valid_signable_account(program_id, product_account, PC_PROD_ACC_SIZE as usize)?;

{
let cmd_args = load::<cmd_hdr_t>(instruction_data)?;
let mut mapping_data = load_checked::<pc_map_table_t>(mapping_account, cmd_args.ver_)?;
let product_data = load_checked::<pc_prod_t>(product_account, cmd_args.ver_)?;

// This assertion is just to make the subtractions below simpler
pyth_assert(mapping_data.num_ >= 1, ProgramError::InvalidArgument)?;
Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than assert, can we used .checked_sub on the subtractions below? We can (and should) enable clippy’s arithmetic lint which will be safer. We should really be using checked arithmetic for everything on-chain.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good call on the arithmetic checks. will enable in a separate pr.

pyth_assert(
pubkey_is_zero(&product_data.px_acc_),
ProgramError::InvalidArgument,
)?;

let product_key = product_account.key.to_bytes();
let product_index = mapping_data
.prod_
.iter()
.position(|x| pubkey_equal(x, &product_key))
.ok_or(ProgramError::InvalidArgument)?;

let num_after_removal: usize = try_convert(
mapping_data
.num_
.checked_sub(1)
.ok_or(ProgramError::InvalidArgument)?,
)?;

let last_key_bytes = mapping_data.prod_[num_after_removal];
pubkey_assign(
&mut mapping_data.prod_[product_index],
bytes_of(&last_key_bytes),
);
pubkey_clear(&mut mapping_data.prod_[num_after_removal]);
mapping_data.num_ = try_convert::<_, u32>(num_after_removal)?;
mapping_data.size_ =
try_convert::<_, u32>(size_of::<pc_map_table_t>() - size_of_val(&mapping_data.prod_))?
+ mapping_data.num_ * try_convert::<_, u32>(size_of::<pc_pub_key_t>())?;
}

// Zero out the balance of the price account to delete it.
// Note that you can't use the system program's transfer instruction to do this operation, as
// that instruction fails if the source account has any data.
let lamports = product_account.lamports();
**product_account.lamports.borrow_mut() = 0;
**funding_account.lamports.borrow_mut() += lamports;

Ok(())
}
1 change: 1 addition & 0 deletions program/rust/src/tests/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ mod test_add_price;
mod test_add_product;
mod test_add_publisher;
mod test_del_price;
mod test_del_product;
mod test_del_publisher;
mod test_init_mapping;
mod test_init_price;
Expand Down
25 changes: 25 additions & 0 deletions program/rust/src/tests/pyth_simulator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ use crate::c_oracle_header::{
command_t_e_cmd_add_price,
command_t_e_cmd_add_product,
command_t_e_cmd_del_price,
command_t_e_cmd_del_product,
command_t_e_cmd_init_mapping,
pc_map_table_t,
pc_price_t,
Expand Down Expand Up @@ -161,6 +162,30 @@ impl PythSimulator {
.map(|_| product_keypair)
}

/// Delete a product account (using the del_product instruction).
pub async fn del_product(
&mut self,
mapping_keypair: &Keypair,
product_keypair: &Keypair,
) -> Result<(), BanksClientError> {
let cmd = cmd_hdr_t {
ver_: PC_VERSION,
cmd_: command_t_e_cmd_del_product as i32,
};
let instruction = Instruction::new_with_bytes(
self.program_id,
bytes_of(&cmd),
vec![
AccountMeta::new(self.payer.pubkey(), true),
AccountMeta::new(mapping_keypair.pubkey(), true),
AccountMeta::new(product_keypair.pubkey(), true),
],
);

self.process_ix(instruction, &vec![&mapping_keypair, &product_keypair])
.await
}

/// Initialize a price account and add it to an existing product account (using the add_price
/// instruction). Returns the keypair associated with the newly-created account.
pub async fn add_price(
Expand Down
93 changes: 93 additions & 0 deletions program/rust/src/tests/test_del_product.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,93 @@
use std::mem::{
size_of,
size_of_val,
};

use solana_program::pubkey::Pubkey;
use solana_sdk::signer::Signer;

use crate::c_oracle_header::{
pc_map_table_t,
pc_pub_key_t,
};
use crate::tests::pyth_simulator::PythSimulator;
use crate::utils::pubkey_equal;

#[tokio::test]
async fn test_del_product() {
let mut sim = PythSimulator::new().await;
let mapping_keypair = sim.init_mapping().await.unwrap();
let product1 = sim.add_product(&mapping_keypair).await.unwrap();
let product2 = sim.add_product(&mapping_keypair).await.unwrap();
let product3 = sim.add_product(&mapping_keypair).await.unwrap();
let product4 = sim.add_product(&mapping_keypair).await.unwrap();
let product5 = sim.add_product(&mapping_keypair).await.unwrap();
let _price3 = sim.add_price(&product3, -8).await.unwrap();

let mapping_keypair2 = sim.init_mapping().await.unwrap();
let product1_2 = sim.add_product(&mapping_keypair2).await.unwrap();

assert!(sim.get_account(product2.pubkey()).await.is_some());
assert!(sim.get_account(product4.pubkey()).await.is_some());

// Can't delete a product with a price account
assert!(sim.del_product(&mapping_keypair, &product3).await.is_err());
// Can't delete mismatched product/mapping accounts
assert!(sim
.del_product(&mapping_keypair, &product1_2)
.await
.is_err());

assert!(sim.del_product(&mapping_keypair, &product2).await.is_ok());

assert!(sim.get_account(product2.pubkey()).await.is_none());

let mapping_data = sim
.get_account_data_as::<pc_map_table_t>(mapping_keypair.pubkey())
.await
.unwrap();
assert!(mapping_product_list_equals(
&mapping_data,
vec![
product1.pubkey(),
product5.pubkey(),
product3.pubkey(),
product4.pubkey()
]
));
assert!(sim.get_account(product5.pubkey()).await.is_some());


assert!(sim.del_product(&mapping_keypair, &product4).await.is_ok());
let mapping_data = sim
.get_account_data_as::<pc_map_table_t>(mapping_keypair.pubkey())
.await
.unwrap();

assert!(mapping_product_list_equals(
&mapping_data,
vec![product1.pubkey(), product5.pubkey(), product3.pubkey()]
));
}

/// Returns true if the list of products in `mapping_data` contains the keys in `expected` (in the
/// same order). Also checks `mapping_data.num_` and `size_`.
fn mapping_product_list_equals(mapping_data: &pc_map_table_t, expected: Vec<Pubkey>) -> bool {
if mapping_data.num_ != expected.len() as u32 {
return false;
}

let expected_size = (size_of::<pc_map_table_t>() - size_of_val(&mapping_data.prod_)
+ expected.len() * size_of::<pc_pub_key_t>()) as u32;
if mapping_data.size_ != expected_size {
return false;
}

for i in 0..expected.len() {
if !pubkey_equal(&mapping_data.prod_[i], &expected[i].to_bytes()) {
return false;
}
}

return true;
}
9 changes: 9 additions & 0 deletions program/rust/src/utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,15 @@ pub fn pubkey_equal(target: &pc_pub_key_t, source: &[u8]) -> bool {
unsafe { target.k1_ == *source }
}

/// Zero out the bytes of `key`.
pub fn pubkey_clear(key: &mut pc_pub_key_t) {
unsafe {
for i in 0..key.k8_.len() {
key.k8_[i] = 0;
}
}
}

/// Convert `x: T` into a `U`, returning the appropriate `OracleError` if the conversion fails.
pub fn try_convert<T, U: TryFrom<T>>(x: T) -> Result<U, OracleError> {
// Note: the error here assumes we're only applying this function to integers right now.
Expand Down