add isGhostListing() with correct semantics; deprecate hasListingBecomeGhosted()#114
Open
joshuahannan wants to merge 2 commits intojosh/report-fixesfrom
Open
add isGhostListing() with correct semantics; deprecate hasListingBecomeGhosted()#114joshuahannan wants to merge 2 commits intojosh/report-fixesfrom
joshuahannan wants to merge 2 commits intojosh/report-fixesfrom
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>
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
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