TransferNFT newOwner validation

[giorgionocera]

In the nft module, inside the nft/keeper/nft.go file, as for #184, verification is not directly performed on the owner address. Even if I know that the SetNFT method (which is called at line 247) generates a panic error if the owner is empty, 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). https://github.com/bitsongofficial/go-bitsong/blob/84691c819214a1f9b9dd844ea2b49b7ccc8ece1b/x/nft/keeper/nft.go#L236-L255

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?

[ryusmo]

I think it's okay to emit event on msg server.

[giorgionocera]

Okay, nice 💪

And what about verifying if the owner address is a valid address?

[ryusmo]

Owner address is verified on ValidateBasic fyi. Usually avoiding duplicated execution would be good.

[giorgionocera]

I agree with you about avoiding duplicated execution. Although, I think here we have two things to highlight:

1. The ValidateBasic method (if I'm not wrong) is executed on the client, so it is not executed if the method is called internally. But on this point, I ask you for confirmation.
2. If I'm not wrong, the ValidateBasic method for the MsgTransferNFT message, only checks for the Sender address and for the NFTId validity, but for the Owner address https://github.com/bitsongofficial/go-bitsong/blob/84691c819214a1f9b9dd844ea2b49b7ccc8ece1b/x/nft/types/msgs.go#L149-L160

[ryusmo]

1. ValidateBasic is executed on chain side as well not only client side
2. Okay - will make verifications for Owner part.

[giorgionocera]

1. Do you mean this? If so, we should implement this. It would be nice!