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

Add token_program constraint to token, mint, and associated token accounts #2460

Merged

Conversation

elliotkennedy
Copy link
Contributor

@elliotkennedy elliotkennedy commented Apr 11, 2023

Overview

It is currently only possible to cpi a single token_program.

As discussed in #2363 & #2386 - it should be possible to override the required token_program account field when intializing a Mint, TokenAccount or AssociatedTokenAccount.

These new constraints allow this, and also act as owner constraints when not used with an init.

This is useful when an instruction needs to support some combination of spl-token and spl-token-2022 mints.

API changes

  • Added a new token_program attribute to TokenAccount, Mint and AssociatedTokenAccount types - this allows you to specify the spl-token or spl-token-2022 token_program field which should be used in constraints and CPI calls - rather than enforcing a single account field called token_program.

Example skeleton endpoint supporting both spl-token and spl-token-2022 mints -

#[derive(Accounts)]
pub struct InitializePool<'info> {

    /// Token A mint
    #[account(
        mint::token_program = token_a_token_program,
    )]
    pub token_a_mint: Box<InterfaceAccount<'info, Mint>>,
    /// Token B mint
    #[account(
        mint::token_program = token_b_token_program,
    )]
    pub token_b_mint: Box<InterfaceAccount<'info, Mint>>,

    #[account(init,
        seeds = [b"pvault_a".as_ref(), pool.key().as_ref(), token_a_mint.key().as_ref()],
        bump,
        payer = admin_authority,
        token::mint = token_a_mint,
        token::authority = pool_authority,
        token::token_program = token_a_token_program,
    )]
    pub token_a_vault: Box<InterfaceAccount<'info, TokenAccount>>,

    #[account(init,
        seeds = [b"pvault_b".as_ref(), pool.key().as_ref(), token_b_mint.key().as_ref()],
        bump,
        payer = admin_authority,
        token::mint = token_b_mint,
        token::authority = pool_authority,
        token::token_program = token_b_token_program,
    )]
    pub token_b_vault: Box<InterfaceAccount<'info, TokenAccount>>,

    pub system_program: Program<'info, System>,
    /// The token program for the token A mint
    pub token_a_token_program: Interface<'info, TokenInterface>,
    /// The token program for the token B mint
    pub token_b_token_program: Interface<'info, TokenInterface>,
}

Discussion

  • In most cases, the owner constraint would be sufficient here, however because of the spl-token cpi call integration in anchor, and the existing requirement to specify a token_program field; a new, specialized, attribute seems correct for this.

Breaking changes

None, where the attribute is not used, the existing token_program account field will be used.

@vercel
Copy link

vercel bot commented Apr 11, 2023

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

A member of the Team first needs to authorize it.

@elliotkennedy elliotkennedy force-pushed the feature/token-program-constraint branch 4 times, most recently from 41c184c to 3a731e9 Compare April 11, 2023 03:42
@elliotkennedy elliotkennedy marked this pull request as ready for review April 11, 2023 04:00
…ounts in order to customise the token program interface which the account refers to
Copy link
Collaborator

@acheroncrypto acheroncrypto left a comment

Choose a reason for hiding this comment

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

Looking great, please see the comments.

lang/src/error.rs Outdated Show resolved Hide resolved
lang/syn/src/lib.rs Outdated Show resolved Hide resolved
lang/syn/src/parser/accounts/constraints.rs Outdated Show resolved Hide resolved
lang/syn/src/parser/accounts/constraints.rs Outdated Show resolved Hide resolved
lang/syn/src/lib.rs Outdated Show resolved Hide resolved
- Add token program constraint for initialized mints.
@elliotkennedy elliotkennedy force-pushed the feature/token-program-constraint branch from e7a54fe to 1a89f5a Compare April 14, 2023 14:43
@elliotkennedy
Copy link
Contributor Author

Looking great, please see the comments.

Thanks so much for your review @acheroncrypto - on addressing the comments I realised that the initialized Mint constraint was not implemented - I have added this in the latest commit. I am currently writing some tests around these constraints, previously I was not aware of the misc test suite and it looks ideal. Will ping when ready for another look.

- Fix ATA `token_program` constraint parsing
- Add `token_program` constraint test cases
@elliotkennedy elliotkennedy force-pushed the feature/token-program-constraint branch from 31fd002 to b8faeca Compare April 16, 2023 20:19
@elliotkennedy
Copy link
Contributor Author

Hey @acheroncrypto, I added a lot of test cases in the last commit - they are summarised in the table below:

init successful initialize Constraint (no init) pass case Constraint (no init) error case init-if-needed successful initialize init-if-needed constraint (no init) pass case init-if-needed constraint (no init) error case
Mint ✔️ ✔️ ✔️ ✔️ ✔️ ✔️
Token ✔️ ✔️ ✔️ ✔️ ✔️ ✔️
ATA ✔️ ✔️ ✔️ ✔️ ✔️ ✔️

I fixed a couple of bugs in the process, mostly around missing init-if-needed constraints.

