dapperlabs / nfl-smart-contracts

11 stars 2 forks source link

Loop issue #8

Open ObjectPlayer opened 2 years ago

ObjectPlayer commented 2 years ago

Hey folks, I just read your AllDay contract for learning purpose, I noticed that, there is a batchDeposit method which is getting a collection of NFTs as input parameter and deposit that NFTs in user collection. You have added loop to deposit each NFT from the collection of NFTs.

        // batchDeposit takes a Collection object as an argument
        // and deposits each contained NFT into this Collection
        //
        pub fun batchDeposit(tokens: @NonFungibleToken.Collection) {
            // Get an array of the IDs to be deposited
            let keys = tokens.getIDs()

            // Iterate through the keys in the collection and deposit each one
            for key in keys {
                self.deposit(token: <-tokens.withdraw(withdrawID: key))
            }

            // Destroy the empty Collection
            destroy tokens
        }

In my opinion there should be a limit on how many NFTs (max) will be deposit through above method, right now there is no any limit, so it will be cause an issue in case of long NFT collection, so there should be limit as pre-condition of method e.g:

        // batchDeposit takes a Collection object as an argument
        // and deposits each contained NFT into this Collection
        //
        pub fun batchDeposit(tokens: @NonFungibleToken.Collection) {
            pre{
            tokens.getIDs().length <= 10 : "maximum 10 NFTs will be deposite through batchDeposit method"
            }
            // Get an array of the IDs to be deposited
            let keys = tokens.getIDs()

            // Iterate through the keys in the collection and deposit each one
            for key in keys {
                self.deposit(token: <-tokens.withdraw(withdrawID: key))
            }

            // Destroy the empty Collection
            destroy tokens
        }

Also you can add if tokens are shouldn't be nil or at least there must be 1 NFT in the collection as pre-condition

Thanks