-
Notifications
You must be signed in to change notification settings - Fork 40
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
base: main
Are you sure you want to change the base?
solana: Add function comments; document some anchor constraints #234
Conversation
@kcsongor I would love your input here regarding the accuracy of my comments and the security concerns about the ownership checks |
1991320
to
4ef566f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great!
solana/programs/wormhole-governance/src/instructions/governance.rs
Outdated
Show resolved
Hide resolved
7100a24
to
39db4fe
Compare
cdbe192
to
0011142
Compare
3605e33
to
3182c6b
Compare
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<...> |
@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 |
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 |
There was a problem hiding this comment.
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.
87bf25c
to
f0484c9
Compare
f4e10ab
to
e9e648f
Compare
e9e648f
to
0cc06b2
Compare
0cc06b2
to
308fa20
Compare
308fa20
to
a3ebbd3
Compare
See also: #232. This PR contains commits from #232.