Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feat: Optional Positional Accounts #2101

Merged
merged 144 commits into from Dec 12, 2022
Merged

Feat: Optional Positional Accounts #2101

merged 144 commits into from Dec 12, 2022

Conversation

stegaBOB
Copy link
Contributor

@stegaBOB stegaBOB commented Aug 4, 2022

Problem

At the moment, the only way to handle accounts that may or may not be passed in to your program is through the remaining_accounts vec. The downside of this is there is no way to easily handle checks and deserialization with the remaining accounts vec, and programs would need to keep track of what accounts should be passed in and it's actually so horrible.

Solution

Implement support for "optional" accounts in the Accounts struct. This would allow for significantly more flexibility while also providing safeguards from shooting oneself in the foot with remaining_accounts

#[derive(Accounts)]
struct OptionalTest<'info> {
    payer: Signer<'info, Pool>,
    ...
    #[account(init, payer = payer, space = AccountStruct::LEN)
    maybe_new: Option<Account<'info, AccountStruct>>,
    // System can now be optional and is tied to whether `maybe_new` is passed in
    system_program: Option<Program<'info, System>>
}

In the example above, the maybe_new account is only created if it is passed in. If not, &ctx.accounts.maybe_new will return None.

How this works?

The key to make everything work is the fact that repeated accounts don't add to the size of the transaction due to how Solana serializes accounts in transactions. We use this fact to denote an optional account with the program Id. When the accounts are passed in, if an account should be ignored, clients will send the program Id instead. In addition, with this implementation, adding on optional accounts at the end of an Accounts struct wouldn't be a breaking change and the program will treat the missing account as None (since older clients will not be passing in this new account).

Thanks to @febo for working on this with me!

stegaBOB and others added 30 commits July 31, 2022 15:32
…/anchor into stegaBOB/feat/optional-accounts
@Henry-E
Copy link
Contributor

Henry-E commented Nov 21, 2022

Under the hood of Option<T>

This is an informal review of the optional accounts pull request.

It adds the ability in Anchor to wrap any generic type T that implements Accounts with an Option<T>. e.g. Option<Signer<'info>> , Option<Program<'info, System>> , Option<Account<'info, DataAccount>>.

The default pubkey that gets passed in when an optional account is None is the program id of the executing program. This is a nice solution because it’s an pubkey that is required with every instruction but doesn’t tend to appear inside the list of accounts in derive(Accounts) structs. However it comes with a number of downsides, namely

  • the program id isn’t available to anchor’s lang modules, e.g. option.rs
  • it can be a little awkward when performing CPI to remember that it’s the target program’s pubkey and not the current program for None accounts and to switch between them appropriately

Additionally it’s important to be aware that the Option<T> doesn’t entirely remove the need for remaining_accounts. For that to be the case, we would need to add Vec<T> to anchor.

The optional functionality changes a reasonable amount in how account constraints are parsed for the macro code generation. It is important to be aware of these changes when adding future constraints or changing existing parsing and macro code. The key points to note are:

  • If an account type is wrapped in Option<T> and it is passed into the program as None, then none of the constraints listed above that account will be checked or executed. E.g. init, has_one, payer, constraint = will all be ignored.
  • The macro code will have to be aware of the possibility that the payer or has_one account constraints may themselves refer to Option<T> accounts. I.e. If has_one = optional_account then
    • the macro code needs to check first whether optional_account is of type Option<T>
    • And if it is of type Option<T> then it needs to add an extra constraint check that optional_account is not None.
  • Some of the internal macro helper functions rely on the inputs already being sanitised and having been checked for Optional<T> inside them. It will be important to not to forget about this requirement of some of the functions! It’s possible there’s some way to enforce that inputs have been correctly sanitised but for now it appears to be purely a comment based approach.

Things that need to be changed or confirmed

  • Does the Option<T> work where T is a nested struct?
  • If it does work with nested structs:
    • Can we add a simple test demonstrating this. Otherwise I’m afraid there might be unexpected behaviours if we don’t double check its usage.
  • If it doesn’t work with nested structs:
    • Can we add a check to abort if people attempt to wrap nested structs in Option<T>
  • Remove the ability to omit optional accounts at the end of an account list. It’s inconsistent with how accounts are normally passed.
  • This functionality appears in at least two places
    • In the macro code, when initing an optional account
    • Inside try_account for the Option<T> struct
  • to_account_infos in Option<T> returns an empty vector when it’s None but in order to be consistent with every other instance of to_account_infos and to_account_metas it should return the program’s account info. Perhaps though this just isn’t possible without extending the macro code.
    • In general it’s unfortunate that the logic for to_account_metas is also in the macro code and not in option.rs . But that is perhaps a problem for future designs of anchor.

Update:

  • The to_account_infos / to_account_metas stuff really isn't that important since it's mostly used by non-anchor programs. So you can skip that if you like
  • We talked and it's cool if we can make omitting optional accounts a feature flag instead

# Conflicts:
#	client/example/src/main.rs
#	tests/misc/tests/misc/misc.ts
@Henry-E
Copy link
Contributor

Henry-E commented Nov 28, 2022

Best to keep the version changes to a separate PR, assuming I can get it merged quick enough for you

@stegaBOB
Copy link
Contributor Author

Best to keep the version changes to a separate PR, assuming I can get it merged quick enough for you

I'll revert that for now. Merging the changes seems to have broken a bunch of things :(

@Henry-E
Copy link
Contributor

Henry-E commented Dec 12, 2022

👏 big round of applause for such a massive contribution here.

Looks good to merge but as with all things smart contract related should treated extremely carefully. The only major issue previously found with anchor was in the init_if_needed code, which allows for optional accounts in a slightly different way than presented here. So anyone coming across this, please be very considerate as to whether you truly need optional accounts in your program or if the client could handle the optional logic instead.

Thanks again for all the hard work @stegaBOB!

@dhruvja
Copy link

dhruvja commented Dec 4, 2023

Are optional accounts allocated in stack by default? If i want to box those accounts, how would i able to do that?
I tried this

/// This failed, due to access violation which i think is because it ran out of memory
escrow_account: Option<Box<Account<'info, TokenAccount>>>, 
// But This worked
escrow_account: Box<Account<'info, TokenAccount>>

How can i allocate optional accounts in heap?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants