Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ADR-043 BaseNFT Module #9284

Closed
wants to merge 2 commits into from
Closed
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
126 changes: 126 additions & 0 deletions docs/architecture/adr-043-basenft-module.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,126 @@
# ADR 43: BaseNFT Module

## Changelog

- 05.05.2021: Initial Draft

## Status

DRAFT

## Abstract
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

@tac0turtle tac0turtle May 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it! Thank you Alex, Fede and Marko! Will expand more on the introduction part.


This ADR defines a generic NFT module of `x/nft` which supports NFTs as a `proto.Any` and contains `BaseNFT` as the default implementation.

## Context

As was discussed in [#9065](https://github.com/cosmos/cosmos-sdk/discussions/9065), several potential solutions can be considered:
- irismod/nft and modules/incubator/nft
- CW721
- DID NFTs
- interNFT

Considering generic usage and compatibility of interchain protocols including IBC and Gravity Bridge, it is preferred to have a very simple NFT module design which only stores NFTs by id and owner.

Application-specific functions (minting, burning, transferring, etc.) should be managed by other modules on Cosmos Hub or on other Zones.

The current design is based on the work done by [IRISnet team](https://github.com/irisnet/irismod/tree/master/modules/nft) and an older implementation in the [Cosmos repository](https://github.com/cosmos/modules/tree/master/incubator/nft).


## Decision

We will create a module `x/nft` which only stores NFTs by id and owner.

The interface for the `x/nft` module:

```go
// NFTI is an interface used to store NFTs at a given id and owner.
type NFTI interface {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
type NFTI interface {
type NFT interface {

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

GetId() string // can not return empty string.
GetOwner() sdk.AccAddress
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is the Owner expected to always be included on the NFT itself? why not have it as a second index key for the nft?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
GetOwner() sdk.AccAddress
GetOwner() sdk.AccAddress
Validate() error

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is the Owner expected 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.

}
```

We will also create `BaseNFT` as the default implementation of the `NFTI` interface:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

explain what a base NFT is and why is it called Base

```proto
message BaseNFT {
option (gogoproto.equal) = true;

string id = 1;
string name = 2;
string uri = 3 [(gogoproto.customname) = "URI"];
string data = 4;
string owner = 5;
}
```

Queries functions for `BaseNFT` is:
```proto
service Query {

// NFT queries NFT details based on id.
rpc NFT(QueryNFTRequest) returns (QueryNFTResponse) {
option (google.api.http).get = "/cosmos/nft/v1beta1/nfts/{id}";
}

// NFTs queries all proposals based on the optional onwer.
rpc NFTs(QueryNFTsRequest) returns (QueryNFTsResponse) {
option (google.api.http).get = "/cosmos/nft/v1beta1/nfts";
}
}

// QueryNFTRequest is the request type for the Query/NFT RPC method
message QueryNFTRequest {
string id = 1;
}

// QueryNFTResponse is the response type for the Query/NFT RPC method
message QueryNFTResponse {
google.protobuf.Any nft = 1 [(cosmos_proto.accepts_interface) = "NFTI", (gogoproto.customname) = "NFT"];
}

// QueryNFTsRequest is the request type for the Query/NFTs RPC method
message QueryNFTsRequest {
string owner = 1;
cosmos.base.query.v1beta1.PageResponse pagination = 2;
}

// QueryNFTsResponse is the response type for the Query/NFTs RPC method
message QueryNFTsResponse {
repeated google.protobuf.Any nfts = 1 [(cosmos_proto.accepts_interface) = "NFTI", (gogoproto.customname) = "NFTs"];
cosmos.base.query.v1beta1.PageResponse pagination = 2;
}
```



## Consequences

### Backward Compatibility

No backwards incompatibilities.

### Positive

- NFT functions available on Cosmos Hub
- NFT module which supports interoperability with IBC and other cross-chain infrastractures like Gravity Bridge
Comment on lines +105 to +106
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


### Negative

### Neutral

- Other functions need more modules. For example, a custody module is needed for NFT trading function, a collectible module is needed for defining NFT properties

## Further Discussions

For other kinds of applications on the Hub, more app-specific modules can be developed in the future:
- `x/collectibles`: grouping NFTs into collections and defining properties of NFTs such as minting, burning and transferring, etc.
- `x/nft_custody`: custody of NFTs to support trading functionality
- `x/nft_marketplace`: selling and buying NFTs using sdk.Coins

Other networks in the Cosmos ecosystem could design and implement their own NFT modules for specific NFT applications and usecases.

## References

- Initial discussion: https://github.com/cosmos/cosmos-sdk/discussions/9065
- x/nft: initialize module: https://github.com/cosmos/cosmos-sdk/pull/9174