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 17 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
152 changes: 152 additions & 0 deletions docs/architecture/adr-043-nft-module.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,152 @@
# ADR 43: NFT Module

## Changelog

- 05.05.2021: Initial Draft

## Status

DRAFT

## Abstract

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

## Context
NFTs are more digital assets than only crypto arts, which is very helpful for accruing value to ATOM. 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 NFTs functions/use cases 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 very simple NFT module design which only stores NFTs by id and owner.

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.

For example, NFTs can be grouped into collections in a collectibles module to define the behaviors such as minting, burning, etc.

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.

```proto
message NFT {
string id = 1;
string owner = 2;
google.protobuf.Any data = 3;
}
haifengxi marked this conversation as resolved.
Show resolved Hide resolved
```

The NFT conforms to the following specifications:
* The Id is an immutable field used as a unique identifier. NFT identifiers don't currently have a naming convention but
can be used in association with existing Hub attributes, e.g., defining an NFT's identifier as an immutable Hub address allows its integration into existing Hub account management modules.
We envision that identifiers can accommodate mint and transfer actions.
The Id is also the primary index for storing NFTs.
```
id (string) --> NFT (bytes)
```
* Owner: This mutable field represents the current account owner (on the Hub) of the NFT, i.e., the account that will have authorization
to perform various activities in the future. This can be a user, a module account, or potentially a future NFT module that adds capabilities.
Owner is also the secondary index for storing NFT IDs owned by an address
```
owner (address) --> []string
```
* Data: This mutable field allows attaching special information to the NFT, for example:
* metadata such as title of the work and URI
* immutable data and parameters (such actual NFT data, hash or seed for generators)
* mutable data and parameters that change when transferring or when certain criteria are met (such as provenance)

This ADR doesn't specify values that this field can take; however, best practices recommend upper-level NFT modules clearly specify its contents.
Although the value of this field doesn't provide 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.

The logic for transferring the ownership of an NFT to another address (no coins involved) should also be defined in this module
```go
func (k Keeper) TransferOwnership(nftID string, currentOwner, newOwner sdk.AccAddress) error
Copy link
Collaborator

Choose a reason for hiding this comment

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

Protobuf Msg service is preferred to specify all transaction methods. Please define Msg service with all RPC methods, as you defined Query.

Copy link
Contributor Author

@chengwenxi chengwenxi Jun 7, 2021

Choose a reason for hiding this comment

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

We only define this method in the keeper to execute common logic without a Msg, the Msg should be defined in the upper-level/app-specific specific module.

Copy link
Collaborator

@robert-zaremba robert-zaremba Jun 7, 2021

Choose a reason for hiding this comment

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

With Protobuf service oriented architecture (which we are using since Stargate), we are exposing module interfaces through Protobuf services. Keeper is rather used as an internal data access layer (however, often protobuf and keeper are implemented by the same object -> keeper, but module user doesn't see it).

So, since Stargate we want to use Proto definitions for module interface, and Keeper if you want to talk about internal data layout in the store.
See ADR-30 (authz module) for an example.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I got it.

I will update this ADR after learning about ADR-30 and ADR-33.

Copy link
Collaborator

Choose a reason for hiding this comment

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

ADR-33 is still in active development. I think ADR-30 will give you a good example. Also, as mentioned in other comment, for events please look at ADR-32

```

Other behaviors of NFT, including minting, burning and other business-logic implementation should be defined in other upper-level modules that import this NFT module.

The query service methods for the `x/nft` module are:
```proto
service Query {
fedekunze marked this conversation as resolved.
Show resolved Hide resolved

// 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 NFTs 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 {
NFT nft = 1;
}

// 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 NFT nfts = 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.

### Positive

- NFT identifiers available on Cosmos Hub
- Ability to build different NFT modules for the Cosmos Hub, e.g., ERC-721.
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
- NFT identifiers available on Cosmos Hub
- Ability to build different NFT modules for the Cosmos Hub, e.g., ERC-721.
- NFT identifiers available for Cosmos Ecosystem
- Common NFT module Interface for the Cosmos Ecosystem, e.g., ERC-721.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is an SDK ADR, so I think we should more focus on the Cosmos ecosystem here.

- 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

- Currently, no methods are defined for this module except to store and retrieve data.
Copy link
Collaborator

Choose a reason for hiding this comment

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

In ### Msg Service section we define methods.


### 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/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
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are those extensions or new modules? How this will work? Will there be a new services in the nft module?


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