feat: add todos from first code reading session (#662)

* feat: add todos from first code reading session

Signed-off-by: Pablo Maldonado <[email protected]>

* refactor: todo

Signed-off-by: Pablo Maldonado <[email protected]>

* fix: lint

Signed-off-by: Pablo Maldonado <[email protected]>

---------

Signed-off-by: Pablo Maldonado <[email protected]>

Author: md0x

programs/svm-spoke/src/instructions/admin.rs

@@ -1,8 +1,10 @@
 use anchor_lang::prelude::*;
 
+// TODO: standardize imports across all files
 use anchor_spl::associated_token::AssociatedToken;
 use anchor_spl::token_interface::{Mint, TokenAccount, TokenInterface};
 
+// TODO: check that the discriminator size is used everywhere
 use crate::constants::DISCRIMINATOR_SIZE;
 use crate::constraints::is_local_or_remote_owner;
 
@@ -13,15 +15,13 @@ use crate::{
     state::{RootBundle, Route, State},
 };
 
-//TODO: there is too much in this file now and it should be split up somewhat.
-
 #[derive(Accounts)]
 #[instruction(seed: u64)]
 pub struct Initialize<'info> {
     #[account(init, // Use init, not init_if_needed to prevent re-initialization.
               payer = signer,
-              space = DISCRIMINATOR_SIZE + State::INIT_SPACE,
-              seeds = [b"state", seed.to_le_bytes().as_ref()],
+              space = DISCRIMINATOR_SIZE + State::INIT_SPACE, // TODO: check that INIT_SPACE is used everywhere
+              seeds = [b"state", seed.to_le_bytes().as_ref()], // TODO: can we set a blank seed? or something better?
               bump)]
     pub state: Account<'info, State>,
 
@@ -107,11 +107,12 @@ pub struct TransferOwnership<'info> {
 
     #[account(
         mut,
-        address = state.owner @ CustomError::NotOwner
+        address = state.owner @ CustomError::NotOwner // TODO: test permissioning with a multi-sig and Squads
     )]
     pub signer: Signer<'info>,
 }
 
+// TODO: check that the recovery flow is similar to the one in EVM
 pub fn transfer_ownership(ctx: Context<TransferOwnership>, new_owner: Pubkey) -> Result<()> {
     let state = &mut ctx.accounts.state;
     state.owner = new_owner;
@@ -138,6 +139,7 @@ pub fn set_cross_domain_admin(
     let state = &mut ctx.accounts.state;
     state.cross_domain_admin = cross_domain_admin;
 
+    // TODO: add lint to make this a 1-liner
     emit_cpi!(SetXDomainAdmin {
         new_admin: cross_domain_admin,
     });
@@ -147,7 +149,7 @@ pub fn set_cross_domain_admin(
 
 #[event_cpi]
 #[derive(Accounts)]
-#[instruction(origin_token: [u8; 32], destination_chain_id: u64)]
+#[instruction(origin_token: [u8; 32], destination_chain_id: u64)] // TODO: is it possible to replace origin_token with Pubkey?
 pub struct SetEnableRoute<'info> {
     #[account(
         mut,
@@ -158,6 +160,7 @@ pub struct SetEnableRoute<'info> {
     #[account(mut)]
     pub payer: Signer<'info>,
 
+    // TODO: check if state needs to be mut here and in other places
     #[account(mut, seeds = [b"state", state.seed.to_le_bytes().as_ref()], bump)]
     pub state: Account<'info, State>,
 
@@ -216,11 +219,12 @@ pub struct RelayRootBundle<'info> {
     )]
     pub signer: Signer<'info>,
 
+    // TODO: standardize usage of state.seed vs state.key()
     #[account(mut, seeds = [b"state", state.seed.to_le_bytes().as_ref()], bump)]
     pub state: Account<'info, State>,
 
     // TODO: consider deriving seed from state.seed instead of state.key() as this could be cheaper (need to verify).
-    #[account(init,
+    #[account(init, // TODO: add comment explaining why init
         payer = signer,
         space = DISCRIMINATOR_SIZE + RootBundle::INIT_SPACE,
         seeds =[b"root_bundle", state.key().as_ref(), state.root_bundle_id.to_le_bytes().as_ref()],
@@ -240,5 +244,9 @@ pub fn relay_root_bundle(
     root_bundle.relayer_refund_root = relayer_refund_root;
     root_bundle.slow_relay_root = slow_relay_root;
     state.root_bundle_id += 1;
+
+    // TODO: add event
     Ok(())
 }
+
+// TODO: add emergency_delete_root_bundle

programs/svm-spoke/src/instructions/deposit.rs

@@ -20,7 +20,7 @@ use anchor_spl::token_interface::{
     output_token: Pubkey,
     input_amount: u64,
     output_amount: u64,
-    destination_chain_id: u64,
+    destination_chain_id: u64, // TODO: we can remove some of these instructions props
     exclusive_relayer: Pubkey,
     quote_timestamp: u32,
     fill_deadline: u32,
@@ -36,6 +36,7 @@ pub struct DepositV3<'info> {
     )]
     pub state: Account<'info, State>,
 
