anchor icon indicating copy to clipboard operation
anchor copied to clipboard

Add `destroy` account constraint, update `close` account constraint behaviour

Open Henry-E opened this issue 3 years ago • 5 comments

Add a destroy=[target] account constraint which

  • reallocs the account space to 8 bytes
  • sends the rest of the lamports to [target]
  • assigns the CLOSED_ACCOUNT_DISCRIMINATOR This will prevent the account from ever being opened against, effectively _destroying_ the account. Compared with close` which will simply return all the rent from an account.

Update the close account constraint logic

  • sends all lamports to the [target]
  • reallocates space on the account to 0
  • assigns the account to the solana program With this new logic, the CLOSED_ACCOUNT_DISCRIMINATOR is no longer needed as it's not possible to reinitialize the account in subsequent transaction simply by sending lamports to it.

Still TODO

  • [ ] A review from someone else that the new constraint logic is valid
  • [x] Adding a program test to the /misc folder
  • [x] Add documentation for the new destroy constraint, and if needed update the close account constraint

Henry-E avatar Aug 29 '22 13:08 Henry-E

@Henry-E is attempting to deploy a commit to the coral-xyz Team on Vercel.

A member of the Team first needs to authorize it.

vercel[bot] avatar Aug 29 '22 13:08 vercel[bot]

Tests passing, just needs a review @armaniferrante @callensm

Henry-E avatar Aug 30 '22 09:08 Henry-E

destroy is a bit misleading to the purpose of the new constraint, maybe renaming it to something like archive would be more semantic?

callensm avatar Aug 30 '22 15:08 callensm

Since the account is being permanently killed (if I'm understanding this PR correctly), would something potentially even stronger like permanently_destroy or destroy_forever be better?

stegaBOB avatar Aug 30 '22 21:08 stegaBOB

We had a bit of bikeshedding discussion already around the name. Anchor tends to have a preference for single word constraints. While I like the verbosity of destroy_forever I'm not sure what it would mean to destroy_temporarily. With close, the account can be opened again or init in our case. There is no general concept of undestroying something.

Any reviews of the code submission itself though. Just want to someone else to double check that it's doing what's expected.

Henry-E avatar Aug 31 '22 09:08 Henry-E

Ok added a bunch of tests to verify the behaviour the of the new constraint.

  • The destroy function won't work outside of the constraint usage as the function will fail to serialize the data as the account has been resized to only 8 bytes of space
  • Add tests to verify that the account can't be reloaded or reinitialized after being destroyed

Obviously if a smart contract dev really wants to, they can just write a custom function and pass in the destroyed account as an account info, then update all of the values on it manually to allow them to reuse the account or get back the 8 bytes of rent.

Henry-E avatar Sep 23 '22 14:09 Henry-E

The only thing left is to maybe rename since the general consensus is that a different name is needed to indicate what constraint does.

A high level description

  • This new constraint works like the close constraint in that it returns the rent exemption solana used by the account, except that it leaves 8 bytes worth of data to indicate that the account is permanently closed and cannot be reinitialized. This avoids some issues with account reinitialization of closed accounts that can lead to exploits, for example you cannot currently close a serum market'smarket account when not all the open_orders accounts have been closed for this reason.

I suppose with this in mind maybe permanently_close is a better name for what is happening here? @stegaBOB

Henry-E avatar Sep 23 '22 14:09 Henry-E

For now it seems like I'm the only person who wants this and right now it will mess a little bit with how close_accounts works, so just going to ditch it temporarily.

Henry-E avatar Nov 14 '22 17:11 Henry-E