Add comments from Jon

#### Problem

P-token is getting ready to be used everywhere, but it hasn't been
reviewed!

#### Summary of changes

Nothing show-stopping, mostly nits and cleanup ideas. Great work!

Author: joncinque

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)?,