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

NFT Module #4209

Merged
merged 180 commits into from Aug 26, 2019

Conversation

@fedekunze
Copy link
Contributor

commented Apr 26, 2019

Closes #4046

Specification: #4766

Closes: #4809


  • Targeted PR against correct branch (see CONTRIBUTING.md)

  • Linked to github-issue with discussion and accepted design OR link to spec that describes this work.

  • Wrote tests

  • Updated relevant documentation (docs/)

  • Added a relevant changelog entry: sdkch add [section] [stanza] [message]

  • rereviewed Files changed in the github PR explorer


For Admin Use:

  • Added appropriate labels to PR (ex. wip, ready-for-review, docs)
  • Reviewers Assigned
  • Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)
okwme and others added 8 commits Apr 15, 2019
x/nfts/client/cli/query.go Outdated Show resolved Hide resolved
x/nfts/client/cli/query.go Outdated Show resolved Hide resolved
x/nfts/querier/querier.go Outdated Show resolved Hide resolved

@fedekunze fedekunze requested a review from okwme Apr 28, 2019

x/nfts/keeper/key.go Outdated Show resolved Hide resolved
x/nfts/types/msgs.go Outdated Show resolved Hide resolved
x/nfts/types/msgs.go Outdated Show resolved Hide resolved
x/nfts/types/msgs.go Outdated Show resolved Hide resolved
fedekunze and others added 2 commits Apr 29, 2019
x/nft/client/cli/query.go Outdated Show resolved Hide resolved
x/nft/client/cli/query.go Outdated Show resolved Hide resolved
okwme and others added 3 commits May 2, 2019
okwme added 5 commits Jul 31, 2019
@rigelrozanski
Copy link
Member

left a comment

Mostly reviewed the spec. Beyond the typos I'm okay with this PR getting merged without the major indexing changes I've suggested so long as issues are opened such that NFT will be updated in future PRs.

After these comments are addressed (as well as merge conflicts) I recommend we merge NFT ASAP in order to include it in our on-going sdk maintenance as core changes are made.

