feat: implement nft module msg server #10074

dreamer-zq · 2021-09-03T06:18:00Z

Description

MsgServer implementation and corresponding keeper methods, 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)

codecov · 2021-09-03T06:27:39Z

Codecov Report

Merging #10074 (8133e0c) into master (0a3660d) will increase coverage by 0.24%.
The diff coverage is 75.53%.

❗ Current head 8133e0c differs from pull request most recent head e38a1fe. Consider uploading reports for the commit e38a1fe to get more accurate results

@@            Coverage Diff             @@
##           master   #10074      +/-   ##
==========================================
+ Coverage   64.19%   64.44%   +0.24%     
==========================================
  Files         575      579       +4     
  Lines       54923    54901      -22     
==========================================
+ Hits        35257    35380     +123     
+ Misses      17663    17509     -154     
- Partials     2003     2012       +9

Impacted Files	Coverage Δ
x/nft/keeper/msg_server.go	`0.00% <0.00%> (ø)`
x/nft/keeper/class.go	`73.33% <73.33%> (ø)`
x/nft/keeper/keeper.go	`77.77% <77.77%> (ø)`
x/nft/keeper/nft.go	`80.16% <80.16%> (ø)`
x/nft/keeper/genesis.go	`85.00% <85.00%> (ø)`
simapp/app.go	`81.93% <100.00%> (+0.78%)`	⬆️
x/nft/keeper/keys.go	`100.00% <100.00%> (ø)`
x/distribution/simulation/operations.go	`80.54% <0.00%> (-9.73%)`	⬇️
x/auth/tx/builder.go	`76.05% <0.00%> (-2.55%)`	⬇️
types/module/module.go	`65.04% <0.00%> (-1.96%)`	⬇️
... and 16 more

x/nft/msgs.go

RiccardoM · 2021-09-30T13:13:10Z

Any updates on this @ryanchristo @clevinson ?

robert-zaremba

Looks good!

let's move the generated proto code to a separate module. I propose x/nft/proto. The reason is related to the semver discussion for Cosmos SDK, and versioning proto definition is one of the requirements. We will do that work for all other modules right after 0.45 release.
let's add integration tests on the proto service level (can be done in a separate PR - we will just need to create a task for it - leaving it to you).
we should avoid panics and Must* methods whenever possible.
also, I would consider dropping the beta name in the proto versions. This doesn't help with versioning in fact. Once it will be used it should not have beta (let's add it to the issue tracker).

robert-zaremba · 2021-10-01T11:53:33Z

x/nft/expected_keepers.go

+type AccountKeeper interface {
+	GetModuleAddress(name string) sdk.AccAddress
+	GetAccount(ctx sdk.Context, addr sdk.AccAddress) authtypes.AccountI
+}


would be good to depend on Query proto services rather than keepers directly, but I think we will firstly need to clean other parts of SDK before requesting that changes (and finalize adr-33).

x/nft/keeper/class.go

x/nft/keeper/keys.go

robert-zaremba · 2021-10-01T12:29:07Z

x/nft/keeper/msg_server.go

+		Id:       msg.Id,
+		Sender:   msg.Sender,
+		Receiver: msg.Receiver,
+	})


x/nft/keeper/nft.go

x/nft/nft.pb.go

charleenfei · 2021-10-01T16:58:36Z

let's move the generated proto code to a separate module. I propose x/nft/proto. The reason is related to the semver discussion for Cosmos SDK, and versioning proto definition is one of the requirements. We will do that work for all other modules right after 0.45 release.

just for clarity: this was decided in the SDK grooming meeting that this change is not necessary for the PR to be merged

robert-zaremba · 2021-10-01T18:02:50Z

just for clarity: this was decided in the SDK grooming meeting that this change is not necessary for the PR to be merged

Yes. Definitely not blocking. IMHO it's good to do it though, because we will already separate more stable proto (which external clients will be able to consume) from the functional implementation. And it's very easy to do that split at this stage when no other external app is depending on it.

x/nft/genesis.go

x/nft/keeper/genesis.go

