onflow / flow-nft

The non-fungible token standard on the Flow blockchain
https://onflow.org
The Unlicense
464 stars 166 forks source link

Destroying an NFT should trigger an event #181

Closed cybercent closed 10 months ago

cybercent commented 11 months ago

Issue to be solved

Currently the only way to know an NFT was moved out of an address is to look for the withdraw/deposit events. Withdrawing an NFT from a collection will trigger the withdraw event.

However self.ownedNFTs.remove(key: nftID) does not trigger a withdraw event so contracts can move NFTs our of collections without triggering events.

Example code from Zeedz:

   pub fun redeem(redeemID: UInt64, message: String){
            let token <- self.ownedNFTs.remove(key: redeemID) ?? panic("Not able to find specified NFT within the owner's collection")
            let zeedle <- token as! @ZeedzINO.NFT

            //  reduce numberOfMinterPerType
            ZeedzINO.numberMintedPerType[zeedle.typeID] = ZeedzINO.numberMintedPerType[zeedle.typeID]! - (1 as UInt64)

            destroy zeedle
            emit Redeemed(id: redeemID, message: message, from: self.owner?.address)
        }

The code above does not trigger a withdraw event so we can't know that the NFT moved or was destroyed.

Suggested Solution

Add event that would be triggered when an NFT is destroyed.

OR

Add way to query an asset's owner:

getOwner(uuid: UInt64): Address?
austinkline commented 11 months ago

Completely agree that destroying an NFT should trigger an event. I believe that there's been some discussion about including that in the NFT V2 standard when emitting events from interfaces gets enabled, then we can guarantee that core events like this one are always emitted

joshuahannan commented 11 months ago

The v2 NFT standard includes most of this: https://github.com/onflow/flow-nft/pull/126 (the event emissions are commented out right now because they don't work with the current version of stable cadence, but they will soon)

please take a look and leave feedback if you have the time. We are going to be including the updated standards in the upcoming stable cadence emulator release so developers can test with them

joshuahannan commented 10 months ago

I'm gonna close this since it is handled in the v2 standards