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

docs: ADR-043 Base NFT Module #9329

Merged
merged 44 commits into from
Jul 12, 2021
Merged
Changes from 40 commits
Commits
Show all changes
44 commits
Select commit Hold shift + click to select a range
8701e97
Add ADR-043 BaseNFT Module
hackmd-deploy May 8, 2021
3876262
Merge branch 'master' into master
shahankhatch May 10, 2021
6c923d3
Adding specifications to NFT fields and clarity on usage.
shahankhatch May 10, 2021
bdb6005
Merge branch 'shahan/adr-043-nft'
chengwenxi May 14, 2021
af68935
resolve #9284 comments
chengwenxi May 17, 2021
be60788
Merge branch 'master' into hulatown/adr-043-nft
chengwenxi May 17, 2021
1759155
Merge branch 'master' into hulatown/adr-043-nft
chengwenxi May 17, 2021
96f9e11
fix typo
chengwenxi May 18, 2021
09c17a6
fix description
chengwenxi May 18, 2021
13cc5a2
rename NFT to BaseNFT
chengwenxi May 18, 2021
435c219
more description
chengwenxi May 25, 2021
1571a59
fix typo
chengwenxi May 25, 2021
1f3ec2a
Adding clarity to ADR 043 nft data field (#9388)
shahankhatch May 26, 2021
b5a36b3
Update docs/architecture/adr-043-nft-module.md
fedekunze May 27, 2021
835b581
Merge branch 'master' into hulatown/adr-043-nft
chengwenxi May 28, 2021
e39f059
apply comments
chengwenxi May 31, 2021
e8e6980
Merge branch 'master' into hulatown/adr-043-nft
fedekunze May 31, 2021
db8bd84
Merge branch 'master' into hulatown/adr-043-nft
mergify[bot] Jun 3, 2021
cc289b6
Merge branch 'master' into hulatown/adr-043-nft
mergify[bot] Jun 3, 2021
3f7c8bb
Merge branch 'master' into hulatown/adr-043-nft
mergify[bot] Jun 3, 2021
1098e27
Merge branch 'master' into hulatown/adr-043-nft
mergify[bot] Jun 3, 2021
55ebf53
Merge branch 'master' into hulatown/adr-043-nft
mergify[bot] Jun 4, 2021
9b4f4cf
Merge branch 'master' into hulatown/adr-043-nft
mergify[bot] Jun 4, 2021
0849698
Merge branch 'master' into hulatown/adr-043-nft
mergify[bot] Jun 4, 2021
ccb2807
Merge branch 'master' into hulatown/adr-043-nft
mergify[bot] Jun 7, 2021
278507b
Merge branch 'master' into hulatown/adr-043-nft
mergify[bot] Jun 7, 2021
8004004
Merge branch 'master' into hulatown/adr-043-nft
mergify[bot] Jun 7, 2021
51df217
apply comments
chengwenxi Jun 10, 2021
5e2f4c3
Merge branch 'master' into hulatown/adr-043-nft
Jun 10, 2021
f1e81fa
Merge branch 'master' into hulatown/adr-043-nft
fedekunze Jun 14, 2021
899e2a2
store nft in bank module
chengwenxi Jun 15, 2021
b641f4c
refactor nft to base nft
chengwenxi Jun 18, 2021
9d60dfd
fix note
chengwenxi Jun 18, 2021
0dbfbdb
rename denom as type -- first batch
Jun 18, 2021
c8642f4
rename denom as type -- second batch
Jun 18, 2021
5152044
minor fix
chengwenxi Jun 18, 2021
0092c53
incorporate feedbacks from 6/18 call
Jun 21, 2021
843f79c
incorporate Billy's feedback
Jul 1, 2021
c98ce3a
fix code formatting
Jul 1, 2021
013e9ca
incorporate feedbacks from aaron, shaun, billy...
Jul 2, 2021
8891fa2
Merge branch 'master' into hulatown/adr-043-nft
chengwenxi Jul 3, 2021
836580a
Merge branch 'master' into hulatown/adr-043-nft
aaronc Jul 6, 2021
afc6680
Merge branch 'master' into hulatown/adr-043-nft
okwme Jul 9, 2021
121dff1
Merge branch 'master' into hulatown/adr-043-nft
okwme Jul 12, 2021
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
320 changes: 320 additions & 0 deletions docs/architecture/adr-043-nft-module.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,320 @@
# ADR 43: NFT Module

## Changelog

- 05.05.2021: Initial Draft
- 07.01.2021: Incorporate Billy's feedback
- 07.02.2021: Incorporate feedbacks from Aaron, Shaun, Billy et al.

## Status

DRAFT

## Abstract

This ADR defines the `x/nft` module which is a generic implementation of NFTs, roughly "compatible" with ERC721.

## Context

NFTs are more than just crypto art, which is very helpful for accruing value to the Cosmos ecosystem. As a result, Cosmos Hub should implement NFT functions and enable a unified mechanism for storing and sending the ownership representative of NFTs as discussed in https://github.com/cosmos/cosmos-sdk/discussions/9065.

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

Since functions/use cases of NFTs are tightly connected with their logic, it is almost impossible to support all the NFTs' use cases in one Cosmos SDK module by defining and implementing different transaction types.

Considering generic usage and compatibility of interchain protocols including IBC and Gravity Bridge, it is preferred to have a generic NFT module design which handles the generic NFTs logic.

This design idea can enable composability that application-specific functions should be managed by other modules on Cosmos Hub or on other Zones by importing the NFT module.

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 contains the following functionality:

- Store NFTs and track their ownership.
- Expose `Keeper` interface for composing modules to mint and burn NFTs.
- Expose external `Message` interface for users to transfer ownership of their NFTs.
- Query NFTs and their supply information.

haifengxi marked this conversation as resolved.
Show resolved Hide resolved
### Types

#### Class

We define a model for NFT **Class**, which is comparable to an ERC721 Contract on Ethereum, under which a collection of NFTs can be created and managed.

```protobuf
message Class {
string id = 1;
string name = 2;
string symbol = 3;
string description = 4;
string uri = 5;
}
haifengxi marked this conversation as resolved.
Show resolved Hide resolved
haifengxi marked this conversation as resolved.
Show resolved Hide resolved
Comment on lines +52 to +58
Copy link
Member

Choose a reason for hiding this comment

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

With the idea of transferring metadata using ICS30, we probably want to think of the minimal on-chain required metadata here. description for instance might be overkill.

```

- `id` is an alphanumeric identifier of the NFT class; it is used as the primary index for storing the class; _required_
- `name` is a descriptive name of the NFT class; _optional_
- `symbol` is the symbol usually shown on exchanges for the NFT class; _optional_
- `description` is a detailed description of the NFT class; _optional_
- `uri` is a URL pointing to an off-chain JSON file that contains metadata about this NFT class ([OpenSea example](https://docs.opensea.io/docs/contract-level-metadata)); _optional_

#### NFT

We define a general model for `NFT` as follows.

```protobuf
message NFT {
chengwenxi marked this conversation as resolved.
Show resolved Hide resolved
string class_id = 1;
string id = 2;
string uri = 3;
google.protobuf.Any data = 10;
}
```

- `class_id` is the identifier of the NFT class where the NFT belongs; _required_
- `id` is an alphanumeric identifier of the NFT, unique within the scope of its class. It is specified by the creator of the NFT and may be expanded to use DID in the future. `class_id` combined with `id` uniquely identifies an NFT and is used as the primary index for storing the NFT; _required_
```
{class_id}/{id} --> NFT (bytes)
```
haifengxi marked this conversation as resolved.
Show resolved Hide resolved
- `uri` is a URL pointing to an off-chain JSON file that contains metadata about this NFT (Ref: [ERC721 standard and OpenSea extension](https://docs.opensea.io/docs/metadata-standards)); _required_
- `data` is a field that CAN be used by composing modules to specify additional properties for the NFT; _optional_

This ADR doesn't specify values that `data` can take; however, best practices recommend upper-level NFT modules clearly specify their contents. Although the value of this field doesn't provide the additional context required to manage NFT records, which means that the field can technically be removed from the specification, the field's existence allows basic informational/UI functionality.

### `Keeper` Interface

```go
type Keeper interface {
NewClass(class Class)
UpdateClass(class Class)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
UpdateClass(class Class)

If class metadata is transferred using ICS30, I'm assuming it would effectively become immutable. i.e. once class metadata is on another chain once, it can no longer be mutated on the origin chain because that could cause inconsistencies. Maybe there are other ways to deal with this but immutability seems the simplest.


Mint(nft NFT,receiver sdk.AccAddress) // updates totalSupply
Burn(classId string, nftId string) // updates totalSupply
Update(nft NFT)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Update(nft NFT)
Update(nft NFT)

We need to be very careful about how this Update method is used if ICS30 transfers metadata. Basically once an NFT is escrowed, metadata would be locked. If there is a module who's logic allows a minter (not owner) of an NFT to modify the metadata, that could result in an inconsistent state.

Transfer(classId string, nftId string, receiver sdk.AccAddress)

GetClass(classId string) Class
GetClasses() []Class

GetNFT(classId string, nftId string) NFT
GetNFTsOfClassByOwner(classId string, owner sdk.AccAddress) []NFT
GetNFTsOfClass(classId string) []NFT

GetOwner(classId string, nftId string) sdk.AccAddress
GetBalance(classId string, owner sdk.AccAddress) uint64
GetTotalSupply(classId string) uint64
}
```

Other business logic implementations should be defined in composing modules that import `x/nft` and use its `Keeper`.

### `Msg` Service

```protobuf
service Msg {
rpc Send(MsgSend) returns (MsgSendResponse);
}

message MsgSend {
string class_id = 1;
string id = 2;
string sender = 3;
string reveiver = 4;
}
message MsgSendResponse {}
```

`MsgSend` can be used to transfer the ownership of an NFT to another address.

The implementation outline of the server is as follows:

```go
type msgServer struct{
k Keeper
}

func (m msgServer) Send(ctx context.Context, msg *types.MsgSend) (*types.MsgSendResponse, error) {
// check current ownership
assertEqual(msg.Sender, m.k.GetOwner(msg.ClassId, msg.Id))

// transfer ownership
m.k.Transfer(msg.ClassId, msg.Id, msg.Receiver)

return &types.MsgSendResponse{}, nil
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

with the x/bank approach, the Mint function should use x/bank service to create denom and set coins. Example pseudo code:

func Mint(ctx, msg) {
  id := "nft:" + nextID()
  k.bank.SetDenomMetaData(...)
  coin := sdk.Coin{denom: id, amount: 1}
  k.bank.MintCoins(types.ModuleName, coin)
  k.bank.SendCoinsFromModuleToAccount(types.ModuleName, msg.Owner, coin)

  // rest of the code is to store NFT specific data in the NFT store
  store := ctx.KVStore(k.storeKey)
  ...
} 

I will write more about it in a general post for this PR.

```

The query service methods for the `x/nft` module are:

```proto
service Query {
fedekunze marked this conversation as resolved.
Show resolved Hide resolved

// Balance queries the number of NFTs of a given class owned by the owner, same as balanceOf in ERC721
rpc Balance(QueryBalanceRequest) returns (QueryBalanceResponse) {
option (google.api.http).get = "/cosmos/nft/v1beta1/balance/{class_id}/{owner}";
}

// Owner queries the owner of the NFT based on its class and id, same as ownerOf in ERC721
rpc Owner(QueryOwnerRequest) returns (QueryOwnerResponse) {
option (google.api.http).get = "/cosmos/nft/v1beta1/owner/{class_id}/{id}";
}

// Supply queries the number of NFTs of a given class, same as totalSupply in ERC721Enumerable
rpc Supply(QuerySupplyRequest) returns (QuerySupplyResponse) {
option (google.api.http).get = "/cosmos/nft/v1beta1/supply/{class_id}";
}

// NFTsOfClassByOwner queries the NFTs of a given class owned by the owner, similar to tokenOfOwnerByIndex in ERC721Enumerable
rpc NFTsOfClassByOwner(QueryNFTsOfClassByOwnerRequest) returns (QueryNFTsResponse) {
option (google.api.http).get = "/cosmos/nft/v1beta1/owned_nfts/{class_id}/{owner}";
}

// NFTsOfClass queries all NFTs of a given class, similar to tokenByIndex in ERC721Enumerable
rpc NFTsOfClass(QueryNFTsOfClassRequest) returns (QueryNFTsResponse) {
option (google.api.http).get = "/cosmos/nft/v1beta1/nfts/{class_id}";
}

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

// Class queries an NFT class based on its id
rpc Class(QueryClassRequest) returns (QueryClassResponse) {
option (google.api.http).get = "/cosmos/nft/v1beta1/classes/{class_id}";
}

// Classes queries all NFT classes
rpc Classes(QueryClassesRequest) returns (QueryClassesResponse) {
option (google.api.http).get = "/cosmos/nft/v1beta1/classes";
}
}

// QueryBalanceRequest is the request type for the Query/Balance RPC method
message QueryBalanceRequest {
string class_id = 1;
string owner = 2;
}

// QueryBalanceResponse is the response type for the Query/Balance RPC method
message QueryBalanceResponse{
uint64 amount = 1;
}

// QueryOwnerRequest is the request type for the Query/Owner RPC method
message QueryOwnerRequest {
string class_id = 1;
string id = 2;
}

// QueryOwnerResponse is the response type for the Query/Owner RPC method
message QueryOwnerResponse{
string owner = 1;
}

// QuerySupplyRequest is the request type for the Query/Supply RPC method
message QuerySupplyRequest {
string class_id = 1;
}

// QuerySupplyResponse is the response type for the Query/Supply RPC method
message QuerySupplyResponse {
uint64 amount = 1;
}

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

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

// QueryNFTsResponse is the response type for the Query/NFTsOfClass and Query/NFTsOfClassByOwner RPC methods
message QueryNFTsResponse {
repeated cosmos.nft.v1beta1.NFT nfts = 1;
cosmos.base.query.v1beta1.PageResponse pagination = 2;
}

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

// QueryNFTResponse is the response type for the Query/NFT RPC method
message QueryNFTResponse {
cosmos.nft.v1beta1.NFT nft = 1;
}

// QueryClassRequest is the request type for the Query/Class RPC method
message QueryClassRequest {
string class_id = 1;
}

// QueryClassResponse is the response type for the Query/Class RPC method
message QueryClassResponse {
cosmos.nft.v1beta1.Class class = 1;
}

// QueryClassesRequest is the request type for the Query/Classes RPC method
message QueryClassesRequest {
// pagination defines an optional pagination for the request.
cosmos.base.query.v1beta1.PageRequest pagination = 1;
}

// QueryClassesResponse is the response type for the Query/Classes RPC method
message QueryClassesResponse {
repeated cosmos.nft.v1beta1.Class classes = 1;
cosmos.base.query.v1beta1.PageResponse pagination = 2;
}
```

## Consequences
haifengxi marked this conversation as resolved.
Show resolved Hide resolved

### Backward Compatibility

No backward incompatibilities.

### Forward Compatibility

This specification conforms to the ERC-721 smart contract specification for NFT identifiers. Note that ERC-721 defines uniqueness based on (contract address, uint256 tokenId), and we conform to this implicitly because a single module is currently aimed to track NFT identifiers. Note: use of the (mutable) data field to determine uniqueness is not safe.s
Copy link
Collaborator

Choose a reason for hiding this comment

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

So maybe we should change the language and use:
This specification follows the ERC-721 [link needed!] smart contract standard....

Also, an ERC-721 implementer has more choices (he creates a new smart contract for every ERC-721 token), here he can't do that.


### Positive

- NFT identifiers available on Cosmos Hub.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Without IID, the NFT identifier is not different from bank identifier and can be confused.

- Ability to build different NFT modules for the Cosmos Hub, e.g., ERC-721.
- NFT module which supports interoperability with IBC and other cross-chain infrastructures like Gravity Bridge
haifengxi marked this conversation as resolved.
Show resolved Hide resolved

### 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:
haifengxi marked this conversation as resolved.
Show resolved Hide resolved

- `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 use cases.

## References

- Initial discussion: https://github.com/cosmos/cosmos-sdk/discussions/9065
- x/nft: initialize module: https://github.com/cosmos/cosmos-sdk/pull/9174
- [ADR 033](https://github.com/cosmos/cosmos-sdk/blob/master/docs/architecture/adr-033-protobuf-inter-module-comm.md)