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: Error on undocumented unsafe account types #1452

Conversation

tomlinton
Copy link
Contributor

@tomlinton tomlinton commented Feb 16, 2022

Adds a documentation requirement on use of UncheckedAccount and AccountInfo.

edit: Unless the account has a seeds or init constraint in the account attribute.

Safety checks are run as part of the extract_idl function so they will run on anchor build or anchor idl parse commands. The extract IDL step comes after cargo build-bpf in the build command so this doesn't prevent the program from being built. We could do some additional things like remove the built program if it errors but it seemed a bit heavy handed.

There's also a flag for Anchor.toml:

[features]
safety_checks = false

I thought this might be handy for some of the tests (e.g. auction-house) but probably won't document it in case it gets used all the time. 🤣

Accompanying doc PR here.

Closes #1387

@tomlinton
Copy link
Contributor Author

@armaniferrante what is your preferred approach for fixing up auction-house for these tests? I can add

safety_checks = false

to features in Anchor.toml to skip it all, or add something like /// SAFETY: This is not intended as production code for each account.

I'm not familiar enough with the program to write sensible comments. I could try and figure it out but it'll hold this up for a while.

@armaniferrante
Copy link
Member

@armaniferrante what is your preferred approach for fixing up auction-house for these tests? I can add

safety_checks = false

to features in Anchor.toml to skip it all, or add something like /// SAFETY: This is not intended as production code for each account.

I'm not familiar enough with the program to write sensible comments. I could try and figure it out but it'll hold this up for a while.

Let's just add safety_checks = false.

@tomlinton tomlinton marked this pull request as ready for review February 17, 2022 21:10
Copy link
Member

@armaniferrante armaniferrante left a comment

Choose a reason for hiding this comment

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

We should add an integration test to assert that the build does actually fail if CHECK isn't provided to prevent regressions.

@armaniferrante
Copy link
Member

Writing the test now.

@armaniferrante armaniferrante merged commit 5ff9947 into coral-xyz:master Feb 17, 2022
@mrmizz
Copy link

mrmizz commented Feb 18, 2022

will this go into 0.22.0 ?

@mrmizz
Copy link

mrmizz commented Feb 18, 2022

what is the proper way to use the program.invoke method ?

@mrmizz
Copy link

mrmizz commented Feb 18, 2022

what is the proper way to use the program.invoke method ?

okay I see, use the .to_account_info from an anchor checked account like Signer or the like. right?

@archseer
Copy link
Contributor

I seem to be getting a false positive on all of our PDAs.

    #[account(seeds = [b"foo", state.key().as_ref()], bump = state.load()?.foo_nonce)]
    pub foo_authority: AccountInfo<'info>,

triggers a warning

@armaniferrante
Copy link
Member

@archseer this is expected. Here's the rationale #1452 (comment). More discussion on this is welcomed.

@fastestmannr
Copy link

This results in a bunch of comments for accounts which are not read (just used for their public key) or for accounts in which we only read/change the lamport balance (should be supported by all accounts). Ideally, this would be a type that enforces just these access methods rather than comments. In my ideal world, UncheckedAccount would only expose lamports and public key and none of the other data in it, and then not need a comment.

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.

Compile time warning whenever AccountInfo or UncheckedAccount is used
7 participants