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] Add Token22 Extensions #2789

Merged
merged 35 commits into from
Apr 11, 2024
Merged

Conversation

Bhargavamacha
Copy link
Contributor

This PR adds support for token extension types to be initialized directly in Anchor, using existing syntax. An example of adding a metadata pointer extension looks like:

pub const EXAMPLE_EXTENSIONS: [ExtensionType; 1] = [ExtensionType::MetadataPointer];

#[account(
  init,
  ...
  mint::extensions = EXAMPLE_EXTENSIONS.to_vec(),
  mint::metadata_pointer_data = MetadataPointerInitializeArgs {
    authority: Some(authority.key()),
    metadata_address: Some(mint.key())
  }
)]
pub mint: Box<Account<'info, Mint>> 

This allows creating accounts with extensions in a declarative way using Anchor's account macros. The code has been tested internally across multiple programs. This PR is meant to start a discussion on the API design and architecture. I will add tests and additional extension types in follow up commits once the approach is agreed upon.

Copy link

vercel bot commented Jan 25, 2024

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

A member of the Team first needs to authorize it.

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, this is a great start!

I will add tests and additional extension types in follow up commits once the approach is agreed upon.

I think your approach is on the right track, however, a more Anchor way to write

mint::metadata_pointer_data = MetadataPointerInitializeArgs {
    authority: Some(authority.key()),
    metadata_address: Some(mint.key())
}

would be something like

mint::metadata_pointer_data::authority = authority,
mint::metadata_pointer_data::metadata_address = mint,

@ChewingGlass
Copy link
Contributor

@Bhargavamacha are you planning on addressing the comments? If not I don't mind taking it over the line. We need this at Helium.

@Bhargavamacha
Copy link
Contributor Author

Hey @ChewingGlass yeah I have the changes and we are currently testing internally, I will push them up soon

* add transfer hook

* add transfer hook update

* add group pointer udpate

* add mint close authority

* fix build issue

* update extension login
@Bhargavamacha
Copy link
Contributor Author

Bhargavamacha commented Feb 7, 2024

@acheroncrypto I updated the code to reflect anchor's style, here is an example:

pub const MINT_EXTENSIONS: [ExtensionType; 4] = [
    ExtensionType::MetadataPointer,
    ExtensionType::GroupMemberPointer,
    ExtensionType::TransferHook,
    ExtensionType::MintCloseAuthority,
];
#[account(
    init,
    ...
    mint::extensions = MINT_EXTENSIONS.to_vec(),
    extensions::metadata_pointer::authority = authority.key(),
    extensions::metadata_pointer::metadata_address = mint.key(),
    extensions::group_member_pointer::authority = authority.key(),
    extensions::transfer_hook::authority = authority.key(),
    extensions::close_authority::authority = receiver.key(),
)]
pub mint: Box<InterfaceAccount<'info, Mint>>

Let me know how that looks

@acheroncrypto
Copy link
Collaborator

Looking great!

One more improvement to the syntax would be to make the .key() implicit similar to other constraints:

#[account(
token::mint = mint,
)]
pub token: Account<'info, TokenAccount>,
pub mint: Account<'info, Mint>,

@Bhargavamacha
Copy link
Contributor Author

I used that syntax because those values can be either accounts or pubkeys passed in, so didn't want to assume

@Bhargavamacha
Copy link
Contributor Author

Hey @acheroncrypto all the functions related to token extensions are complete and gtg, lmk when you get a chance to review

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, it looks well-written from an initial look but it would be great to have tests for these before giving a more comprehensive review. Would you be able to add some tests for token extensions in spl-tests?

spl/src/token_2022_extensions/confidential_transfer.rs Outdated Show resolved Hide resolved
spl/src/token_2022_extensions/confidential_transfer_fee.rs Outdated Show resolved Hide resolved
@balmy-gazebo
Copy link

gm, using this for WNS NFTs, any chance this will be merged soon so we can deploy a crate?

niks3089 and others added 2 commits March 1, 2024 16:41
* start tests

* update tests

* remove anchor binary
@Bhargavamacha
Copy link
Contributor Author

Hey @acheroncrypto added tests if you wanna have a look

@Bhargavamacha
Copy link
Contributor Author

Hi @acheroncrypto, when you have a moment, could you take a look at this? There are quite a few people waiting for it, so would really appreciate your review whenever you get the chance. Thanks in advance. I also have a different architectural question. There are some extensions that would need to be initialized depending on what args are passed in the instruction, like permanent delegate. Do you think we could accept Option<Pubkey> as an arg for such kind of extensions so that they don't need to be initialized when not required?

@Aiyualive
Copy link

Gm @acheroncrypto!
May I ask, when do you think this PR could be approved?
I am curious o know since we are slightly blocked by this and we are looking to make proper time estimations for our presale supporters :)

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.

Hi @acheroncrypto, when you have a moment, could you take a look at this? There are quite a few people waiting for it, so would really appreciate your review whenever you get the chance.

Looking forward to merge this asap, however, correct me if I'm wrong but currently only Mint accounts with init attribute seems to be fully supported, and others e.g. extensions constraint seem to be not fully supported i.e., they have parse implementation but they doesn't have codegen implementation:

