Code Review Culture
On Solana, a missed has_one constraint can cost millions. Code review culture balances teaching juniors Sealevel patterns with unblocking shipping - high-signal comments on security and account semantics, low noise on formatting (rustfmt handles that).
Recipe
Quick-reference recipe card - copy-paste ready.
## Solana PR Review Checklist
- [ ] Account owner/signer/writable flags correct
- [ ] PDA seeds and bumps validated
- [ ] CPI program IDs hardcoded or allowlisted
- [ ] Arithmetic checked (no unchecked add)
- [ ] Tests: happy + attacker path (LiteSVM)
- [ ] IDL/client impact noted in PR descriptionWhen to reach for this:
- Onboarding reviewers new to Anchor
- Post-incident "how did this pass review?"
- Setting team SLAs and norms
- Scaling review load across time zones
Working Example
// GOOD review comment - specific, teaches, suggests fix
// "Attacker can pass foreign vault as `user_vault` - add:
// constraint = user_vault.owner == user.key()
// See: sealevel attacks - account substitution"
// LOW-SIGNAL - avoid
// "Rename this variable to `amt`"PR description template:
## Change
Add withdraw_v2 instruction
## Security notes
New CPI to Jupiter; program ID allowlisted in constants
## Test plan
- anchor test
- LiteSVM exploit replay #42 from auditWhat this demonstrates:
- Security-first comment framing with reference material
- PR author surfaces CPI and test evidence upfront
- Reduces back-and-forth on account constraints
Deep Dive
Review SLAs
| PR type | First response | Approval target |
|---|---|---|
| Hotfix | 1 hour | Same day |
| Program logic | 1 business day | 2 days |
| Client UI | 1 business day | 2 days |
Roles
- Author owns tests and description
- Reviewer owns security checklist
- Tech lead breaks tie on architecture
Gotchas
- LGTM without reading
#[account]- #1 failure mode. Fix: Mandatory account section review. - Style nits blocking merge - Demoralizes; rustfmt/clippy in CI. Fix:
[nit]prefix optional. - Rubber-stamp reviews - Audit finds obvious bugs. Fix: Rotate reviewers; spot audits.
- Huge PRs - Nobody reviews deeply. Fix: 400-line guideline; split program/client.
- Review as gatekeeping - Juniors stop asking. Fix: Pair on first Sealevel PR.
Alternatives
| Alternative | Use When | Don't Use When |
|---|---|---|
| Mob review | Complex CPI change | Routine client fix |
| Audit-only | Immutable program | Ongoing upgrades |
| Automated lint only | Typos | Account semantics |
FAQs
How many approvers for program?
Two for production programs/; one for docs-only.
Bot comments?
Clippy, anchor checks, cargo audit in CI - humans focus on semantics.
Review own code?
Never sole approver on own PR for program changes.
Disagreement?
Escalate to tech lead; ADR if architectural.
Teaching in comments?
Link internal doc or Sealevel catalog; keep tone collaborative.
Review IDL changes?
Same rigor as Rust - clients depend on it.
Timezone async?
Video only for RFC; PR async with clear SLA.
Metrics?
Time-to-first-review, comment resolution time, post-merge defect rate.
Intern PRs?
Senior paired reviewer; extra kind explainer comments.
Hotfix bypass?
Post-merge review within 24h; incident commander can merge with one senior.
Related
- Running Design Reviews - pre-PR design
- Mentoring & Leveling - grow reviewers
- Sealevel Attacks Catalog - review reference
Stack versions: This page was written for Agave 4.1.1, Solana CLI 3.0.10, Anchor 0.32.1, anchor-lang 0.32.1, Rust 1.91.1, @solana/kit 7.0.0, Surfpool 0.12.0, and LiteSVM 0.6.x.