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

Introduce the #[interface] attribute for overriding the default discriminator #2728

Merged
merged 10 commits into from
Dec 17, 2023

Conversation

buffalojoec
Copy link
Contributor

@buffalojoec buffalojoec commented Dec 13, 2023

This PR introduces the #[interface] attribute for instruction functions in Anchor programs.

When a function is annotated with #[interface(..)], a particular instruction from an SPL interface - such as SPL Transfer Hook - can be provided as an argument. This argument will instruct Anchor to override the default discriminator (a hash of "global" and the function name) to instead use the interface instruction's discriminator, as defined in the interface crate.

Here's a snippet of a Transfer Hook program written in Anchor:

mod my_hook_program {
    #[interface(spl_transfer_hook_interface::initialize_extra_account_meta_list)]
    pub fn initialize(ctx: Context<Initialize>, metas: Vec<AnchorExtraAccountMeta>) -> Result<()> {
        /* ... */
    }

    #[interface(spl_transfer_hook_interface::execute)]
    pub fn execute(ctx: Context<Execute>, amount: u64) -> Result<()> {
        /* ... */
    }
}

With these annotations in place, these will be the new discriminators, respectively:

Accounts context is unchanged.

Closes #2723

Copy link

vercel bot commented Dec 13, 2023

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

A member of the Team first needs to authorize it.

@buffalojoec
Copy link
Contributor Author

buffalojoec commented Dec 13, 2023

@acheroncrypto A couple of flags:

  • I can't think of an idiomatic way to add support for this in the instruction coder's decode function. Are you open to changing how the layouts are stored?
  • When the interface-instructions feature is enabled, Anchor throws the below warning trace on build. Have you seen this before? (Note this is even if I start from master and just add the feature with no new code)
Error: Function _ZN106_$LT$core..iter..adapters..GenericShunt$LT$I$C$R$GT$$u20$as$u20$core..iter..traits..iterator..Iterator$GT$4next17h7adef51f44f8e442E Stack offset of 30848 exceeded max offset of 4096 by 26752 bytes, please minimize large stack variables
Error: Function _ZN10anchor_syn7codegen8accounts11constraints9linearize17h92d75d9d4dd862feE Stack offset of 22624 exceeded max offset of 4096 by 18528 bytes, please minimize large stack variables
Error: Function _ZN10anchor_syn7codegen8accounts8generics17he85b1e7c96261c3eE Stack offset of 4504 exceeded max offset of 4096 by 408 bytes, please minimize large stack variables
Error: Function _ZN10anchor_syn6parser8accounts11constraints5parse17h9c1a9fbf1ca518f7E Stack offset of 29856 exceeded max offset of 4096 by 25760 bytes, please minimize large stack variables
Error: Function _ZN10anchor_syn6parser8accounts11constraints22ConstraintGroupBuilder5build17hfc3c764227ad11e0E Stack offset of 30848 exceeded max offset of 4096 by 26752 bytes, please minimize large stack variables
Error: Function _ZN10anchor_syn6parser8accounts5parse17h7cbbe77d97239a5aE Stack offset of 14448 exceeded max offset of 4096 by 10352 bytes, please minimize large stack variables
Error: Function _ZN10anchor_syn6parser8accounts19parse_account_field17h49818181fa8cf20aE Stack offset of 36368 exceeded max offset of 4096 by 32272 bytes, please minimize large stack variables

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.

Thank you for taking an interest in contributing to Anchor!

  • I can't think of an idiomatic way to add support for this in the instruction coder's decode function. Are you open to changing how the layouts are stored?

If the current way doesn't allow for it then sure.

  • When the interface-instructions feature is enabled, Anchor throws the below warning trace on build. Have you seen this before? (Note this is even if I start from master and just add the feature with no new code)
Error: Function _ZN106_$LT$core..iter..adapters..GenericShunt$LT$I$C$R$GT$$u20$as$u20$core..iter..traits..iterator..Iterator$GT$4next17h7adef51f44f8e442E Stack offset of 30848 exceeded max offset of 4096 by 26752 bytes, please minimize large stack variables
Error: Function _ZN10anchor_syn7codegen8accounts11constraints9linearize17h92d75d9d4dd862feE Stack offset of 22624 exceeded max offset of 4096 by 18528 bytes, please minimize large stack variables
Error: Function _ZN10anchor_syn7codegen8accounts8generics17he85b1e7c96261c3eE Stack offset of 4504 exceeded max offset of 4096 by 408 bytes, please minimize large stack variables
Error: Function _ZN10anchor_syn6parser8accounts11constraints5parse17h9c1a9fbf1ca518f7E Stack offset of 29856 exceeded max offset of 4096 by 25760 bytes, please minimize large stack variables
Error: Function _ZN10anchor_syn6parser8accounts11constraints22ConstraintGroupBuilder5build17hfc3c764227ad11e0E Stack offset of 30848 exceeded max offset of 4096 by 26752 bytes, please minimize large stack variables
Error: Function _ZN10anchor_syn6parser8accounts5parse17h7cbbe77d97239a5aE Stack offset of 14448 exceeded max offset of 4096 by 10352 bytes, please minimize large stack variables
Error: Function _ZN10anchor_syn6parser8accounts19parse_account_field17h49818181fa8cf20aE Stack offset of 36368 exceeded max offset of 4096 by 32272 bytes, please minimize large stack variables

