Feat: Update AccountsClose to be safe to call manually (#2209)

* fix other lints to make the test pass

(cherry picked from commit d6e43c1ed6a176caa60dfdabe406ec125c3d34cf)

* update close to make it safe to call manually

* fix test script

* re-add safety warnings for deprecated account types

* update close checking logic

* readd logic for deprecated methods

* add additional checks to account_loader in exit

CHANGELOG.md

@@ -27,6 +27,7 @@ The minor version will be incremented upon a breaking change and the patch versi
 * ts: Add nested PDA inference ([#2194](https://github.com/coral-xyz/anchor/pull/2194))
 * ts: Add ability to resolve missing accounts with a custom resolver ([#2194](https://github.com/coral-xyz/anchor/pull/2194))
 * ts: Update the Solana web3 library used by anchor ts to version 1.64.0 ([#2220](https://github.com/coral-xyz/anchor/issues/2220))
+* lang: Updates `AccountsClose` to make it safe to call manually ([#2209](https://github.com/coral-xyz/anchor/pull/2209))
 
 ### Fixes
 

lang/src/accounts/account.rs

@@ -336,8 +336,8 @@ impl<'info, T: AccountSerialize + AccountDeserialize + Owner + Clone> AccountsEx
     for Account<'info, T>
 {
     fn exit(&self, program_id: &Pubkey) -> Result<()> {
-        // Only persist if the owner is the current program.
-        if &T::owner() == program_id {
+        // Only persist if the owner is the current program and the account is not closed.
+        if &T::owner() == program_id && !crate::common::is_closed(&self.info) {
             let info = self.to_account_info();
             let mut data = info.try_borrow_mut_data()?;
             let dst: &mut [u8] = &mut data;
@@ -348,13 +348,6 @@ impl<'info, T: AccountSerialize + AccountDeserialize + Owner + Clone> AccountsEx
     }
 }
 
-/// This function is for INTERNAL USE ONLY.
-/// Do NOT use this function in a program.
-/// Manual closing of `Account<'info, T>` types is NOT supported.
-///
-/// Details: Using `close` with `Account<'info, T>` is not safe because
-/// it requires the `mut` constraint but for that type the constraint
-/// overwrites the "closed account" discriminator at the end of the instruction.
 impl<'info, T: AccountSerialize + AccountDeserialize + Owner + Clone> AccountsClose<'info>
     for Account<'info, T>
 {

lang/src/accounts/account_loader.rs

@@ -235,22 +235,18 @@ impl<'info, T: ZeroCopy + Owner> Accounts<'info> for AccountLoader<'info, T> {
 
 impl<'info, T: ZeroCopy + Owner> AccountsExit<'info> for AccountLoader<'info, T> {
     // The account *cannot* be loaded when this is called.
-    fn exit(&self, _program_id: &Pubkey) -> Result<()> {
-        let mut data = self.acc_info.try_borrow_mut_data()?;
-        let dst: &mut [u8] = &mut data;
-        let mut writer = BpfWriter::new(dst);
-        writer.write_all(&T::discriminator()).unwrap();
+    fn exit(&self, program_id: &Pubkey) -> Result<()> {
+        // Only persist if the owner is the current program and the account is not closed.
+        if &T::owner() == program_id && !crate::common::is_closed(&self.acc_info) {
+            let mut data = self.acc_info.try_borrow_mut_data()?;
+            let dst: &mut [u8] = &mut data;
+            let mut writer = BpfWriter::new(dst);
+            writer.write_all(&T::discriminator()).unwrap();
+        }
         Ok(())
     }
 }
 
-/// This function is for INTERNAL USE ONLY.
-/// Do NOT use this function in a program.
-/// Manual closing of `AccountLoader<'info, T>` types is NOT supported.
-///
-/// Details: Using `close` with `AccountLoader<'info, T>` is not safe because
-/// it requires the `mut` constraint but for that type the constraint
-/// overwrites the "closed account" discriminator at the end of the instruction.
 impl<'info, T: ZeroCopy + Owner> AccountsClose<'info> for AccountLoader<'info, T> {
     fn close(&self, sol_destination: AccountInfo<'info>) -> Result<()> {
         crate::common::close(self.to_account_info(), sol_destination)

lang/src/accounts/loader.rs

@@ -179,21 +179,17 @@ impl<'info, T: ZeroCopy> Accounts<'info> for Loader<'info, T> {
 impl<'info, T: ZeroCopy> AccountsExit<'info> for Loader<'info, T> {
     // The account *cannot* be loaded when this is called.
     fn exit(&self, _program_id: &Pubkey) -> Result<()> {
-        let mut data = self.acc_info.try_borrow_mut_data()?;
-        let dst: &mut [u8] = &mut data;
-        let mut writer = BpfWriter::new(dst);
-        writer.write_all(&T::discriminator()).unwrap();
+        // Only persist if the account is not closed.
+        if !crate::common::is_closed(&self.acc_info) {
+            let mut data = self.acc_info.try_borrow_mut_data()?;
+            let dst: &mut [u8] = &mut data;
+            let mut writer = BpfWriter::new(dst);
+            writer.write_all(&T::discriminator()).unwrap();
+        }
         Ok(())
     }
 }
 
-/// This function is for INTERNAL USE ONLY.
-/// Do NOT use this function in a program.
-/// Manual closing of `Loader<'info, T>` types is NOT supported.
-///
-/// Details: Using `close` with `Loader<'info, T>` is not safe because
-/// it requires the `mut` constraint but for that type the constraint
-/// overwrites the "closed account" discriminator at the end of the instruction.
 #[allow(deprecated)]
 impl<'info, T: ZeroCopy> AccountsClose<'info> for Loader<'info, T> {
     fn close(&self, sol_destination: AccountInfo<'info>) -> Result<()> {

lang/src/accounts/program_account.rs

@@ -99,22 +99,18 @@ impl<'info, T: AccountSerialize + AccountDeserialize + Clone> AccountsExit<'info
     for ProgramAccount<'info, T>
 {
     fn exit(&self, _program_id: &Pubkey) -> Result<()> {
-        let info = self.to_account_info();
-        let mut data = info.try_borrow_mut_data()?;
-        let dst: &mut [u8] = &mut data;
-        let mut writer = BpfWriter::new(dst);
-        self.inner.account.try_serialize(&mut writer)?;
+        // Only persist if the account is not closed.
+        if !crate::common::is_closed(&self.inner.info) {
+            let info = self.to_account_info();
+            let mut data = info.try_borrow_mut_data()?;
+            let dst: &mut [u8] = &mut data;
+            let mut writer = BpfWriter::new(dst);
+            self.inner.account.try_serialize(&mut writer)?;
+        }
         Ok(())
     }
 }
 
-/// This function is for INTERNAL USE ONLY.
-/// Do NOT use this function in a program.
-/// Manual closing of `ProgramAccount<'info, T>` types is NOT supported.
-///
-/// Details: Using `close` with `ProgramAccount<'info, T>` is not safe because
-/// it requires the `mut` constraint but for that type the constraint
-/// overwrites the "closed account" discriminator at the end of the instruction.
 #[allow(deprecated)]
 impl<'info, T: AccountSerialize + AccountDeserialize + Clone> AccountsClose<'info>
     for ProgramAccount<'info, T>

lang/src/common.rs

@@ -1,3 +1,4 @@
+use crate::prelude::{Id, System};
 use crate::Result;
 use solana_program::account_info::AccountInfo;
 use solana_program::system_program;
@@ -12,3 +13,7 @@ pub fn close<'info>(info: AccountInfo<'info>, sol_destination: AccountInfo<'info
     info.assign(&system_program::ID);
     info.realloc(0, false).map_err(Into::into)
 }
+
+pub fn is_closed(info: &AccountInfo) -> bool {
+    info.owner == &System::id() && info.data_is_empty()
+}

lang/src/lib.rs

@@ -242,8 +242,8 @@ pub mod prelude {
         program, require, require_eq, require_gt, require_gte, require_keys_eq, require_keys_neq,
         require_neq, solana_program::bpf_loader_upgradeable::UpgradeableLoaderState, source, state,
         system_program::System, zero_copy, AccountDeserialize, AccountSerialize, Accounts,
-        AccountsExit, AnchorDeserialize, AnchorSerialize, Id, Key, Owner, ProgramData, Result,
-        ToAccountInfo, ToAccountInfos, ToAccountMetas,
+        AccountsClose, AccountsExit, AnchorDeserialize, AnchorSerialize, Id, Key, Owner,
+        ProgramData, Result, ToAccountInfo, ToAccountInfos, ToAccountMetas,
     };
     pub use anchor_attribute_error::*;
     pub use borsh;

tests/misc/programs/misc/src/context.rs

@@ -182,6 +182,22 @@ pub struct TestClose<'info> {
     sol_dest: AccountInfo<'info>,
 }
 
+#[derive(Accounts)]
+pub struct TestCloseTwice<'info> {
+    #[account(mut, close = sol_dest)]
+    pub data: Account<'info, Data>,
+    /// CHECK:
+    pub sol_dest: AccountInfo<'info>,
+}
+
+#[derive(Accounts)]
+pub struct TestCloseMut<'info> {
+    #[account(mut)]
+    pub data: Account<'info, Data>,
+    /// CHECK:
+    pub sol_dest: AccountInfo<'info>,
+}
+
 #[derive(Accounts)]
 pub struct TestU16<'info> {
     #[account(zero)]

tests/misc/programs/misc/src/lib.rs

@@ -124,6 +124,24 @@ pub mod misc {
         Ok(())
     }
 
+    pub fn test_close_twice(ctx: Context<TestCloseTwice>) -> Result<()> {
+        let data_account = &ctx.accounts.data;
+        let sol_dest_info = ctx.accounts.sol_dest.to_account_info();
+        data_account.close(sol_dest_info)?;
+        let data_account_info: &AccountInfo = data_account.as_ref();
+        require_keys_eq!(*data_account_info.owner, System::id());
+        Ok(())
+    }
+
+    pub fn test_close_mut(ctx: Context<TestCloseMut>) -> Result<()> {
+        let data_account = &ctx.accounts.data;
+        let sol_dest_info = ctx.accounts.sol_dest.to_account_info();
+        data_account.close(sol_dest_info)?;
+        let data_account_info: &AccountInfo = data_account.as_ref();
+        require_keys_eq!(*data_account_info.owner, System::id());
+        Ok(())
+    }
+
     pub fn test_instruction_constraint(
         _ctx: Context<TestInstructionConstraint>,
         _nonce: u8,

tests/misc/tests/misc/misc.ts

@@ -283,37 +283,140 @@ describe("misc", () => {
   });
 
   it("Can close an account", async () => {
-    const openAccount = await program.provider.connection.getAccountInfo(
-      data.publicKey
-    );
+    const connection = program.provider.connection;
+    const openAccount = await connection.getAccountInfo(data.publicKey);
+
     assert.isNotNull(openAccount);
+    const openAccountBalance = openAccount.lamports;
+    // double balance to calculate closed balance correctly
+    const transferIx = anchor.web3.SystemProgram.transfer({
+      fromPubkey: provider.wallet.publicKey,
+      toPubkey: data.publicKey,
+      lamports: openAccountBalance,
+    });
+    const transferTransaction = new anchor.web3.Transaction().add(transferIx);
+    await provider.sendAndConfirm(transferTransaction);
 
     let beforeBalance = (
-      await program.provider.connection.getAccountInfo(
-        provider.wallet.publicKey
-      )
+      await connection.getAccountInfo(provider.wallet.publicKey)
     ).lamports;
 
-    await program.rpc.testClose({
-      accounts: {
+    await program.methods
+      .testClose()
+      .accounts({
         data: data.publicKey,
         solDest: provider.wallet.publicKey,
-      },
+      })
+      .postInstructions([transferIx])
+      .rpc();
+
+    let afterBalance = (
+      await connection.getAccountInfo(provider.wallet.publicKey)
+    ).lamports;
+
+    // Retrieved rent exemption sol.
+    expect(afterBalance > beforeBalance).to.be.true;
+
+    const closedAccount = await connection.getAccountInfo(data.publicKey);
+
+    assert.isTrue(closedAccount.data.length === 0);
+    assert.isTrue(closedAccount.owner.equals(SystemProgram.programId));
+  });
+
+  it("Can close an account twice", async () => {
+    const data = anchor.web3.Keypair.generate();
+    await program.methods
+      .initialize(new anchor.BN(10), new anchor.BN(10))
+      .accounts({ data: data.publicKey })
+      .preInstructions([await program.account.data.createInstruction(data)])
+      .signers([data])
+      .rpc();
+
+    const connection = program.provider.connection;
+    const openAccount = await connection.getAccountInfo(data.publicKey);
+    assert.isNotNull(openAccount);
+
+    const openAccountBalance = openAccount.lamports;
+    // double balance to calculate closed balance correctly
+    const transferIx = anchor.web3.SystemProgram.transfer({
+      fromPubkey: provider.wallet.publicKey,
+      toPubkey: data.publicKey,
+      lamports: openAccountBalance,
+    });
+    const transferTransaction = new anchor.web3.Transaction().add(transferIx);
+    await provider.sendAndConfirm(transferTransaction);
+
+    let beforeBalance = (
+      await connection.getAccountInfo(provider.wallet.publicKey)
+    ).lamports;
+
+    await program.methods
+      .testCloseTwice()
+      .accounts({
+        data: data.publicKey,
+        solDest: provider.wallet.publicKey,
+      })
+      .postInstructions([transferIx])
+      .rpc();
+
+    let afterBalance = (
+      await connection.getAccountInfo(provider.wallet.publicKey)
+    ).lamports;
+
+    // Retrieved rent exemption sol.
+    expect(afterBalance > beforeBalance).to.be.true;
+
+    const closedAccount = await connection.getAccountInfo(data.publicKey);
+    assert.isTrue(closedAccount.data.length === 0);
+    assert.isTrue(closedAccount.owner.equals(SystemProgram.programId));
+  });
+
+  it("Can close a mut account manually", async () => {
+    const data = anchor.web3.Keypair.generate();
+    await program.methods
+      .initialize(new anchor.BN(10), new anchor.BN(10))
+      .accounts({ data: data.publicKey })
+      .preInstructions([await program.account.data.createInstruction(data)])
+      .signers([data])
+      .rpc();
+
+    const connection = program.provider.connection;
+    const openAccount = await connection.getAccountInfo(data.publicKey);
+
+    assert.isNotNull(openAccount);
+    const openAccountBalance = openAccount.lamports;
+    // double balance to calculate closed balance correctly
+    const transferIx = anchor.web3.SystemProgram.transfer({
+      fromPubkey: provider.wallet.publicKey,
+      toPubkey: data.publicKey,
+      lamports: openAccountBalance,
     });
+    const transferTransaction = new anchor.web3.Transaction().add(transferIx);
+    await provider.sendAndConfirm(transferTransaction);
+
+    let beforeBalance = (
+      await connection.getAccountInfo(provider.wallet.publicKey)
+    ).lamports;
+
+    await program.methods
+      .testCloseMut()
+      .accounts({
+        data: data.publicKey,
+        solDest: provider.wallet.publicKey,
+      })
+      .postInstructions([transferIx])
+      .rpc();
 
     let afterBalance = (
-      await program.provider.connection.getAccountInfo(
-        provider.wallet.publicKey
-      )
+      await connection.getAccountInfo(provider.wallet.publicKey)
     ).lamports;
 
     // Retrieved rent exemption sol.
     expect(afterBalance > beforeBalance).to.be.true;
 
-    const closedAccount = await program.provider.connection.getAccountInfo(
-      data.publicKey
-    );
-    assert.isNull(closedAccount);
+    const closedAccount = await connection.getAccountInfo(data.publicKey);
+    assert.isTrue(closedAccount.data.length === 0);
+    assert.isTrue(closedAccount.owner.equals(SystemProgram.programId));
   });
 
   it("Can use instruction data in accounts constraints", async () => {