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
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion solana/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -65,4 +65,3 @@ codegen-units = 1
opt-level = 3
incremental = false
codegen-units = 1

26 changes: 1 addition & 25 deletions solana/idl/json/dummy_transfer_hook.json
Original file line number Diff line number Diff line change
Expand Up @@ -30,11 +30,6 @@
"isMut": false,
"isSigner": false
},
{
"name": "counter",
"isMut": true,
"isSigner": false
},
{
"name": "systemProgram",
"isMut": false,
Expand Down Expand Up @@ -78,11 +73,6 @@
"docs": [
"computes and the on-chain code correctly passes on the PDA."
]
},
{
"name": "counter",
"isMut": true,
"isSigner": false
}
],
"args": [
Expand All @@ -93,21 +83,7 @@
]
}
],
"accounts": [
{
"name": "Counter",
"type": {
"kind": "struct",
"fields": [
{
"name": "count",
"type": "u64"
}
]
}
}
],
"metadata": {
"address": "BgabMDLaxsyB7eGMBt9L22MSk9KMrL4zY2iNe14kyFP5"
"address": "7R368vaW4Ztt8ShPuBRaaCqSTumLCVMbc1na4ajR5y4G"
}
}
5 changes: 4 additions & 1 deletion solana/idl/json/example_native_token_transfers.json
Original file line number Diff line number Diff line change
Expand Up @@ -870,7 +870,10 @@
{
"name": "emitter",
"isMut": false,
"isSigner": false
"isSigner": false,
"docs": [
"enforced by the [`CpiContext`] call in [`post_message`]."
]
},
{
"name": "wormhole",
Expand Down
48 changes: 0 additions & 48 deletions solana/idl/ts/dummy_transfer_hook.ts
Original file line number Diff line number Diff line change
Expand Up @@ -30,11 +30,6 @@ export type DummyTransferHook = {
"isMut": false,
"isSigner": false
},
{
"name": "counter",
"isMut": true,
"isSigner": false
},
{
"name": "systemProgram",
"isMut": false,
Expand Down Expand Up @@ -78,11 +73,6 @@ export type DummyTransferHook = {
"docs": [
"computes and the on-chain code correctly passes on the PDA."
]
},
{
"name": "counter",
"isMut": true,
"isSigner": false
}
],
"args": [
Expand All @@ -92,20 +82,6 @@ export type DummyTransferHook = {
}
]
}
],
"accounts": [
{
"name": "counter",
"type": {
"kind": "struct",
"fields": [
{
"name": "count",
"type": "u64"
}
]
}
}
]
};

