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 a constant for ANCHOR_DISCRIMINATOR_SIZE? #3005

Closed
mikemaccana opened this issue Jun 4, 2024 · 5 comments
Closed

Add a constant for ANCHOR_DISCRIMINATOR_SIZE? #3005

mikemaccana opened this issue Jun 4, 2024 · 5 comments
Labels
enhancement New feature or request lang

Comments

@mikemaccana
Copy link
Contributor

mikemaccana commented Jun 4, 2024

Heya! @nickfrosty and I were talking and wondering what you think here.

Right now when teaching Anchor we avoid magic numbers by getting people to add pub const ANCHOR_DISCRIMINATOR: usize = 8; to almost every new project, and get people to write...

space = ANCHOR_DISCRIMINATOR + SomeStruct::INIT_SPACE,

...when they need to initialize new accounts. I'm wondering what a better solution would be:

  • Include an ANCHOR_DISCRIMINATOR const out of the box?
  • Have something like INIT_SPACE that included the discriminator size, eg TOTAL_SPACE or similar?

I'm happy to send a PR. Just LMK what you think.

@acheroncrypto acheroncrypto added enhancement New feature or request lang labels Jun 5, 2024
@acheroncrypto
Copy link
Collaborator

  • Include an ANCHOR_DISCRIMINATOR const out of the box?

There are plans to support dynamic discriminator length which would make this redundant. This is also one of the main reasons why the new IDL spec have required discriminator field.

  • Have something like INIT_SPACE that included the discriminator size, eg TOTAL_SPACE or similar?

Yeah, tbh I think InitSpace should have included the discriminator by default since it's pretty much only being used in init contexts anyway. That being said, making that change now would be a breaking change, and it's likely many people will keep adding the discriminator size if we were to make changes to InitSpace. Instead, we can deprecate InitSpace, and have a new one TotalSpace (or Space for simplicity) like you've suggested.

I also think it's possible to completely hide the discriminator logic from the user's POV.

@Aursen
Copy link
Contributor

Aursen commented Jul 4, 2024

The macro needs to be rewrite totally. The thing with the approach is to differentiate account struct and included struct in the derive macro. Also derive automatically basic types to be able to call for example u64::INIT_SPACE. Also allow to bypass the space with an attribute raw_space. The macro INIT_SPACE is a giant mess

@acheroncrypto
Copy link
Collaborator

FYI, you can use

MyAccount::DISCRIMINATOR.len()

rather than hardcoding or declaring a new constant, which also works with custom discriminators:

space = MyAccount::DISCRIMINATOR.len() + core::mem::size_of::<MyAccount>(),

Closing as this solves the "magic number" problem, and #3184 is more fitting for future discussions.

@mikemaccana
Copy link
Contributor Author

Thanks @acheroncrypto ! I gather DISCRIMINATOR appears in Anchor next release (I can't find it in 0.30.1)?

@acheroncrypto
Copy link
Collaborator

It also exists in v0.30.1, but it's not exported from prelude. You need to bring the trait to scope to get access to the DISCRIMINATOR constant:

use anchor_lang::Discriminator;

The import won't be needed in the next version (#3075).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request lang
Projects
None yet
Development

No branches or pull requests

3 participants