add isGhostListing() with correct semantics; deprecate hasListingBecomeGhosted()#114
Merged
Merged
Conversation
…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>
nvdtf
approved these changes
Apr 6, 2026
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
hasListingBecomeGhosted()has inverted return semantics relative to its name: it returnstruewhen the NFT is still present (not ghosted) andfalsewhen the NFT is absent (ghosted). A GitHub code search shows that every public integrator that calls the function already compensates for this by negating the result with!. Fixing the semantics in place would silently break all of them.This PR takes the additive approach:
isGhostListing(): BooltoListingPublicandListingwith correct semantics (true= ghosted)cleanupGhostListingsto useisGhostListing()directly instead of!hasListingBecomeGhosted()hasListingBecomeGhosted()in the interface, implementation, and both scripts that call it — without changing their behaviourscripts/is_ghost_listing.cdcandscripts/read_all_unique_ghost_listings_v2.cdcusing the new functiontestIsGhostListingcovering both new scriptsdocs/documentation.mdto documentisGhostListing()as the primary API and clearly markhasListingBecomeGhosted()as deprecated with an explanation of the inversionTest plan
make cipassestestIsGhostListingconfirmsisGhostListing()returnsfalsebefore the NFT is burned andtrueaftertestCleanupGhostListingsstill passes (exercisescleanupGhostListingswhich now callsisGhostListing())has_listing_become_ghosted.cdc,read_all_unique_ghost_listings.cdc) are unchanged in behaviour🤖 Generated with Claude Code