docs/spec/SPEC-SPEC.md Outdated Show resolved Hide resolved
}
```

An `IDCollection` is similar to a `Collection` except instead of containing NFTs it only contains an array of `NFT` IDs. This saves storage by avoiding redundancy.

This comment has been minimized.

Copy link
@rigelrozanski

rigelrozanski Aug 2, 2019

Member

EDIT:
we should most likely eliminate these structs if they are expected to be stored anywhere and simply replace them with indexes on the NFT object see my next comment


couple things, a) in the above owner struct the field is of type IDCollections however you only define the IDCollection (singular) struct below, we should probably also include the line:

type IDCollections [] IDCollection 

just for clarity.

b) type Collection should probably expose a function GetIDCollections() IDCollections and similarly NFT should expose a function GetIDCollection() IDCollection. These functions are worth mentioning maybe? add this into the NFT type interface at maybe?

This comment has been minimized.

Copy link
@rigelrozanski

rigelrozanski Aug 26, 2019

Member

Opened the issue #4955

docs/spec/nft/01_concepts.md Outdated Show resolved Hide resolved
type IDCollection struct {
Denom string `json:"denom"`
IDs []string `json:"IDs"`
}

This comment has been minimized.

Copy link
@rigelrozanski

rigelrozanski Aug 2, 2019

Member

Yo. Okay so I think that the NFT design should be restructured to eliminate the:

  • Owner struct
  • IDCollection struct

We should instead by applying the use of secondary indexes similarly to how the staking module stores references to the same validators by: operator-address, consensus-address, power, etc. (see example https://github.com/cosmos/cosmos-sdk/blob/master/x/staking/keeper/validator.go#L70)
(see spec: https://github.com/cosmos/cosmos-sdk/blob/master/docs/spec/staking/01_state.md#validator)

This way we should be able to easily create functions on the NFTKeeper to get the NFTs efficiently without requiring these extra object. For example I'd suggest the keeper look like:

type NFTKeeper interface {
    GetNFTByID(string) NFT
    GetCollectionByDenom(string) []*NFT
    GetCollectionsByOwner(sdk.AccAddress) []Collection
}

Or something along those lines.

This comment has been minimized.

Copy link
@rigelrozanski

rigelrozanski Aug 26, 2019

Member

Opened the issue #4955


As all NFTs belong to a specific `Collection`, they are kept on store in an array
within each `Collection`. Every time an NFT that belongs to a collection is updated,
it needs to be updated on the corresponding NFT array on the corresponding `Collection`.

This comment has been minimized.

Copy link
@rigelrozanski

rigelrozanski Aug 2, 2019

Member

whoa - are you saying NFTs are stored in two places? that sounds like asking for trouble. We should most certainly only store pointer objects in the collection to avoid having to update both. (See my suggested code change to the spec).

This comment has been minimized.

Copy link
@rigelrozanski

rigelrozanski Aug 26, 2019

Member

Also referenced in #4955

docs/spec/nft/05_future_improvements.md Outdated Show resolved Hide resolved
docs/spec/nft/05_future_improvements.md Outdated Show resolved Hide resolved
docs/spec/nft/README.md Outdated Show resolved Hide resolved
fedekunze and others added 5 commits Aug 2, 2019
Update docs/spec/nft/README.md
Co-Authored-By: frog power 4000 <rigel.rozanski@gmail.com>
Apply suggestions from code review
Co-Authored-By: frog power 4000 <rigel.rozanski@gmail.com>
@tnachen

This comment has been minimized.

Copy link
Contributor

commented Aug 22, 2019

@alexanderbez

This comment has been minimized.

Copy link
Contributor

commented Aug 22, 2019

I would not advise blocking this PR on my review (although I will still review). Please resolve CI and merge conflict issues and merge with preferably @rigelrozanski's approval.

simapp/sim_test.go Show resolved Hide resolved
simapp/sim_test.go Show resolved Hide resolved
simapp/sim_test.go Show resolved Hide resolved
x/nft/client/cli/query.go Outdated Show resolved Hide resolved
x/nft/simulation/operations/msgs.go Outdated Show resolved Hide resolved
x/nft/internal/keeper/querier_test.go Show resolved Hide resolved
x/nft/internal/keeper/querier_test.go Show resolved Hide resolved
x/nft/internal/keeper/querier_test.go Show resolved Hide resolved
x/nft/internal/keeper/querier_test.go Show resolved Hide resolved
x/nft/internal/keeper/querier_test.go Show resolved Hide resolved

@fedekunze fedekunze requested a review from rigelrozanski Aug 24, 2019

simapp/sim_test.go Show resolved Hide resolved
simapp/sim_test.go Show resolved Hide resolved
simapp/sim_test.go Show resolved Hide resolved
x/nft/internal/keeper/querier_test.go Show resolved Hide resolved
x/nft/internal/keeper/querier_test.go Show resolved Hide resolved
x/nft/internal/types/owners_test.go Show resolved Hide resolved
@RiccardoM

This comment has been minimized.

Copy link
Contributor

commented Aug 26, 2019

Sorry for asking guys, but is this PR going to be merged anytime soon?

@fedekunze

This comment has been minimized.

Copy link
Contributor Author

commented Aug 26, 2019

@RiccardoM yeah, @rigelrozanski just needs to approve + merge

@rigelrozanski
Copy link
Member

left a comment

Approved - indexing issue opened as an offshoot here (#4955)

simapp/app.go Outdated Show resolved Hide resolved
Update simapp/app.go
Co-Authored-By: Bot from GolangCI <42910462+golangcibot@users.noreply.github.com>

@rigelrozanski rigelrozanski merged commit eeb847c into master Aug 26, 2019

7 of 10 checks passed

ci/circleci: test_sim_after_import CircleCI is running your tests
Details
ci/circleci: test_sim_import_export CircleCI is running your tests
Details
ci/circleci: test_sim_multi_seed_short CircleCI is running your tests
Details
GolangCI No issues found!
Details
ci/circleci: build_docs-1 Your tests passed on CircleCI!
Details
ci/circleci: check_statik Your tests passed on CircleCI!
Details
ci/circleci: setup_dependencies Your tests passed on CircleCI!
Details
ci/circleci: test_cover Your tests passed on CircleCI!
Details
ci/circleci: test_sim_nondeterminism Your tests passed on CircleCI!
Details
ci/circleci: upload_coverage Your tests passed on CircleCI!
Details

@rigelrozanski rigelrozanski deleted the billy/nft branch Aug 26, 2019

@fedekunze fedekunze referenced this pull request Aug 27, 2019
0 of 4 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.