I did not fix #2447 and there are no test cases around ATA initialization with token-2022; however the token account and mint token-2022 test cases are covered in the new token-wrapper test suite.

I think this is now good to ship - please take another look when you can - thanks!

@acheroncrypto
Copy link
Collaborator

In most cases, the owner constraint would be sufficient here, however because of the spl-token cpi call integration in anchor, and the existing requirement to specify a token_program field; a new, specialized, attribute seems correct for this.

As you've stated, there are some cases that wouldn't work with just an owner constraint but I have to say this does feel a bit of an anti-pattern that we are specifying the program when using InterfaceAccount since the whole point of InterfaceAccount is to deserialize into the given type with multiple owners safely. Not a problem with this PR but I'd like to know your opinion on this too.

on addressing the comments I realised that the initialized Mint constraint was not implemented

Not specifically speaking of this PR but there is quite a bit of duplicate code on these checks which would make us much more likely to miss some of the implementations.

I fixed a couple of bugs in the process, mostly around missing init-if-needed constraints.

Thank you for fixing those and also for writing tests that cover many cases.

@elliotkennedy
Copy link
Contributor Author

In most cases, the owner constraint would be sufficient here, however because of the spl-token cpi call integration in anchor, and the existing requirement to specify a token_program field; a new, specialized, attribute seems correct for this.

As you've stated, there are some cases that wouldn't work with just an owner constraint but I have to say this does feel a bit of an anti-pattern that we are specifying the program when using InterfaceAccount since the whole point of InterfaceAccount is to deserialize into the given type with multiple owners safely. Not a problem with this PR but I'd like to know your opinion on this too.

I take your point ;) - however I think of this as more of a reference from an InterfaceAccount -> Interface; or perhaps a reference to a runtime label in cases where the implementation is ambiguous and can't be inferred safely.

My opinion is that in general there should be a preference for inferring the interface implementation but any time you want to mix implementations of the same interface you will need some some sort of type reference. Particularly with Solana requiring program state up-front.

I think there is a wider question about how interfaces should work in Solana and that probably needs to be addressed in the runtime. I believe it is on the roadmap for v2 but I don't have any idea how it should look. However it is a distinct problem to the current anchor notion of interfaces.

Another thought is perhaps an InterfaceAccount specific new impl/implementation constraint would be preferable to reusing owner but I don't think there is a requirement for it yet.

Another approach would be to inverse the reference/constraint - Interface -> InterfaceAccount but I don't think this would be as intuitive.

I think removing the token_program convention would be less clean for anchor code in general. Perhaps you could generify the token_program field conventional approach by adding a program_field() method to the Interface accounts. This would return the string field name which needs to be added to the Accounts struct. You would also need a constraint to activate it as the InterfaceAccount does not always require the Interface.

There's another question about new token 2022 features - how should they be implemented? Do we continue adding constraints to initialise Mints with new flags?

on addressing the comments I realised that the initialized Mint constraint was not implemented

Not specifically speaking of this PR but there is quite a bit of duplicate code on these checks which would make us much more likely to miss some of the implementations.

I agree :) I think there is definitely scope for reuse around init-if-needed and init - it is too easy to miss atm as they are complete reimplementations of the same thing.

I fixed a couple of bugs in the process, mostly around missing init-if-needed constraints.

Thank you for fixing those and also for writing tests that cover many cases.

Pleasure + very, very relieved to look in the misc package :)

@acheroncrypto
Copy link
Collaborator

My opinion is that in general there should be a preference for inferring the interface implementation but any time you want to mix implementations of the same interface you will need some some sort of type reference. Particularly with Solana requiring program state up-front.

I think we agree on this part but the implementation here is only focused on the token program and any other interface would need to be implemented seperately, which again is going to add lots of duplication. I'm thinking out loud and don't think this is a problem at the moment since token program interface is going to be by far the most important one for people.

There's another question about new token 2022 features - how should they be implemented? Do we continue adding constraints to initialise Mints with new flags?

Good question, I think adding new flags for the new token 2022 features would be what people would expect from Anchor since it's the way Anchor does things but I'm open to new ideas on this subject.

I agree :) I think there is definitely scope for reuse around init-if-needed and init - it is too easy to miss atm as they are complete reimplementations of the same thing.

Yes, definitely! I think this looks good to merge and clean up can be made in a seperate PR.

Copy link
Collaborator

@acheroncrypto acheroncrypto left a comment

Choose a reason for hiding this comment

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

Thanks again for such a great PR! It's well written, no breaking changes, fixes bugs and adds many test cases. 👏

@acheroncrypto acheroncrypto merged commit 670b4f5 into coral-xyz:master Apr 19, 2023
@elliotkennedy
Copy link
Contributor Author

Thanks again for such a great PR! It's well written, no breaking changes, fixes bugs and adds many test cases. clap

Thanks for your review & kind words @acheroncrypto !

