fix cleanupGhostListings leaking listedNFTs entry for ghost listing#115
Open
joshuahannan wants to merge 2 commits intojosh/is-ghost-listingfrom
Open
fix cleanupGhostListings leaking listedNFTs entry for ghost listing#115joshuahannan wants to merge 2 commits intojosh/is-ghost-listingfrom
joshuahannan wants to merge 2 commits intojosh/is-ghost-listingfrom
Conversation
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
1 participant
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
Fixes Bug 1 (Critical) from the code review:
cleanupGhostListingsremoves the ghost listing fromself.listingsand cleans up its duplicates, but never callsremoveDuplicateListingfor the ghost listing itself. This leaves a dangling entry inlistedNFTs— the secondary index that tracks which listing IDs exist per NFT type+ID.Trace with listings A, B, C all for the same NFT:
self.listings.remove(A)— gone from listingsgetDuplicateListingIDs(A)→ returns[B, C](listedNFTs still has A, B, C)cleanup(B)→ callsremoveDuplicateListing(B)→ listedNFTs =[A, C]cleanup(C)→ callsremoveDuplicateListing(C)→ listedNFTs =[A]Burner.burn(A)— burned, but listedNFTs still has[A]← dangling entryEvery other removal path (
removeListing,cleanupPurchasedListings,cleanup) correctly callsremoveDuplicateListingfor the primary listing before processing duplicates.Fix: call
removeDuplicateListingfor the ghost listing immediately after removing it fromself.listings, and before callinggetDuplicateListingIDs. This ensuresgetDuplicateListingIDssees the correct state and returns only the true duplicates.Changes
contracts/NFTStorefrontV2.cdc— insertremoveDuplicateListingcall incleanupGhostListingsscripts/get_existing_listing_ids.cdc— new script wrappinggetExistingListingIDsfor test usetests/NFTStorefrontV2_test.cdc—testCleanupGhostListingsRemovesListedNFTsEntrycreates two listings for the same NFT, ghosts both, cleans up, and assertsgetExistingListingIDsreturns emptyTest plan
make cipassestestCleanupGhostListingsRemovesListedNFTsEntryfails on the unfixed contract and passes on the fix (regression test for the danglinglistedNFTsentry)testCleanupGhostListingsstill passes🤖 Generated with Claude Code