pub struct ConstraintExtensionGroup {

ConstraintExtensionGroup is unused in the codebase.

Other than that, the changes follow the standards of the codebase and look great, thanks!

I also have a different architectural question. There are some extensions that would need to be initialized depending on what args are passed in the instruction, like permanent delegate. Do you think we could accept Option<Pubkey> as an arg for such kind of extensions so that they don't need to be initialized when not required?

Can we not handle that internally? Not specifying those constraints should be treated the same as passing None.

lang/syn/src/codegen/accounts/constraints.rs Outdated Show resolved Hide resolved
lang/syn/src/lib.rs Outdated Show resolved Hide resolved
lang/syn/src/lib.rs Outdated Show resolved Hide resolved
spl/src/token_2022_extensions/token_metadata.rs Outdated Show resolved Hide resolved
spl/src/token_2022_extensions/token_metadata.rs Outdated Show resolved Hide resolved
spl/src/token_interface.rs Outdated Show resolved Hide resolved
spl/src/token_2022_extensions/permanent_delegate.rs Outdated Show resolved Hide resolved
tests/spl/token-extensions/tests/token-extensions.ts Outdated Show resolved Hide resolved
@Bhargavamacha
Copy link
Contributor Author

Can we not handle that internally? Not specifying those constraints should be treated the same as passing None.
I am talking about the following case:

let permanent_delegate: Option<Pubkey>;
#[account(
   ...
   extensions::permanent_delegate::delegate = permanent_delegate

Since extensions::permanent_delegate::delegate needs to be mentioned in code, but the creator wants it to be initialized based on parameters passed in the instruction.

but currently only Mint accounts with init attribute seems to be fully supported, and others e.g. extensions constraint seem to be not fully supported i.e., they have parse implementation but they doesn't have codegen implementation:

Yes the support is only for init, I am guessing you meant they have codegen implementation and not parse implementation. That's because each extension data can be fetched like below and I think it'll be a waste of compute units trying to parse all the extensions every time

pub fn get_extension_data<T: Extension + Pod>(account: &mut AccountInfo) -> Result<T> {
    let mint_data = account.data.borrow();
    let mint_with_extension = StateWithExtensions::<Mint>::unpack(&mint_data)?;
    let extension_data = *mint_with_extension.get_extension::<T>()?;
    Ok(extension_data)
}

I could possibly just include this function in anchor.

* remove extensions and derive

* use &

* fix &

* remove optional permanent delegate and run pretiiter
* remove extensions and derive

* use &

* fix &

* remove optional permanent delegate and run pretiiter

* formatting

* fix merge issue
spl/Cargo.toml Outdated Show resolved Hide resolved
@Bhargavamacha
Copy link
Contributor Author

borsh 0.9.3 in solana-program 1.18.8 is cyclically linked to an older version of ahash and is causing build failures @acheroncrypto

@acheroncrypto
Copy link
Collaborator

borsh 0.9.3 in solana-program 1.18.8 is cyclically linked to an older version of ahash and is causing build failures @acheroncrypto

Are you on solana-cli 1.18.8? The master branch is also on 1.18.8 (#2867), which doesn't have that problem.

@Bhargavamacha
Copy link
Contributor Author

Yes I am on 1.18.8. fyi, the memory overflow issue seems to be still happening on 1.18.8

@acheroncrypto
Copy link
Collaborator

CI errors seem to be unrelated to ahash:

   Compiling token-proxy v0.1.0 (/home/runner/work/anchor/anchor/tests/spl/token-proxy/programs/token-proxy)
error[E0433]: failed to resolve: could not find `spl_token` in `anchor_spl`
   --> programs/token-proxy/src/lib.rs:189:10
    |
189 | #[derive(Accounts)]
    |          ^^^^^^^^ could not find `spl_token` in `anchor_spl`

@Bhargavamacha
Copy link
Contributor Author

Fixed that error, accidentally feature gated token22. FIgured out the ahash issue, just had to cargo clean my program, was a dangling dependency.

@Bhargavamacha
Copy link
Contributor Author

Bhargavamacha commented Apr 9, 2024

Small issue, since token_2022_extensions is not a default feature, the extension code in codegen fails if the feature is not enabled. I am wondering if we should add a token_2022_extensions feature to lang also, or make token_2022_extensions a default feature(Don't think we should do this since it adds a lot of extra size to the binary)

@Bhargavamacha
Copy link
Contributor Author

CI now passing @acheroncrypto

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.

Small issue, since token_2022_extensions is not a default feature, the extension code in codegen fails if the feature is not enabled. I am wondering if we should add a token_2022_extensions feature to lang also, or make token_2022_extensions a default feature(Don't think we should do this since it adds a lot of extra size to the binary)

Did you check how much extra size it adds? I don't think it would be a lot because we have bench tests here which results in a CI error if there is more than 1% difference in the binary size of the bench program.

Last thing before we merge, could you note this feature in the CHANGELOG?

@Bhargavamacha
Copy link
Contributor Author

done

@balmy-gazebo
Copy link

Will this be included in next deploy? @acheroncrypto

@acheroncrypto
Copy link
Collaborator

Will this be included in next deploy? @acheroncrypto

@balmy-gazebo yeah, doing the last checks and will merge afterwards.

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.

Finally able to merge this, thank you for this amazing PR!

@acheroncrypto acheroncrypto merged commit e3ced78 into coral-xyz:master Apr 11, 2024
48 of 49 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lang next Required for the next release spl
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants