General Review of Contracts - Githubissues

flow-hydraulics / flow-pds

This repository is currently not maintained

4 stars 8 forks source link

General Review of Contracts #58

Closed rheaplex closed 2 years ago

rheaplex commented 2 years ago

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

PackNFT.cdc

See #54 .
See #55 .
see #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 #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 #54 .
See #56 .
See #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 commented 2 years ago

Follow up actions to the comments.

PackNFT.cdc

See #54 .

Please see comments

See #55 .
see #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 https://github.com/flow-hydraulics/flow-pds/pull/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 #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 https://github.com/flow-hydraulics/flow-pds/pull/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 https://github.com/flow-hydraulics/flow-pds/pull/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 https://github.com/flow-hydraulics/flow-pds/pull/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 https://github.com/flow-hydraulics/flow-pds/pull/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 https://github.com/flow-hydraulics/flow-pds/pull/74

PDS.cdc

See #54 .
See #56 .
See #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 https://github.com/flow-hydraulics/flow-pds/pull/74

92: pub var DistId: UInt64

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

Please See https://github.com/flow-hydraulics/flow-pds/pull/74

186: if !pdsCollection.check(){

This block can be an assert().

Please See https://github.com/flow-hydraulics/flow-pds/pull/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 https://github.com/flow-hydraulics/flow-pds/pull/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