Add ADR-043 BaseNFT Module #9284
Conversation
github-actions
bot
added
T: ADR
|
|
||
| DRAFT | ||
|
|
||
| ## Abstract |
There was a problem hiding this comment.
Thanks @hulatown! But this ADR does not mention or talk about what NFTs are, why they're needed in the SDK, what owners are, what IDs are and their relationships. Any chance you can expand upon this ADR a bit more please?
There was a problem hiding this comment.
I'd personally like to see a written tradeoff analysis of the alternatives outlined in the Context section, i.e why is this the best approach?
There was a problem hiding this comment.
It was discussed this module would have its own go.mod. This would coincide with needing direct owners for maintenance. Would be good to add a few sentences on this.
There was a problem hiding this comment.
Got it! Thank you Alex, Fede and Marko! Will expand more on the introduction part.
| } | ||
| ``` | ||
|
|
||
| We will also create `BaseNFT` as the default implementation of the `NFTI` interface: |
There was a problem hiding this comment.
explain what a base NFT is and why is it called Base
|
|
||
| ```go | ||
| // NFTI is an interface used to store NFTs at a given id and owner. | ||
| type NFTI interface { |
There was a problem hiding this comment.
| type NFTI interface { | |
| type NFT interface { |
There was a problem hiding this comment.
Got it! Is the naming convention is the interface name without "I"? I see mixed usage in different places, like: https://github.com/cosmos/cosmos-sdk/blob/master/x/auth/keeper/keeper.go But will follow the convention suggestion
| // NFTI is an interface used to store NFTs at a given id and owner. | ||
| type NFTI interface { | ||
| GetId() string // can not return empty string. | ||
| GetOwner() sdk.AccAddress |
There was a problem hiding this comment.
is the Owner expected to always be included on the NFT itself? why not have it as a second index key for the nft?
There was a problem hiding this comment.
| GetOwner() sdk.AccAddress | |
| GetOwner() sdk.AccAddress | |
| Validate() error |
There was a problem hiding this comment.
is the
Ownerexpected to always be included on the NFT itself?
I think it's needed, and we should keep the transfer function(include ownership check) in the x/nft module. So /ibc/applications/nft module can use it to generate the ibc-nft transaction without importing the x/nft/collectibles module.
| - NFT functions available on Cosmos Hub | ||
| - NFT module which supports interoperability with IBC and other cross-chain infrastractures like Gravity Bridge |
There was a problem hiding this comment.
I think these are not decided by the ADR process, but by the Hub CIP committee and governance. Also, there is no current implementation for the second item so I wouldn't consider it as positive for now.
|
Thanks for writing this up @hulatown I've made changes in my branch here, https://github.com/cosmos/cosmos-sdk/tree/shahan/adr-043-nft:
Please let me know what you think. As an aside, I don't know if you need special permissions, but if you can push your work into a |
|
Thank you @shahankhatch ! Your input looks great! And I think I have no permission for the moment to create a branch. Would you please help to grant the permission? I will follow your way to create a new push and probably will need to close the current PR and create a new one based on the branch when ready.
|
|
Replaced by #9329 |
Description
ref: #9174 #9065
This ADR defines a generic NFT module of
x/nftwhich supports NFTs as aproto.Anyand containsBaseNFTas the default implementation.Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
docs/) or specification (x/<module>/spec/)godoccomments.Unreleasedsection inCHANGELOG.mdFiles changedin the Github PR explorerCodecov Reportin the comment section below once CI passes