Add comments from Jon

Cargo.toml

@@ -1,6 +1,7 @@
 [workspace]
 resolver = "2"
 members = ["program"]
+exclude = ["p-token"]
 
 [workspace.lints.rust.unexpected_cfgs]
 level = "warn"

interface/Cargo.toml

@@ -1,15 +1,15 @@
 [package]
 name = "token-interface"
 version = "0.0.0"
-edition = { workspace = true }
+edition = "2021"
 readme = "./README.md"
-license = { workspace = true }
-repository = { workspace = true }
+license = "Apache-2.0"
+#repository = { workspace = true }
 publish = false
 
 [lib]
 crate-type = ["rlib"]
 
 [dependencies]
-pinocchio = { workspace = true }
-pinocchio-pubkey = { workspace = true }
+pinocchio = { path = "../../../pinocchio/sdk/pinocchio" }
+pinocchio-pubkey = { path = "../../../pinocchio/sdk/pubkey" }

interface/src/instruction.rs

@@ -360,6 +360,7 @@ pub enum TokenInstruction<'a> {
     ///
     ///   0. `[writable]`  The account to initialize.
     ///   1. `[]` The mint this account will be associated with.
+    ///   JC nit: numbering here
     ///   3. `[]` Rent sysvar
     InitializeAccount2 {
         /// The new account's owner/multisignature.

interface/src/state/account.rs

@@ -56,6 +56,12 @@ impl Account {
     }
 
     #[inline(always)]
+    // JC: I can't remember if we discussed this, but is this the current token
+    // program behavior? ie it only sets whether the option is set, without
+    // setting the value to all zeroes?
+    // To be clear, this behavior is totally fine to me, but if there's a
+    // difference we need to point it out. People may be relying on the previous
+    // behavior for things like `get_program_accounts`
     pub fn clear_delegate(&mut self) {
         self.delegate.0[0] = 0;
     }
@@ -110,6 +116,9 @@ impl Account {
     }
 
     #[inline(always)]
+    // JC: This is mentioned elsewhere with the delegate, but is this the current
+    // behavior? Or does it zero out the whole thing if the close authority is
+    // cleared?
     pub fn clear_close_authority(&mut self) {
         self.close_authority.0[0] = 0;
     }

interface/src/state/mint.rs

@@ -38,11 +38,17 @@ impl Mint {
     }
 
     #[inline(always)]
+    // JC nit: since there's never a case of `uninitializing` a mint, maybe this
+    // could not take the value and just set `is_initialized` to `1`
     pub fn set_initialized(&mut self, value: bool) {
         self.is_initialized = value as u8;
     }
 
     #[inline(always)]
+    // JC: this one worries me in particular, for people who might be doing
+    // `get_program_accounts` with just their mint authority -- if we don't
+    // clear the whole 36 bytes, they might get false positives. But either way,
+    // this might be the behavior now, in which case, no problem!
     pub fn clear_mint_authority(&mut self) {
         self.mint_authority.0[0] = 0;
     }
@@ -63,6 +69,8 @@ impl Mint {
     }
 
     #[inline(always)]
+    // JC: same as with the other COption<Pubkey> fields, check that this is the
+    // current behavior
     pub fn clear_freeze_authority(&mut self) {
         self.freeze_authority.0[0] = 0;
     }

interface/src/state/mod.rs

@@ -12,6 +12,10 @@ pub type COption<T> = ([u8; 4], T);
 ///
 /// It is up to the type implementing this trait to guarantee that the cast is safe,
 /// i.e., that the fields of the type are well aligned and there are no padding bytes.
+/// JC nit: typically, these are called `Pod`s, but that term is a little bit
+/// loaded with bytemuck unfortunately! We can probably come up with another
+/// name, maybe `PackedType` since the bits are packed, or `Transmutable` to
+/// indicate that it's safe to transmute.
 pub trait RawType {
     /// The length of the type.
     ///

p-token/Cargo.toml

@@ -1,10 +1,10 @@
 [package]
 name = "token-program"
 version = "0.0.0"
-edition = { workspace = true }
+edition = "2021"
 readme = "./README.md"
-license = { workspace = true }
-repository = { workspace = true }
+license = "Apache-2.0"
+#repository = { workspace = true }
 publish = false
 
 [package.metadata.solana]
@@ -18,8 +18,8 @@ logging = []
 test-sbf = []
 
 [dependencies]
-pinocchio = { workspace = true }
-pinocchio-log = { workspace = true }
+pinocchio = { path = "../../../pinocchio/sdk/pinocchio" }
+pinocchio-log = { path = "../../../pinocchio/sdk/log/crate" }
 token-interface = { version = "^0", path = "../interface" }
 
 [dev-dependencies]

p-token/src/entrypoint.rs

@@ -37,6 +37,8 @@ pub fn process_instruction(
         .split_first()
         .ok_or(ProgramError::InvalidInstructionData)?;
 
+    // JC: for readability, is it possible to have a u8 enum with the instruction
+    // type? I don't think it should add any compute usage
     match *discriminator {
         // 0 - InitializeMint
         0 => {

p-token/src/processor/approve_checked.rs

@@ -4,8 +4,11 @@ use super::shared;
 
 #[inline(always)]
 pub fn process_approve_checked(accounts: &[AccountInfo], instruction_data: &[u8]) -> ProgramResult {
+    // JC: this will panic if the instruction data is too short -- let's avoid
+    // panicking
     let (amount, decimals) = instruction_data.split_at(core::mem::size_of::<u64>());
     let amount = u64::from_le_bytes(
+        // JC: if the length is checked earlier, then we should be able to unwrap this
         amount
             .try_into()
             .map_err(|_error| ProgramError::InvalidInstructionData)?,

p-token/src/processor/burn_checked.rs

@@ -9,6 +9,8 @@ pub fn process_burn_checked(accounts: &[AccountInfo], instruction_data: &[u8]) -
         let (amount, decimals) = instruction_data.split_at(core::mem::size_of::<u64>());
         (
             u64::from_le_bytes(
+                // JC: this should be unwrappabable since the length was just checked
+                // and the split amount is valid -- if it gives CU gains, go for it!
                 amount
                     .try_into()
                     .map_err(|_error| ProgramError::InvalidInstructionData)?,

p-token/src/processor/close_account.rs

@@ -9,6 +9,8 @@ use token_interface::{
 use super::validate_owner;
 
 /// Incinerator (`1nc1nerator11111111111111111111111111111111`) address.
+/// JC nit: any reason to not use the one defined in the interface instead? Or
+/// is this defined elsewhere in the SDK?
 const INCINERATOR_ID: Pubkey = [
     0, 51, 144, 114, 141, 52, 17, 96, 121, 189, 201, 17, 191, 255, 0, 219, 212, 77, 46, 205, 204,
     247, 156, 166, 225, 0, 56, 225, 0, 0, 0, 0,
@@ -24,6 +26,7 @@ pub fn process_close_account(accounts: &[AccountInfo]) -> ProgramResult {
     // Comparing whether the AccountInfo's "point" to the same account or
     // not - this is a faster comparison since it just checks the internal
     // raw pointer.
+    // JC: I love this, very clever!
     if source_account_info == destination_account_info {
         return Err(ProgramError::InvalidAccountData);
     } else {

p-token/src/processor/initialize_account2.rs

@@ -13,6 +13,8 @@ pub fn process_initialize_account2(
     instruction_data: &[u8],
 ) -> ProgramResult {
     // SAFETY: validate `instruction_data` length.
+    // JC nit: can this be done as efficiently without using unsafe? meaning,
+    // just reinterpreting the slice as a ref to an array?
     let owner = unsafe {
         if instruction_data.len() != PUBKEY_BYTES {
             return Err(ProgramError::InvalidInstructionData);

p-token/src/processor/initialize_account3.rs

@@ -1,7 +1,7 @@
 use pinocchio::{
     account_info::AccountInfo,
     program_error::ProgramError,
-    pubkey::{Pubkey, PUBKEY_BYTES},
+    pubkey::PUBKEY_BYTES,
     ProgramResult,
 };
 
@@ -13,12 +13,10 @@ pub fn process_initialize_account3(
     instruction_data: &[u8],
 ) -> ProgramResult {
     // SAFETY: validate `instruction_data` length.
-    let owner = unsafe {
-        if instruction_data.len() != PUBKEY_BYTES {
-            return Err(ProgramError::InvalidInstructionData);
-        } else {
-            &*(instruction_data.as_ptr() as *const Pubkey)
-        }
-    };
+    // JC Nit: same here, can this be done without unsafe? Like this?
+    if instruction_data.len() != PUBKEY_BYTES {
+        return Err(ProgramError::InvalidInstructionData);
+    }
+    let owner = instruction_data.try_into().unwrap();
     shared::initialize_account::process_initialize_account(accounts, Some(owner), false)
 }

p-token/src/processor/initialize_immutable_owner.rs

@@ -14,6 +14,7 @@ pub fn process_initialize_immutable_owner(accounts: &[AccountInfo]) -> ProgramRe
     if account.is_initialized() {
         return Err(TokenError::AlreadyInUse.into());
     }
+    // JC nit: People hate this for the CU waste, so let's remove it completely
     msg!("Please upgrade to SPL Token 2022 for immutable owner support");
     Ok(())
 }

p-token/src/processor/initialize_mint.rs

@@ -11,10 +11,14 @@ use token_interface::{
     state::{load_mut_unchecked, mint::Mint, Initializable},
 };
 
+// JC nit: for consistency with the rest, can this implementation be moved to a
+// file under `shared/` like the others?
 #[inline(always)]
 pub fn process_initialize_mint(
     accounts: &[AccountInfo],
     instruction_data: &[u8],
+    // JC nit: same as with initialize_account, can this have a different name
+    // like `get_rent_sysvar_with_syscall` or `rent_sysvar_account_provided`?
     rent_sysvar_account: bool,
 ) -> ProgramResult {
     // Validates the instruction data.
@@ -71,6 +75,8 @@ pub fn process_initialize_mint(
 }
 
 /// Instruction data for the `InitializeMint` instruction.
+/// JC: for legibility and safety, can this hold onto the ref instead of a pointer?
+/// Or does that introduce too many bounds checks and increase CUs a lot?
 pub struct InitializeMint<'a> {
     raw: *const u8,
 

p-token/src/processor/mint_to_checked.rs

@@ -7,6 +7,7 @@ pub fn process_mint_to_checked(accounts: &[AccountInfo], instruction_data: &[u8]
     // expected u64 (8) + u8 (1)
     let (amount, decimals) = if instruction_data.len() == 9 {
         let (amount, decimals) = instruction_data.split_at(core::mem::size_of::<u64>());
+        // JC nit: feel free to unwrap these since you just checked the length!
         (
             u64::from_le_bytes(
                 amount

p-token/src/processor/mod.rs

@@ -84,6 +84,12 @@ const MAX_FORMATTED_DIGITS: usize = u8::MAX as usize + 2;
 
 /// Checks that the account is owned by the expected program.
 #[inline(always)]
+// JC: the previous implementation of the token program didn't hard-code the
+// expected program id, which made it possible for people to deploy their own
+// token programs.
+// Would it make performance worse to use the `program_id` from the entrypoint?
+// If so, we can stick with this, but if it doesn't add overhead, it'd be best
+// to keep the generic program id.
 fn check_account_owner(account_info: &AccountInfo) -> ProgramResult {
     if &TOKEN_PROGRAM_ID != account_info.owner() {
         Err(ProgramError::IncorrectProgramId)
@@ -111,6 +117,12 @@ fn validate_owner(
     {
         // SAFETY: the caller guarantees that there are no mutable borrows of `owner_account_info`
         // account data and the `load` validates that the account is initialized.
+        //
+        // JC nit: Since multiple borrows could still be theoretically possible if
+        // an account is self-owned, it might clearer to say that this is safe
+        // because `Multisig` accounts are only ever loaded in this function,
+        // which means that previous loads will have already failed by the time
+        // we get here.
         let multisig = unsafe { load::<Multisig>(owner_account_info.borrow_data_unchecked())? };
 
         let mut num_signers = 0;
@@ -152,7 +164,21 @@ fn try_ui_amount_into_amount(ui_amount: &str, decimals: u8) -> Result<u64, Progr
 
     // Validates the input.
 
+    // JC nit: I'm not totally sure this needs to be mutable, might be clearer
+    // to redefine it later or simply use `amount_str.len()` explicitly everywhere
     let mut length = amount_str.len();
+    // JC nit: clever! but can we rename this something like `max_after_decimal_length`?
+    // I got a bit confused about why this is "expected", and I think it'll make
+    // the check clearer.
+    //
+    // Alternatively, you can do something like:
+    // ```
+    // let max_input_digits = length + max(after_decimal.len(), decimals);
+    // ```
+    // To make the check totally clear.
+    //
+    // Or maybe even easier, remove this and just check against `length + decimals`
+    // since there's also a check for `after_decimals.len() > decimals` anyway.
     let expected_after_decimal_length = max(after_decimal.len(), decimals);
 
     if (amount_str.is_empty() && after_decimal.is_empty())
@@ -171,6 +197,7 @@ fn try_ui_amount_into_amount(ui_amount: &str, decimals: u8) -> Result<u64, Progr
 
     // SAFETY: the total length of `amount_str` and `after_decimal` is less than
     // `MAX_DIGITS_U64`.
+    // JC nit: did you mean `MAX_FORMATTED_DIGITS`?
     unsafe {
         sol_memcpy(slice, amount_str.as_bytes(), length);
 
@@ -186,6 +213,8 @@ fn try_ui_amount_into_amount(ui_amount: &str, decimals: u8) -> Result<u64, Progr
 
     // SAFETY: `digits` is an array of `MaybeUninit<u8>`, which has the same memory
     // layout as `u8`.
+    // JC nit: for safety, can we use a slice here instead? Or does it use up
+    // too many CUs?
     let ptr = unsafe { digits.as_mut_ptr().add(length) };
 
     for offset in 0..remaining {

p-token/src/processor/set_authority.rs

@@ -111,6 +111,9 @@ pub fn process_set_authority(accounts: &[AccountInfo], instruction_data: &[u8])
     Ok(())
 }
 
+// JC nit: same as with the initialize data, can we use slices and avoid so
+// much unsafe code? Especially because we need to pull out the authority type
+// and the new authority no matter what
 struct SetAuthority<'a> {
     raw: *const u8,
 

p-token/src/processor/shared/burn.rs

@@ -37,6 +37,9 @@ pub fn process_burn(
 
     // SAFETY: single mutable borrow to `mint_info` account data and
     // `load_mut` validates that the mint is initialized.
+    // JC nit: to be totally clear, we can again say that an account cannot be
+    // both a token account and a mint, so if duplicates are passed, one of them
+    // will fail the `load`
     let mint = unsafe { load_mut::<Mint>(mint_info.borrow_mut_data_unchecked())? };
 
     if mint_info.key() != &source_account.mint {
@@ -78,6 +81,8 @@ pub fn process_burn(
     } else {
         source_account.set_amount(updated_source_amount);
 
+        // JC nit: I think we could safely unwrap this, but I'm not sure if
+        // we get any real performance benefits
         let mint_supply = mint
             .supply()
             .checked_sub(amount)

p-token/src/processor/shared/initialize_account.rs

@@ -20,6 +20,9 @@ use crate::processor::check_account_owner;
 pub fn process_initialize_account(
     accounts: &[AccountInfo],
     owner: Option<&Pubkey>,
+    // JC nit: can call this `rent_sysvar_account_in_accounts` or make it the
+    // opposite, ie. if it's *true* then use the syscall, so something like
+    // `get_rent_sysvar_with_syscall`
     rent_sysvar_account: bool,
 ) -> ProgramResult {
     // Accounts expected depend on whether we have the `rent_sysvar` account or not.
@@ -86,6 +89,10 @@ pub fn process_initialize_account(
         account.set_native(true);
         account.set_native_amount(minimum_balance);
         // SAFETY: single mutable borrow to `new_account_info` lamports.
+        // JC nit: up to you, but since you've already checked that
+        // `lamports() < minimum_balance` earlier, you don't actually need
+        // checked math. If it shaves CUs, go for it! But definitely add a
+        // comment explaining why
         unsafe {
             account.set_amount(
                 new_account_info

p-token/src/processor/shared/initialize_multisig.rs

@@ -13,6 +13,7 @@ use token_interface::{
 pub fn process_initialize_multisig(
     accounts: &[AccountInfo],
     m: u8,
+    // JC nit: same here, whatever name we choose for this variable
     rent_sysvar_account: bool,
 ) -> ProgramResult {
     // Accounts expected depend on whether we have the `rent_sysvar` account or not.
@@ -57,6 +58,9 @@ pub fn process_initialize_multisig(
     multisig.m = m;
     multisig.n = remaining.len() as u8;
 
+    // JC nit: just wondering, does it make any difference in CUs to change
+    // `is_valid_signer_index` to take in a `u8` instead of a `usize` and avoid
+    // widening?
     if !Multisig::is_valid_signer_index(multisig.n as usize) {
         return Err(TokenError::InvalidNumberOfProvidedSigners.into());
     }

p-token/src/processor/shared/mint_to.rs

@@ -51,6 +51,7 @@ pub fn process_mint_to(
     }
 
     if amount == 0 {
+        // JC nit: please add a comment here explaining what this is for
         check_account_owner(mint_info)?;
         check_account_owner(destination_account_info)?;
     } else {
@@ -60,6 +61,10 @@ pub fn process_mint_to(
             .ok_or(TokenError::Overflow)?;
         destination_account.set_amount(destination_amount);
 
+        // JC nit: if you do this checked addition first, ie. before adding the
+        // amount to `destination_account`, then you don't need to do checked
+        // math there! If there's no overflow on the supply, then there can't
+        // be an overflow on the destination account
         let mint_supply = mint
             .supply()
             .checked_add(amount)