+    // TODO: linter to format this line
     #[account(mut, seeds = [b"route", input_token.as_ref(), state.key().as_ref(), destination_chain_id.to_le_bytes().as_ref()], bump)]
     pub route: Account<'info, Route>,
 
@@ -58,6 +59,7 @@ pub struct DepositV3<'info> {
     )]
     pub vault: InterfaceAccount<'info, TokenAccount>,
 
+    // TODO: why are we using mint::token_program,token::token_program and associated_token::token_program?
     #[account(
         mut,
         mint::token_program = token_program,
@@ -133,3 +135,5 @@ pub fn deposit_v3(
 
     Ok(())
 }
+
+// TODO: do we need other flavours of deposit? like speed up deposit

programs/svm-spoke/src/instructions/fill.rs

@@ -20,7 +20,7 @@ use crate::{
 pub struct FillV3Relay<'info> {
     #[account(
         mut,
-        seeds = [b"state", state.seed.to_le_bytes().as_ref()],
+        seeds = [b"state", state.seed.to_le_bytes().as_ref()], // TODO: make consistent decision where to put owner constraints
         bump,
         constraint = !state.paused_fills @ CustomError::FillsArePaused
     )]
@@ -40,14 +40,14 @@ pub struct FillV3Relay<'info> {
 
     #[account(
         mut,
-        token::token_program = token_program,
+        token::token_program = token_program, // TODO: consistent token imports
         address = relay_data.output_token @ CustomError::InvalidMint
     )]
     pub mint_account: InterfaceAccount<'info, Mint>,
 
     #[account(
         mut,
-        associated_token::mint = mint_account,
+        associated_token::mint = mint_account, // TODO: consistent token imports
         associated_token::authority = relayer,
         associated_token::token_program = token_program
     )]
@@ -65,7 +65,7 @@ pub struct FillV3Relay<'info> {
         init_if_needed,
         payer = signer,
         space = DISCRIMINATOR_SIZE + FillStatusAccount::INIT_SPACE,
-        seeds = [b"fills", relay_hash.as_ref()],
+        seeds = [b"fills", relay_hash.as_ref()], // TODO: can we calculate the relay_hash from the state and relay_data?
         bump,
         // Make sure caller provided relay_hash used in PDA seeds is valid.
         constraint = is_relay_hash_valid(&relay_hash, &relay_data, &state) @ CustomError::InvalidRelayHash
@@ -77,7 +77,9 @@ pub struct FillV3Relay<'info> {
     pub system_program: Program<'info, System>,
 }
 
-#[derive(AnchorSerialize, AnchorDeserialize, Clone)]
+#[derive(AnchorSerialize, AnchorDeserialize, Clone)] // TODO: do we need all of these?
+
+// TODO: should we move this data structure to a common module?
 pub struct V3RelayData {
     pub depositor: Pubkey,
     pub recipient: Pubkey,
@@ -126,6 +128,7 @@ pub fn fill_v3_relay(
 
     // Invoke the transfer_checked instruction on the token program
     let transfer_accounts = TransferChecked {
+        // TODO: check what happens if the relayer and recipient are the same
         from: ctx.accounts.relayer_token_account.to_account_info(),
         mint: ctx.accounts.mint_account.to_account_info(),
         to: ctx.accounts.recipient_token_account.to_account_info(),
@@ -145,8 +148,10 @@ pub fn fill_v3_relay(
     fill_status_account.status = FillStatus::Filled;
     fill_status_account.relayer = *ctx.accounts.signer.key;
 
+    // TODO: remove msg! everywhere
     msg!("Tokens transferred successfully.");
 
+    // TODO: there might be a better way to do this
     // Emit the FilledV3Relay event
     let message_clone = relay_data.message.clone(); // Clone the message before it is moved
 
@@ -184,7 +189,7 @@ pub struct CloseFillPda<'info> {
 
     #[account(
         mut,
-        address = fill_status.relayer @ CustomError::NotRelayer
+        address = fill_status.relayer @ CustomError::NotRelayer // TODO: check that this doesn't break PR 653
     )]
     pub signer: Signer<'info>,