Fix minor code review issues in NFTStorefrontV2#113
Merged
joshuahannan merged 9 commits intomainfrom Apr 6, 2026
Merged
Conversation
- Issue 2: borrowNFT() — replace force-unwrap on provider borrow() with if-let so a revoked capability returns nil instead of panicking - Issue 4: Listing.init — replace nft! force-unwrap of borrowNFT result with ?? panic(...) for a descriptive error when the NFT is absent - Issue 6: purchase() — assert commissionReceiver.check() before the allowlist loop so a revoked capability fails with a clear message - Issue 9: getDuplicateListingIDs / getExistingListingIDs — replace manual index loop with firstIndex(of:); mark getExistingListingIDs view in interface and implementation; update MaliciousStorefrontV2 to conform - Issue 10: createListing — remove force-unwrap anti-pattern on allowedCommissionReceivers, build addresses array directly - Issue 11: createListing — replace self.owner?.address! with self.owner!.address - Issue 12: cleanupExpiredListings — simplify index + UInt64(1) to index + 1 - Issue 13: setCustomID — add missing doc comment - Issue 14: burnCallback / ResourceDestroyed — add missing doc comments Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
AGENTS.md is a symlink to CLAUDE.md. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
joshuahannan
changed the title
Regenerate assets.go to embed the updated NFTStorefrontV2.cdc and MaliciousStorefrontV2.cdc after the report fix commits. Update CLAUDE.md to clarify that make ci (not make test) is the required pre-commit gate. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…tional difference Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
nvdtf
approved these changes
Apr 6, 2026
…meGhosted() hasListingBecomeGhosted() returns true when the NFT is still present and false when absent — the opposite of what its name implies. Existing callers in the wild already compensate with !, so fixing the semantics in place would silently break them. Instead, add isGhostListing() with correct semantics and deprecate the old function without changing its behaviour. Changes: - Add isGhostListing() to ListingPublic interface and Listing resource - Update cleanupGhostListings to use isGhostListing() directly - Add deprecation notices to hasListingBecomeGhosted() in interface, impl, and scripts - Add is_ghost_listing.cdc and read_all_unique_ghost_listings_v2.cdc scripts - Add testIsGhostListing covering both new scripts - Update documentation to clearly mark hasListingBecomeGhosted() as deprecated Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
testIsGhostListing emits one additional ListingCompleted event (on ghost listing cleanup), shifting the cumulative event count seen by testRemoveItem from 5 to 6. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
After removing the ghost listing from self.listings, the function fetched duplicate listing IDs and cleaned each one up — but never called removeDuplicateListing for the ghost listing itself. Every other removal path (removeListing, cleanupPurchasedListings, cleanup) correctly removes the primary listing from listedNFTs before processing duplicates. The fix calls removeDuplicateListing for the ghost listing immediately after removing it from self.listings, and before getDuplicateListingIDs is called. This ensures getDuplicateListingIDs sees the correct state and the listedNFTs entry is fully cleared. Also adds: - scripts/get_existing_listing_ids.cdc to expose getExistingListingIDs for tests - testCleanupGhostListingsRemovesListedNFTsEntry to regression-test the fix Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The previous commit called removeDuplicateListing on the primary listing before getDuplicateListingIDs, but getDuplicateListingIDs uses contains(listingID) as a guard — once the primary is removed from listedNFTs, it returns [] and duplicates are never cleaned up. Correct order: fetch duplicates first (while the primary is still in listedNFTs), then call removeDuplicateListing for the primary, then clean up each duplicate. Also bumps testRemoveItem's accumulated ListingCompleted event count from 6 to 8: testCleanupGhostListingsRemovesListedNFTsEntry emits two additional events (one for the primary ghost, one for its duplicate). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Addresses issues identified in a security and code quality review of
NFTStorefrontV2.cdc.borrowNFT()— replaceborrow()!force-unwrap withif-letso a revoked provider capability returnsnilinstead of panicking, honoring the documented return contractListing.init— replace silentnft!force-unwrap ofborrowNFTresult with?? panic(...)for a descriptive error when the NFT ID is absent from the collectionpurchase()— assertcommissionReceiver.check()before the allowlist loop so a revoked capability fails with a clear message rather than a confusingborrow()panicgetDuplicateListingIDs— replace manual index loop withfirstIndex(of:); markgetExistingListingIDsasviewin interface, implementation, andMaliciousStorefrontV2createListing— eliminateallowedCommissionReceivers!.append()force-unwrap anti-pattern by building a local array directlycreateListing— replaceself.owner?.address!optional-chain-then-force-unwrap withself.owner!.addresscleanupExpiredListings— simplifyindex + UInt64(1)toindex + 1setCustomIDburnCallbackandResourceDestroyedTest plan
make testpasses (all 17 Cadence tests + Go tests green)make cipasses🤖 Generated with Claude Code