@elliotkennedy elliotkennedy deleted the feature/token-program-constraint branch April 19, 2023 08:50
@elliotkennedy elliotkennedy restored the feature/token-program-constraint branch April 19, 2023 08:53
@Dodecahedr0x
Copy link

I'm trying to use this in my project but I get a Signature verification failed error coming from the token account.

I create it like so:

#[account(
    init,
    payer = payer,
    token::mint = mint,
    token::authority = creator,
    token::token_program = token_program
)]
pub token_account: InterfaceAccount<'info, TokenAccount>,

And removing it makes my instruction work. I suppose I need to sign the IX using the token account keys but I don't have them since I want an ATA PDA.

Is this expected behavior? Is there a way around it?

@elliotkennedy
Copy link
Contributor Author

elliotkennedy commented Aug 10, 2023

I'm trying to use this in my project but I get a Signature verification failed error coming from the token account.

I create it like so:

#[account(
    init,
    payer = payer,
    token::mint = mint,
    token::authority = creator,
    token::token_program = token_program
)]
pub token_account: InterfaceAccount<'info, TokenAccount>,

And removing it makes my instruction work. I suppose I need to sign the IX using the token account keys but I don't have them since I want an ATA PDA.

Is this expected behavior? Is there a way around it?

This is expected. You need to use the AssociatedToken type to get anchor to invoke the correct CPIs iirc.

Try -

#[account(
    init,
    payer = payer,
    token::mint = mint,
    token::authority = creator,
    token::token_program = token_program
)]
pub token_account: InterfaceAccount<'info, AssociatedToken>,

Edit: this bug might also be a blocker - #2541

@Dodecahedr0x
Copy link

Dodecahedr0x commented Aug 10, 2023

Thanks for your answer!

Using anchor_spl::associated_token::AssociatedToken gets me this error:

error[E0277]: the trait bound `AssociatedToken: anchor_lang::AccountSerialize` is not satisfied
   --> programs/nft-standard/src/instructions/create_metadata.rs:84:24
    |
84  |     pub token_account: InterfaceAccount<'info, AssociatedToken>,
    |                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `anchor_lang::AccountSerialize` is not implemented for `AssociatedToken`
    |
    = help: the following other types implement trait `anchor_lang::AccountSerialize`:
              AuthoritiesGroup
              UpgradeableLoaderState
              __idl::IdlAccount
              anchor_lang::ProgramData
              anchor_lang::idl::IdlAccount
              anchor_spl::token::Mint
              anchor_spl::token::TokenAccount
              anchor_spl::token_interface::Mint
            and 2 others
note: required by a bound in `anchor_lang::prelude::InterfaceAccount`
   --> /home/dode/.cargo/registry/src/index.crates.io-6f17d22bba15001f/anchor-lang-0.28.0/src/accounts/interface_account.rs:160:39
    |
160 | pub struct InterfaceAccount<'info, T: AccountSerialize + AccountDeserialize + Clone> {
    |                                       ^^^^^^^^^^^^^^^^ required by this bound in `InterfaceAccount`

And another saying field 'owner' of 'InterfaceAccount' is private

@elliotkennedy
Copy link
Contributor Author

Thanks for your answer!

Using anchor_spl::associated_token::AssociatedToken gets me this error:

error[E0277]: the trait bound `AssociatedToken: anchor_lang::AccountSerialize` is not satisfied
   --> programs/nft-standard/src/instructions/create_metadata.rs:84:24
    |
84  |     pub token_account: InterfaceAccount<'info, AssociatedToken>,
    |                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `anchor_lang::AccountSerialize` is not implemented for `AssociatedToken`
    |
    = help: the following other types implement trait `anchor_lang::AccountSerialize`:
              AuthoritiesGroup
              UpgradeableLoaderState
              __idl::IdlAccount
              anchor_lang::ProgramData
              anchor_lang::idl::IdlAccount
              anchor_spl::token::Mint
              anchor_spl::token::TokenAccount
              anchor_spl::token_interface::Mint
            and 2 others
note: required by a bound in `anchor_lang::prelude::InterfaceAccount`
   --> /home/dode/.cargo/registry/src/index.crates.io-6f17d22bba15001f/anchor-lang-0.28.0/src/accounts/interface_account.rs:160:39
    |
160 | pub struct InterfaceAccount<'info, T: AccountSerialize + AccountDeserialize + Clone> {
    |                                       ^^^^^^^^^^^^^^^^ required by this bound in `InterfaceAccount`

And another saying field 'owner' of 'InterfaceAccount' is private

Apologies I misremembered the syntax there -

Try this:

#[account(
    init,
    payer = payer,
    associated_token::mint = mint,
    associated_token::authority = creator,
    associated_token::token_program = token_program
)]
pub token_account: InterfaceAccount<'info, TokenAccount>,

pub associated_token_program: Program<'info, AssociatedToken>,

@Dodecahedr0x
Copy link

Dodecahedr0x commented Aug 10, 2023

It worked! I was missing the associated program but the error got me confused..

Thanks a lot, I will point to this thread on StackExchange

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.

3 participants