feat: M011 CosmWasm marketplace-curation contract with unit tests#74
feat: M011 CosmWasm marketplace-curation contract with unit tests#74brawlaphant wants to merge 1 commit intoregen-network:mainfrom
Conversation
Implements the Marketplace Curation & Quality Signals mechanism (M011) as a CosmWasm smart contract. Covers the full collection lifecycle (create, activate, add/remove batches, challenge, suspend, close), curation staking with bond management, quality score submission, reward distribution via trade fees, and slashing for challenger wins. Includes 32 unit tests covering all SPEC acceptance criteria and reference implementation test vectors. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review
This pull request introduces the marketplace-curation CosmWasm contract, which manages curated collections of batches, quality scores, and a challenge system. The feedback focuses on scalability concerns inherent to CosmWasm storage patterns. Specifically, storing collection members and quality history as vectors within structs or maps will lead to excessive gas costs as data grows; these should be refactored into separate maps or paginated queries. Additionally, query responses and logic for listing scores should be optimized with pagination and indexing to ensure long-term reliability.
| pub bond_amount: Uint128, | ||
| pub bond_remaining: Uint128, | ||
| pub status: CollectionStatus, | ||
| pub members: Vec<String>, |
There was a problem hiding this comment.
Storing collection members as a Vec<String> inside the Collection struct is a significant scalability risk. In CosmWasm, the entire struct must be loaded from and saved to storage during any operation that modifies the collection (e.g., adding/removing batches, resolving challenges). As the number of members grows, the gas cost will increase linearly, eventually leading to transactions failing due to the block gas limit. It is recommended to remove this field and use a separate Map<(u64, &str), Empty> to track membership, where the key is (collection_id, batch_denom).
| pub const QUALITY_SCORES: Map<&str, QualityScore> = Map::new("quality_scores"); | ||
|
|
||
| /// batch_denom -> Vec<QualityScore> (append-only history) | ||
| pub const QUALITY_HISTORY: Map<&str, Vec<QualityScore>> = Map::new("quality_history"); |
There was a problem hiding this comment.
Using a Vec to store quality history for a batch is an anti-pattern in CosmWasm for data that grows indefinitely. Since the specification requires an append-only history (Security Invariant #7), this vector will grow with every score submission. Loading the entire history to append a new score or to query it will become increasingly expensive and eventually exceed gas or memory limits. It is recommended to use a composite key to store history entries individually.
| pub const QUALITY_HISTORY: Map<&str, Vec<QualityScore>> = Map::new("quality_history"); | |
| pub const QUALITY_HISTORY: Map<(&str, u64), QualityScore> = Map::new("quality_history"); |
| #[returns(QualityHistoryResponse)] | ||
| QualityHistory { batch_denom: String }, |
There was a problem hiding this comment.
The QualityHistory query returns a full list of historical scores. Since this history is append-only and grows indefinitely, this query will eventually fail or become too expensive to execute. This endpoint should be updated to support pagination (e.g., with start_after and limit parameters) to ensure long-term reliability.
|
|
||
| /// Return the listing score for a batch across all collections it appears in | ||
| #[returns(ListingScoreResponse)] | ||
| ListingScore { batch_denom: String }, |
There was a problem hiding this comment.
The ListingScore query implementation should be carefully designed for scalability. If calculating the collection_count requires iterating over all collections to find where a batch is listed, the query will become increasingly slow and expensive as the number of collections grows. Consider maintaining an inverse index in the state (e.g., a Map mapping batch_denom to a count or a list of collection_ids) to optimize this lookup.
| pub bond_amount: Uint128, | ||
| pub bond_remaining: Uint128, | ||
| pub status: String, | ||
| pub members: Vec<String>, |
There was a problem hiding this comment.
Including the full members list in the CollectionResponse is problematic for scalability. If a collection contains a large number of batches, this response will become too large for the network to handle efficiently. It is better to remove this field from the general collection response and provide a separate paginated query specifically for listing the members of a collection.
Summary
contracts/marketplace-curation/Contract Structure
Cargo.tomlsrc/lib.rssrc/error.rssrc/state.rssrc/msg.rssrc/contract.rsKey Design Decisions
Test Coverage
All 9 SPEC acceptance tests plus additional edge cases:
Test plan
cargo test— 32 tests pass🤖 Generated with Claude Code