x/nft/keeper/keys.go

clevinson

Some comments as I was going through this today. Didn't fully finish, but going OOO so wanted to post what I have so far.

Thanks for this work!

x/nft/genesis.go

x/nft/keeper/keeper_test.go

…plement-nft-msgServer

dreamer-zq · 2021-10-08T03:28:35Z

Looks good!

let's move the generated proto code to a separate module. I propose x/nft/proto. The reason is related to the semver discussion for Cosmos SDK, and versioning proto definition is one of the requirements. We will do that work for all other modules right after 0.45 release.

let's add integration tests on the proto service level (can be done in a separate PR - we will just need to create a task for it - leaving it to you).

we should avoid panics and Must* methods whenever possible.

also, I would consider dropping the beta name in the proto versions. This doesn't help with versioning in fact. Once it will be used it should not have beta (let's add it to the issue tracker).

When I tried to move the generated proto file to the x/nft/proto package, I encountered the following problems:

MsgSend in the msgs.go file implements the sdk.Msg interface, and needs to be moved to the x/nft/proto package (if this is done, in order to avoid circular dependencies, you also need to move errors.go to the x/nft/proto package , It feels inappropriate to do so)
The unexported method in the x/nft/proto package is used in codec.go: _Msg_serviceDesc.

Do you have any good solutions to the above problems?

catShaark · 2021-10-09T11:48:14Z

Your mint method checks if the class of the minted nft exists, but there's no msg to create new class. So how can user mint new nft ?

dreamer-zq · 2021-10-11T01:34:14Z

Your mint method checks if the class of the minted nft exists, but there's no msg to create new class. So how can user mint new nft ?

According to the design of adr-043-nft, this module is the low-level design of nft, which only exposes the methods and storage of nft general implementation. Users need to implement the upper-level logic according to their own business rules, such as the definition of msg (except for nft.MsgSend )

catShaark · 2021-10-11T11:18:39Z

Your mint method checks if the class of the minted nft exists, but there's no msg to create new class. So how can user mint new nft ?

According to the design of adr-043-nft, this module is the low-level design of nft, which only exposes the methods and storage of nft general implementation. Users need to implement the upper-level logic according to their own business rules, such as the definition of msg (except for nft.MsgSend )

ohh, I get it now, thank u

robert-zaremba · 2021-10-11T11:45:25Z

@dreamer-zq , the previous question shows that we should better explain that in the ADR. Could you add a paragraph in the ADR highlighting what client module must provide (at minimum)?

robert-zaremba · 2021-10-11T11:49:52Z

We are in fact discussing solutions on the future migration of the core modules. We don't have a plan finalized yet. If you encounter some problems, then you can skip it and we will try to refactor it later.

1. `MsgSend` in the msgs.go file implements the `sdk.Msg` interface, and needs to be moved to the x/nft/proto package (if this is done, in order to avoid circular dependencies, you also need to move errors.go to the x/nft/proto package , It feels inappropriate to do so)

We have few ideas, but non of them is finalized (eg: wrappers, implementer function as an attribute)... you could include the implementation in the proto package... but again - if this will cause some doubts then let's skip it.

2. The unexported method in the x/nft/proto package is used in codec.go: `_Msg_serviceDesc`.

You can add a method in x/nft/proto which will return _Msg_serviceDesc (eg: GetMsgServiceDesc() ).

aaronc · 2021-10-11T14:28:05Z

I advise that we pause refactoring generated proto code into separate go modules here. I think our team should lead doing this refactoring once we've figured out a strategy, and this should be done in a separate PR. We should also use the x/nft/types package for this, not x/nft/proto IMHO. But again, let's just focus on merging the basic msg server functionality here.

dreamer-zq · 2021-10-13T07:14:51Z

@dreamer-zq , the previous question shows that we should better explain that in the ADR. Could you add a paragraph in the ADR highlighting what client module must provide (at minimum)?

I have added some instructions in adr-33

…plement-nft-msgServer

robert-zaremba

Left few more comments.

