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

solana: Add function comments; document some anchor constraints #234

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

johnsaigle
Copy link
Collaborator

@johnsaigle johnsaigle commented Feb 29, 2024

  • Add linting directives for cargo dylint to ignore ownership and singer checks in particular cases
  • Add function documentation and some clarifying notes

See also: #232. This PR contains commits from #232.

@johnsaigle
Copy link
Collaborator Author

@kcsongor I would love your input here regarding the accuracy of my comments and the security concerns about the ownership checks

Copy link
Contributor

@kcsongor kcsongor left a comment

Choose a reason for hiding this comment

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

great!

@johnsaigle johnsaigle force-pushed the solana/governance-checks branch 2 times, most recently from 7100a24 to 39db4fe Compare March 1, 2024 14:20
@johnsaigle johnsaigle changed the title solana: Add comments and linting directives in governance program solana: Add comments and linting directives Mar 5, 2024
@johnsaigle johnsaigle force-pushed the solana/governance-checks branch 2 times, most recently from cdbe192 to 0011142 Compare March 6, 2024 20:07
@johnsaigle
Copy link
Collaborator Author

johnsaigle commented Mar 7, 2024

@kcsongor Could you re-review when you get some time? This resolves all of the false positives.

If it looks good to you and we merge it, we can also close #232 as this PR contains all of those commits

@kcsongor
Copy link
Contributor

kcsongor commented Mar 7, 2024

I like the comments, but lints themselves seem redundant. Looks like it flags false positives for things that anchor checks. How about we drop the linter and just keep the comments? The convention is moving those account security related comments above the account like

/// CHECK: <comment here>
pub account_foo: Account<...>

@johnsaigle
Copy link
Collaborator Author

@kcsongor Did you change your mind from what we talked about in other PR? #232 (comment) 😅

Ultimately, I think you're right. After working with them a bunch I think these lints are meant for Solana itself more than Anchor. I can rework this PR so that contains only the function comments and the security notes formatted as CHECK: statements

@kcsongor
Copy link
Contributor

kcsongor commented Mar 7, 2024

Apologies for flip-flopping on this! As you worked out all the false positives I think the picture it paints is that it's not as useful as it first seemed.

@@ -204,6 +207,8 @@ pub struct RegisterTransceiver<'info> {
pub payer: Signer<'info>,

#[account(executable)]
// CHECK: Missing ownership check is OK here: Transceiver is intended to be an arbitrary
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@kcsongor I'm actually unsure about this annotation. Do we want to constrain the Transceiver in some way?
This is a privileged action so it's probably OK.

@johnsaigle johnsaigle changed the title solana: Add comments and linting directives solana: Add function comments; document some anchor constaints Mar 8, 2024
@johnsaigle johnsaigle changed the title solana: Add function comments; document some anchor constaints solana: Add function comments; document some anchor constraints Mar 8, 2024
@johnsaigle johnsaigle force-pushed the solana/governance-checks branch 3 times, most recently from f4e10ab to e9e648f Compare March 18, 2024 14:49
@johnsaigle
Copy link
Collaborator Author

I duplicated a lot of this effort in #330 but some of the comments in this PR are still useful. #330 should probably be merged first and I can rebase this PR on that one

@johnsaigle johnsaigle marked this pull request as draft March 21, 2024 19:23
@nik-suri nik-suri added the solana Change to Solana programs label Mar 25, 2024
@nik-suri nik-suri added the documentation Improvements or additions to documentation label May 8, 2024
@johnsaigle johnsaigle marked this pull request as ready for review May 9, 2024 14:11
@johnsaigle johnsaigle removed the on hold label May 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation solana Change to Solana programs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants