feat: generate nft protobuf #9747

dreamer-zq · 2021-07-22T03:27:32Z

Description

Generate nft protobuf, refer #9826.

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

confirmed the correct type prefix in the PR title
confirmed ! in the type prefix if API or client breaking change
confirmed all author checklist items have been addressed
reviewed state machine logic
reviewed API design and naming
reviewed documentation is accurate
reviewed tests and test coverage
manually tested (if applicable)

aaronc

Was this intended to be against master?

Let's remove gogoproto annotations because we're migrating to golang v2 protobuf and these don't seem necessary.

proto/cosmos/nft/v1beta1/event.proto

proto/cosmos/nft/v1beta1/genesis.proto

chengwenxi · 2021-07-23T01:59:15Z

Was this intended to be against master?

Yes, we hope that this module is completed when it is merged into master, so that it will not affect the release of new versions based on the master branch.

If this consideration is redundant, we can directly PR to the master.

aaronc · 2021-07-30T14:18:41Z

Was this intended to be against master?

Yes, we hope that this module is completed when it is merged into master, so that it will not affect the release of new versions based on the master branch.

If this consideration is redundant, we can directly PR to the master.

Let's please target PRs against master. Thanks 🙏

charleenfei · 2021-08-03T08:15:41Z

Was this intended to be against master?

Yes, we hope that this module is completed when it is merged into master, so that it will not affect the release of new versions based on the master branch.
If this consideration is redundant, we can directly PR to the master.

Let's please target PRs against master. Thanks 🙏

bumping this nft adr review! @okwme @aaronc

robert-zaremba

Looks good. Pre-approving, but left few comments

robert-zaremba · 2021-08-10T22:48:25Z

proto/cosmos/nft/v1beta1/nft.proto

+  string name        = 2;
+  string symbol      = 3;
+  string description = 4;
+  string uri         = 5;


shall we add: uri_hash - this will be a hash of the document pointed by URI - similar to what we have in bank.proto metadata,

Please add a comment to the uri_hash

robert-zaremba · 2021-08-10T22:49:42Z

proto/cosmos/nft/v1beta1/nft.proto

+message NFT {
+  string              class_id = 1;
+  string              id       = 2;
+  string              uri      = 3;


let's add uri_hash as well.

robert-zaremba · 2021-08-10T22:50:27Z

proto/cosmos/nft/v1beta1/nft.proto

+  string              class_id = 1;
+  string              id       = 2;
+  string              uri      = 3;
+  google.protobuf.Any data     = 10;


please add comment to the data field. Both data and any has almost no meaning.

robert-zaremba · 2021-08-10T22:51:36Z

proto/cosmos/nft/v1beta1/query.proto

+    option (google.api.http).get = "/cosmos/nft/v1beta1/owner/{class_id}/{id}";
+  }
+
+  // Supply queries the number of nft based on the class, same as totalSupply of ERC721


Suggested change

// Supply queries the number of nft based on the class, same as totalSupply of ERC721

// Supply queries the number of NFTs from the given class, same as totalSupply of ERC721.

robert-zaremba · 2021-08-10T22:58:48Z

proto/cosmos/nft/v1beta1/genesis.proto

+
+import "cosmos/nft/v1beta1/nft.proto";
+
+option go_package = "github.com/cosmos/cosmos-sdk/x/nft";


how about putting genesis into a subpackage?

Do you mean that genesis output to github.com/cosmos/cosmos-sdk/x/nft/genesis?

yes. What do you think about this idea?

When thinking more about it - I would propose to split it in the following way:

x/nft -- for all things related to RPC

x/nft/dal -- for all things related to storage (data access layer)
This way we have a clear domains.

PS: we think to do the same for eco-credit module

BTW: Aaron pointed in a chat, that this will require splitting the proto packages as well:

proto/cosmos/nft/v1beta1

proto/cosmos/nft/dal/v1beta1

The main point for breaking down the proto into 2 (or more) packages is to have more freedom to upgrading it in the future:

the protobuf service API and related types should be rather stable and we want to avoid any breaking change. We will version it.

the storage types can be in an internal package and don't need to be versioned. If "tomorrow" we will find a better storage layout we can update the module without bumping the API version.

Okay, got it

Let's please not use the term dal. I would use state instead. I also don't want to delay this PR unnecessarily, and would prefer to do this design in a more general way for the SDK.

If so, how can this PR be modified?

Recently, @aaronc pointed that the store proto version is important for merkle proofs. So, we will probably need to discuss more about it.
TL;DR - let's merge this in one package.

robert-zaremba

Looks good. Let's add a comment to the uri_hash attribute.

robert-zaremba · 2021-08-17T22:33:02Z

proto/cosmos/nft/v1beta1/nft.proto

+  string name        = 2;
+  string symbol      = 3;
+  string description = 4;
+  string uri         = 5;


Please add a comment to the uri_hash

…smos-sdk into zhiqiang/nft-proto

robert-zaremba · 2021-08-20T13:28:00Z

Is there anything left for finalizing this PR? We will need a second review. Not sure if @aaronc will have a time (he is starting holidays next week). So maybe @okwme or @alexanderbez ?

proto/cosmos/nft/v1beta1/nft.proto

aaronc

LGTM!

But I would really like there to be more documentation comments.

(side note: I am a bit uncomfortable that we disabled COMMENT_FIELD in buf.yaml. can we maybe change this @AmauryM @ryanchristo ?)

amaury1093 · 2021-08-20T16:46:07Z

(side note: I am a bit uncomfortable that we disabled COMMENT_FIELD in buf.yaml. can we maybe change this @AmauryM @ryanchristo ?)

Yeah, I agree. #9978

codecov · 2021-08-24T05:51:10Z

Codecov Report

Merging #9747 (3747e37) into master (54b0556) will increase coverage by 0.00%.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #9747   +/-   ##
=======================================
  Coverage   63.56%   63.56%           
=======================================
  Files         572      572           
  Lines       53584    53584           
=======================================
+ Hits        34058    34059    +1     
+ Misses      17583    17582    -1     
  Partials     1943     1943

Impacted Files	Coverage Δ
crypto/keys/internal/ecdsa/privkey.go	`84.21% <0.00%> (+1.75%)`	⬆️

ryanchristo

Are we waiting for comments on the message fields before merging?

There are very few descriptions for message fields generated in the proto documentation. I think it would be preferred to address adding the comments within this pull request rather than having to play catch up in #9978 or a followup pull request.

The group module provides a good example of how thorough comments should be:
https://github.com/cosmos/cosmos-sdk/blob/master/proto/cosmos/group/v1beta1/tx.proto

ryanchristo · 2021-08-25T21:12:38Z

docs/core/proto-docs.md

