dojo-review

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

Analyzes your code for:

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

Interactive mode:

```

"Review my Dojo project"

```

I'll ask about:

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

Direct mode:

```

"Review the combat system for security issues"

"Check if my models follow ECS patterns"

```

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() { ... }

```

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]

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 });

}

```

After code review:

1. Fix identified issues
2. Add missing tests
3. Re-run review to verify fixes
4. Use dojo-test skill to ensure tests pass
5. Use dojo-deploy skill when ready

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