Command to delete a product (#262)

* first commit of del_product

* cleanup

* PR comments

Co-authored-by: Jayant Krishnamurthy <[email protected]>

program/c/src/oracle/oracle.h

@@ -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

program/rust/src/processor.rs

@@ -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,
@@ -29,6 +30,7 @@ use crate::rust_oracle::{
     add_product,
     add_publisher,
     del_price,
+    del_product,
     del_publisher,
     init_mapping,
     init_price,
@@ -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()),
     }
 }

program/rust/src/rust_oracle.rs

@@ -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,
@@ -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)?;
+        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(())
+}

program/rust/src/tests/mod.rs

@@ -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;

program/rust/src/tests/pyth_simulator.rs

@@ -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,
@@ -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(

program/rust/src/tests/test_del_product.rs

@@ -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;
+}

program/rust/src/utils.rs

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