Most importantly, we need to implement UnpackInterfaces for the NFT type.

x/nft/module/module.go

x/nft/keeper/class.go

x/nft/validation.go

…plement-nft-msgServer

robert-zaremba

All good! Thanks for the PR!

…plement-nft-msgServer

aaronc · 2021-10-20T17:33:12Z

Happy to merge this. Can someone take a look at the test failures?

aaronc · 2021-10-27T14:06:14Z

What's up with CI? @robert-zaremba @AmauryM ? We really need to fix these issues. Is there anything in CI actually preventing this from being merged? Also is this go module actually getting tested in CI?

amaury1093 · 2021-10-27T14:12:01Z

There's a couple of flaky tests #9010. No-one has found their root cause yet.

The current way we're circumventing this is by re-running the tests, which I just did

## Description MsgServer implementation and corresponding keeper methods, refer cosmos#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... - [ ] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title - [ ] added `!` to the type prefix if API or client breaking change - [ ] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#pr-targeting)) - [ ] provided a link to the relevant issue or specification - [ ] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/master/docs/building-modules) - [ ] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#testing) - [ ] added a changelog entry to `CHANGELOG.md` - [ ] included comments for [documenting Go code](https://blog.golang.org/godoc) - [ ] updated the relevant documentation or specification - [ ] reviewed "Files changed" and left comments if necessary - [ ] confirmed all CI checks have passed ### 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](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) 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)

Dreamer added 2 commits September 3, 2021 14:03

implement nft msgServer

59ce1a2

add comment

36a50b1

dreamer-zq requested review from aaronc and alexanderbez as code owners September 3, 2021 06:18

github-actions bot added the C:CLI label Sep 3, 2021

chengwenxi mentioned this pull request Sep 3, 2021

Implement ADR-043-nft #9826

Closed

13 tasks

tac0turtle reviewed Sep 3, 2021

View reviewed changes

x/nft/msgs.go Outdated Show resolved Hide resolved

remove legacy support

fcb7fba

ryanchristo assigned clevinson Sep 7, 2021

robert-zaremba reviewed Oct 1, 2021

View reviewed changes

fedekunze reviewed Oct 2, 2021

View reviewed changes

x/nft/genesis.go Outdated Show resolved Hide resolved

x/nft/genesis.go Show resolved Hide resolved

x/nft/keeper/genesis.go Outdated Show resolved Hide resolved

x/nft/keeper/keys.go Show resolved Hide resolved

clevinson reviewed Oct 7, 2021

View reviewed changes

x/nft/genesis.go Show resolved Hide resolved

x/nft/keeper/keeper_test.go Show resolved Hide resolved

x/nft/keeper/keeper_test.go Outdated Show resolved Hide resolved

x/nft/keeper/keeper_test.go Outdated Show resolved Hide resolved

Merge branch 'master' of https://github.com/cosmos/cosmos-sdk into im…

cd881c8

…plement-nft-msgServer

github-actions bot removed the C:CLI label Oct 8, 2021

Dreamer added 2 commits October 8, 2021 17:08

apply comments from github

5692305

rename variable

844453d

apply comment from github

7ed9d6d

github-actions bot added the T: ADR An issue or PR relating to an architectural decision record label Oct 13, 2021

Dreamer added 4 commits October 13, 2021 15:17

resolve conflict

906acb2

resolve conflict

8791b08

resolve conflict

da47700

Merge branch 'master' of https://github.com/cosmos/cosmos-sdk into im…

7b30a6c

…plement-nft-msgServer

clevinson requested review from robert-zaremba and clevinson October 15, 2021 14:51

robert-zaremba reviewed Oct 15, 2021

View reviewed changes

x/nft/module/module.go Show resolved Hide resolved

x/nft/keeper/class.go Outdated Show resolved Hide resolved

x/nft/validation.go Outdated Show resolved Hide resolved

x/nft/validation.go Outdated Show resolved Hide resolved

x/nft/validation.go Outdated Show resolved Hide resolved

Dreamer added 4 commits October 18, 2021 09:48

apply comment from github

