General Review of Contracts

[rheaplex]

These are looking good! Some more fine-grained comments...

PackNFT.cdc

• See Recommendation: Include Interfaces In Implementation Contracts #54 .

• See Recommendation: Include NFT Collection Public Interface Methods in Type-Specific Public Interface #55 .

• see Recommendation: Prefer Borrowing To Withdrawing and Depositing #56 .

28: pub fun mint(commitHash: String, issuer: Address) {

• Return the NFT and place it into the collection in the transaction rather than handling fetching the capability here. This makes minting multiple items more efficient.

60: pub var status: String

• This could be an enum.

64: pub fun getCommitHash(): String {

• commitHash is pub let, so this function is not needed.

70: return self._verify(nfts: self.NFTs, salt: self.salt!, commitHash: self.commitHash)

• Note that ! will panic/crash if salt is not set. You could use if let to check if the salt is set, and if not return false if that would make sense. Or if transactions should fail when the salt isn't set this is fine.

83: pub fun getSalt(): String? {

• salt is pub let, so this function is not needed.

97: let hash = HashAlgorithm.SHA2_256.hash(hashString.utf8)

• To build on Simplify hashString() #57, you could hash the concatenated UUIDs of the resources with the salt here.

121: panic("commitHash was not verified")

• This could be an assert() at line 115.

174: let nonfungibleToken <- token

• This assignment isn't needed. See: https://github.com/onflow/flow-nft/blob/master/contracts/ExampleNFT.cdc#L36

204: pub fun borrowPackNFT(id: UInt64): &IPackNFT.NFT {

• I recommend having the concrete type borrow function return an optional, see: https://github.com/onflow/kitty-items/blob/master/cadence/contracts/KittyItems.cdc#L117

228..229: let c <- create Collection() return <- c

• You can return the result of the create directly, e.g. return <- create Collection() .

233: adminAccount: AuthAccount,

• If this is the contract account, it is available as self.account. If it is not, this will require an extra signature which will require more co-ordination to deploy.

PDS.cdc

• See Recommendation: Include Interfaces In Implementation Contracts #54 .

• See Recommendation: Prefer Borrowing To Withdrawing and Depositing #56 .

• See Simplify hashString() #57 .

60: pub fun mintPackNFT(commitHashes: [String], issuer: Address){

• As you note, this is a tricky one. Usually returning an NFT for the transaction to store works well (see PackNFT.cdc L28 above).

92: pub var DistId: UInt64

• For a contract-level var, prefer nextDistId or distCount .

186: if !pdsCollection.check(){

• This block can be an assert().

193: access(contract) fun releaseEscrow(nftIds: [UInt64], owner: Address) {

• Prefer passing borrowed reference to passing address. Transactions are usually a safer (if more verbose) place to handle borrowing.

208: pub fun createPackIssuer (): @PackIssuer{

• Should anyone/everyone be able to create PackIssuers or should this be an admin capability within a DistributionCreator or some other constraint? It's fine if not, I just wanted to check. :-)

[whalelephant]

Follow up actions to the comments.

PackNFT.cdc

• See Recommendation: Include Interfaces In Implementation Contracts #54 .

Please see comments

• See Recommendation: Include NFT Collection Public Interface Methods in Type-Specific Public Interface #55 .

• see Recommendation: Prefer Borrowing To Withdrawing and Depositing #56 .

28: pub fun mint(commitHash: String, issuer: Address) {

• Return the NFT and place it into the collection in the transaction rather than handling fetching the capability here. This makes minting multiple items more efficient.

Please See #74

60: pub var status: String

• This could be an enum.

64: pub fun getCommitHash(): String {

• commitHash is pub let, so this function is not needed.

No longer applicable

70: return self._verify(nfts: self.NFTs, salt: self.salt!, commitHash: self.commitHash)

• Note that ! will panic/crash if salt is not set. You could use if let to check if the salt is set, and if not return false if that would make sense. Or if transactions should fail when the salt isn't set this is fine.

This shoud crash without salt

83: pub fun getSalt(): String? {

• salt is pub let, so this function is not needed.

No longer applicable

97: let hash = HashAlgorithm.SHA2_256.hash(hashString.utf8)

• To build on Simplify hashString() #57, you could hash the concatenated UUIDs of the resources with the salt here.

Please see comment on #57

121: panic("commitHash was not verified")

• This could be an assert() at line 115.

Please See #74

174: let nonfungibleToken <- token

• This assignment isn't needed. See: https://github.com/onflow/flow-nft/blob/master/contracts/ExampleNFT.cdc#L36

Please See #74

204: pub fun borrowPackNFT(id: UInt64): &IPackNFT.NFT {

• I recommend having the concrete type borrow function return an optional, see: https://github.com/onflow/kitty-items/blob/master/cadence/contracts/KittyItems.cdc#L117

Please See #74

228..229: let c <- create Collection() return <- c

• You can return the result of the create directly, e.g. return <- create Collection() .

Please See #74

233: adminAccount: AuthAccount,

• If this is the contract account, it is available as self.account. If it is not, this will require an extra signature which will require more co-ordination to deploy.

Please See #74

PDS.cdc

• See Recommendation: Include Interfaces In Implementation Contracts #54 .

• See Recommendation: Prefer Borrowing To Withdrawing and Depositing #56 .

• See Simplify hashString() #57 .

Please see comments

60: pub fun mintPackNFT(commitHashes: [String], issuer: Address){

• As you note, this is a tricky one. Usually returning an NFT for the transaction to store works well (see PackNFT.cdc L28 above).

Please See #74

92: pub var DistId: UInt64

• For a contract-level var, prefer nextDistId or distCount .

Please See #74

186: if !pdsCollection.check(){

• This block can be an assert().

Please See #74

193: access(contract) fun releaseEscrow(nftIds: [UInt64], owner: Address) {

• Prefer passing borrowed reference to passing address. Transactions are usually a safer (if more verbose) place to handle borrowing.

Please See #74

208: pub fun createPackIssuer (): @PackIssuer{

• Should anyone/everyone be able to create PackIssuers or should this be an admin capability within a DistributionCreator or some other constraint? It's fine if not, I just wanted to check. :-)

The PackIssuer should be created by anyone as a capability receiver for DistCap