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

lang: remove default space calc #1519

Merged

Conversation

paul-schaaf
Copy link
Contributor

@paul-schaaf paul-schaaf commented Feb 24, 2022

closes #1563

force usage of the space constraint with init

@paul-schaaf paul-schaaf marked this pull request as draft February 24, 2022 23:07
@paul-schaaf
Copy link
Contributor Author

I remember us talking about this but just wanted to check whether you still think this should be done before I continue? @armaniferrante

@armaniferrante
Copy link
Member

I remember us talking about this but just wanted to check whether you still think this should be done before I continue? @armaniferrante

I'm for it.

@paul-schaaf
Copy link
Contributor Author

paul-schaaf commented Feb 26, 2022

what do you think about adding a len attribute #[len(50)]. Alternatively we could add #[len(x)] attributes on each field. If it's not given on the field, try to infer it. e.g. u64 -> 8. len should be its own attribute and not be an attribute only on #[account] so it can also be used with types that only implement serialize traits

that would then generate a

impl #name {
    pub const LEN: usize = #sum_lengths;
}

If we do this, we'd better do it in a different PR but in the same release imo.

@paul-schaaf paul-schaaf marked this pull request as ready for review February 26, 2022 17:40
@armaniferrante
Copy link
Member

what do you think about adding a len attribute

Happy to discuss it in an issue. At first glance, I'm not super excited about it and would rather explicitly state the length via space.

@armaniferrante
Copy link
Member

Let's wait to merge this until after the next patch release.

pub end_deposits: i64,
pub end_ido: i64,
pub end_escrow: i64,
pub start_ido: i64, // 8
Copy link
Member

Choose a reason for hiding this comment

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

No need to change this. But generally I'm not a fan of these comments. They don't really add any useful information.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • writing the individual parts down explicitly before summing them up reduces likelihood of mistakes
  • it becomes easier to parse mentally because you dont need to go through the calculation steps again. Then again, how often do you need to parse the space of an account? probably not often.

Copy link
Member

Choose a reason for hiding this comment

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

  • the second bullet point is better solved imo by having a cumulative sum comment rather than saying the iindividual sizes.

tests/cfo/programs/cfo/src/lib.rs Outdated Show resolved Hide resolved
tests/cfo/programs/cfo/src/lib.rs Outdated Show resolved Hide resolved
tests/cfo/programs/cfo/src/lib.rs Outdated Show resolved Hide resolved
tests/cfo/programs/cfo/src/lib.rs Outdated Show resolved Hide resolved
tests/ido-pool/programs/ido-pool/src/lib.rs Outdated Show resolved Hide resolved
tests/ido-pool/programs/ido-pool/src/lib.rs Outdated Show resolved Hide resolved
tests/ido-pool/programs/ido-pool/src/lib.rs Outdated Show resolved Hide resolved
tests/misc/programs/misc/src/account.rs Show resolved Hide resolved
lang/syn/src/parser/accounts/constraints.rs Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@armaniferrante armaniferrante merged commit c8d8cac into coral-xyz:master Mar 7, 2022
@utx0
Copy link
Contributor

utx0 commented Mar 8, 2022

Whats the reasoning for removing this? Was quite a handy feature for me?

@paul-schaaf
Copy link
Contributor Author

@utx0 reasoning is in the issue

NBNARADHYA pushed a commit to MLH-Fellowship/anchor that referenced this pull request Mar 8, 2022
NBNARADHYA pushed a commit to MLH-Fellowship/anchor that referenced this pull request Mar 9, 2022
NBNARADHYA pushed a commit to MLH-Fellowship/anchor that referenced this pull request Mar 9, 2022
NBNARADHYA pushed a commit to MLH-Fellowship/anchor that referenced this pull request Mar 9, 2022
NBNARADHYA pushed a commit to MLH-Fellowship/anchor that referenced this pull request Mar 9, 2022
NBNARADHYA pushed a commit to MLH-Fellowship/anchor that referenced this pull request Mar 9, 2022
@daweth
Copy link

daweth commented Apr 6, 2022

is there a guide on how to calculate account space?

@ronanyeah
Copy link

I usually have something like this:

use anchor_client::anchor_lang::AnchorSerialize;

fn main() {
    let pubkey = str::parse::<Pubkey>("11111111111111111111111111111111").unwrap();

    let value = your_program::YourStruct {
        pubkey_option: Some(pubkey),
        integer: u64::MAX,
    };

    println!("size: {}", value.try_to_vec().unwrap().len());
}

@utx0
Copy link
Contributor

utx0 commented Apr 7, 2022

is there a guide on how to calculate account space?

I am doing this:

    #[account(
        init,
        space = 8 + mem::size_of::<PoolState>(),
        payer = payer,
        seeds = [ POOL_STATE_SEED, lp_token_mint.key().as_ref() ],
        bump,
    )]
    pub pool_state: Box<Account<'info, PoolState>>,

@paul-schaaf
Copy link
Contributor Author

there is a guide in the book. https://book.anchor-lang.com/chapter_5/space.html

@utx0 we recommend not using mem::size_of. (see the tic-tac-toe milestone project in the book for an explanation)

also, going forward, please dont use issues to discuss support questions and use the discord instead. https://discord.gg/rnNpg7Ys.

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.

lang: remove space inference via derive(Default)
5 participants