Implement GearDetails Struct and Update Spawn Logic #155
Conversation
WalkthroughAdds a new GearDetails model and implementation, helper functions to generate/validate GearDetails, tests, and updates the core spawn_items API to accept an Array, validate each entry, create/mint items per detail with incremental IDs, and emit a GearSpawned event. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller as Caller
participant Core as CoreSystem
participant Validator as GearDetails.validate
participant IDGen as ID Generator
participant Storage as Storage
participant Mint as Minting
participant Events as EventEmitter
Caller->>Core: spawn_items(gear_details: Array<GearDetails>)
loop for each details in gear_details
Core->>Validator: validate(@details)
alt valid
Core->>IDGen: generate_incremental_ids(details.gear_type)
IDGen-->>Core: item_id
Core->>Storage: write Gear{ id: item_id, ... from details, spawned=true }
Core->>Mint: mint(item_id, quantity=details.total_count)
Core->>Events: collect item_id
else invalid
Core-->>Caller: revert / panic
end
end
Core->>Events: emit GearSpawned(collected_item_ids)
Events-->>Caller: event emitted
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/systems/core.cairo (1)
46-73: Use the correct namespace when reading the Contract in spawn_items
dojo_initwrites theContractmodel into"coa_contracts", butspawn_itemscallsworld_default()(namespace"coa"), soworld.read_model(COA_CONTRACTS)returns an empty struct (admin = 0) and the admin check always fails. Change in src/systems/core.cairo (lines 78–83):- let mut world = self.world_default(); + let mut world = self.world(@"coa_contracts");
🧹 Nitpick comments (8)
src/models/gear.cairo (4)
61-75: Consider deriving Introspect on GearDetails for UI/schema tooling.If your ABI/UI relies on Introspect (common in Dojo/sozo tooling), add it here to avoid extra wrapper types downstream.
Apply:
-#[derive(Drop, Copy, Serde)] +#[derive(Drop, Copy, Serde, Introspect)] pub struct GearDetails {
199-203: Remove no-op non-negativity checks on u256; they’re always true.min_xp_needed is u256, so “>= 0” never fails. Keep validations meaningful and consistent in both new() and validate().
- assert(min_xp_needed >= 0, 'Min XP cannot be negative');- && *self.min_xp_needed >= 0Also applies to: 211-216
185-207: Centralize validation by reusing validate() in new().Avoid duplication and future drift between new() and validate().
) -> GearDetails { - // Validate inputs - assert(gear_type != GearType::None, 'Gear type cannot be None'); - assert(min_xp_needed >= 0, 'Min XP cannot be negative'); - assert(base_damage <= 1000, 'Base damage exceeds max (1000)'); - assert(max_upgrade_level > 0, 'Max upgrade level must be > 0'); - assert(total_count > 0, 'Total count must be > 0'); - - GearDetails { + let details = GearDetails { gear_type, min_xp_needed, base_damage, max_upgrade_level, total_count, variation_ref, - } + }; + assert(details.validate(), 'Invalid GearDetails'); + details }
509-531: Fix should_panic expected syntax for clearer/robust tests.Use a plain string (as in helpers tests) for consistency and to avoid parser quirks.
- #[should_panic(expected: ('Gear type cannot be None',))] + #[should_panic(expected: 'Gear type cannot be None')] ... - #[should_panic(expected: ('Base damage exceeds max (1000)',))] + #[should_panic(expected: 'Base damage exceeds max (1000)')] ... - #[should_panic(expected: ('Max upgrade level must be > 0',))] + #[should_panic(expected: 'Max upgrade level must be > 0')] ... - #[should_panic(expected: ('Total count must be > 0',))] + #[should_panic(expected: 'Total count must be > 0')]src/helpers/gear.cairo (3)
185-201: validate_gear_details_array: early-exit on first invalid is good.Optional: return index of the invalid element for better UX.
-pub fn validate_gear_details_array(details: @Array<GearDetails>) -> bool { +pub fn validate_gear_details_array(details: @Array<GearDetails>) -> bool { let mut i = 0; let mut valid = true;(If you want indices, change the return type to Option and return Some(i) on failure.)
202-214: generate_batch_gear_details: handle zero amount explicitly.Minor clarity; avoids entering the loop when amount == 0.
pub fn generate_batch_gear_details(amount: u32) -> Array<GearDetails> { let mut result = array![]; let mut i = 0; + if amount == 0 { + return result; + }
216-251: Tests: add a zero-amount batch case and assert validate() on all generated.Strengthens boundary coverage and guards against regressions.
#[test] fn test_generate_batch_gear_details() { let details = generate_batch_gear_details(3); assert(details.len() == 3, 'Should generate 3 gear details'); assert(validate_gear_details_array(@details), 'Batch should be valid'); } + + #[test] + fn test_generate_batch_gear_details_zero() { + let details = generate_batch_gear_details(0); + assert(details.len() == 0, 'Zero amount should yield empty array'); + }src/systems/core.cairo (1)
116-123: Event payload omits minted amounts.If UIs need quantities, emit (id, amount) pairs rather than only ids.
- let event = GearSpawned { admin: caller, items }; + // Consider: items_with_amounts: Array<(u256, u256)> + let event = GearSpawned { admin: caller, items };(Requires updating GearSpawned model; optional.)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/helpers/gear.cairo(2 hunks)src/models/gear.cairo(3 hunks)src/systems/core.cairo(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-and-test
🔇 Additional comments (2)
src/helpers/gear.cairo (1)
1-1: Import set looks good.Brings GearDetailsImpl for method resolution (validate) — necessary for method calls on the struct value.
src/systems/core.cairo (1)
6-10: No remaining internal spawn_items calls; ensure external callers, bindings, and ABI/UI reflect the new Array signature.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/systems/core.cairo (1)
78-83: Critical: world namespace mismatch (coa_contracts vs coa) will break admin checks and ERC1155 reads.dojo_init writes Operator/Contract to @"coa_contracts", but most methods (incl. spawn_items) read via world_default() -> @"coa". This makes contract.admin zeroed on reads and will revert admin asserts and/or use wrong addresses.
Fix by aligning world_default with dojo_init:
@@ - fn world_default(self: @ContractState) -> WorldStorage { - self.world(@"coa") - } + fn world_default(self: @ContractState) -> WorldStorage { + // Keep in sync with dojo_init() + self.world(@"coa_contracts") + }Optionally, add a single source-of-truth helper if you must use multiple namespaces.
Also applies to: 46-56, 252-255
♻️ Duplicate comments (1)
src/systems/core.cairo (1)
109-112: Previous panic fixed — good recovery.Setting spawned: false before flipping to true addresses the earlier always-revert bug called out in prior review.
🧹 Nitpick comments (4)
src/systems/core.cairo (4)
98-114: Assert on freshly constructed gear is tautological; drop it (or check storage instead).You initialize spawned: false, then assert(!gear.spawned) which is always true and only wastes gas. If you meant to prevent double-spawn, check storage by attempting a read/exists on item_id.
Minimal gas-saving change:
let mut gear = Gear { id: item_id, item_type, asset_id: item_id, variation_ref: details.variation_ref, total_count: details.total_count, in_action: false, upgrade_level: 0, owner: contract_address_const::<0>(), max_upgrade_level: details.max_upgrade_level, min_xp_needed: details.min_xp_needed, spawned: false, }; - - assert(!gear.spawned, 'Gear already spawned'); gear.spawned = true; world.write_model(@gear);
106-107: Consider aligning model owner with minted custody (warehouse).You mint tokens to contract.warehouse but set model.owner to 0. If any logic or analytics rely on owner, set it to warehouse at spawn for consistency.
- owner: contract_address_const::<0>(), + owner: contract.warehouse,If 0 is intentional to denote “unclaimed,” confirm is_available_for_pickup() and any queries won’t misinterpret this.
Also applies to: 117-119
4-4: Typos in comments.“tournamemnts”, “genrator” → “tournaments”, “generator”.
-/// Spawn tournamemnts and side quests here, if necessary. +/// Spawn tournaments and side quests here, if necessary. @@ - // random gear item genrator + // random gear item generatorAlso applies to: 138-139
43-43: Unused constant.GEAR isn’t referenced; consider removing to avoid warnings.
- const GEAR: felt252 = 'GEAR';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/helpers/gear.cairo(2 hunks)src/systems/core.cairo(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/helpers/gear.cairo
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-and-test
🔇 Additional comments (2)
src/systems/core.cairo (2)
6-6: Imports update looks correct.GearDetails is properly imported alongside Gear.
9-9: Public ABI change: verify all callers and regenerate artifactsFile: src/systems/core.cairo:9
fn spawn_items(ref self: TContractState, gear_details: Array<GearDetails>);spawn_items now takes
Array<GearDetails>. Ensure any external callers, scripts, and UI bindings are updated accordingly and re-generate the ABI and client types.
| assert(!gear.spawned, 'Gear_already_spawned'); | ||
| while i < gear_details.len() { | ||
| let details = *gear_details.at(i); | ||
| assert(details.validate(), 'Invalid gear details'); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Confirm validation enforces safe mint bounds (non-zero, sane caps).
Ensure details.validate() guarantees:
- total_count > 0 and within ERC1155 mint limits,
- max_upgrade_level and min_xp_needed within expected ranges,
- variation_ref and base_damage valid for gear_type.
Run:
Also applies to: 117-119
🏁 Script executed:
#!/bin/bash
# Inspect GearDetails.validate implementation and invariants
rg -n -C3 'impl\s+.*GearDetails' src models helpers
rg -n -C3 '\bfn\s+validate\s*\(' src models helpersLength of output: 1006
Validate must enforce total_count, max_upgrade_level, and variation_ref constraints
validate() currently only checks
- gear_type ≠ None
- min_xp_needed ≥ 0
- base_damage ≤ 1000
Add checks for:
- total_count > 0 and ≤ ERC1155_MAX_MINT
- max_upgrade_level within allowed range
- variation_ref valid for the given gear_type
File: src/models/gear.cairo, fn validate (around lines 210–213)
| let item_id: u256 = self.generate_incremental_ids(details.gear_type.into()); | ||
| let item_type: felt252 = details.gear_type.into(); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify GearType → u256 mapping suits ID partitioning.
generate_incremental_ids groups by item_id.high; confirm Into for GearType sets the gear type in the high 128 bits (low = 0), else IDs won’t partition per type.
Run:
🏁 Script executed:
#!/bin/bash
# Locate GearType->u256 conversion
rg -n -C3 'impl\s+(Into|From)<\s*u256\s*>\s+for\s+GearType' src models helpers
rg -n -C3 'fn\s+into\(\)\s*->\s*u256' src models helpersLength of output: 189
#!/bin/bash
# 1. List all Cairo files containing GearType
rg -l 'GearType' -g '*.cairo'
# 2. For each of those files, search for any Into/From<u256> impls or into() calls
for f in $(rg -l 'GearType' -g '*.cairo'); do
echo "=== $f ==="
rg -n -C3 'impl\s+(Into|From)<\s*u256\s*>\s+for\s+GearType' "$f"
rg -n -C3 'into\(' "$f" | grep -i GearType || true
done🏁 Script executed:
#!/bin/bash
# 1. Locate GearType definition
rg -l 'enum GearType' -g '*.cairo'
rg -n -C3 'enum GearType' -g '*.cairo'
# 2. Search for any explicit impl converting GearType to u256
rg -n -C3 'impl\s+(Into|From)<\s*u256\s*>\s+for\s+GearType' -g '*.cairo'
# 3. Inspect generate_incremental_ids to verify how high/low bits are used
rg -n -C3 'func generate_incremental_ids' -g '*.cairo'Length of output: 347
Map GearType into high 128 bits for proper ID partitioning. The call details.gear_type.into() converts the enum to a u256 with its numeric value in the low 128 bits (high=0), but generate_incremental_ids groups IDs by the high 128 bits—so all items end up in the same partition. In src/systems/core.cairo (lines 95–96), replace the direct into() call with an explicit high-bit assignment, for example:
let type_prefix: u256 = u256{ high: details.gear_type.into(), low: 0 };
let item_id: u256 = self.generate_incremental_ids(type_prefix);🤖 Prompt for AI Agents
In src/systems/core.cairo around lines 95–96, the call details.gear_type.into()
is being converted into the low 128 bits, so generate_incremental_ids sees all
zeros in the high 128 bits and groups all items together; update the code to
construct a u256 with the gear type placed in the high 128 bits and low set to
0, then pass that u256 as the prefix to generate_incremental_ids (i.e., build a
u256 with high = details.gear_type.into() and low = 0 before calling
generate_incremental_ids).
2 participants
Description
Implements the
GearDetailsstruct for item spawning and updatesspawn_itemsto use it. Adds helper functions for randomization and validation. Ensures serialization and tests for default/boundary cases.Changes
GearDetailsstruct inmodels/gear.cairo: Includesgear_type,min_xp_needed,base_damage,max_upgrade_level,total_count,variation_ref. Noidfield.spawn_itemsinsrc/core/CoreActions.cairo: AcceptsArray<GearDetails>, validates, generates IDs, mints items, emitsGearSpawnedevent.helpers/gear.cairo:random_gear_details,validate_gear_details_array,generate_batch_gear_detailswith tests.sozo buildandsozo testpass.Acceptance Criteria
GearDetailsstruct exists, documented, excludesid.spawn_itemsprocessesGearDetails.Testing
sozo build: No errors.sozo test: All tests pass (validation, edge cases, randomization).spawn_itemstesting with sample inputs.Files
models/gear.cairosrc/core/CoreActions.cairohelpers/gear.cairoCloses #93
Summary by CodeRabbit
New Features
Refactor