+| Field | Type | Label | Description |
+| ----- | ---- | ----- | ----------- |
+| `class_id` | [string](#string) |  |  |
+| `id` | [string](#string) |  |  |
+| `owner` | [string](#string) |  |  |


Here's an example of the generated protobuf documentation without comments.

robert-zaremba · 2021-08-27T16:16:16Z

I believe this is ready for merge - please check if all doc comments are there.

orijbot · 2021-08-31T11:10:57Z

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

chengwenxi and others added 5 commits April 22, 2021 15:30

initialize nft structure

e1ea803

Merge branch 'master' into vincent/nft

08cc1c7

Merge branch 'master' into vincent/nft

42a99c9

Merge branch 'master' into vincent/nft

9c68b5a

generate nft protobuf

5c73e72

dreamer-zq requested review from aaronc and alexanderbez as code owners July 22, 2021 03:27

dreamer-zq changed the title ~~Generate nft protobuf~~ feat: generate nft protobuf Jul 22, 2021

chengwenxi mentioned this pull request Jul 22, 2021

feat: initialize module x/nft #9174

Closed

aaronc reviewed Jul 22, 2021

View reviewed changes

proto/cosmos/nft/v1beta1/event.proto Show resolved Hide resolved

proto/cosmos/nft/v1beta1/genesis.proto Outdated Show resolved Hide resolved

remove gogoproto annotations

c33da0d

robert-zaremba self-assigned this Jul 28, 2021

robert-zaremba added this to the v0.44 milestone Jul 28, 2021

robert-zaremba added the C: Proto Proto definition and proto release label Jul 28, 2021

chengwenxi changed the base branch from vincent/nft to master July 31, 2021 11:20

Merge branch 'master' into zhiqiang/nft-proto

2660649

chengwenxi mentioned this pull request Jul 31, 2021

Implement ADR-043-nft #9826

Closed

13 tasks

chengwenxi and others added 2 commits August 3, 2021 16:37

Merge branch 'master' into zhiqiang/nft-proto

f443b10

Merge branch 'master' into zhiqiang/nft-proto

daae65c

robert-zaremba approved these changes Aug 10, 2021

View reviewed changes

Dreamer added 3 commits August 11, 2021 09:51

apply comments from github

4aad943

Merge branch 'master' into zhiqiang/nft-proto

8a8e429

Merge branch 'master' into zhiqiang/nft-proto

a1b171e

robert-zaremba approved these changes Aug 17, 2021

View reviewed changes

Dreamer added 3 commits August 18, 2021 17:14

apply comment from github

e4bbd6a

Merge branch 'zhiqiang/nft-proto' of https://github.com/dreamer-zq/co…

21801e7

…smos-sdk into zhiqiang/nft-proto

update comment

f2d15c0

Merge branch 'master' into zhiqiang/nft-proto

4b1ceec

chengwenxi requested a review from aaronc August 20, 2021 09:14

aaronc reviewed Aug 20, 2021

View reviewed changes

proto/cosmos/nft/v1beta1/nft.proto Outdated Show resolved Hide resolved

aaronc approved these changes Aug 20, 2021

View reviewed changes

amaury1093 mentioned this pull request Aug 20, 2021

Re-enable COMMENT_FIELD buf lint check #9978

Closed

4 tasks

Zhiqiang Zhang added 3 commits August 23, 2021 10:09

Merge branch 'master' into zhiqiang/nft-proto

792eb3d

apply comments from github

590788c

Merge branch 'master' into zhiqiang/nft-proto

72cebc2

ryanchristo reviewed Aug 25, 2021

View reviewed changes

add some comments

ca97ed3

tac0turtle added the A:automerge Automatically merge PR once all prerequisites pass. label Aug 31, 2021

Merge branch 'master' into zhiqiang/nft-proto

3747e37

mergify bot merged commit 6bddcd5 into cosmos:master Aug 31, 2021

chengwenxi deleted the zhiqiang/nft-proto branch August 31, 2021 11:22

robert-zaremba mentioned this pull request Dec 9, 2021

Should metadata fields in projects / credit classes / credit batches be matched with a field for metadata_hash? regen-network/regen-ledger#668

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: generate nft protobuf #9747

feat: generate nft protobuf #9747

dreamer-zq commented Jul 22, 2021 •

edited by chengwenxi

Loading

aaronc left a comment

chengwenxi commented Jul 23, 2021 •

edited

Loading

aaronc commented Jul 30, 2021

charleenfei commented Aug 3, 2021

robert-zaremba left a comment

robert-zaremba Aug 10, 2021

robert-zaremba Aug 17, 2021

robert-zaremba Aug 10, 2021

robert-zaremba Aug 10, 2021

robert-zaremba Aug 10, 2021

robert-zaremba Aug 10, 2021

dreamer-zq Aug 11, 2021

robert-zaremba Aug 11, 2021

robert-zaremba Aug 11, 2021 •

edited

Loading

robert-zaremba Aug 11, 2021

robert-zaremba Aug 12, 2021 •

edited

Loading

dreamer-zq Aug 13, 2021

aaronc Aug 13, 2021

dreamer-zq Aug 16, 2021

robert-zaremba Aug 17, 2021

robert-zaremba left a comment

robert-zaremba Aug 17, 2021

robert-zaremba commented Aug 20, 2021

aaronc left a comment

amaury1093 commented Aug 20, 2021

codecov bot commented Aug 24, 2021 •

edited

Loading

ryanchristo left a comment

ryanchristo Aug 25, 2021

robert-zaremba commented Aug 27, 2021

orijbot commented Aug 31, 2021

	// Supply queries the number of nft based on the class, same as totalSupply of ERC721
	// Supply queries the number of NFTs from the given class, same as totalSupply of ERC721.


		import "cosmos/nft/v1beta1/nft.proto";

		option go_package = "github.com/cosmos/cosmos-sdk/x/nft";

feat: generate nft protobuf #9747

feat: generate nft protobuf #9747

Conversation

dreamer-zq commented Jul 22, 2021 • edited by chengwenxi Loading

Description

Author Checklist

Reviewers Checklist

aaronc left a comment

Choose a reason for hiding this comment

chengwenxi commented Jul 23, 2021 • edited Loading

aaronc commented Jul 30, 2021

charleenfei commented Aug 3, 2021

robert-zaremba left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robert-zaremba Aug 11, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robert-zaremba Aug 12, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robert-zaremba left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robert-zaremba commented Aug 20, 2021

aaronc left a comment

Choose a reason for hiding this comment

amaury1093 commented Aug 20, 2021

codecov bot commented Aug 24, 2021 • edited Loading

Codecov Report

ryanchristo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robert-zaremba commented Aug 27, 2021

orijbot commented Aug 31, 2021

dreamer-zq commented Jul 22, 2021 •

edited by chengwenxi

Loading

chengwenxi commented Jul 23, 2021 •

edited

Loading

robert-zaremba Aug 11, 2021 •

edited

Loading

robert-zaremba Aug 12, 2021 •

edited

Loading

codecov bot commented Aug 24, 2021 •

edited

Loading