Expand Down Expand Up @@ -141,11 +117,6 @@ export const IDL: DummyTransferHook = {
"isMut": false,
"isSigner": false
},
{
"name": "counter",
"isMut": true,
"isSigner": false
},
{
"name": "systemProgram",
"isMut": false,
Expand Down Expand Up @@ -189,11 +160,6 @@ export const IDL: DummyTransferHook = {
"docs": [
"computes and the on-chain code correctly passes on the PDA."
]
},
{
"name": "counter",
"isMut": true,
"isSigner": false
}
],
"args": [
Expand All @@ -203,19 +169,5 @@ export const IDL: DummyTransferHook = {
}
]
}
],
"accounts": [
{
"name": "counter",
"type": {
"kind": "struct",
"fields": [
{
"name": "count",
"type": "u64"
}
]
}
}
]
};
10 changes: 8 additions & 2 deletions solana/idl/ts/example_native_token_transfers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -870,7 +870,10 @@ export type ExampleNativeTokenTransfers = {
{
"name": "emitter",
"isMut": false,
"isSigner": false
"isSigner": false,
"docs": [
"enforced by the [`CpiContext`] call in [`post_message`]."
]
},
{
"name": "wormhole",
Expand Down Expand Up @@ -2752,7 +2755,10 @@ export const IDL: ExampleNativeTokenTransfers = {
{
"name": "emitter",
"isMut": false,
"isSigner": false
"isSigner": false,
"docs": [
"enforced by the [`CpiContext`] call in [`post_message`]."
]
},
{
"name": "wormhole",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,9 @@ pub struct TransferOwnership<'info> {
pub owner: Signer<'info>,

/// CHECK: This account will be the signer in the [claim_ownership] instruction.
// new_owner is not expected to interact with this instruction. Instead, they call [`claim_ownership`].
// The intention of new_owner is that it could be an arbitrary account so no constraints are
// required here.
new_owner: AccountInfo<'info>,

#[account(
Expand Down Expand Up @@ -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.

// program.
pub transceiver: AccountInfo<'info>,

#[account(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,8 @@ pub struct ReleaseInboundMint<'info> {
/// Setting this flag to `false` is useful when bundling this instruction
/// together with [`crate::instructions::redeem`] in a transaction, so that the minting
/// is attempted optimistically.
/// SECURITY: Signer checks are disabled here because anyone is permitted to send a release
/// transaction.
pub fn release_inbound_mint(
ctx: Context<ReleaseInboundMint>,
args: ReleaseInboundArgs,
Expand Down Expand Up @@ -119,6 +121,8 @@ pub struct ReleaseInboundUnlock<'info> {
/// Setting this flag to `false` is useful when bundling this instruction
/// together with [`crate::instructions::redeem`], so that the unlocking
/// is attempted optimistically.
/// SECURITY: Signer checks are disabled here because anyone is permitted to send a release
/// transaction.
pub fn release_inbound_unlock(
ctx: Context<ReleaseInboundUnlock>,
args: ReleaseInboundArgs,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,9 @@ pub struct TransferBurn<'info> {
pub session_authority: AccountInfo<'info>,
}

// TODO: fees for relaying?
/// Burns tokens and issues a corresponding notification to the outbox of the connected
/// [`NttManagerPeer`].
pub fn transfer_burn(ctx: Context<TransferBurn>, args: TransferArgs) -> Result<()> {
require_eq!(
ctx.accounts.common.config.mode,
Expand Down Expand Up @@ -215,6 +218,11 @@ pub struct TransferLock<'info> {
pub custody: InterfaceAccount<'info, token_interface::TokenAccount>,
}

// TODO: fees for relaying?
// TODO: factor out common bits
/// Locks tokens and issues a corresponding notification to the outbox of the connected
/// [`NttManagerPeer`].
#[allow(unknown_lints)]
pub fn transfer_lock(ctx: Context<TransferLock>, args: TransferArgs) -> Result<()> {
require_eq!(
ctx.accounts.common.config.mode,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,10 @@ pub struct WormholeAccounts<'info> {
pub rent: Sysvar<'info, Rent>,
}

/// SECURITY: Owner checks are disabled. Each of [`WormholeAccounts::bridge`], [`WormholeAccounts::fee_collector`],
/// and [`WormholeAccounts::sequence`] must be checked by the Wormhole core bridge.
/// SECURITY: Signer checks are disabled. The only valid sender is the
/// [`wormhole::PostMessage::emitter`], enforced by the [`CpiContext`] below.
pub fn post_message<'info, A: TypePrefixedPayload>(
wormhole: &WormholeAccounts<'info>,
payer: AccountInfo<'info>,
Expand Down Expand Up @@ -68,6 +72,8 @@ pub fn post_message<'info, A: TypePrefixedPayload>(
Ok(())
}

/// SECURITY: Owner and signer checks are not performed here as this private function is used only by
/// [`post_message`].
fn pay_wormhole_fee<'info>(
wormhole: &WormholeAccounts<'info>,
payer: &AccountInfo<'info>,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@ pub struct BroadcastId<'info> {
seeds = [b"emitter"],
bump
)]
/// CHECK: The only valid sender is the [`wormhole::PostMessage::emitter`]
/// enforced by the [`CpiContext`] call in [`post_message`].
pub emitter: UncheckedAccount<'info>,

pub wormhole: WormholeAccounts<'info>,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,9 @@ pub struct BroadcastPeerArgs {
pub chain_id: u16,
}

/// SECURITY: Owner checks are disabled. [`BroadcastPeer::emitter`] is enforced to be a PDA.
#[allow(unknown_lints)]
#[allow(missing_owner_check)]
pub fn broadcast_peer(ctx: Context<BroadcastPeer>, args: BroadcastPeerArgs) -> Result<()> {
let accs = ctx.accounts;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
//! instruction is able to invoke the program's admin instructions.
//!
//! The instruction needs to be encoded in the VAA payload, with all the
//! accounts. These accounts may be in any order, with two placeholder accounts:
//! accounts. These accounts may be in any order and may include two additional placeholder accounts:
//! - [`OWNER`]: the program will replace this account with the governance PDA
//! - [`PAYER`]: the program will replace this account with the payer account
use std::io;
Expand Down Expand Up @@ -449,6 +449,13 @@ impl From<AccountMeta> for Acc {
}
}

/// Processes a VAA [wormhole_anchor_sdk::wormhole::PostedVaa] sent to this program via a Guardian set.
/// The VAA's payload contains an instruction and relevant Accounts that it requires. This program
/// performs verification of the VAA's contents and then performs a Cross Program Invocation if all
/// verification succeeds.
/// NOTE: The VAA instruction may contain placeholder accounts with Pubkeys set to hard-coded values [OWNER] and [PAYER].
/// These keys are overwritten. Because they are placeholders, we do not need to enforce e.g.
/// ownership checks.
pub fn governance<'info>(ctx: Context<'_, '_, '_, 'info, Governance<'info>>) -> Result<()> {
let vaa_data = ctx.accounts.vaa.data();

Expand All @@ -458,6 +465,12 @@ pub fn governance<'info>(ctx: Context<'_, '_, '_, 'info, Governance<'info>>) ->
bump: ctx.bumps.replay,
});

// Iterate over a copy of all accounts provided in the VAA payload.
kcsongor marked this conversation as resolved.
Show resolved Hide resolved
// If the Pubkey for an account is equal to the hard-coded OWNER constant, overwrite with the
// governance program's Pubkey.
// If the Pubkey for an account is equal to the hard-coded PAYER constant, overwrite with the
// payer program account's pubkey. (This must also be the Signer for the creator of the
// Governance account.)
instruction.accounts.iter_mut().for_each(|acc| {
if acc.pubkey == OWNER {
acc.pubkey = ctx.accounts.governance.key();
Expand Down
Loading