PrintEdition refactor

[giorgionocera]

In the nft module, similarly to the CreateNFT method which lays on the nft/keeper/nft.go file, also the PrintEditionmethod needs a refactor.

Here, depending on the results of using the CollectionID together with the MetadataID inside the NFT ID (as discussed in #159), maybe, the CollectionID can be omitted.

Verification is not directly performed on the owner address. I know that the SetNFT method (which is called at line 227) generates a panic error if the owner is empty, but maybe, it could be kinder to add a check before and avoid the panic generation on the SetNFT method (which should only be the fallback).

With respect to the MasterEdition, a check is done on the nillability of the MasterEdition attribute of the metadata (discussion on issue #179). And, if it is not null, before printing, it is checked if the supply already reached the maxSupply. https://github.com/bitsongofficial/go-bitsong/blob/84691c819214a1f9b9dd844ea2b49b7ccc8ece1b/x/nft/keeper/nft.go#L201-L203

Here, even if it is a very low-priority task since it is not a bug, it could be useful to invert the proposition from metadata.MasterEdition.MaxSupply <= metadata.MasterEdition.Supply to metadata.MasterEdition.Supply >= metadata.MasterEdition.MaxSupply to increase readability.

Also here, to optimize the computation expenses, it could be better to perform the “payment" (deduct fee operation) at the beginning and so, moving the 209-218 lines at the top of the method, to avoid waste of computation for addresses that cannot pay for (considering the discussion on fees on #183). https://github.com/bitsongofficial/go-bitsong/blob/84691c819214a1f9b9dd844ea2b49b7ccc8ece1b/x/nft/keeper/nft.go#L209-L218

You could try this through the TestMsgServerPrintEdition test method available on the nft/keeper/msg_server_test.go file.

In the end, also here 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?

[giorgionocera]

As for #180, 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.

[ryusmo]

For low priority tasks, can we put the label and carry out later? Also I think for moving code pieces from here to there isn't big priority I guess.

[giorgionocera]

Yeah, that part of the issue can be performed with a low priority (also moving code pieces is not a big priority ATM). WRT the label, you should talk with @angelorc, I cannot do it, nor assign the task to someone 😜

[ryusmo]

But can you put label for low priorites? So that I can perform the changes based on priorities.

[giorgionocera]

I cannot add labels, I'm only a contributor. Moreover, the issue is made up of different tasks:

• [ ] Owner address verification (Medium priority)
• [ ] Based on 159, Verify ic CollectionID is really needed (Medium Priority)
• [ ] Based on 179 verify conditions for MasterEdition nillability (Medium Priority)
• [ ] Invert preposition (Low Priority)
• [ ] Deduct Fees (Medium Priority)
• [ ] Move out the Event Emission (Low Priority)
• [ ] Move the method to keeper file (Low Priority)

[ryusmo]

Okay

[giorgionocera]

To get a better flow of thought, I would move the update of the supply (lines 206 and 207) to immediately after the creation of the new NFT (after the current line 227).

https://github.com/bitsongofficial/go-bitsong/blob/84691c819214a1f9b9dd844ea2b49b7ccc8ece1b/x/nft/keeper/nft.go#L177-L234