BUG: Sharded Collection Contract Deposit event has `nil` owner field

[joshuahannan]

Description

In the sharded collection contract deposit function, the function removes the needed collection from the dictionary before depositing the NFT and putting the collection back.

            // Remove the collection
            let collection <- self.collections.remove(key: bucket)!

            // Deposit the nft into the bucket
            collection.deposit(token: <-token)

            // Put the Collection back in storage
            self.collections[bucket] <-! collection

With a recent upgrade to cadence, this will clear the owner field of the collection, which means that the deposit event that is emitted will have nil as the owner. Services monitoring these events will not be able to monitor any NFTs that are deposited into a sharded collection.

Recommendation

Borrow a reference to the collection instead of removing it from the dictionary, like we do in the top shot smart contract

            // Find the bucket this corresponds to
            let bucket = token.id % self.numBuckets

            let collectionRef = &self.collections[bucket] as! &TopShot.Collection

            // Deposit the nft into the bucket
            collectionRef.deposit(token: <-token)

@sadief

[sadief]

hey @joshuahannan thank you for this! Would this still apply if we aren't using sharded collections for NFL ALLDAY? Since we knew the spork was happening on Dec 8th with the storage layer upgrade we set up the minting account with a normal collection.

[joshuahannan]

If this contract was never deployed, then it wouldn't matter, but if that is the case, can you just remove it from this repo so there isn't any more confusion?

[sadief]

Good point, absolutely - PR here! https://github.com/dapperlabs/nfl-smart-contracts/pull/6 @joshuahannan