Dojo Code Review

Review your Dojo code for common issues, security concerns, and optimization opportunities.

When to Use This Skill

•"Review my Dojo code"
•"Check this system for issues"
•"Audit my models"
•"Review before deploying"

What This Skill Does

Analyzes your code for:

•Model design patterns
•System implementation issues
•Security vulnerabilities
•Gas optimization opportunities
•Test coverage gaps
•Common mistakes

Quick Start

Interactive mode:

code

"Review my Dojo project"

I'll ask about:

•What to review (models, systems, tests, all)
•Focus areas (security, performance)

Direct mode:

code

"Review the combat system for security issues"
"Check if my models follow ECS patterns"

Review Categories

Model Review

Checks:

•Required trait derivations (Drop, Serde)
•Key fields defined correctly (#[key])
•Keys come before data fields
•Appropriate field types
•Small, focused models (ECS principle)

Common issues:

cairo

// ❌ Missing required traits
#[dojo::model]
struct Position { ... }

// ✅ Required traits
#[derive(Drop, Serde)]
#[dojo::model]
struct Position { ... }

// ❌ Keys after data fields
struct Example {
    data: u32,
    #[key]
    id: u32,  // Must come first!
}

// ✅ Keys first
struct Example {
    #[key]
    id: u32,
    data: u32,
}

System Review

Checks:

•Proper interface definition (#[starknet::interface])
•Contract attribute (#[dojo::contract])
•World access with namespace (self.world(@"namespace"))
•Input validation
•Event emissions
•Error messages

Common issues:

cairo

// ❌ No input validation
fn set_health(ref self: ContractState, health: u8) {
    // Could be zero or invalid!
}

// ✅ Input validation
fn set_health(ref self: ContractState, health: u8) {
    assert(health > 0 && health <= 100, 'invalid health');
}

// ❌ No authorization check for sensitive function
fn admin_function(ref self: ContractState) {
    // Anyone can call!
}

// ✅ Authorization check
fn admin_function(ref self: ContractState) {
    let mut world = self.world_default();
    let caller = get_caller_address();
    assert(
        world.is_owner(selector_from_tag!("my_game"), caller),
        'not authorized'
    );
}

Security Review

Checks:

•Authorization on sensitive functions
•Integer overflow/underflow
•Access control for model writes
•State consistency

Common vulnerabilities:

cairo

// ❌ Integer underflow
health.current -= damage;  // Could underflow if damage > current!

// ✅ Safe subtraction
health.current = if health.current > damage {
    health.current - damage
} else {
    0
};

// ❌ Missing ownership check
fn transfer_nft(ref self: ContractState, token_id: u256, to: ContractAddress) {
    // Anyone can transfer anyone's NFT!
    let mut nft: NFT = world.read_model(token_id);
    nft.owner = to;
    world.write_model(@nft);
}

// ✅ Ownership check
fn transfer_nft(ref self: ContractState, token_id: u256, to: ContractAddress) {
    let mut world = self.world_default();
    let caller = get_caller_address();

    let mut nft: NFT = world.read_model(token_id);
    assert(nft.owner == caller, 'not owner');

    nft.owner = to;
    world.write_model(@nft);
}

Gas Optimization

Checks:

•Minimal model reads/writes
•Efficient data types
•Unnecessary computations

Optimization opportunities:

cairo

// ❌ Multiple reads of same model
let pos: Position = world.read_model(player);
let x = pos.x;
let pos2: Position = world.read_model(player);  // Duplicate!
let y = pos2.y;

// ✅ Single read
let pos: Position = world.read_model(player);
let x = pos.x;
let y = pos.y;

// ❌ Oversized types
struct Position {
    #[key]
    player: ContractAddress,
    x: u128,  // Overkill for coordinates
    y: u128,
}

// ✅ Appropriate types
struct Position {
    #[key]
    player: ContractAddress,
    x: u32,  // Sufficient for most games
    y: u32,
}

Test Coverage

Checks:

•Unit tests for models
•Integration tests for systems
•Edge case coverage
•Failure case testing

Coverage gaps:

cairo

// Missing tests:
// - Boundary values (max health, zero health)
// - Unauthorized access
// - Invalid inputs
// - Full workflow integration

// Add:
#[test]
fn test_health_bounds() { ... }

#[test]
#[should_panic(expected: ('not authorized',))]
fn test_unauthorized_access() { ... }

#[test]
fn test_spawn_move_attack_flow() { ... }

Review Checklist

Models

• All models derive Drop and Serde
• Key fields have #[key] attribute
• Keys come before data fields
• Models are small and focused
• Field types are appropriate size
• Custom types derive Introspect

Systems

• Interface uses #[starknet::interface]
• Contract uses #[dojo::contract]
• World access uses correct namespace
• Input validation for all parameters
• Clear error messages
• Events emitted for important actions
• Caller identity verified when needed

Security

• Sensitive functions check permissions
• Integer math handles over/underflow
• Ownership verified before transfers
• State changes are atomic

Performance

• Minimal model reads per function
• Efficient data types used
• No unnecessary computations

Tests

• Unit tests for models
• Integration tests for systems
• Edge cases tested
• Failure cases tested with #[should_panic]

Common Anti-Patterns

God Models

cairo

// ❌ Everything in one model
#[dojo::model]
struct Player {
    #[key] player: ContractAddress,
    x: u32, y: u32,  // Position
    health: u8, mana: u8,  // Stats
    gold: u32, items: u8,  // Inventory
    level: u8, xp: u32,  // Progress
}

// ✅ Separate concerns (ECS pattern)
Position { player, x, y }
Stats { player, health, mana }
Inventory { player, gold, items }
Progress { player, level, xp }

Missing world_default Helper

cairo

// ❌ Repeating namespace everywhere
fn spawn(ref self: ContractState) {
    let mut world = self.world(@"my_game");
    // ...
}

fn move(ref self: ContractState, direction: u8) {
    let mut world = self.world(@"my_game");
    // ...
}

// ✅ Use internal helper
#[generate_trait]
impl InternalImpl of InternalTrait {
    fn world_default(self: @ContractState) -> dojo::world::WorldStorage {
        self.world(@"my_game")
    }
}

fn spawn(ref self: ContractState) {
    let mut world = self.world_default();
    // ...
}

Not Emitting Events

cairo

// ❌ No events
fn transfer(ref self: ContractState, to: ContractAddress, amount: u256) {
    // Transfer logic but no event
}

// ✅ Emit events for indexing
fn transfer(ref self: ContractState, to: ContractAddress, amount: u256) {
    // Transfer logic
    world.emit_event(@Transferred { from, to, amount });
}

Next Steps

After code review:

•Fix identified issues
•Add missing tests
•Re-run review to verify fixes
•Use dojo-test skill to ensure tests pass
•Use dojo-deploy skill when ready

Related Skills

•dojo-model: Fix model issues
•dojo-system: Fix system issues
•dojo-test: Add missing tests
•dojo-deploy: Deploy after review