Skip to content

lang: remove default space calc#1519

Merged
armaniferrante merged 12 commits into
otter-sec:masterfrom
paul-schaaf:lang/remove-default-space
Mar 7, 2022
Merged

lang: remove default space calc#1519
armaniferrante merged 12 commits into
otter-sec:masterfrom
paul-schaaf:lang/remove-default-space

Conversation

@paul-schaaf

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

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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
Copy Markdown
Contributor

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

paul-schaaf commented Feb 26, 2022

Copy link
Copy Markdown
Contributor Author

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
Copy Markdown
Contributor

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
Copy Markdown
Contributor

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
Copy Markdown
Contributor

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
Copy Markdown
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
Copy Markdown
Contributor

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.

Comment thread tests/misc/programs/misc/src/context.rs
Comment thread lang/syn/src/lib.rs
Comment thread tests/cfo/programs/cfo/src/lib.rs Outdated
Comment thread tests/cfo/programs/cfo/src/lib.rs Outdated
Comment thread tests/cfo/programs/cfo/src/lib.rs Outdated
Comment thread tests/cfo/programs/cfo/src/lib.rs Outdated
Comment thread tests/ido-pool/programs/ido-pool/src/lib.rs Outdated
Comment thread tests/ido-pool/programs/ido-pool/src/lib.rs Outdated
Comment thread tests/ido-pool/programs/ido-pool/src/lib.rs Outdated
Comment thread tests/misc/programs/misc/src/account.rs
Comment thread lang/syn/src/parser/accounts/constraints.rs Outdated
Comment thread CHANGELOG.md Outdated
@armaniferrante armaniferrante merged commit c8d8cac into otter-sec:master Mar 7, 2022
@utx0

utx0 commented Mar 8, 2022

Copy link
Copy Markdown
Contributor

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

@paul-schaaf

Copy link
Copy Markdown
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

daweth commented Apr 6, 2022

Copy link
Copy Markdown

is there a guide on how to calculate account space?

@ronanyeah

Copy link
Copy Markdown

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

utx0 commented Apr 7, 2022

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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.

Otter-0x4ka5h pushed a commit to Otter-0x4ka5h/anchor that referenced this pull request Mar 25, 2026
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