This is because you've included anchor-syn as a dependency to anchor-lang but anchor-syn is mainly used for creating the macros so it doesn't/shouldn't run in BPF:

anchor/lang/Cargo.toml

Lines 47 to 48 in c402972

# `anchor-syn` should only be included with `idl-build` feature
anchor-syn = { path = "./syn", version = "0.29.0", optional = true }

lang/syn/Cargo.toml Outdated Show resolved Hide resolved
ts/packages/anchor/src/coder/borsh/instruction.ts Outdated Show resolved Hide resolved
ts/packages/anchor/src/program/namespace/methods.ts Outdated Show resolved Hide resolved
@buffalojoec
Copy link
Contributor Author

This is because you've included anchor-syn as a dependency to anchor-lang but anchor-syn is mainly used for creating the macros so it doesn't/shouldn't run in BPF:

Thanks! This did the trick. I've also resolved your other comments. Last piece is the decoder.

@buffalojoec
Copy link
Contributor Author

Ok @acheroncrypto the last commit is the decoder implementation. Actually came out much smoother than I thought. Lmk any thoughts.

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.

Ok @acheroncrypto the last commit is the decoder implementation. Actually came out much smoother than I thought. Lmk any thoughts.

Only minimal changes and it's looking great!

lang/syn/src/parser/spl_interface.rs Show resolved Hide resolved
lang/syn/src/parser/spl_interface.rs Outdated Show resolved Hide resolved
lang/syn/src/parser/spl_interface.rs Show resolved Hide resolved
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.

Awesome work!

Before we get this in, could you note this change in the CHANGELOG?

lang/attribute/program/src/lib.rs Show resolved Hide resolved
ts/packages/anchor/src/program/namespace/methods.ts Outdated Show resolved Hide resolved
@acheroncrypto
Copy link
Collaborator

This looks ready to merge but before we do so, let me know if you have thoughts on the following:

  1. Anchor used to have #[interface] macro which was removed a couple versions ago. I don't think it matters much since it wasn't being used a lot and also last version didn't have it.

  2. This feature could become obsolete once we have custom discriminator support:

    #[discriminator(...)]

    It would be a superset of the #[interface(...)] macro i.e. it would support the same inputs to the current #[interface(...)] macro and some other ways to generate the discriminator. There is no ETA on it yet because it also depends on other features.

    I don't see it as a problem since the changes in this PR are minimal and can easily be ported to the new macro anyway.

I think it's best to merge this right now since people have already been asking for this feature.

@buffalojoec
Copy link
Contributor Author

This looks ready to merge but before we do so, let me know if you have thoughts on the following:

  1. Anchor used to have #[interface] macro which was removed a couple versions ago. I don't think it matters much since it wasn't being used a lot and also last version didn't have it.

  2. This feature could become obsolete once we have custom discriminator support:

    #[discriminator(...)]

    It would be a superset of the #[interface(...)] macro i.e. it would support the same inputs to the current #[interface(...)] macro and some other ways to generate the discriminator. There is no ETA on it yet because it also depends on other features.
    I don't see it as a problem since the changes in this PR are minimal and can easily be ported to the new macro anyway.

I think it's best to merge this right now since people have already been asking for this feature.

I'm willing to rename the macro and the builder method to #[discriminator(..)]/.discriminator(..), however it would mean - at the moment - that any string inputs would be hashed and the first 8 bytes of the hash would be used as the discriminator. If we ever wanted to change this behavior for more customization, we'd have to re-roll the two anyway.

With that being said, I think it might be a little confusing for people if they can use #[discriminator(..)] but can't use any discriminators except for SPL Transfer Hook string-based inputs. It might throw people off, and they might even start requesting more discriminator customization as a result (this could happen anyway, though).

I guess it's completely up to you!

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.

Thank you!

@acheroncrypto acheroncrypto merged commit 13fc0bb into coral-xyz:master Dec 17, 2023
48 of 49 checks passed
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.

Add support for transfer hook programs and overriding the default instruction discriminator
2 participants