fixed warnings due to outdated methods and imports#18
fixed warnings due to outdated methods and imports#18Pfed-prog wants to merge 4 commits intoRegen-Bazaar:mainfrom
Conversation
WalkthroughTests and contracts were updated for Env API changes and stronger typing. Marketplace was refactored to a typed NFT model (new NFT/NFTStatus types, typed storage and events, create/buy/update/toggle flows) and monetary fields changed from u128 to i128. Several tests use explicit types and env.register(..., ()). Changes
Sequence Diagram(s)sequenceDiagram
actor T as Test
participant E as Env
participant F as AssetFactory
participant TC as TokenContract
participant C as StellarAssetClient
participant M as Marketplace
Note over T,E: Token and marketplace registration (updated)
T->>E: register_stellar_asset_contract_v2(...)
E-->>T: TokenContract handle
T->>TC: address()
TC-->>T: token_address
T->>C: StellarAssetClient::new(&env, &token_address)
T->>E: register(MarketplaceContract, ())
E-->>T: marketplace_id
sequenceDiagram
actor User as Caller
participant M as MarketplaceContract
participant S as Storage
participant T as TokenClient
participant Ev as Events
Note over Caller,M: create_nft(creator, name, desc, category, image_url, price:i128, token_address)
Caller->>M: create_nft(...)
M->>S: get_next_nft_id() -> id (u64)
M->>S: set_nft(id, NFT{... price:i128, status, timestamps })
M->>S: add_creator_nft / add_owner_nft / add_category_nft / add_all_nft_id
M->>Ev: emit_nft_created(NFT)
M-->>Caller: Result<u64, ContractError>
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 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 |
Pfed-prog
changed the title
![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 (4)
contracts/market-place-contract/src/contract.rs (4)
19-41: Missing creator authorization in create_nftAnyone can mint on behalf of any address. Require the creator’s auth.
Apply:
) -> Result<u64, ContractError> { if price == 0 { return Err(ContractError::InvalidPrice); } + // Ensure only the creator can mint their NFT + creator.require_auth(); + let nft_id: u64 = get_next_nft_id(env);
72-78: Unsafe u128 → i128 cast and missing buyer auth in buy_nft
- Casting
u128price toi128withascan overflow and produce negatives.- Buyer auth is required before transferring their tokens.
Apply:
- let token_client: token::TokenClient<'_> = token::Client::new(env, &nft.token_address); - token_client.transfer(buyer, &nft.owner, &(nft.price as i128)); + // Buyer must authorize spending + buyer.require_auth(); + + let token_client: token::TokenClient<'_> = token::Client::new(env, &nft.token_address); + // Safely convert price to i128 expected by the token contract + let amount: i128 = match i128::try_from(nft.price) { + Ok(v) => v, + Err(_) => return Err(ContractError::InvalidPrice), + }; + token_client.transfer(buyer, &nft.owner, &amount);Also applies to: 76-80
108-116: Missing creator authorization in update_nft_priceOnly the creator should be able to change price, and they must authorize the call.
Apply:
pub fn update_nft_price( env: &Env, creator: &Address, nft_id: u64, new_price: u128, ) -> Result<(), ContractError> { let mut nft: NFT = get_nft(env, nft_id)?; + creator.require_auth(); + if nft.creator != *creator { return Err(ContractError::CreatorOnly); }
134-144: Missing creator authorization in toggle_nft_availabilityToggling availability should require the creator’s auth.
Apply:
) -> Result<(), ContractError> { let mut nft: NFT = get_nft(env, nft_id)?; + creator.require_auth(); + if nft.creator != *creator { return Err(ContractError::CreatorOnly); }
🧹 Nitpick comments (5)
impact-buyer/src/test.rs (1)
8-8: Import shift to explicit interfaces looks goodSwitching to explicit
NftClient(and away from a wildcard) aligns with the repo’s move toward stronger typing.If
NftInterfaceremains unused in this file, consider removing it to avoid an unused import warning.contracts/market-place-contract/src/storage.rs (2)
62-66: Guard the ID counter against overflow (defensive)While practically unlikely,
u64wrap is silent. Consider saturating or erroring ifcurrent == u64::MAX.Apply:
pub fn get_next_nft_id(env: &Env) -> u64 { - let current: u64 = get_nft_counter(env); - let next: u64 = current + 1; + let current: u64 = get_nft_counter(env); + let next: u64 = match current.checked_add(1) { + Some(v) => v, + None => { + // Option A: saturate + // return current; + // Option B: panic or introduce a ContractError and bubble it up. + panic!("NFT id overflow") + } + }; env.storage().instance().set(&NFT_COUNTER, &next); next }
32-43: Unify counter keying (cleanup)You define
StorageKey::NFTCounterbut use a separateNFT_COUNTERSymbol. Prefer one approach to avoid drift.Two options:
- Use
StorageKey::NFTCounteringet_nft_counter/get_next_nft_id.- Or remove
StorageKey::NFTCounterif you’ll keep theSymbolkey.Also applies to: 44-46, 57-66
contracts/market-place-contract/src/contract.rs (2)
114-121: Block price updates after a sale (policy suggestion)Prevent changing price once an NFT is sold.
Apply:
if new_price == 0 { return Err(ContractError::InvalidPrice); } + if matches!(nft.status, NFTStatus::Sold) { + return Err(ContractError::OperationNotAllowed); + }
221-241: Consider deriving totals from actual stored NFTs
total_nftsuses the counter, which may diverge if ever pruned. If you want current total, compute fromget_all_nft_ids(env).len().
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
contracts/hello-world/src/test.rs(1 hunks)contracts/market-place-contract/src/contract.rs(13 hunks)contracts/market-place-contract/src/events.rs(4 hunks)contracts/market-place-contract/src/lib.rs(0 hunks)contracts/market-place-contract/src/storage.rs(2 hunks)contracts/market-place-contract/src/test.rs(2 hunks)contracts/staking/src/lib.rs(9 hunks)impact-buyer/src/test.rs(1 hunks)
💤 Files with no reviewable changes (1)
- contracts/market-place-contract/src/lib.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- contracts/market-place-contract/src/test.rs
🧰 Additional context used
🧬 Code graph analysis (3)
contracts/market-place-contract/src/contract.rs (2)
contracts/market-place-contract/src/storage.rs (13)
get_next_nft_id(62-67)get_nft(71-74)set_nft(76-79)add_creator_nft(88-93)add_owner_nft(100-105)add_category_nft(121-126)add_all_nft_id(135-139)remove_owner_nft(107-114)get_all_nft_ids(128-133)get_nft_counter(58-60)get_owner_nfts(95-98)get_category_nfts(116-119)get_creator_nfts(83-86)contracts/market-place-contract/src/events.rs (4)
emit_nft_created(40-56)emit_nft_sold(58-74)emit_nft_price_updated(76-90)emit_nft_availability_toggled(92-104)
contracts/staking/src/lib.rs (3)
impact-buyer/src/test.rs (1)
owner(39-45)contracts/staking/src/interfaces.rs (1)
owner(8-8)contracts/staking/src/test.rs (1)
owner(15-23)
contracts/market-place-contract/src/storage.rs (2)
contracts/market-place-contract/src/contract.rs (1)
get_nft(159-161)contracts/market-place-contract/src/lib.rs (1)
get_nft(83-85)
🔇 Additional comments (2)
contracts/market-place-contract/src/events.rs (1)
48-55: Type annotations are fine; no behavior changeAdding explicit event variable types is harmless and can aid readability. Emission topics and payloads remain unchanged.
Also applies to: 66-73, 83-90, 98-104
contracts/hello-world/src/test.rs (1)
3-4: LGTM: explicit typing and updated registrationTests now use explicit types and the newer
env.registerpath; assertions remain intact.Also applies to: 9-11, 13-13
| use soroban_sdk::{token, Address, Env, String, Vec, Map}; | ||
|
|
||
| use crate::{NFT, NFTStatus}; | ||
| use crate::error::ContractError; | ||
| use crate::storage::{get_next_nft_id, get_nft as other_get_nft, set_nft, add_creator_nft, add_owner_nft, add_category_nft, add_all_nft_id, remove_owner_nft, get_all_nft_ids, get_nft_counter, get_owner_nfts, get_category_nfts, get_creator_nfts}; | ||
| use crate::events::{emit_nft_created, emit_nft_sold, emit_nft_price_updated, emit_nft_availability_toggled}; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Follow-up: unify monetary type to i128 across the module
Soroban token amounts are i128. Using u128 for price forces conversions and risks overflow. Consider migrating price (and related event fields) to i128 to remove casts and tighten type safety.
Run this to scan for auth and amount conversions in the marketplace contract:
Also applies to: 23-39, 60-99, 101-126, 128-154
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== require_auth occurrences =="
rg -n -C2 --type=rust '\brequire_auth\s*\(' contracts/market-place-contract/src/contract.rs
echo
echo "== transfer calls and amount types =="
rg -n -C2 --type=rust 'token_client\.transfer\(' contracts/market-place-contract/src/contract.rs
echo
echo "== price field type =="
rg -n -C2 --type=rust 'pub\s+price:\s*(i128|u128)' contracts/market-place-contract/src/{storage.rs,events.rs}Length of output: 30
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Locating relevant files =="
CONTRACT_FILE=$(find contracts -type f -name contract.rs)
STORAGE_FILE=$(find contracts -type f -name storage.rs)
EVENTS_FILE=$(find contracts -type f -name events.rs)
echo "contract: $CONTRACT_FILE"
echo "storage: $STORAGE_FILE"
echo "events: $EVENTS_FILE"
echo
echo "== require_auth occurrences =="
rg -n -C2 --type=rust '\brequire_auth\s*\(' "$CONTRACT_FILE"
echo
echo "== transfer calls and amount types =="
rg -n -C2 --type=rust 'token_client\.transfer\(' "$CONTRACT_FILE"
echo
echo "== price field type in storage and events =="
rg -n -C2 --type=rust 'pub\s+price:\s*(i128|u128)' "$STORAGE_FILE" "$EVENTS_FILE" || trueLength of output: 229
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="contracts/market-place-contract/src/contract.rs"
STORAGE="contracts/market-place-contract/src/storage.rs"
EVENTS="contracts/market-place-contract/src/events.rs"
echo "== require_auth via Env =="
rg -n '\.require_auth' "$FILE" || true
echo
echo "== transfer calls =="
rg -n '\btransfer\s*\(' "$FILE" || true
echo
echo "== price field declarations =="
rg -n 'pub\s+price\s*:' "$STORAGE" "$EVENTS" || trueLength of output: 361
Unify price to i128 across the module
Change all pub price: u128 declarations to i128 in
- contracts/market-place-contract/src/storage.rs (line 23)
- contracts/market-place-contract/src/events.rs (lines 9, 19)
and remove theas i128cast onnft.pricein contract.rs (e.g. the transfer call at line 77) to align with Soroban’s i128 token amounts.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
contracts/market-place-contract/src/contract.rs (1)
77-77: Remove redundant type cast.Now that
nft.priceisi128(changed in storage.rs line 23), the castas i128on line 77 is unnecessary and can be removed for clarity.Apply this diff:
- token_client.transfer(buyer, &nft.owner, &(nft.price as i128)); + token_client.transfer(buyer, &nft.owner, &nft.price);
🧹 Nitpick comments (3)
contracts/market-place-contract/src/storage.rs (1)
63-64: Explicit type annotations improve clarity.The addition of explicit type annotations throughout the storage functions enhances code readability and makes the intent clearer, though the compiler could infer these types. This is a good practice for contract code where clarity is paramount.
Also applies to: 72-72, 77-77, 84-84, 89-90, 96-96, 101-102, 108-109, 117-117, 122-123, 136-136
contracts/market-place-contract/src/events.rs (1)
48-48: Explicit event type annotations enhance clarity.The explicit type annotations when constructing event structs improve readability and make the code's intent clearer, consistent with the annotation approach used throughout the refactor.
Also applies to: 66-66, 83-83, 98-98
contracts/market-place-contract/src/contract.rs (1)
23-24: Explicit type annotations enhance code clarity.The addition of explicit type annotations throughout the contract functions improves readability and makes the code's intent clearer. While the compiler can infer these types, the explicit annotations are beneficial for contract code where clarity and maintainability are important.
Also applies to: 26-26, 66-66, 76-76, 79-79, 106-106, 108-108, 118-118, 134-134, 140-140, 149-149, 165-166, 179-180, 193-194, 207-208, 221-221, 223-223, 226-228
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
contracts/market-place-contract/src/contract.rs(13 hunks)contracts/market-place-contract/src/events.rs(7 hunks)contracts/market-place-contract/src/lib.rs(2 hunks)contracts/market-place-contract/src/storage.rs(3 hunks)contracts/market-place-contract/src/test.rs(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
contracts/market-place-contract/src/storage.rs (2)
contracts/market-place-contract/src/contract.rs (1)
get_nft(159-161)contracts/market-place-contract/src/lib.rs (1)
get_nft(83-85)
contracts/market-place-contract/src/contract.rs (2)
contracts/market-place-contract/src/storage.rs (13)
get_next_nft_id(62-67)get_nft(71-74)set_nft(76-79)add_creator_nft(88-93)add_owner_nft(100-105)add_category_nft(121-126)add_all_nft_id(135-139)remove_owner_nft(107-114)get_all_nft_ids(128-133)get_nft_counter(58-60)get_owner_nfts(95-98)get_category_nfts(116-119)get_creator_nfts(83-86)contracts/market-place-contract/src/events.rs (4)
emit_nft_created(40-56)emit_nft_sold(58-74)emit_nft_price_updated(76-90)emit_nft_availability_toggled(92-104)
🔇 Additional comments (7)
contracts/market-place-contract/src/lib.rs (1)
37-37: LGTM! Price type correctly unified to i128.The migration of price parameters from
u128toi128aligns with Soroban's token standard, which usesi128for all token amounts. This change eliminates unnecessary type conversions and improves type safety throughout the contract.Also applies to: 64-64
contracts/market-place-contract/src/storage.rs (1)
23-23: Core type change correctly applied.The NFT struct's
pricefield migration toi128is the foundation for the contract-wide type unification. This change is consistent with Soroban token standards and eliminates type conversion overhead.contracts/market-place-contract/src/test.rs (3)
7-12: Test imports and constants correctly updated.The test module now uses the current Soroban SDK imports (
StellarAssetClient, explicit contract imports) and aligns theNFT_PRICEconstant with the contract's i128 price type. These changes are necessary to work with the updated contract API.Also applies to: 14-14
39-41: Token contract setup modernized correctly.The test now uses
register_stellar_asset_contract_v2andStellarAssetClient, which are the current recommended approaches in the Soroban SDK. The pattern of registering the asset, retrieving its address, and constructing the client is correct.
46-46: Contract registration updated to current API.The switch from
env.register_contract(None, MarketplaceContract)toenv.register(MarketplaceContract, ())aligns with the current Soroban SDK API and resolves deprecation warnings. The empty tuple()represents no constructor arguments.contracts/market-place-contract/src/events.rs (1)
9-9: Event price types correctly unified to i128.All event structures and emission functions now consistently use
i128for price-related fields. This ensures that emitted events accurately reflect the contract's internal state and align with Soroban token standards, providing correct type information to external consumers.Also applies to: 19-19, 28-29, 45-45, 63-63, 80-81
contracts/market-place-contract/src/contract.rs (1)
3-6: Imports consolidated and function signatures updated correctly.The explicit imports from storage and events modules improve code organization, and the
price: i128parameter increate_nftaligns with the contract-wide type unification. These changes support better maintainability and type safety.Also applies to: 16-16
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
contracts/market-place-contract/src/contract.rs (1)
220-242: LGTM! Stats computation is correct.The function properly aggregates marketplace statistics with explicit typing. The iteration over all NFTs to compute counts is acceptable for moderate-sized collections.
Optional: For very large NFT collections, consider maintaining incremental counters (updated on state changes) to avoid full iteration on each stats query.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
contracts/market-place-contract/src/contract.rs(13 hunks)contracts/staking/src/lib.rs(9 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- contracts/staking/src/lib.rs
🧰 Additional context used
🧬 Code graph analysis (1)
contracts/market-place-contract/src/contract.rs (2)
contracts/market-place-contract/src/storage.rs (13)
get_next_nft_id(62-67)get_nft(71-74)set_nft(76-79)add_creator_nft(88-93)add_owner_nft(100-105)add_category_nft(121-126)add_all_nft_id(135-139)remove_owner_nft(107-114)get_all_nft_ids(128-133)get_nft_counter(58-60)get_owner_nfts(95-98)get_category_nfts(116-119)get_creator_nfts(83-86)contracts/market-place-contract/src/events.rs (4)
emit_nft_created(40-56)emit_nft_sold(58-74)emit_nft_price_updated(76-90)emit_nft_availability_toggled(92-104)
🔇 Additional comments (9)
contracts/market-place-contract/src/contract.rs (9)
3-6: LGTM! Clean import organization.The imports are well-organized with explicit types. The alias
other_get_nftappropriately avoids naming collision with the publicget_nftfunction.
16-16: Price type correctly migrated to i128.The price parameter now uses
i128, aligning with Soroban's token amount type and addressing the previous review feedback.
23-39: LGTM! Well-structured NFT initialization.The NFT construction is complete with all required fields, proper typing (
u64for IDs and timestamps,i128for price), and correct initialization of status and temporal fields.
76-77: Cast removed as price is now i128.The token transfer correctly uses
nft.pricedirectly without casting, as the field is nowi128throughout. This addresses the previous review feedback.
66-99: LGTM! Ownership transfer logic is sound.The buy flow properly:
- Validates NFT availability and ownership
- Transfers tokens using the typed token client
- Updates ownership records in storage
- Sets sold status and timestamp
- Emits the appropriate event
106-126: LGTM! Price update flow is correct.The function properly validates creator ownership and price validity, updates the NFT with the new
i128price, and emits an event capturing both old and new values.
129-154: LGTM! Availability toggle logic is sound.The function correctly:
- Validates creator ownership
- Toggles between Available/Unavailable states
- Prevents toggling sold NFTs
- Derives the boolean flag idiomatically with
matches!- Emits the appropriate event
159-161: LGTM! Clean error handling.The function appropriately delegates to the storage layer and maps
Noneto a domain error, providing a clean public API.
164-217: LGTM! Consistent read-only query pattern.All NFT query functions follow a clean, consistent pattern:
- Fetch ID lists from storage
- Iterate and resolve each ID to an NFT
- Gracefully skip missing entries
- Return typed
Vec<NFT>The explicit type annotations improve code clarity.
1 participant
Summary by CodeRabbit
New Features
Refactor
Tests