f0f2750

fix conflict

ecc2585

Merge branch 'master' of https://github.com/cosmos/cosmos-sdk into im…

22aa3d5

…plement-nft-msgServer

fix conflict

d6c965f

robert-zaremba approved these changes Oct 18, 2021

View reviewed changes

Dreamer and others added 3 commits October 20, 2021 09:26

Merge branch 'master' of https://github.com/cosmos/cosmos-sdk into im…

58130f9

…plement-nft-msgServer

merge from master

20ed66b

Merge branch 'master' into implement-nft-msgServer

d523160

aaronc approved these changes Oct 20, 2021

View reviewed changes

aaronc added A:automerge Automatically merge PR once all prerequisites pass. C:x/nft labels Oct 20, 2021

aaronc and others added 4 commits October 20, 2021 13:33

Merge branch 'master' into implement-nft-msgServer

c698e31

fix test error

8133e0c

Merge branch 'master' into implement-nft-msgServer

59e566c

Merge branch 'master' into implement-nft-msgServer

a44e772

Merge branch 'master' into implement-nft-msgServer

e38a1fe

mergify bot merged commit 1d1fcc9 into cosmos:master Oct 27, 2021

chengwenxi deleted the implement-nft-msgServer branch October 29, 2021 03:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: implement nft module msg server #10074

feat: implement nft module msg server #10074

dreamer-zq commented Sep 3, 2021 •

edited by chengwenxi

Loading

codecov bot commented Sep 3, 2021 •

edited

Loading

RiccardoM commented Sep 30, 2021

robert-zaremba left a comment

robert-zaremba Oct 1, 2021

robert-zaremba Oct 1, 2021

charleenfei commented Oct 1, 2021

robert-zaremba commented Oct 1, 2021

clevinson left a comment

dreamer-zq commented Oct 8, 2021 •

edited

Loading

catShaark commented Oct 9, 2021

dreamer-zq commented Oct 11, 2021 •

edited

Loading

catShaark commented Oct 11, 2021

robert-zaremba commented Oct 11, 2021

robert-zaremba commented Oct 11, 2021

aaronc commented Oct 11, 2021

dreamer-zq commented Oct 13, 2021

robert-zaremba left a comment

robert-zaremba left a comment

aaronc commented Oct 20, 2021

aaronc commented Oct 27, 2021

amaury1093 commented Oct 27, 2021

feat: implement nft module msg server #10074

feat: implement nft module msg server #10074

Conversation

dreamer-zq commented Sep 3, 2021 • edited by chengwenxi Loading

Description

Author Checklist

Reviewers Checklist

codecov bot commented Sep 3, 2021 • edited Loading

Codecov Report

RiccardoM commented Sep 30, 2021

robert-zaremba left a comment

Choose a reason for hiding this comment

robert-zaremba Oct 1, 2021

Choose a reason for hiding this comment

robert-zaremba Oct 1, 2021

Choose a reason for hiding this comment

charleenfei commented Oct 1, 2021

robert-zaremba commented Oct 1, 2021

clevinson left a comment

Choose a reason for hiding this comment

dreamer-zq commented Oct 8, 2021 • edited Loading

catShaark commented Oct 9, 2021

dreamer-zq commented Oct 11, 2021 • edited Loading

catShaark commented Oct 11, 2021

robert-zaremba commented Oct 11, 2021

robert-zaremba commented Oct 11, 2021

aaronc commented Oct 11, 2021

dreamer-zq commented Oct 13, 2021

robert-zaremba left a comment

Choose a reason for hiding this comment

robert-zaremba left a comment

Choose a reason for hiding this comment

aaronc commented Oct 20, 2021

aaronc commented Oct 27, 2021

amaury1093 commented Oct 27, 2021

dreamer-zq commented Sep 3, 2021 •

edited by chengwenxi

Loading

codecov bot commented Sep 3, 2021 •

edited

Loading

dreamer-zq commented Oct 8, 2021 •

edited

Loading

dreamer-zq commented Oct 11, 2021 •

edited

Loading