Conversation
joshuahannan
left a comment
There was a problem hiding this comment.
The change to TopShot looks good.
There are still some other contracts and transactions that use destroy that should be changed to Burner, like the destroy moments transactions.
Additionally, some of the contracts use the old pattern of removing an old resource from a mapping in order to store a new one there, but you can just switch to force assign instead:
// this
let oldToken <- self.ownedNFTs[id] <- newToken
destroy oldToken
// can be changed to this
self.ownedNFTs[id] <-! newToken
If we're going to close the destroy things properly issue, we should include this here too as well as making sure all instances of destroy properly use Burner. I'd also recommend making sure that the other dapper sports contracts don't use this pattern in their specific destroy NFT transactions/functions
joshuahannan
left a comment
There was a problem hiding this comment.
looks good, but you're still missing the Burner usage in these:
- https://github.com/dapperlabs/nba-smart-contracts/blob/master/transactions/user/destroy_moments.cdc
- https://github.com/dapperlabs/nba-smart-contracts/blob/master/transactions/user/destroy_moments_v2.cdc
- https://github.com/dapperlabs/nba-smart-contracts/blob/master/transactions/user/lock_fake_nft.cdc
Just want to make sure we're thorough
Resolve Use Burner.burn when destroying resources and Update destroyMoments() to call withdraw()