Fix minor code review issues in NFTStorefrontV2

.gitignore

@@ -6,4 +6,5 @@ coverage.json
 .env
 .DS_Store
 lcov.info
-imports/*
+imports/*
+REPORT.md

AGENTS.md

@@ -0,0 +1 @@
+CLAUDE.md

CLAUDE.md

@@ -0,0 +1,101 @@
+# CLAUDE.md
+
+This file provides guidance to Claude Code (claude.ai/code) when working with code in this repository.
+
+## Commands
+
+### Pre-commit requirement for Cadence changes
+
+Any changes to `.cdc` files must pass `make ci` before committing:
+
+```sh
+make ci
+```
+
+`make ci` runs the Go tests and all Cadence tests with coverage, mirroring the CI pipeline. It must be green before every commit.
+
+### Run all tests
+```sh
+flow test --cover --covercode="contracts/NFTStorefrontV2.cdc" tests/NFTStorefrontV2_test.cdc
+flow test --cover --covercode="contracts/NFTStorefront.cdc" tests/NFTStorefrontV1_test.cdc
+```
+
+### Run a single test function
+```sh
+flow test --filter <TestFunctionName> tests/NFTStorefrontV2_test.cdc
+```
+
+### Deploy to emulator
+```sh
+flow emulator start
+flow deploy --network emulator
+```
+
+### Install/update dependencies
+```sh
+flow dependencies install
+```
+
+## Architecture
+
+### Contracts
+
+**`contracts/NFTStorefrontV2.cdc`** — The canonical, recommended contract. All new integrations should target this version.
+
+**`contracts/NFTStorefront.cdc`** — V1, no longer actively supported. Maintained for backwards compatibility only.
+
+Both are deployed to the same address on mainnet (`0x4eb8a10cb9f87357`) and testnet.
+
+### Core Resource Model
+
+Each seller account holds a single `Storefront` resource (stored at `NFTStorefrontV2.StorefrontStoragePath`). Within it, individual `Listing` resources represent NFTs offered for sale. The key design properties:
+
+- **Non-custodial**: NFTs remain in the seller's collection until purchase. A `Listing` holds an `auth(NonFungibleToken.Withdraw)` provider capability, not the NFT itself.
+- **One NFT, many listings**: The same NFT can have multiple active `Listing`s simultaneously (e.g., one per marketplace, or one per accepted token type).
+- **Generic types**: `sell_item.cdc` and `buy_item.cdc` accept `nftTypeIdentifier` and `ftTypeIdentifier` strings, resolved via `MetadataViews.resolveContractViewFromTypeIdentifier`. No NFT- or FT-specific imports needed in those transactions.
+
+### Payment Flow (`Listing.purchase`)
+
+`salePrice = commissionAmount + sum(saleCuts)`
+
+On purchase:
+1. Commission is routed to `commissionRecipient` (must be one of `marketplacesCapability` if that list is non-nil).
+2. Each `SaleCut` is paid to its receiver capability; failures emit `UnpaidReceiver` rather than reverting.
+3. The NFT is withdrawn from the seller's collection via the stored provider capability and returned to the caller.
+
+### Ghost Listings
+
+A listing becomes "ghosted" when the underlying NFT is no longer present in the provider capability (transferred out or sold via another listing). Ghost listings:
+- Do not revert on detection but will revert on purchase attempt.
+- Can be checked via `Listing.hasListingBecomeGhosted()`.
+- Can be cleaned up via `transactions/cleanup_ghost_listing.cdc` or `transactions/cleanup_purchased_listings.cdc`.
+
+### Key V2 Additions over V1
+
+| Feature | V1 | V2 |
+|---|---|---|
+| Commission / marketplace cuts | No | Yes (`commissionAmount` + `marketplacesCapability`) |
+| Listing expiry | No | Yes (`expiry: UInt64` unix timestamp) |
+| Ghost listing detection | No | Yes (`hasListingBecomeGhosted()`) |
+| Duplicate listing cleanup | No | Yes (`getDuplicateListingIDs` / `cleanupPurchasedListings`) |
+| Custom dapp ID | No | Yes (`customID: String?`) |
+
+### Transaction Layout
+
+- `transactions/` — V2 storefront transactions (use these)
+- `transactions-v1/` — V1 storefront transactions (legacy)
+- `transactions/hybrid-custody/` — Selling NFTs from child accounts via HybridCustody
+- `scripts/` — Read-only queries (listing details, ghost detection, etc.)
+
+### Testing
+
+Tests use the **Cadence Testing Framework** (`import Test`). Contract aliases for `testing` network are defined in `flow.json`. Test helper utilities are in `tests/test_helpers.cdc`. Security regression tests use `contracts/utility/test/MaliciousStorefrontV1.cdc` and `MaliciousStorefrontV2.cdc` to verify that a malicious storefront cannot substitute a different NFT during purchase.
+
+### Contract Addresses
+
+| Network | Address |
+|---|---|
+| Mainnet | `0x4eb8a10cb9f87357` |
+| Testnet | `0x2d55b98eb200daef` (V2), `0x94b06cfca1d8a476` (V1) |
+| Emulator | `0xf8d6e0586b0a20c7` |
+| Testing framework | `0x0000000000000007` (V2), `0x0000000000000006` (V1) |

contracts/NFTStorefrontV2.cdc

@@ -160,6 +160,8 @@ access(all) contract NFTStorefrontV2 {
             self.purchased = true
         }
 
+        /// Updates the custom identifier string used to distinguish events from different dApps.
+        /// May be set to nil to clear it.
         access(contract) fun setCustomID(customID: String?){
             self.customID = customID
         }
@@ -267,12 +269,23 @@ access(all) contract NFTStorefrontV2 {
         /// This capability allows the resource to withdraw *any* NFT, so you should be careful when giving
         /// such a capability to a resource and always check its code to make sure it will use it in the
         /// way that it claims.
+        ///
+        /// The field type uses `&{NonFungibleToken.Provider, NonFungibleToken.CollectionPublic}` while the
+        /// `init` parameter accepts `&{NonFungibleToken.Collection}`. These differ intentionally: callers
+        /// pass the narrower `Collection` type (which is the standard capability type issued to sellers),
+        /// and the assignment is valid because `NonFungibleToken.Collection` conforms to both `Provider`
+        /// and `CollectionPublic`. Aligning the two to the same type would be a breaking change for existing
+        /// integrations that already hold `&{NonFungibleToken.Collection}` capabilities.
         access(contract) let nftProviderCapability: Capability<auth(NonFungibleToken.Withdraw) &{NonFungibleToken.Provider, NonFungibleToken.CollectionPublic}>
 
         /// An optional list of marketplaces capabilities that are approved 
         /// to receive the marketplace commission.
         access(contract) let marketplacesCapability: [Capability<&{FungibleToken.Receiver}>]?
 
+        /// Called by Burner.burn when this Listing is destroyed.
+        /// Emits ListingCompleted only if the listing was not already marked as purchased,
+        /// since purchase() emits the event for the purchased case.
+        /// If this logic changes, revisit Storefront.removeListing() and Storefront.cleanup().
         access(contract) fun burnCallback() {
             // If the listing has not been purchased, we regard it as completed here.
             // Otherwise we regard it as completed in purchase().
@@ -303,12 +316,15 @@ access(all) contract NFTStorefrontV2 {
         /// it will return nil.
         ///
         access(all) fun borrowNFT(): &{NonFungibleToken.NFT}? {
-            if let ref = self.nftProviderCapability.borrow()!.borrowNFT(self.details.nftID) {
-                if ref.isInstance(self.details.nftType) && ref.id == self.details.nftID {
-                    return ref
+            // If the provider capability has been revoked, return nil rather than panicking,
+            // as the doc contract promises nil for any absent/unavailable NFT.
+            if let providerRef = self.nftProviderCapability.borrow() {
+                if let ref = providerRef.borrowNFT(self.details.nftID) {
+                    if ref.isInstance(self.details.nftType) && ref.id == self.details.nftID {
+                        return ref
+                    }
                 }
             }
-
             return nil
         }
 
@@ -365,6 +381,12 @@ access(all) contract NFTStorefrontV2 {
                 // If commission recipient is nil, Throw panic.
                 let commissionReceiver = commissionRecipient
                     ?? panic("NFTStorefrontV2.Listing.purchase: Commission recipient can't be nil")
+                // Verify the capability is valid before performing the allowlist check,
+                // so a revoked capability produces a clear error rather than a confusing borrow failure.
+                assert(
+                    commissionReceiver.check(),
+                    message: "NFTStorefrontV2.Listing.purchase: The provided commission recipient capability is invalid"
+                )
                 if self.marketplacesCapability != nil {
                     var isCommissionRecipientHasValidType = false
                     var isCommissionRecipientAuthorised = false
@@ -480,8 +502,8 @@ access(all) contract NFTStorefrontV2 {
             return <-nft
         }
 
-        // destructor event
-        //
+        /// Emitted automatically by the Cadence runtime when this Listing resource is destroyed.
+        /// Captures a snapshot of key listing fields at destruction time.
         access(all) event ResourceDestroyed(
             listingResourceID: UInt64 = self.uuid,
             storefrontResourceID: UInt64 = self.details.storefrontID,
@@ -537,11 +559,13 @@ access(all) contract NFTStorefrontV2 {
                 provider != nil,
                 message: "NFTStorefrontV2.Listing.init: Cannot initialize Listing, the NFT Provider Capability is invalid!")
 
-            // This will precondition assert if the token is not available.
+            // Verify the NFT exists in the collection and matches the declared type.
+            // We will check again at purchase time; this is an early-fail guard at listing creation.
             let nft = provider!.borrowNFT(self.details.nftID)
+                ?? panic("NFTStorefrontV2.Listing.init: NFT with ID \(self.details.nftID) does not exist in the provided collection")
             assert(
-                nft!.getType() == self.details.nftType,
-                message: "NFTStorefrontV2.Listing.init: Cannot initialize Listing! The type of the token for sale <\(nft!.getType().identifier)> is not of specified type in the listing <\(self.details.nftType.identifier)>"
+                nft.getType() == self.details.nftType,
+                message: "NFTStorefrontV2.Listing.init: Cannot initialize Listing! The type of the token for sale <\(nft.getType().identifier)> is not of specified type in the listing <\(self.details.nftType.identifier)>"
             )
         }
     }
@@ -587,7 +611,7 @@ access(all) contract NFTStorefrontV2 {
         }
         access(all) fun cleanupExpiredListings(fromIndex: UInt64, toIndex: UInt64)
         access(contract) fun cleanup(listingResourceID: UInt64)
-        access(all) fun getExistingListingIDs(nftType: Type, nftID: UInt64): [UInt64]
+        access(all) view fun getExistingListingIDs(nftType: Type, nftID: UInt64): [UInt64]
         access(all) fun cleanupPurchasedListings(listingResourceID: UInt64)
         access(all) fun cleanupGhostListings(listingResourceID: UInt64)
    }
@@ -657,19 +681,19 @@ access(all) contract NFTStorefrontV2 {
             // Add the `listingResourceID` in the tracked listings.
             self.addDuplicateListing(nftIdentifier: nftType.identifier, nftID: nftID, listingResourceID: listingResourceID)
 
-            // Scraping addresses from the capabilities to emit in the event.
-            var allowedCommissionReceivers : [Address]? = nil
+            // Extract the address from each marketplace capability for inclusion in the event.
+            // nil marketplacesCapability means open commission (any recipient allowed).
+            var allowedCommissionReceivers: [Address]? = nil
             if let allowedReceivers = marketplacesCapability {
-                // Small hack here to make `allowedCommissionReceivers` variable compatible to
-                // array properties.
-                allowedCommissionReceivers = []
-                for receiver in allowedReceivers {
-                    allowedCommissionReceivers!.append(receiver.address)
+                var addresses: [Address] = []
+                for cap in allowedReceivers {
+                    addresses.append(cap.address)
                 }
+                allowedCommissionReceivers = addresses
             }
 
             emit ListingAvailable(
-                storefrontAddress: self.owner?.address!,
+                storefrontAddress: self.owner!.address,
                 listingResourceID: listingResourceID,
                 nftType: nftType,
                 nftUUID: uuid,
@@ -732,11 +756,11 @@ access(all) contract NFTStorefrontV2 {
         /// getExistingListingIDs
         /// Returns an array of listing IDs of the given `nftType` and `nftID`.
         ///
-        access(all) fun getExistingListingIDs(nftType: Type, nftID: UInt64): [UInt64] {
+        access(all) view fun getExistingListingIDs(nftType: Type, nftID: UInt64): [UInt64] {
             if self.listedNFTs[nftType.identifier] == nil || self.listedNFTs[nftType.identifier]![nftID] == nil {
                 return []
             }
-            var listingIDs = self.listedNFTs[nftType.identifier]![nftID]!
+            let listingIDs = self.listedNFTs[nftType.identifier]![nftID]!
             return listingIDs
         }
 
@@ -763,21 +787,14 @@ access(all) contract NFTStorefrontV2 {
         access(all) fun getDuplicateListingIDs(nftType: Type, nftID: UInt64, listingID: UInt64): [UInt64] {
             var listingIDs = self.getExistingListingIDs(nftType: nftType, nftID: nftID)
 
-            // Verify that given listing Id also a part of the `listingIds`
-            let doesListingExist = listingIDs.contains(listingID)
-            // Find out the index of the existing listing.
-            if doesListingExist {
-                var index: Int = 0
-                for id in listingIDs {
-                    if id == listingID {
-                        break
-                    }
-                    index = index + 1
-                }
-                listingIDs.remove(at:index)
+            // Only return duplicates if the given listingID is actually tracked; otherwise
+            // there is nothing to deduplicate against.
+            if listingIDs.contains(listingID) {
+                let index = listingIDs.firstIndex(of: listingID)!
+                listingIDs.remove(at: index)
                 return listingIDs
-            } 
-           return []
+            }
+            return []
         }
 
         /// cleanupExpiredListings
@@ -801,7 +818,7 @@ access(all) contract NFTStorefrontV2 {
                         self.cleanup(listingResourceID: listingsIDs[index])
                     }
                 }
-                index = index + UInt64(1) 
+                index = index + 1
             }
         } 
 

contracts/utility/test/MaliciousStorefrontV2.cdc

@@ -41,7 +41,7 @@ access(all) contract MaliciousStorefrontV2 {
             return
         }
 
-        access(all) fun getExistingListingIDs(nftType: Type, nftID: UInt64): [UInt64] {
+        access(all) view fun getExistingListingIDs(nftType: Type, nftID: UInt64): [UInt64] {
             return self.storefrontCap.borrow()!.getExistingListingIDs(nftType: nftType, nftID: nftID)
         }