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

Additional anchor types #115

Merged
merged 14 commits into from
Feb 14, 2024
Merged

Additional anchor types #115

merged 14 commits into from
Feb 14, 2024

Conversation

Ikrk
Copy link
Contributor

@Ikrk Ikrk commented Feb 2, 2024

Added support for all Anchor types. Now, any context structure supported by Anchor 0.29.0 should be correctly parsed and the appropriate snapshot structs generated correctly.

Before this change, the parsing was done "manually". Now however we are using the parser directly from Anchor that has been exposed in Anchor 0.29.0 when using the idl-build feature flag. The types Interface and InterfaceAccount were not tested.

Also, this PR changes the way how the snapshot accounts are constructed. Now only the explicitly optional accounts and the accounts that have the init or close constraints are wrapped into the Option type. That make the work with accounts within the check methods easier because it is not necessary to always verify if there is Some account.

Finally, this PR modularized the implementation within the snapshot_generator.rs file to make it more readable.

@Ikrk Ikrk requested a review from lukacan February 2, 2024 16:02
@Ikrk Ikrk changed the base branch from master to develop February 2, 2024 16:10
@Ikrk Ikrk marked this pull request as ready for review February 3, 2024 00:31
@Ikrk
Copy link
Contributor Author

Ikrk commented Feb 3, 2024

The accounts snapshots code generation was changed and the tests do not pass anymore. I will correct it next week.

@lukacan
Copy link
Collaborator

lukacan commented Feb 6, 2024

When used on fuzz_example3, the freshly generated account_snapshots.rs contains an error for "escrow_pda_authority". The account is expected to be:

  • pub escrow_pda_authority: AccountInfo<'info>, // inside WithdrawUnlockedSnapshot

However, the code is generated as:
let escrow_pda_authority = accounts_iter
.next()
.ok_or(FuzzingError::NotEnoughAccounts)?;

where escrow_pda_authority has type Option<AccountInfo<'_>>

Copy link
Collaborator

@lukacan lukacan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is another issue. Currently, the escrow_pda_authority account inside WithdrawUnlockedSnapshot is expected to be of type AccountInfo. However, the bank's client returns None if the account is not found. The get_or_create_account function for the AccountsStorage<PdaStore> implementation derives the address. However, the account corresponding to the derived address is not initialized with the client. Since escrow_pda_authority is also not explicitly initialized within the program, the bank's client always returns None for this account.

  • One possible solution:

Call the set_account function of the bank's client inside get_or_create_account for AccountsStorage<PdaStore>. However, while testing this, I identified at least two scenarios we want to avoid:

  • We always want to set the owner of the PDA account as SYSTEM_PROGRAM_ID. If set to a specific program ID, we will not be able to initialize a new PDA account within the program because the owner will not be system_program -> not able to assign and allocate.
  • Before calling set_account, we need to check if the PDA account is not already initialized. This is because set_account "Create or overwrite an account", so already initialized PDA accounts could be overwritten with fresh accounts -> this is because AccountID for the PDAStore can be different however seeds (and therefore) Address will be the same.

@lukacan
Copy link
Collaborator

lukacan commented Feb 9, 2024

I made some additional testing today and so far I found another issues:

  • Boxed accounts are not correctly generated, field within the XYSnapshot struct expects:
    • Box<Account<'info, T>>,
    • However, boxed account within deserialize_option is of type T (not Boxed)
  • If we use PDA accounts for Data accounts, we can argue that it is in the user`s hands to define correct seeds within each get_accounts instruction for the PDA (Data Account)
    • However for regular keypair accounts, even though the FuzzAccounts structure is global, AccountIDs are local for every instruction -> two instructions -> two (often)different AccountIDs -> two different Accounts -> first account is initialized within the first instruction as data Account -> second account cannot be deserialized for the second instruction as it is a different account.

EDIT: I was further testing account types based on anchor_lang::accounts for Anchor 0.28.0, and my suggestions/observations:

  • Account
    • If PDA, user uses get_or_create_account with seeds specified by them, so even AccountIDs are different across instructions, the user can specify correct seeds -> resulting in the correct PDA
    • If keypair, user can use constant AccountIDs (instead of the ones generated by fuzzer) across multiple instructions resulting in the same account returned from each get_or_create_account -> Initialized Account will correctly deserialize in subsequent deserialize_options functions (however I do not like the constants).
  • AccountInfo, we can maybe wrap this in Option<>
  • AccountLoader, looks good
    • for account read/write within the check function, it is needed to use load/load_mut
    • AccountLoader::try_from checks for the account owner and discriminator, so same case as for the Account?
  • Boxed I would suggest to handle this as Account , without Boxing
    • for Box<InterfaceAccount<T>> , handle it as InterfaceAccount<T>
  • Interface, should be the same as Program
  • InterfaceAccount, should be the same as Account
  • Option<T>, handle as Option<T>
    • we can have Option<Box<T>> , would handle as Option<T>
  • Program, in the user`s hands
  • Signer, no problem here
  • SystemAccount, no problem
  • Sysvar, in user`s hands
  • UncheckedAccount, same as AccountInfo

@Ikrk
Copy link
Contributor Author

Ikrk commented Feb 13, 2024

Finally I have made following modifications:

  • Boxed accounts are handled as normal accounts.
  • Init and close constraints on accounts generate an optional Snapshot account.
    • this is because the same Snapshot struct is used for the snapshot before and after the instruction. In case of account with the Account type, it is not possible to deserialize the account before initialization or after close. Therefore the account will be set to None.
  • In case of AccountInfo, UncheckedAccount and Signer, the user can pass uninitialized accounts to the program and the runtime will provide an empty account to the program. This behavior is simulated to avoid AccountNotFound errors.

@lukacan
Copy link
Collaborator

lukacan commented Feb 14, 2024

Good job !
I did some testing and it looks like everything generates correctly.

@lukacan lukacan merged commit b676b79 into develop Feb 14, 2024
7 checks passed
@lukacan lukacan deleted the additional-anchor-types branch February 14, 2024 12:57
This was referenced Feb 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants