Refactor serializer in Serviceability

CHANGELOG.md

@@ -13,6 +13,10 @@ All notable changes to this project will be documented in this file.
 - Onchain programs
   - Allow contributor owner to update ops manager key
   - Add new arguments on create interface cli command
+  - Added the **INSTRUCTION_GUIDELINES** document defining the standard for instruction creation.
+  - Enforce best practices for instruction implementation across onchain programs
+  - Add missing system program account owner checks in multiple instructions
+  - Refactor codebase for improved maintainability and future development
 - Internet Latency Telemetry
   - Fixed a bug that prevented unresponsive ripeatlas probes from being replaced
   - Fixed a bug that caused ripeatlas samples to be dropped when they were delayed to the next collection cycle

smartcontract/programs/doublezero-serviceability/INSTRUCTION_GUIDELINES.md

@@ -0,0 +1,145 @@
+# Instruction Implementation Guidelines
+
+This document describes the required steps and best practices for implementing a new instruction in this Solana program. All developers must follow these guidelines to ensure correctness, security, and maintainability.
+
+---
+
+## 1. Account Parsing and Validation
+
+- Parse all accounts in the order expected by the instruction.
+- For each account, check:
+  - **Ownership:** Ensure the account is owned by the expected program (usually `program_id`).
+  - **Signer:** Verify that required accounts (e.g., payer) are signers.
+  - **Writable:** Ensure accounts that will be mutated are marked as writable.
+  - **System Program:** If the system program is required, check its address matches `solana_program::system_program::id()`.
+  - **PDA Validation:** If the instruction involves a PDA, derive the expected PDA and bump seed, and check both against the provided account and arguments.
+
+**Example:**
+```rust
+let mgroup_account = next_account_info(accounts_iter)?;
+let globalstate_account = next_account_info(accounts_iter)?;
+let payer_account = next_account_info(accounts_iter)?;
+let system_program = next_account_info(accounts_iter)?;
+
+assert!(payer_account.is_signer, "Payer must be a signer");
+assert_eq!(globalstate_account.owner, program_id, "Invalid GlobalState Account Owner");
+assert_eq!(*system_program.unsigned_key(), solana_program::system_program::id(), "Invalid System Program Account Owner");
+assert!(mgroup_account.is_writable, "PDA Account is not writable");
+
+let (expected_pda_account, bump_seed) = get_multicastgroup_pda(program_id, value.index);
+assert_eq!(mgroup_account.key, &expected_pda_account, "Invalid MulticastGroup Pubkey");
+assert_eq!(bump_seed, value.bump_seed, "Invalid MulticastGroup Bump Seed");
+```
+
+---
+
+## 2. Input and Business Logic Validation
+
+- Validate and normalize all input arguments (e.g., using helpers like `validate_account_code`).
+- Deserialize account data using the appropriate helper (e.g., `try_from`).
+- Validate any business-specific invariants (e.g., allowlist membership).
+- Check for account initialization state as needed.
+
+**Example:**
+```rust
+let code = validate_account_code(&value.code).map_err(|_| DoubleZeroError::InvalidAccountCode)?;
+let mut globalstate = GlobalState::try_from(globalstate_account)?;
+globalstate.account_index += 1;
+
+if !globalstate.foundation_allowlist.contains(payer_account.key) {
+    return Err(DoubleZeroError::NotAllowed.into());
+}
+
+if !mgroup_account.data_is_empty() {
+    return Err(ProgramError::AccountAlreadyInitialized);
+}
+```
+
+---
+
+## 3. State Construction and Mutation
+
+- Construct new state objects as needed, using validated and normalized data.
+- Mutate in-memory state only after all checks and validations have passed.
+
+**Example:**
+```rust
+let multicastgroup = MulticastGroup {
+    account_type: AccountType::MulticastGroup,
+    owner: value.owner,
+    index: globalstate.account_index,
+    bump_seed,
+    tenant_pk: Pubkey::default(),
+    code,
+    multicast_ip: std::net::Ipv4Addr::UNSPECIFIED,
+    max_bandwidth: value.max_bandwidth,
+    status: MulticastGroupStatus::Pending,
+    publisher_count: 0,
+    subscriber_count: 0,
+};
+```
+
+---
+
+## 4. State Serialization and Account Creation
+
+- Use the approved helpers (e.g., `try_acc_create`, `try_acc_write`) to create and write account data.
+- Pass all required seeds and bump seeds for PDA creation.
+- Ensure the payer is authorized to pay for the write if required.
+
+**Example:**
+```rust
+try_acc_create(
+    &multicastgroup,
+    mgroup_account,
+    payer_account,
+    system_program,
+    program_id,
+    &[
+        SEED_PREFIX,
+        SEED_MULTICAST_GROUP,
+        &globalstate.account_index.to_le_bytes(),
+        &[bump_seed],
+    ],
+)?;
+try_acc_write(&globalstate, globalstate_account, payer_account, accounts)?;
+```
+
+---
+
+## 5. Error Handling
+
+- Use explicit error returns for all failure cases.
+- Avoid panics or unchecked unwraps in production code.
+- Use descriptive error messages and custom error types where appropriate.
+
+---
+
+## 6. Logging (Optional for Tests)
+
+- Use logging macros (e.g., `msg!`) for debugging and test builds as needed.
+
+---
+
+## 7. Return
+
+- Return `Ok(())` on success.
+
+---
+
+## 8. Summary Checklist
+
+- [ ] Parse and validate all accounts.
+- [ ] Check signers, ownership, and PDA derivation.
+- [ ] Validate and normalize input arguments.
+- [ ] Deserialize and validate state.
+- [ ] Check business logic and invariants.
+- [ ] Construct and mutate state.
+- [ ] Create and write account data.
+- [ ] Handle errors explicitly.
+- [ ] Log for debugging if needed.
+- [ ] Return success.
+
+---
+
+By following these steps, you ensure that new instructions are secure, robust, and consistent with the rest of the codebase.

smartcontract/programs/doublezero-serviceability/src/addresses/mod.rs

@@ -1,4 +1,3 @@
 pub mod devnet;
-pub mod doublezero_foundation;
 pub mod mainnet;
 pub mod testnet;

smartcontract/programs/doublezero-serviceability/src/entrypoint.rs

@@ -71,7 +71,7 @@ use crate::{
                     remove::process_remove_multicast_sub_allowlist,
                 },
             },
-            closeaccount::process_deactivate_multicastgroup,
+            closeaccount::process_closeaccount_multicastgroup,
             create::process_create_multicastgroup,
             delete::process_delete_multicastgroup,
             reactivate::process_reactivate_multicastgroup,
@@ -266,7 +266,7 @@ pub fn process_instruction(
             process_update_multicastgroup(program_id, accounts, &value)?
         }
         DoubleZeroInstruction::DeactivateMulticastGroup(value) => {
-            process_deactivate_multicastgroup(program_id, accounts, &value)?
+            process_closeaccount_multicastgroup(program_id, accounts, &value)?
         }
         DoubleZeroInstruction::AddMulticastGroupPubAllowlist(value) => {
             process_add_multicastgroup_pub_allowlist(program_id, accounts, &value)?

smartcontract/programs/doublezero-serviceability/src/error.rs

@@ -129,6 +129,8 @@ pub enum DoubleZeroError {
     InvalidUserPubkey, // variant 61
     #[error("Invalid Public IP: IP conflicts with DZ prefix")]
     InvalidPublicIp, // variant 62
+    #[error("Invalid Foundation Allowlist: cannot be empty")]
+    InvalidFoundationAllowlist, // variant 63
 }
 
 impl From<DoubleZeroError> for ProgramError {
@@ -197,6 +199,7 @@ impl From<DoubleZeroError> for ProgramError {
             DoubleZeroError::InvalidActualLocation => ProgramError::Custom(60),
             DoubleZeroError::InvalidUserPubkey => ProgramError::Custom(61),
             DoubleZeroError::InvalidPublicIp => ProgramError::Custom(62),
+            DoubleZeroError::InvalidFoundationAllowlist => ProgramError::Custom(63),
         }
     }
 }
@@ -266,6 +269,7 @@ impl From<u32> for DoubleZeroError {
             60 => DoubleZeroError::InvalidActualLocation,
             61 => DoubleZeroError::InvalidUserPubkey,
             62 => DoubleZeroError::InvalidPublicIp,
+            63 => DoubleZeroError::InvalidFoundationAllowlist,
             _ => DoubleZeroError::Custom(e),
         }
     }
@@ -355,6 +359,7 @@ mod tests {
             InvalidActualLocation,
             InvalidUserPubkey,
             InvalidPublicIp,
+            InvalidFoundationAllowlist,
         ];
         for err in variants {
             let pe: ProgramError = err.clone().into();