[ Feat] : impl Incomplete Marketplace System #159
Conversation
WalkthroughExpands the marketplace API with analytics and fee/auction helpers, replaces/remove operations (adds purchase_item, admin_emergency_return), extends removal semantics to include reasons, introduces MarketAnalytics and PlatformFees models, adds FeeUtils and AuctionUtils modules, and enriches events with financial and auction fields. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Buyer
participant Marketplace
participant ERC20 as ERC20 Client
participant ERC1155 as ERC1155 Client
participant Analytics as MarketAnalytics
Buyer->>Marketplace: purchase_item(item_id, quantity)
Marketplace->>Marketplace: _validate_price(...) / _calculate_platform_fee(...)
Marketplace->>ERC20: transfer_from(Buyer -> ContractEscrow, platform_fee)
Marketplace->>ERC20: transfer_from(Buyer -> Seller, seller_amount)
Marketplace->>ERC1155: safe_transfer_from(Escrow -> Buyer, item_id, quantity)
Marketplace-->>Buyer: emit ItemPurchased(... platform_fee, seller_amount)
Marketplace->>Analytics: _track_market_activity(SOLD, total_price)
sequenceDiagram
autonumber
actor User
participant Marketplace
participant ERC20 as ERC20 Client
participant ERC1155 as ERC1155 Client
participant Analytics as MarketAnalytics
User->>Marketplace: start_auction(item_id, duration, starting_bid)
Marketplace->>Marketplace: _validate_auction_duration(duration)
Marketplace-->>User: emit AuctionStarted(starting_bid, duration)
Marketplace->>Analytics: _track_auction_activity(STARTED)
User->>Marketplace: place_bid(auction_id, amount)
Marketplace->>Marketplace: calculate_minimum_bid_increment(...)
Marketplace->>ERC20: transfer_from(bidder -> Escrow, amount)
Marketplace->>ERC20: refund(previous_bidder, previous_bid)
Marketplace-->>User: emit BidPlaced(... previous_bid, previous_bidder)
Marketplace->>Analytics: _track_market_activity(BID)
User->>Marketplace: end_auction(auction_id)
alt has winning bid
Marketplace->>ERC20: settle(platform_fee, seller_amount)
Marketplace->>ERC1155: transfer Escrow -> Winner
Marketplace-->>User: emit AuctionEnded(SOLD)
Marketplace->>Analytics: _track_market_activity(SOLD, final_price)
else no bids
Marketplace->>ERC1155: return item to seller
Marketplace-->>User: emit AuctionEnded(NO_SALE)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (9)
src/market/marketplace.cairo (9)
26-46: Init lacks re‑init guard.
ALREADY_INITIALIZEDexists but is not enforced. Prevent state clobber on repeated init.fn init(ref self: ContractState) { let mut world = self.world_default(); - let contract: Contract = world.read_model(0); + let contract: Contract = world.read_model(0); + // Prevent re-init if config already set (next ids non-zero is a simple guard) + let existing: Config = world.read_model(0); + assert(existing.next_market_id == 0 && existing.next_item_id == 0 && existing.next_auction_id == 0, Errors::ALREADY_INITIALIZED);
80-95: Withdrawing “fees” can drain bidder escrow. Gate withdrawals or track fees.Current logic allows withdrawing any contract balance while auctions can be active and holding bidder funds.
Minimal safe gate (until a fees ledger is added):
fn withdraw_platform_fees(ref self: ContractState, to: ContractAddress, amount: u256) { let mut world = self.world_default(); - let contract: Contract = world.read_model(0); + let contract: Contract = world.read_model(0); + // Disallow withdrawals while auctions are active + let analytics: MarketAnalytics = world.read_model(0); + assert(analytics.active_auctions == 0, Errors::UNAUTHORIZED_CALLER);Recommended follow-up: persist a
fees_accrued: u256and withdraw strictly from that balance.
150-158: Registration fee path misses allowance check.Balance check alone isn’t sufficient; enforce ERC20 allowance before
transfer_from.if caller != contract.admin { let client = self.erc20_client(); - assert( - client.balance_of(caller) >= contract.registration_fee, - Errors::INSUFFICIENT_BALANCE, - ); - client.transfer_from(caller, get_contract_address(), contract.registration_fee); + let fee = contract.registration_fee; + assert(client.balance_of(caller) >= fee, Errors::INSUFFICIENT_BALANCE); + assert(client.allowance(caller, get_contract_address()) >= fee, Errors::INSUFFICIENT_ALLOWANCE); + client.transfer_from(caller, get_contract_address(), fee); }
166-241: ERC1155 operator/escrow approval not verified; transfers may revert.
- Moving items from seller to escrow requires marketplace to be approved operator for the seller.
- Releasing from escrow to buyers requires marketplace to be operator for
escrow_addressunlessescrow_address == get_contract_address().Add pre-transfer approval asserts and emit escrow ops:
let market: MarketData = world.read_model(market_id); assert(market.owner == caller, Errors::UNAUTHORIZED_CALLER); assert(market.is_active, Errors::MARKET_INACTIVE); let client = self.erc1155_client(); +// Ensure marketplace is approved by seller +assert( + client.is_approved_for_all(caller, get_contract_address()), + Errors::INSUFFICIENT_ALLOWANCE +); +// If escrow is external, ensure marketplace is approved by escrow as well +if contract.escrow_address != get_contract_address() { + assert( + client.is_approved_for_all(contract.escrow_address, get_contract_address()), + Errors::INSUFFICIENT_ALLOWANCE + ); +} ... client.safe_batch_transfer_from( caller, contract.escrow_address, ids.span(), amounts.span(), ArrayTrait::new().span(), ); +// Emit escrow operation for observability +// world.emit_event(@EscrowOperation { item_id: 0, operation: 'DEPOSIT', from: caller, to: contract.escrow_address, token_id: 0, quantity: ids.len().into() });Note: You’ll need to import
is_approved_for_allin the ERC1155 dispatcher trait and theEscrowOperationevent at the top.
291-349: Good: purchase splits platform fee and seller proceeds, checks allowance. Add fee event and consider single pull.
- Emit
PlatformFeeCalculatedfor consistent accounting.- Optional: pull full
total_priceinto the contract thentransferseller_amountto the seller. This aligns accounting with auctions and simplifies fee reporting.Minimal event addition:
let platform_fee = self._calculate_platform_fee(total_price); let seller_amount = total_price - platform_fee; ... world.write_model(@item); ... client.transfer_from(caller, get_contract_address(), platform_fee); client.transfer_from(caller, item.owner, seller_amount); ... // Track market activity self._track_market_activity('SOLD', total_price); +// Emit fee calculation +world.emit_event(@PlatformFeeCalculated { + transaction_id: item_id, + total_price, + fee_percentage: FeeUtils::DEFAULT_PLATFORM_FEE_BASIS_POINTS, + platform_fee, + seller_amount, +});
502-554: End auction: fee accounting and escrow emits.
- Emit
PlatformFeeCalculatedon settlement.- Consider emitting
EscrowOperation('RELEASE'/'RETURN')consistently.} else { // Complete the sale with fee calculation let total_bid = auction.highest_bid; let platform_fee = self._calculate_platform_fee(total_bid); let seller_amount = total_bid - platform_fee; ... self.erc20_client().transfer(item.owner, seller_amount); + world.emit_event(@PlatformFeeCalculated { + transaction_id: auction_id, + total_price: total_bid, + fee_percentage: FeeUtils::DEFAULT_PLATFORM_FEE_BASIS_POINTS, + platform_fee, + seller_amount, + }); ... self .erc1155_client() .safe_transfer_from( contract.escrow_address, auction.highest_bidder, item.token_id, 1_u256, ArrayTrait::new().span(), ); + // world.emit_event(@EscrowOperation { item_id: item.item_id, operation: 'RELEASE', from: contract.escrow_address, to: auction.highest_bidder, token_id: item.token_id, quantity: 1_u256 }); }
556-584: Remove item: missing pause guard; quantity not cleared; emit escrow op.fn remove_item_from_market(ref self: ContractState, item_id: u256, reason: felt252) { let caller = get_caller_address(); let mut world = self.world_default(); let mut item: MarketItem = world.read_model(item_id); let contract: Contract = world.read_model(0); let market: MarketData = world.read_model(item.market_id); + assert(!contract.paused, Errors::CONTRACT_PAUSED); assert(item.owner == caller, Errors::NOT_ITEM_OWNER); assert(item.is_available, Errors::ITEM_NOT_AVAILABLE); assert(market.owner == caller, Errors::UNAUTHORIZED_CALLER); assert(market.is_active, Errors::MARKET_INACTIVE); - item.is_available = false; + item.is_available = false; + item.quantity = 0; world.write_model(@item); ... self .erc1155_client() .safe_transfer_from( contract.escrow_address, caller, item.token_id, - item.quantity, + 1_u256, ArrayTrait::new().span(), ); + // world.emit_event(@EscrowOperation { item_id, operation: 'RETURN', from: contract.escrow_address, to: caller, token_id: item.token_id, quantity: 1_u256 });
586-627: Bulk remove: missing reasons length check and pause guard.fn bulk_remove_items( - ref self: ContractState, item_ids: Array<u256>, reasons: Array<felt252>, + ref self: ContractState, item_ids: Array<u256>, reasons: Array<felt252>, ) { let caller = get_caller_address(); let mut world = self.world_default(); let contract: Contract = world.read_model(0); + assert(!contract.paused, Errors::CONTRACT_PAUSED); + assert(item_ids.len() == reasons.len(), Errors::INVALID_PRICES_SIZE);
698-706: Type mismatch: day is u64, model key is u256.Likely a compile error. Cast to u256 before using as a key.
- let day = get_block_timestamp() / SECONDS_PER_DAY; + let day: u256 = (get_block_timestamp() / SECONDS_PER_DAY).into(); ... - let key = (user, day); + let key = (user, day);
🧹 Nitpick comments (4)
src/models/marketplace.cairo (1)
118-127: Error constants: unused and naming consistency.
PLATFORM_FEE_CALCULATION_ERRORis defined but never used; several other new errors are also unused. Either wire them where intended (fee math/escrow paths) or remove to avoid drift.src/market/marketplace.cairo (3)
281-289: Emit escrow operation and guard approvals on mint-to-escrow.After minting to escrow, emit
EscrowOperation('DEPOSIT', ...)for analytics/forensics parity with move_to_market.
408-456: Auction start: error constant and initial bidder semantics.
- The check
assert(item.is_available, Errors::ITEM_NOT_LISTED);should useITEM_NOT_AVAILABLEfor clarity.- Current pattern (seller as initial highest bidder) works with your refund guard, but document this invariant.
-assert(item.is_available, Errors::ITEM_NOT_LISTED); +assert(item.is_available, Errors::ITEM_NOT_AVAILABLE);
752-771: Use the shared ERC1155 client for consistency.Minor consistency nit; also consider emitting
EscrowOperation('RETURN', ...).- IERC1155Dispatcher { contract_address: contract.erc1155 } - .safe_transfer_from( + self.erc1155_client() + .safe_transfer_from(
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/market/interface.cairo(2 hunks)src/market/marketplace.cairo(23 hunks)src/models/marketplace.cairo(7 hunks)
🔇 Additional comments (6)
src/models/marketplace.cairo (2)
25-36: Analytics model looks good; stable base for metrics.No blockers. Fields cover listings, sales, volume, bids, and active auctions.
169-171: ItemPurchased enrichment is correct.Including
platform_feeandseller_amountwill simplify downstream accounting and analytics.src/market/interface.cairo (2)
49-61: FeeUtils is fine and matches basis‑points math.2.5% with 10,000 divisor is correct and consistent with typical bps handling.
18-18: PR summary mismatch — withdraw_platform_fees still present in interfacesrc/market/interface.cairo:18 still declares:
fn withdraw_platform_fees(ref self: TContractState, to: ContractAddress, amount: u256);
Either remove this method from the interface (and update the PR/AI summary) or update all IMarketplace implementors to match.src/market/marketplace.cairo (2)
458-500: Bid handling is mostly solid; refund guard prevents seller windfall.No change requested. The min-increment fix in AuctionUtils will propagate here.
Add a test where
starting_bid > 0, one bid, and ensure contract balance equalsamountpost-bid, with no refund to seller.
678-687: Token client accessors are good.Centralizing token addresses reduces duplication and misreads.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/models/marketplace.cairo (1)
129-131: Add a MarketAnalyticsUpdated event for indexers.
Define an event and emit it whenever analytics are updated.Apply this diff to define the event:
@@ #[derive(Drop, Serde)] #[dojo::event] pub struct PlatformFeesWithdrawn { @@ } +#[derive(Drop, Serde)] +#[dojo::event] +pub struct MarketAnalyticsUpdated { + pub timestamp: u64, + pub total_listings: u256, + pub total_sales: u256, + pub total_volume: u256, + pub total_bids: u256, + pub active_auctions: u256, +}Also applies to: 256-256
src/market/marketplace.cairo (1)
605-646: bulk_remove_items: validate reasons length and respect pause.
Add paused guard and parallel-array length check (and use new INVALID_REASONS_SIZE).Apply this diff:
fn bulk_remove_items( ref self: ContractState, item_ids: Array<u256>, reasons: Array<felt252>, ) { let caller = get_caller_address(); let mut world = self.world_default(); let contract: Contract = world.read_model(0); + assert(!contract.paused, Errors::CONTRACT_PAUSED); + assert(item_ids.len() == reasons.len(), Errors::INVALID_REASONS_SIZE);
♻️ Duplicate comments (7)
src/models/marketplace.cairo (1)
247-255: EmergencyAction event is never emitted; wire it up.
Emit on pause/unpause and admin_emergency_return.Apply this diff to emit in pause/unpause (emit details you prefer):
@@ fn pause_marketplace(ref self: ContractState) { @@ contract.paused = true; world.write_model(@contract); + world.emit_event(@MarketplaceEmergencyAction { + action: 'PAUSE', + admin: caller, + timestamp: get_block_timestamp(), + details: 'OK', + }); } @@ fn unpause_marketplace(ref self: ContractState) { @@ contract.paused = false; world.write_model(@contract); + world.emit_event(@MarketplaceEmergencyAction { + action: 'UNPAUSE', + admin: caller, + timestamp: get_block_timestamp(), + details: 'OK', + }); }src/market/marketplace.cairo (5)
193-201: move_to_market: consider emitting PriceValidationFailed/EscrowOperation events.
You have related error/event intents; emit them where assertions can fail and around escrow transfers for observability.Also applies to: 215-217, 238-240
521-573: end_auction: finalize sale but don’t account net fees.
Platform fee remains in contract (good), but get_total_collected_fees view mislabels balance as “fees.” Track fees in storage and increment here by platform_fee, or rename the view.Example storage-backed increment (pseudo):
- let platform_fee = self._calculate_platform_fee(total_bid); + let platform_fee = self._calculate_platform_fee(total_bid); + // total_collected_fees += platform_fee; world.write_model(@fees);
681-689: get_total_collected_fees returns raw balance, not net fees.
Same issue as interface; fix semantics or rename API.Suggested rename (non-breaking alternative: keep function but document clearly and add a new get_contract_token_balance).
740-769: Analytics writes lack event emission.
Emit MarketAnalyticsUpdated after updates to keep indexers in sync.Apply this diff:
@@ fn _track_market_activity - world.write_model(@analytics); + world.write_model(@analytics); + world.emit_event(@MarketAnalyticsUpdated { + timestamp: get_block_timestamp(), + total_listings: analytics.total_listings, + total_sales: analytics.total_sales, + total_volume: analytics.total_volume, + total_bids: analytics.total_bids, + active_auctions: analytics.active_auctions, + }); @@ fn _track_auction_activity - world.write_model(@analytics); + world.write_model(@analytics); + world.emit_event(@MarketAnalyticsUpdated { + timestamp: get_block_timestamp(), + total_listings: analytics.total_listings, + total_sales: analytics.total_sales, + total_volume: analytics.total_volume, + total_bids: analytics.total_bids, + active_auctions: analytics.active_auctions, + });
35-46: Initialize analytics: also emit MarketAnalyticsUpdated.
Emit after first write so indexers have a baseline.Apply this diff (and add import of MarketAnalyticsUpdated at top):
@@ - world.write_model(@analytics); + world.write_model(@analytics); + world.emit_event(@MarketAnalyticsUpdated { + timestamp: get_block_timestamp(), + total_listings: 0, + total_sales: 0, + total_volume: 0, + total_bids: 0, + active_auctions: 0, + });Also extend the imports list:
- MarketRegistered, ItemsMovedToMarket, GearAddedToMarket, ItemPurchased, AuctionStarted, - BidPlaced, AuctionEnded, ItemRemovedFromMarket, ItemPriceUpdated, ConfigUpdated, + MarketRegistered, ItemsMovedToMarket, GearAddedToMarket, ItemPurchased, AuctionStarted, + BidPlaced, AuctionEnded, ItemRemovedFromMarket, ItemPriceUpdated, ConfigUpdated, + MarketAnalyticsUpdated,src/market/interface.cairo (1)
45-47: get_total_collected_fees mislabels contract balance as “fees.”
Either track net fees in storage or rename to avoid confusion (mirrors past feedback).Suggested direction (storage-backed total):
@@ pub trait IMarketplace<TContractState> { - fn get_total_collected_fees(self: @TContractState) -> u256; + fn get_total_collected_fees(self: @TContractState) -> u256; // return storage-backed net feesAnd update impl to read storage (see marketplace.cairo comment).
🧹 Nitpick comments (8)
src/models/marketplace.cairo (2)
88-127: Errors set has overlap/unused constants; tighten it.
Both INVALID_AUCTION_DURATION and AUCTION_DURATION_TOO_SHORT/LONG exist; pick one approach. PLATFORM_FEE_CALCULATION_ERROR and ESCROW_TRANSFER_FAILED aren’t used. Consider pruning or wiring them where intended.
88-127: Add INVALID_REASONS_SIZE error.
Used by bulk_remove_items to validate parallel arrays.Apply this diff:
@@ pub mod Errors { - pub const INVALID_PRICES_SIZE: felt252 = 'Invalid prices/items size'; + pub const INVALID_PRICES_SIZE: felt252 = 'Invalid prices/items size'; + pub const INVALID_REASONS_SIZE: felt252 = 'Invalid reasons/items size';src/market/interface.cairo (1)
50-62: FeeUtils: LGTM.
Math is correct (2.5% via BPS). Consider using calculate_seller_amount in impl for consistency.src/market/marketplace.cairo (5)
169-177: Registration fee flow: check allowance for UX consistency.
Other flows pre-check allowance; do the same here.Apply this diff:
if caller != contract.admin { let client = self.erc20_client(); assert( client.balance_of(caller) >= contract.registration_fee, Errors::INSUFFICIENT_BALANCE, ); + let allowance = client.allowance(caller, get_contract_address()); + assert(allowance >= contract.registration_fee, Errors::INSUFFICIENT_ALLOWANCE); client.transfer_from(caller, get_contract_address(), contract.registration_fee); }
326-343: purchase_item flow: LGTM with fee split and allowance checks.
Consider using FeeUtils::calculate_seller_amount for symmetry.Also applies to: 355-367
575-603: remove_item_from_market ignores pause state.
Other state-mutating ops guard with CONTRACT_PAUSED. Add the same.Apply this diff:
let mut item: MarketItem = world.read_model(item_id); let contract: Contract = world.read_model(0); let market: MarketData = world.read_model(item.market_id); + assert(!contract.paused, Errors::CONTRACT_PAUSED);
727-739: Price bounds are hardcoded; prefer configurability.
Expose min/max via storage or constants in interface to avoid magic numbers.
790-803: Auction auto-extension: OK but guard against underflow.
You assert end_time in place_bid; still, prefer checked math or early return if current_time >= end_time.Apply this diff:
let current_time = get_block_timestamp(); - let time_remaining = auction.end_time - current_time; + if current_time >= auction.end_time { return; } + let time_remaining = auction.end_time - current_time;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/market/interface.cairo(2 hunks)src/market/marketplace.cairo(24 hunks)src/models/marketplace.cairo(7 hunks)
🔇 Additional comments (11)
src/models/marketplace.cairo (5)
25-36: MarketAnalytics model: LGTM.
Structure is sane for on-chain counters.
169-171: ItemPurchased enriched payload: LGTM.
Fee fields are correctly added.
210-213: AuctionStarted fields: LGTM.
Including starting_bid and duration improves traceability.
223-225: BidPlaced fields: LGTM.
previous_bid and previous_bidder are useful for indexers.
239-245: ItemRemovedFromMarket reason: LGTM.
Reason field will help audits.src/market/interface.cairo (3)
21-22: admin_emergency_return in API: OK; ensure impl updates state and emits events.
Confirm item state is cleared and both ItemRemovedFromMarket and MarketplaceEmergencyAction are emitted.
27-31: purchase/remove API updates: LGTM.
purchase_item addition and reasoned removes align with objectives.
66-77: AuctionUtils: LGTM, BPS math fixed.
DEFAULT_BID_INCREMENT_BASIS_POINTS=500 with /10000 is correct.src/market/marketplace.cairo (3)
303-305: Analytics tracking on mint: LGTM.
Counts volume as price*quantity—reasonable.
477-519: Bid flow: LGTM.
Min increment via BPS, immediate refund of previous bidder, and allowance checks look solid.
24-24: Verify market call-sites & subgraph mappingsScanned repo: signatures are declared in src/market/interface.cairo and implemented in src/market/marketplace.cairo (purchase_item — line 310; remove_item_from_market — line 575; bulk_remove_items — line 605). No internal call-sites invoking these functions were found (only their definitions). No emit_event(@MarketAnalyticsUpdated) was found; marketplace emits MarketRegistered, ItemsMovedToMarket, GearAddedToMarket, ItemPriceUpdated, ItemRemovedFromMarket and auction-related events.
Verify external callers (frontend, other contracts, tests) were updated to the new signatures and update your subgraph/indexer mappings and ABI to index the new/renamed events (add MarketAnalyticsUpdated if it should exist).
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/market/marketplace.cairo (4)
92-101: Fee withdrawal ignores accounting; can drain bidder deposits.Must cap withdrawals by available protocol fees and update total_withdrawn_fees.
fn withdraw_platform_fees(ref self: ContractState, to: ContractAddress, amount: u256) { let mut world = self.world_default(); - let contract: Contract = world.read_model(0); + let contract: Contract = world.read_model(0); let caller = get_caller_address(); assert(caller == contract.admin, Errors::NOT_ADMIN); - let client = self.erc20_client(); + let client = self.erc20_client(); let contract_address = get_contract_address(); let contract_balance = client.balance_of(contract_address); - assert(amount <= contract_balance, Errors::INSUFFICIENT_BALANCE); + assert(amount <= contract_balance, Errors::INSUFFICIENT_BALANCE); + + // Enforce fee accounting + let mut platform_fees: PlatformFees = world.read_model(0); + let available = platform_fees.total_collected_fees - platform_fees.total_withdrawn_fees; + assert(amount <= available, Errors::INSUFFICIENT_BALANCE); client.transfer(to, amount); - let event = PlatformFeesWithdrawn { to, amount, withdrawn_by: caller }; + // Update accounting + platform_fees.total_withdrawn_fees += amount; + world.write_model(@platform_fees); + + let event = PlatformFeesWithdrawn { to, amount, withdrawn_by: caller }; world.emit_event(@event); }
103-123: Emit emergency actions for pause/unpause.Operationally useful and matches MarketplaceEmergencyAction event.
fn pause_marketplace(ref self: ContractState) { let mut world = self.world_default(); let mut contract: Contract = world.read_model(0); let caller = get_caller_address(); assert(caller == contract.admin, Errors::NOT_ADMIN); contract.paused = true; world.write_model(@contract); + world.emit_event(@MarketplaceEmergencyAction { + action: 'PAUSE', + admin: caller, + timestamp: get_block_timestamp(), + details: 'MARKET_PAUSED', + }); } fn unpause_marketplace(ref self: ContractState) { let mut world = self.world_default(); let mut contract: Contract = world.read_model(0); let caller = get_caller_address(); assert(caller == contract.admin, Errors::NOT_ADMIN); contract.paused = false; world.write_model(@contract); + world.emit_event(@MarketplaceEmergencyAction { + action: 'UNPAUSE', + admin: caller, + timestamp: get_block_timestamp(), + details: 'MARKET_UNPAUSED', + }); }
547-599: End-auction settlement: add “no-bid” item removal signal.When returning to seller, emit ItemRemovedFromMarket for parity with other exits.
if auction.highest_bidder == item.owner { // No valid bids - return item to seller - self._return_item_to_seller(auction.item_id); + self._return_item_to_seller(auction.item_id); + world.emit_event(@ItemRemovedFromMarket { + item_id: auction.item_id, owner: item.owner, reason: 'AUCTION_NO_BID', + }); } else {
631-672: Validate reasons length matches item_ids.Currently may panic on reasons.at(i). Add length check once.
fn bulk_remove_items( ref self: ContractState, item_ids: Array<u256>, reasons: Array<felt252>, ) { + assert(item_ids.len() == reasons.len(), Errors::INVALID_PRICES_SIZE);
♻️ Duplicate comments (3)
src/market/marketplace.cairo (2)
779-808: Emit MarketAnalyticsUpdated after writes.This was raised earlier; still missing. Emit after both trackers to keep indexers synced.
world.write_model(@analytics); +world.emit_event(@MarketAnalyticsUpdated { + timestamp: get_block_timestamp(), + total_listings: analytics.total_listings, + total_sales: analytics.total_sales, + total_volume: analytics.total_volume, + total_bids: analytics.total_bids, + active_auctions: analytics.active_auctions, +});Apply the same in _track_auction_activity after write_model.
810-827: _return_item_to_seller should reflect full state transition via event.world.write_model(@item); IERC1155Dispatcher { contract_address: contract.erc1155 } .safe_transfer_from( contract.escrow_address, item.owner, item.token_id, 1_u256, ArrayTrait::new().span(), ); +// Signal removal to off-chain consumers +world.emit_event(@ItemRemovedFromMarket { item_id, owner: item.owner, reason: 'RETURNED' });src/models/marketplace.cairo (1)
25-36: Add an Analytics-updated event to support off-chain sync.Analytics are mutated but never emitted. Define a MarketAnalyticsUpdated event so indexers can consume deltas.
+#[derive(Drop, Serde)] +#[dojo::event] +pub struct MarketAnalyticsUpdated { + pub timestamp: u64, + pub total_listings: u256, + pub total_sales: u256, + pub total_volume: u256, + pub total_bids: u256, + pub active_auctions: u256, +}
🧹 Nitpick comments (7)
src/models/marketplace.cairo (1)
121-121: Fix typo in error message string.Minor but user-facing.
- pub const NOT_BUY_ITEM_OWNER_ALLOWED: felt252 = 'Item owner not_allow'; + pub const NOT_BUY_ITEM_OWNER_ALLOWED: felt252 = 'Item owner not allowed';src/market/marketplace.cairo (5)
193-201: Check allowance before charging registration fee.Avoid relying on transfer_from failure for UX; we have INSUFFICIENT_ALLOWANCE already.
if caller != contract.admin { let client = self.erc20_client(); assert( client.balance_of(caller) >= contract.registration_fee, Errors::INSUFFICIENT_BALANCE, ); + let allowance = client.allowance(caller, get_contract_address()); + assert(allowance >= contract.registration_fee, Errors::INSUFFICIENT_ALLOWANCE); client.transfer_from(caller, get_contract_address(), contract.registration_fee); self._record_platform_fee(contract.registration_fee); }
354-358: Emit removal event when stock hits zero.Helps indexers mark item off-market after direct sale.
item.quantity -= quantity; if item.quantity == 0 { item.is_available = false; + world.emit_event(@ItemRemovedFromMarket { item_id, owner: item.owner, reason: 'SOLD_OUT' }); } world.write_model(@item); ... // Track market activity self._track_market_activity('SOLD', total_price);Also applies to: 381-383
601-629: Removal flow generally OK. Consider zeroing quantity for clarity.item.is_available = false; +item.quantity = 0; world.write_model(@item);
707-727: Naming mismatch: “get_total_collected_fees” returns net available.Either rename to get_available_platform_fees or document clearly; you already expose get_platform_fees_details.
Would you like me to generate a follow-up PR to rename and update callsites?
829-842: Minor: protect against negative time_remaining.Defensive: early-return if current_time >= end_time.
if auction.active { let current_time = get_block_timestamp(); - let time_remaining = auction.end_time - current_time; + if current_time >= auction.end_time { + return (); + } + let time_remaining = auction.end_time - current_time;src/market/interface.cairo (1)
45-49: Clarify fee view naming to avoid ambiguity.get_total_collected_fees returns net-available (collected - withdrawn). Consider renaming to get_available_platform_fees in a follow-up to avoid confusion.
If you want, I can open an issue and a small PR to rename and update docs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/market/interface.cairo(2 hunks)src/market/marketplace.cairo(24 hunks)src/models/marketplace.cairo(7 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 (5)
src/models/marketplace.cairo (2)
88-96: LGTM: Fee accounting storage model is appropriate.
167-181: Event enrichment looks good.Including platform_fee and seller_amount in ItemPurchased is helpful for analytics.
src/market/marketplace.cairo (1)
503-545: Bid path looks correct and safe; refund sequencing LGTM.src/market/interface.cairo (2)
52-64: FeeUtils looks correct; aligns with 2.5% requirement.
67-84: AuctionUtils basis-points math fixed.Calculation now divides by 10,000; good.
| let analytics = MarketAnalytics { | ||
| id: 0, | ||
| total_listings: 0, | ||
| total_sales: 0, | ||
| total_volume: 0, | ||
| total_bids: 0, | ||
| active_auctions: 0, | ||
| }; | ||
|
|
||
| let platform_fees = PlatformFees { | ||
| id: 0, total_collected_fees: 0, total_withdrawn_fees: 0, | ||
| }; | ||
|
|
||
| world.write_model(@config); | ||
| world.write_model(@analytics); | ||
| world.write_model(@platform_fees); | ||
| } |
There was a problem hiding this comment.
Guard init against re-initialization.
Calling init twice will reset counters/analytics/fees. Add ALREADY_INITIALIZED check.
fn init(ref self: ContractState) {
let mut world = self.world_default();
- let contract: Contract = world.read_model(0);
+ let contract: Contract = world.read_model(0);
let admin = get_caller_address();
assert(contract.admin == admin, Errors::UNAUTHORIZED_CALLER);
- let config = Config { id: 0, next_market_id: 1, next_item_id: 1, next_auction_id: 1 };
+ // Prevent re-init
+ let existing: Config = world.read_model(0);
+ assert(existing.next_market_id == 0, Errors::ALREADY_INITIALIZED);
+
+ let config = Config { id: 0, next_market_id: 1, next_item_id: 1, next_auction_id: 1 };📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let analytics = MarketAnalytics { | |
| id: 0, | |
| total_listings: 0, | |
| total_sales: 0, | |
| total_volume: 0, | |
| total_bids: 0, | |
| active_auctions: 0, | |
| }; | |
| let platform_fees = PlatformFees { | |
| id: 0, total_collected_fees: 0, total_withdrawn_fees: 0, | |
| }; | |
| world.write_model(@config); | |
| world.write_model(@analytics); | |
| world.write_model(@platform_fees); | |
| } | |
| fn init(ref self: ContractState) { | |
| let mut world = self.world_default(); | |
| let contract: Contract = world.read_model(0); | |
| let admin = get_caller_address(); | |
| assert(contract.admin == admin, Errors::UNAUTHORIZED_CALLER); | |
| // Prevent re-init | |
| let existing: Config = world.read_model(0); | |
| assert(existing.next_market_id == 0, Errors::ALREADY_INITIALIZED); | |
| let config = Config { id: 0, next_market_id: 1, next_item_id: 1, next_auction_id: 1 }; | |
| let analytics = MarketAnalytics { | |
| id: 0, | |
| total_listings: 0, | |
| total_sales: 0, | |
| total_volume: 0, | |
| total_bids: 0, | |
| active_auctions: 0, | |
| }; | |
| let platform_fees = PlatformFees { | |
| id: 0, total_collected_fees: 0, total_withdrawn_fees: 0, | |
| }; | |
| world.write_model(@config); | |
| world.write_model(@analytics); | |
| world.write_model(@platform_fees); | |
| } |
🤖 Prompt for AI Agents
In src/market/marketplace.cairo around lines 36–52, the init block currently
overwrites models on every call; add a guard to prevent re-initialization by
first reading the existing config model and reverting with an
ALREADY_INITIALIZED error if it exists (e.g., read world.read_model(@config) or
fetch config.id and if config.id != 0 then revert ALREADY_INITIALIZED). If
ALREADY_INITIALIZED constant/error is not defined, add it to the error enum;
otherwise proceed to create analytics/platform_fees only when no existing config
is found so repeated init calls do not reset counters.
2 participants
Close #143 🚀 PR: Complete Marketplace Trading Functions
📌 Summary
This PR implements missing core marketplace functionality including platform fees, auction management, price validation, and analytics tracking. It fulfills the requirements outlined in issue [#143](https://github.com/.../issues/143).
✅ Changes Made
_calculate_platform_fee()_validate_price()_check_and_extend_auction()_track_market_activity()admin_emergency_return()purchase_item()andend_auction()🎯 Expected Behavior
✅ Acceptance Criteria Fulfilled
Summary by CodeRabbit
New Features
Enhancements