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

feat: initialize module x/nft #9174

Closed
wants to merge 4 commits into from
Closed

feat: initialize module x/nft #9174

wants to merge 4 commits into from

Conversation

chengwenxi
Copy link
Contributor

@chengwenxi chengwenxi commented Apr 22, 2021

Summary

Implements ADR-043-nft.

A generic module to store NFTs based on discussion #9065 and PR #9329.

  1. x/nft only stores NFTs by id and owner. NFT contains a data field as a proto.Any.
  2. The data field CAN be used by composing modules to specify additional properties for the NFT to support their own use cases, such as ERC721 transferred from Ethereum through the Gravity bridge.

@tac0turtle
Copy link
Member

tac0turtle commented Apr 22, 2021

Thank you for opening this PR. I would love to see this be the first item that goes through the newly established CIP process since it may live on the hub. @ebuchman what do you think?

(https://github.com/cosmos/cips)

@hulatown hulatown mentioned this pull request May 8, 2021
9 tasks
@chengwenxi chengwenxi mentioned this pull request May 14, 2021
9 tasks
@dreamer-zq dreamer-zq mentioned this pull request Jun 4, 2021
9 tasks
@github-actions
Copy link
Contributor

github-actions bot commented Jun 7, 2021

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale label Jun 7, 2021
@github-actions github-actions bot closed this Jun 14, 2021
@chengwenxi chengwenxi reopened this Jun 14, 2021
@github-actions github-actions bot closed this Jun 21, 2021
@chengwenxi chengwenxi reopened this Jun 29, 2021
@dreamer-zq dreamer-zq mentioned this pull request Jun 29, 2021
19 tasks
@github-actions github-actions bot closed this Jul 6, 2021
@chengwenxi chengwenxi reopened this Jul 20, 2021
@chengwenxi chengwenxi marked this pull request as ready for review July 20, 2021 03:44
@chengwenxi chengwenxi changed the title x/nft: initialize module feat: initialize module x/nft Jul 20, 2021
@orijbot
Copy link

orijbot commented Jul 21, 2021

Visit https://dashboard.github.orijtech.com?pr=9174&repo=cosmos%2Fcosmos-sdk to see benchmark details.

@okwme
Copy link
Contributor

okwme commented Jul 21, 2021

Looks like there are some linting errors that came up from golangci-lint:

 Running [/home/runner/golangci-lint-1.39.0-linux-amd64/golangci-lint run --out-format=github-actions --timeout 10m] in [] ...
  Error: struct field `ClassId` should be `ClassID` (golint)
  Error: struct field `ClassId` should be `ClassID` (golint)
  Error: struct field `ClassId` should be `ClassID` (golint)
  Error: struct field `ClassId` should be `ClassID` (golint)
  Error: struct field `ClassId` should be `ClassID` (golint)
  Error: struct field `Id` should be `ID` (golint)
  Error: struct field `ClassId` should be `ClassID` (golint)
  Error: struct field `Id` should be `ID` (golint)
  Error: struct field `ClassId` should be `ClassID` (golint)
  Error: struct field `Id` should be `ID` (golint)
  Error: struct field `ClassId` should be `ClassID` (golint)
  Error: struct field `Id` should be `ID` (golint)
  Error: struct field `ClassId` should be `ClassID` (golint)
  Error: struct field `ClassId` should be `ClassID` (golint)
  Error: struct field `ClassId` should be `ClassID` (golint)
  Error: struct field `ClassId` should be `ClassID` (golint)
  Error: struct field `ClassId` should be `ClassID` (golint)
  Error: struct field `ClassId` should be `ClassID` (golint)
  Error: struct field `ClassId` should be `ClassID` (golint)
  Error: struct field `ClassId` should be `ClassID` (golint)
  Error: struct field `ClassId` should be `ClassID` (golint)
  Error: struct field `ClassId` should be `ClassID` (golint)
  Error: struct field `ClassId` should be `ClassID` (golint)
  Error: struct field `Id` should be `ID` (golint)
  Error: struct field `ClassId` should be `ClassID` (golint)
  Error: struct field `Id` should be `ID` (golint)
  Error: struct field `ClassId` should be `ClassID` (golint)
  Error: struct field `Id` should be `ID` (golint)
  Error: struct field `ClassId` should be `ClassID` (golint)
  Error: struct field `Id` should be `ID` (golint)
  Error: struct field `ClassId` should be `ClassID` (golint)
  Error: struct field `ClassId` should be `ClassID` (golint)
  Error: struct field `ClassId` should be `ClassID` (golint)

Copy link
Member

@aaronc aaronc left a comment

Choose a reason for hiding this comment

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

Can we please have this broken up into smaller PRs? It is really hard to do good reviews of such large pieces of work all at once.

I would suggest individual PRs roughly in this order:

  • just the .proto files
  • just the MsgServer implementation and corresponding keeper methods
  • just the QueryServer implementation and corresponding keeper methods
  • CLI
  • simulations

@chengwenxi
Copy link
Contributor Author

chengwenxi commented Jul 22, 2021

Can we please have this broken up into smaller PRs? It is really hard to do good reviews of such large pieces of work all at once.

Good suggestion, We will split into several PRs to this branch(vincent/nft).

  • Proto files, feat: generate nft protobuf #9747
  • MsgServer implementation and corresponding keeper methods
  • QueryServer implementation and corresponding keeper methods
  • CLI
  • Simulations

@aaronc
Copy link
Member

aaronc commented Jul 22, 2021

Can we please have this broken up into smaller PRs? It is really hard to do good reviews of such large pieces of work all at once.

Good suggestion, We will split into several PRs to this branch(vincent/nft).

  • Proto files, feat: generate nft protobuf #9747
  • MsgServer implementation and corresponding keeper methods
  • QueryServer implementation and corresponding keeper methods
  • CLI
  • Simulations

Thanks! Please make PRs against master if you want the SDK team to review.

@robert-zaremba robert-zaremba self-assigned this Jul 29, 2021
@chengwenxi
Copy link
Contributor Author

I created issue #9826 to track the nft related PRs, so I closed it.

@chengwenxi chengwenxi closed this Jul 31, 2021
@alexanderbez alexanderbez deleted the vincent/nft branch April 22, 2022 13:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants