bitsongofficial
CreateNFT refactor
In the nft module, inside the CreateNFT method which lays on the nft/keeper/nft.go file, metadata for the new NFT to create are generated.
On these metadata, no verification is performed.
For example, when the creators are added, no verification is done on the string. It can also be an invalid string and not an address:
https://github.com/bitsongofficial/go-bitsong/blob/84691c819214a1f9b9dd844ea2b49b7ccc8ece1b/x/nft/keeper/nft.go#L137-L139
Moreover, IMHO we could improve the check for the verification and make it automatically verified a creator if he is also the sender of the transaction.
Verification is neither performed on the MetadataAuthority (a.k.a. updateAuthority) and MintAuthority (which should be set as sender.string()) addresses and name and uri of the metadata (which can also be empty strings or very long strings with any kind of character) nor on the sellerFeeBasisPoints, which can be set to any positive number (IMHO, even if on that a verification is done client-side, it is not enough, since this module can also be called internally).
Moreover, I do not understand one thing:
Why do we have to pass the presaleHappened (which corresponds to the PrimarySaleHappened)?
Since, by the implementation, the first owner must be the creator of the NFT (I am not sure that this is a nice behavior #181 ) https://github.com/bitsongofficial/go-bitsong/blob/84691c819214a1f9b9dd844ea2b49b7ccc8ece1b/x/nft/keeper/nft.go#L163-L168 It cannot be sold at the creation.
With respect to the MasterEdition, a check is done on the nillability of the MasterEdition attribute of the message
https://github.com/bitsongofficial/go-bitsong/blob/84691c819214a1f9b9dd844ea2b49b7ccc8ece1b/x/nft/keeper/nft.go#L140-L145
We need to understand the difference between a nil value for the MasterEdition or a MasterEdition with a MaxSupply = 1 (discussion on issue #179).
To optimize the computation expenses, it could be better to perform the “payment" (deduct fee operation) at the beginning and so, moving the 152-160 lines at the top of the method, to avoid waste of computation for addresses that cannot pay for https://github.com/bitsongofficial/go-bitsong/blob/84691c819214a1f9b9dd844ea2b49b7ccc8ece1b/x/nft/keeper/nft.go#L152-L160
You could try this through the TestMsgServerCreateNFT test method available on the nft/keeper/msg_server_test.go file.
In the end, I don't like to split events between msg_server and the keeper and, for this reason, I would like to emit events directly on the methods belonging to msg_server (as it is done for the SignMetadata method for example).
What do you think?
I think that, since its logic and controls, this method could be moved inside the nft/keeper/keeper.go file, in order to maintain only the basic methods inside each object file.
To get a better flow of thought, I would move the update of the lastMetadataID (line 135) to immediately after the creation of the new metadata (after the current line 146).
https://github.com/bitsongofficial/go-bitsong/blob/84691c819214a1f9b9dd844ea2b49b7ccc8ece1b/x/nft/keeper/nft.go#L122-L175