Add NFT functionality to Cosmos SDK #23279
Conversation
Related to cosmos#20791 Add full NFT module functionality to Cosmos SDK. * Add `MsgNewClass`, `MsgUpdateClass`, `MsgMintNFT`, `MsgBurnNFT`, and `MsgUpdateNFT` methods to `x/nft/keeper/keeper.go`. * Add `MsgNewClass`, `MsgUpdateClass`, `MsgMintNFT`, `MsgBurnNFT`, and `MsgUpdateNFT` messages to `x/nft/proto/cosmos/nft/v1beta1/tx.proto`. * Update `x/nft/keeper/msg_server.go` to implement the new methods for creating, minting, burning, and updating NFTs. * Update `x/nft/proto/cosmos/nft/v1beta1/nft.proto` to include the new methods for creating, minting, burning, and updating NFTs. * Add tests for the new methods in `x/nft/keeper/keeper_test.go` and `x/nft/keeper/msg_server_test.go`.
vishwamartur
requested review from
alpe,
lucaslopezf and
a team
as code owners
📝 WalkthroughWalkthroughThis pull request introduces comprehensive NFT (Non-Fungible Token) management functionality to the Cosmos SDK. The changes add methods for creating, updating, minting, and burning NFT classes and individual NFTs across multiple files. The implementation includes new message handlers, keeper methods, and protobuf definitions that enable full lifecycle management of NFTs within the blockchain ecosystem. Changes
Sequence DiagramsequenceDiagram
participant Client
participant MsgServer
participant Keeper
participant Store
Client->>MsgServer: MsgNewClass
MsgServer->>Keeper: NewClass()
Keeper->>Store: SaveClass()
Store-->>Keeper: Class Saved
Keeper-->>MsgServer: Success Response
MsgServer-->>Client: Confirm Class Creation
Client->>MsgServer: MsgMintNFT
MsgServer->>Keeper: MintNFT()
Keeper->>Store: Mint()
Store-->>Keeper: NFT Created
Keeper-->>MsgServer: Success Response
MsgServer-->>Client: Confirm NFT Minting
Possibly Related PRs
Suggested Labels
Suggested Reviewers
Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (2)
x/nft/proto/cosmos/nft/v1beta1/tx.proto (2)
16-29: Enhance RPC method documentation with parameter descriptions and examples.While the RPC methods are well-structured, their documentation could be more comprehensive. Consider adding:
- Parameter descriptions
- Expected behavior
- Error conditions
- Usage examples
Example enhancement for NewClass:
- // NewClass defines a method to create a new NFT class. + // NewClass creates a new NFT class with the specified parameters. + // Returns an error if: + // - class_id already exists + // - creator address is invalid + // Example: NewClass(class_id: "artwork", name: "Digital Art Collection", ...)
116-140: Add supply management to MsgMintNFT.Consider adding fields to manage token supply:
- Maximum supply per class
- Current supply tracking
This helps prevent unlimited minting and provides better control over NFT scarcity.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
x/nft/keeper/keeper.go(1 hunks)x/nft/keeper/keeper_test.go(1 hunks)x/nft/keeper/msg_server.go(1 hunks)x/nft/keeper/msg_server_test.go(1 hunks)x/nft/proto/cosmos/nft/v1beta1/tx.proto(2 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
x/nft/keeper/msg_server_test.go (2)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
x/nft/keeper/keeper_test.go (2)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
x/nft/keeper/msg_server.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
x/nft/keeper/keeper.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🪛 golangci-lint (1.62.2)
x/nft/keeper/msg_server_test.go
135-135: method TestSuite.TestMsgNewClass already declared at keeper/keeper_test.go:403:21
(typecheck)
160-160: method TestSuite.TestMsgUpdateClass already declared at keeper/keeper_test.go:428:21
(typecheck)
196-196: method TestSuite.TestMsgMintNFT already declared at keeper/keeper_test.go:464:21
(typecheck)
229-229: method TestSuite.TestMsgBurnNFT already declared at keeper/keeper_test.go:497:21
(typecheck)
262-262: method TestSuite.TestMsgUpdateNFT already declared at keeper/keeper_test.go:530:21
(typecheck)
x/nft/keeper/keeper_test.go
404-404: undefined: nft.MsgNewClass
(typecheck)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Summary
🔇 Additional comments (1)
x/nft/proto/cosmos/nft/v1beta1/tx.proto (1)
Line range hint
1-186: Verify integration with related NFT files.Let's ensure consistency across the NFT module implementation.
✅ Verification successful
NFT module integration verified successfully
The NFT messages are well-integrated with consistent field definitions across the module, comprehensive test coverage, and proper handler implementations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify NFT message handling implementation # Check for corresponding handler implementations echo "Checking message handlers..." rg -l "func.*MsgServer.*NFT" x/nft/keeper/ # Verify test coverage echo "Checking test coverage..." rg -l "Test.*NFT" x/nft/**/*_test.go # Check for consistent field names echo "Checking field consistency..." rg "class_id|id|uri|uri_hash|data" x/nft/proto/cosmos/nft/v1beta1/*.protoLength of output: 10694
| // MsgUpdateClass updates an existing NFT class | ||
| func (k Keeper) MsgUpdateClass(ctx context.Context, msg *nft.MsgUpdateClass) (*nft.MsgUpdateClassResponse, error) { | ||
| class := nft.Class{ | ||
| Id: msg.ClassId, | ||
| Name: msg.Name, | ||
| Symbol: msg.Symbol, | ||
| Description: msg.Description, | ||
| Uri: msg.Uri, | ||
| UriHash: msg.UriHash, | ||
| Data: msg.Data, | ||
| } | ||
| if err := k.UpdateClass(ctx, class); err != nil { | ||
| return nil, err | ||
| } | ||
| return &nft.MsgUpdateClassResponse{}, nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation and authorization checks for class updates.
The function needs the following improvements:
- Add input validation for required fields (ClassId).
- Add authorization check to control who can update NFT classes.
- Emit an event when a class is updated for better observability.
Apply this diff to add the suggested improvements:
func (k Keeper) MsgUpdateClass(ctx context.Context, msg *nft.MsgUpdateClass) (*nft.MsgUpdateClassResponse, error) {
+ if msg.ClassId == "" {
+ return nil, nft.ErrEmptyClassID
+ }
+
+ sender, err := k.ac.StringToBytes(msg.Sender)
+ if err != nil {
+ return nil, errorsmod.Wrapf(sdkerrors.ErrInvalidAddress, "Invalid sender address (%s)", msg.Sender)
+ }
+
+ // Verify sender is authorized to update the class
+ if !k.IsClassEditor(ctx, msg.ClassId, sender) {
+ return nil, errorsmod.Wrapf(sdkerrors.ErrUnauthorized, "%s is not authorized to update class %s", msg.Sender, msg.ClassId)
+ }
+
class := nft.Class{
Id: msg.ClassId,
Name: msg.Name,
Symbol: msg.Symbol,
Description: msg.Description,
Uri: msg.Uri,
UriHash: msg.UriHash,
Data: msg.Data,
}
if err := k.UpdateClass(ctx, class); err != nil {
return nil, err
}
+
+ if err = k.EventService.EventManager(ctx).Emit(&nft.EventUpdateClass{
+ ClassId: msg.ClassId,
+ Sender: msg.Sender,
+ }); err != nil {
+ return nil, err
+ }
+
return &nft.MsgUpdateClassResponse{}, nil
}Committable suggestion skipped: line range outside the PR's diff.
| // MsgUpdateNFT updates an existing NFT | ||
| func (k Keeper) MsgUpdateNFT(ctx context.Context, msg *nft.MsgUpdateNFT) (*nft.MsgUpdateNFTResponse, error) { | ||
| nft := nft.NFT{ | ||
| ClassId: msg.ClassId, | ||
| Id: msg.Id, | ||
| Uri: msg.Uri, | ||
| UriHash: msg.UriHash, | ||
| Data: msg.Data, | ||
| } | ||
| if err := k.Update(ctx, nft); err != nil { | ||
| return nil, err | ||
| } | ||
| return &nft.MsgUpdateNFTResponse{}, nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation and authorization checks for NFT updates.
The function needs the following improvements:
- Add input validation for required fields (ClassId, Id).
- Add authorization check to ensure only the NFT owner can update it.
- Emit an event when an NFT is updated for better observability.
Apply this diff to add the suggested improvements:
func (k Keeper) MsgUpdateNFT(ctx context.Context, msg *nft.MsgUpdateNFT) (*nft.MsgUpdateNFTResponse, error) {
+ if msg.ClassId == "" {
+ return nil, nft.ErrEmptyClassID
+ }
+ if msg.Id == "" {
+ return nil, nft.ErrEmptyNFTID
+ }
+
+ sender, err := k.ac.StringToBytes(msg.Sender)
+ if err != nil {
+ return nil, errorsmod.Wrapf(sdkerrors.ErrInvalidAddress, "Invalid sender address (%s)", msg.Sender)
+ }
+
+ // Verify sender owns the NFT
+ owner := k.GetOwner(ctx, msg.ClassId, msg.Id)
+ if !bytes.Equal(owner, sender) {
+ return nil, errorsmod.Wrapf(sdkerrors.ErrUnauthorized, "%s is not the owner of NFT %s", msg.Sender, msg.Id)
+ }
+
nft := nft.NFT{
ClassId: msg.ClassId,
Id: msg.Id,
Uri: msg.Uri,
UriHash: msg.UriHash,
Data: msg.Data,
}
if err := k.Update(ctx, nft); err != nil {
return nil, err
}
+
+ if err = k.EventService.EventManager(ctx).Emit(&nft.EventUpdateNFT{
+ ClassId: msg.ClassId,
+ Id: msg.Id,
+ Sender: msg.Sender,
+ }); err != nil {
+ return nil, err
+ }
+
return &nft.MsgUpdateNFTResponse{}, nil
}Committable suggestion skipped: line range outside the PR's diff.
| // MsgNewClass creates a new NFT class | ||
| func (k Keeper) MsgNewClass(ctx context.Context, msg *nft.MsgNewClass) (*nft.MsgNewClassResponse, error) { | ||
| class := nft.Class{ | ||
| Id: msg.ClassId, | ||
| Name: msg.Name, | ||
| Symbol: msg.Symbol, | ||
| Description: msg.Description, | ||
| Uri: msg.Uri, | ||
| UriHash: msg.UriHash, | ||
| Data: msg.Data, | ||
| } | ||
| if err := k.SaveClass(ctx, class); err != nil { | ||
| return nil, err | ||
| } | ||
| return &nft.MsgNewClassResponse{}, nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation and authorization checks for class creation.
The function needs the following improvements:
- Add input validation for required fields (ClassId, Name, Symbol).
- Add authorization check to control who can create NFT classes.
- Emit an event when a class is created for better observability.
Apply this diff to add the suggested improvements:
func (k Keeper) MsgNewClass(ctx context.Context, msg *nft.MsgNewClass) (*nft.MsgNewClassResponse, error) {
+ if msg.ClassId == "" {
+ return nil, nft.ErrEmptyClassID
+ }
+ if msg.Name == "" {
+ return nil, nft.ErrEmptyClassName
+ }
+ if msg.Symbol == "" {
+ return nil, nft.ErrEmptyClassSymbol
+ }
+
+ sender, err := k.ac.StringToBytes(msg.Sender)
+ if err != nil {
+ return nil, errorsmod.Wrapf(sdkerrors.ErrInvalidAddress, "Invalid sender address (%s)", msg.Sender)
+ }
+
class := nft.Class{
Id: msg.ClassId,
Name: msg.Name,
Symbol: msg.Symbol,
Description: msg.Description,
Uri: msg.Uri,
UriHash: msg.UriHash,
Data: msg.Data,
}
if err := k.SaveClass(ctx, class); err != nil {
return nil, err
}
+
+ if err = k.EventService.EventManager(ctx).Emit(&nft.EventNewClass{
+ ClassId: msg.ClassId,
+ Sender: msg.Sender,
+ }); err != nil {
+ return nil, err
+ }
+
return &nft.MsgNewClassResponse{}, nil
}Committable suggestion skipped: line range outside the PR's diff.
| // MsgMintNFT mints a new NFT | ||
| func (k Keeper) MsgMintNFT(ctx context.Context, msg *nft.MsgMintNFT) (*nft.MsgMintNFTResponse, error) { | ||
| nft := nft.NFT{ | ||
| ClassId: msg.ClassId, | ||
| Id: msg.Id, | ||
| Uri: msg.Uri, | ||
| UriHash: msg.UriHash, | ||
| Data: msg.Data, | ||
| } | ||
| if err := k.Mint(ctx, nft, msg.Receiver); err != nil { | ||
| return nil, err | ||
| } | ||
| return &nft.MsgMintNFTResponse{}, nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation and authorization checks for NFT minting.
The function needs the following improvements:
- Add input validation for required fields (ClassId, Id, Receiver).
- Add authorization check to control who can mint NFTs.
- Emit an event when an NFT is minted for better observability.
Apply this diff to add the suggested improvements:
func (k Keeper) MsgMintNFT(ctx context.Context, msg *nft.MsgMintNFT) (*nft.MsgMintNFTResponse, error) {
+ if msg.ClassId == "" {
+ return nil, nft.ErrEmptyClassID
+ }
+ if msg.Id == "" {
+ return nil, nft.ErrEmptyNFTID
+ }
+ if msg.Receiver == "" {
+ return nil, nft.ErrEmptyReceiver
+ }
+
+ sender, err := k.ac.StringToBytes(msg.Sender)
+ if err != nil {
+ return nil, errorsmod.Wrapf(sdkerrors.ErrInvalidAddress, "Invalid sender address (%s)", msg.Sender)
+ }
+
+ // Verify sender is authorized to mint NFTs for this class
+ if !k.IsMinter(ctx, msg.ClassId, sender) {
+ return nil, errorsmod.Wrapf(sdkerrors.ErrUnauthorized, "%s is not authorized to mint NFTs for class %s", msg.Sender, msg.ClassId)
+ }
+
+ receiver, err := k.ac.StringToBytes(msg.Receiver)
+ if err != nil {
+ return nil, errorsmod.Wrapf(sdkerrors.ErrInvalidAddress, "Invalid receiver address (%s)", msg.Receiver)
+ }
+
nft := nft.NFT{
ClassId: msg.ClassId,
Id: msg.Id,
Uri: msg.Uri,
UriHash: msg.UriHash,
Data: msg.Data,
}
- if err := k.Mint(ctx, nft, msg.Receiver); err != nil {
+ if err := k.Mint(ctx, nft, receiver); err != nil {
return nil, err
}
+
+ if err = k.EventService.EventManager(ctx).Emit(&nft.EventMintNFT{
+ ClassId: msg.ClassId,
+ Id: msg.Id,
+ Sender: msg.Sender,
+ Receiver: msg.Receiver,
+ }); err != nil {
+ return nil, err
+ }
+
return &nft.MsgMintNFTResponse{}, nil
}Committable suggestion skipped: line range outside the PR's diff.
| // MsgBurnNFT burns an existing NFT | ||
| func (k Keeper) MsgBurnNFT(ctx context.Context, msg *nft.MsgBurnNFT) (*nft.MsgBurnNFTResponse, error) { | ||
| if err := k.Burn(ctx, msg.ClassId, msg.Id); err != nil { | ||
| return nil, err | ||
| } | ||
| return &nft.MsgBurnNFTResponse{}, nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation and authorization checks for NFT burning.
The function needs the following improvements:
- Add input validation for required fields (ClassId, Id).
- Add authorization check to ensure only the NFT owner can burn it.
- Emit an event when an NFT is burned for better observability.
Apply this diff to add the suggested improvements:
func (k Keeper) MsgBurnNFT(ctx context.Context, msg *nft.MsgBurnNFT) (*nft.MsgBurnNFTResponse, error) {
+ if msg.ClassId == "" {
+ return nil, nft.ErrEmptyClassID
+ }
+ if msg.Id == "" {
+ return nil, nft.ErrEmptyNFTID
+ }
+
+ sender, err := k.ac.StringToBytes(msg.Sender)
+ if err != nil {
+ return nil, errorsmod.Wrapf(sdkerrors.ErrInvalidAddress, "Invalid sender address (%s)", msg.Sender)
+ }
+
+ // Verify sender owns the NFT
+ owner := k.GetOwner(ctx, msg.ClassId, msg.Id)
+ if !bytes.Equal(owner, sender) {
+ return nil, errorsmod.Wrapf(sdkerrors.ErrUnauthorized, "%s is not the owner of NFT %s", msg.Sender, msg.Id)
+ }
+
if err := k.Burn(ctx, msg.ClassId, msg.Id); err != nil {
return nil, err
}
+
+ if err = k.EventService.EventManager(ctx).Emit(&nft.EventBurnNFT{
+ ClassId: msg.ClassId,
+ Id: msg.Id,
+ Sender: msg.Sender,
+ }); err != nil {
+ return nil, err
+ }
+
return &nft.MsgBurnNFTResponse{}, nil
}Committable suggestion skipped: line range outside the PR's diff.
| // NewClass implements NewClass method of the types.MsgServer. | ||
| func (k Keeper) NewClass(ctx context.Context, msg *nft.MsgNewClass) (*nft.MsgNewClassResponse, error) { | ||
| class := nft.Class{ | ||
| Id: msg.ClassId, | ||
| Name: msg.Name, | ||
| Symbol: msg.Symbol, | ||
| Description: msg.Description, | ||
| Uri: msg.Uri, | ||
| UriHash: msg.UriHash, | ||
| Data: msg.Data, | ||
| } | ||
| if err := k.SaveClass(ctx, class); err != nil { | ||
| return nil, err | ||
| } | ||
| return &nft.MsgNewClassResponse{}, nil | ||
| } | ||
|
|
||
| // UpdateClass implements UpdateClass method of the types.MsgServer. | ||
| func (k Keeper) UpdateClass(ctx context.Context, msg *nft.MsgUpdateClass) (*nft.MsgUpdateClassResponse, error) { | ||
| class := nft.Class{ | ||
| Id: msg.ClassId, | ||
| Name: msg.Name, | ||
| Symbol: msg.Symbol, | ||
| Description: msg.Description, | ||
| Uri: msg.Uri, | ||
| UriHash: msg.UriHash, | ||
| Data: msg.Data, | ||
| } | ||
| if err := k.UpdateClass(ctx, class); err != nil { | ||
| return nil, err | ||
| } | ||
| return &nft.MsgUpdateClassResponse{}, nil | ||
| } | ||
|
|
||
| // MintNFT implements MintNFT method of the types.MsgServer. | ||
| func (k Keeper) MintNFT(ctx context.Context, msg *nft.MsgMintNFT) (*nft.MsgMintNFTResponse, error) { | ||
| nft := nft.NFT{ | ||
| ClassId: msg.ClassId, | ||
| Id: msg.Id, | ||
| Uri: msg.Uri, | ||
| UriHash: msg.UriHash, | ||
| Data: msg.Data, | ||
| } | ||
| if err := k.Mint(ctx, nft, msg.Receiver); err != nil { | ||
| return nil, err | ||
| } | ||
| return &nft.MsgMintNFTResponse{}, nil | ||
| } | ||
|
|
||
| // BurnNFT implements BurnNFT method of the types.MsgServer. | ||
| func (k Keeper) BurnNFT(ctx context.Context, msg *nft.MsgBurnNFT) (*nft.MsgBurnNFTResponse, error) { | ||
| if err := k.Burn(ctx, msg.ClassId, msg.Id); err != nil { | ||
| return nil, err | ||
| } | ||
| return &nft.MsgBurnNFTResponse{}, nil | ||
| } | ||
|
|
||
| // UpdateNFT implements UpdateNFT method of the types.MsgServer. | ||
| func (k Keeper) UpdateNFT(ctx context.Context, msg *nft.MsgUpdateNFT) (*nft.MsgUpdateNFTResponse, error) { | ||
| nft := nft.NFT{ | ||
| ClassId: msg.ClassId, | ||
| Id: msg.Id, | ||
| Uri: msg.Uri, | ||
| UriHash: msg.UriHash, | ||
| Data: msg.Data, | ||
| } | ||
| if err := k.Update(ctx, nft); err != nil { | ||
| return nil, err | ||
| } | ||
| return &nft.MsgUpdateNFTResponse{}, nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove code duplication between keeper.go and msg_server.go.
The functions in this file duplicate the logic from keeper.go. To improve maintainability and reduce the risk of inconsistencies, consider one of these approaches:
- Move the validation and authorization logic to
keeper.goand have the message server functions simply delegate to the keeper functions. - Move all the logic to the message server and have the keeper functions delegate to the message server.
Here's an example of approach #1 for the NewClass function:
func (k Keeper) NewClass(ctx context.Context, msg *nft.MsgNewClass) (*nft.MsgNewClassResponse, error) {
- class := nft.Class{
- Id: msg.ClassId,
- Name: msg.Name,
- Symbol: msg.Symbol,
- Description: msg.Description,
- Uri: msg.Uri,
- UriHash: msg.UriHash,
- Data: msg.Data,
- }
- if err := k.SaveClass(ctx, class); err != nil {
- return nil, err
- }
- return &nft.MsgNewClassResponse{}, nil
+ return k.MsgNewClass(ctx, msg)
}Committable suggestion skipped: line range outside the PR's diff.
| Uri: testClassURI, | ||
| UriHash: testClassURIHash, | ||
| } | ||
| err := s.nftKeeper.SaveClass(s.ctx, class) | ||
| s.Require().NoError(err) | ||
|
|
||
| msg := &nft.MsgMintNFT{ | ||
| ClassId: testClassID, | ||
| Id: testID, | ||
| Uri: testURI, | ||
| UriHash: testURIHash, | ||
| Receiver: s.encodedAddrs[0], | ||
| } | ||
|
|
||
| _, err = s.nftKeeper.MsgMintNFT(s.ctx, msg) | ||
| s.Require().NoError(err) | ||
|
|
||
| actual, has := s.nftKeeper.GetNFT(s.ctx, testClassID, testID) | ||
| s.Require().True(has) | ||
| s.Require().EqualValues(nft.NFT{ | ||
| ClassId: testClassID, | ||
| Id: testID, | ||
| Uri: testURI, | ||
| UriHash: testURIHash, | ||
| }, actual) | ||
| } | ||
|
|
||
| func (s *TestSuite) TestMsgBurnNFT() { | ||
| class := nft.Class{ | ||
| Id: testClassID, | ||
| Name: testClassName, | ||
| Symbol: testClassSymbol, | ||
| Description: testClassDescription, | ||
| Uri: testClassURI, | ||
| UriHash: testClassURIHash, | ||
| } | ||
| err := s.nftKeeper.SaveClass(s.ctx, class) | ||
| s.Require().NoError(err) | ||
|
|
||
| nft := nft.NFT{ | ||
| ClassId: testClassID, | ||
| Id: testID, | ||
| Uri: testURI, | ||
| UriHash: testURIHash, | ||
| } | ||
| err = s.nftKeeper.Mint(s.ctx, nft, s.addrs[0]) | ||
| s.Require().NoError(err) | ||
|
|
||
| msg := &nft.MsgBurnNFT{ | ||
| ClassId: testClassID, | ||
| Id: testID, | ||
| } | ||
|
|
||
| _, err = s.nftKeeper.MsgBurnNFT(s.ctx, msg) | ||
| s.Require().NoError(err) | ||
|
|
||
| _, has := s.nftKeeper.GetNFT(s.ctx, testClassID, testID) | ||
| s.Require().False(has) | ||
| } | ||
|
|
||
| func (s *TestSuite) TestMsgUpdateNFT() { | ||
| class := nft.Class{ | ||
| Id: testClassID, | ||
| Name: testClassName, | ||
| Symbol: testClassSymbol, | ||
| Description: testClassDescription, | ||
| Uri: testClassURI, | ||
| UriHash: testClassURIHash, | ||
| } | ||
| err := s.nftKeeper.SaveClass(s.ctx, class) | ||
| s.Require().NoError(err) | ||
|
|
||
| nft := nft.NFT{ | ||
| ClassId: testClassID, | ||
| Id: testID, | ||
| Uri: testURI, | ||
| UriHash: testURIHash, | ||
| } | ||
| err = s.nftKeeper.Mint(s.ctx, nft, s.addrs[0]) | ||
| s.Require().NoError(err) | ||
|
|
||
| msg := &nft.MsgUpdateNFT{ | ||
| ClassId: testClassID, | ||
| Id: testID, | ||
| Uri: "Updated URI", | ||
| UriHash: "Updated URI Hash", | ||
| } | ||
|
|
||
| _, err = s.nftKeeper.MsgUpdateNFT(s.ctx, msg) | ||
| s.Require().NoError(err) | ||
|
|
||
| actual, has := s.nftKeeper.GetNFT(s.ctx, testClassID, testID) | ||
| s.Require().True(has) | ||
| s.Require().EqualValues(nft.NFT{ | ||
| ClassId: testClassID, | ||
| Id: testID, | ||
| Uri: "Updated URI", | ||
| UriHash: "Updated URI Hash", | ||
| }, actual) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove test code duplication between keeper_test.go and msg_server_test.go.
The test functions in this file duplicate the tests from keeper_test.go. To improve maintainability:
- Move common test setup and helper functions to a shared test utility package.
- Focus msg_server tests on message handling specifics while keeper tests focus on core functionality.
Example of a shared test utility:
// testutil/nft_test_utils.go
package testutil
func SetupNFTClass(s TestingSuite, class nft.Class) error {
return s.nftKeeper.SaveClass(s.ctx, class)
}
func SetupNFT(s TestingSuite, nft nft.NFT, owner sdk.AccAddress) error {
return s.nftKeeper.Mint(s.ctx, nft, owner)
}🧰 Tools
🪛 golangci-lint (1.62.2)
135-135: method TestSuite.TestMsgNewClass already declared at keeper/keeper_test.go:403:21
(typecheck)
160-160: method TestSuite.TestMsgUpdateClass already declared at keeper/keeper_test.go:428:21
(typecheck)
196-196: method TestSuite.TestMsgMintNFT already declared at keeper/keeper_test.go:464:21
(typecheck)
229-229: method TestSuite.TestMsgBurnNFT already declared at keeper/keeper_test.go:497:21
(typecheck)
262-262: method TestSuite.TestMsgUpdateNFT already declared at keeper/keeper_test.go:530:21
(typecheck)
| Uri: testClassURI, | ||
| UriHash: testClassURIHash, | ||
| } | ||
| err := s.nftKeeper.SaveClass(s.ctx, class) | ||
| s.Require().NoError(err) | ||
|
|
||
| msg := &nft.MsgMintNFT{ | ||
| ClassId: testClassID, | ||
| Id: testID, | ||
| Uri: testURI, | ||
| UriHash: testURIHash, | ||
| Receiver: s.encodedAddrs[0], | ||
| } | ||
|
|
||
| _, err = s.nftKeeper.MsgMintNFT(s.ctx, msg) | ||
| s.Require().NoError(err) | ||
|
|
||
| actual, has := s.nftKeeper.GetNFT(s.ctx, testClassID, testID) | ||
| s.Require().True(has) | ||
| s.Require().EqualValues(nft.NFT{ | ||
| ClassId: testClassID, | ||
| Id: testID, | ||
| Uri: testURI, | ||
| UriHash: testURIHash, | ||
| }, actual) | ||
| } | ||
|
|
||
| func (s *TestSuite) TestMsgBurnNFT() { | ||
| class := nft.Class{ | ||
| Id: testClassID, | ||
| Name: testClassName, | ||
| Symbol: testClassSymbol, | ||
| Description: testClassDescription, | ||
| Uri: testClassURI, | ||
| UriHash: testClassURIHash, | ||
| } | ||
| err := s.nftKeeper.SaveClass(s.ctx, class) | ||
| s.Require().NoError(err) | ||
|
|
||
| nft := nft.NFT{ | ||
| ClassId: testClassID, | ||
| Id: testID, | ||
| Uri: testURI, | ||
| UriHash: testURIHash, | ||
| } | ||
| err = s.nftKeeper.Mint(s.ctx, nft, s.addrs[0]) | ||
| s.Require().NoError(err) | ||
|
|
||
| msg := &nft.MsgBurnNFT{ | ||
| ClassId: testClassID, | ||
| Id: testID, | ||
| } | ||
|
|
||
| _, err = s.nftKeeper.MsgBurnNFT(s.ctx, msg) | ||
| s.Require().NoError(err) | ||
|
|
||
| _, has := s.nftKeeper.GetNFT(s.ctx, testClassID, testID) | ||
| s.Require().False(has) | ||
| } | ||
|
|
||
| func (s *TestSuite) TestMsgUpdateNFT() { | ||
| class := nft.Class{ | ||
| Id: testClassID, | ||
| Name: testClassName, | ||
| Symbol: testClassSymbol, | ||
| Description: testClassDescription, | ||
| Uri: testClassURI, | ||
| UriHash: testClassURIHash, | ||
| } | ||
| err := s.nftKeeper.SaveClass(s.ctx, class) | ||
| s.Require().NoError(err) | ||
|
|
||
| nft := nft.NFT{ | ||
| ClassId: testClassID, | ||
| Id: testID, | ||
| Uri: testURI, | ||
| UriHash: testURIHash, | ||
| } | ||
| err = s.nftKeeper.Mint(s.ctx, nft, s.addrs[0]) | ||
| s.Require().NoError(err) | ||
|
|
||
| msg := &nft.MsgUpdateNFT{ | ||
| ClassId: testClassID, | ||
| Id: testID, | ||
| Uri: "Updated URI", | ||
| UriHash: "Updated URI Hash", | ||
| } | ||
|
|
||
| _, err = s.nftKeeper.MsgUpdateNFT(s.ctx, msg) | ||
| s.Require().NoError(err) | ||
|
|
||
| actual, has := s.nftKeeper.GetNFT(s.ctx, testClassID, testID) | ||
| s.Require().True(has) | ||
| s.Require().EqualValues(nft.NFT{ | ||
| ClassId: testClassID, | ||
| Id: testID, | ||
| Uri: "Updated URI", | ||
| UriHash: "Updated URI Hash", | ||
| }, actual) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve test coverage for error cases and validation.
The test functions need the following improvements:
- Add test cases for input validation errors (empty IDs, invalid addresses).
- Add test cases for authorization errors (unauthorized sender).
- Add test cases for event emission.
Example test cases for TestMsgNewClass:
func (s *TestSuite) TestMsgNewClass() {
testCases := []struct {
name string
msg *nft.MsgNewClass
expErr bool
errMsg string
}{
{
name: "empty class id",
msg: &nft.MsgNewClass{
ClassId: "",
Name: testClassName,
},
expErr: true,
errMsg: "empty class id",
},
{
name: "unauthorized sender",
msg: &nft.MsgNewClass{
ClassId: testClassID,
Name: testClassName,
Sender: "unauthorized",
},
expErr: true,
errMsg: "unauthorized",
},
// Add more test cases
}
for _, tc := range testCases {
s.Run(tc.name, func() {
_, err := s.nftKeeper.MsgNewClass(s.ctx, tc.msg)
if tc.expErr {
s.Require().Error(err)
s.Require().Contains(err.Error(), tc.errMsg)
} else {
s.Require().NoError(err)
// Verify event emission
events := s.ctx.EventManager().Events()
s.Require().Len(events, 1)
s.Require().Equal("new_class", events[0].Type)
}
})
}
}🧰 Tools
🪛 golangci-lint (1.62.2)
404-404: undefined: nft.MsgNewClass
(typecheck)
| message MsgBurnNFT { | ||
| option (cosmos.msg.v1.signer) = "burner"; | ||
|
|
||
| // class_id defines the unique identifier of the nft classification, similar to the contract address of ERC721 | ||
| string class_id = 1; | ||
|
|
||
| // id defines the unique identification of nft | ||
| string id = 2; | ||
|
|
||
| // burner is the address of the burner of the NFT | ||
| string burner = 3 [(cosmos_proto.scalar) = "cosmos.AddressString"]; | ||
| } |
There was a problem hiding this comment.
Enhance security for NFT modification operations.
Both MsgBurnNFT and MsgUpdateNFT should include ownership verification:
- Add previous owner signature requirement
- Include ownership transfer history
This prevents unauthorized modifications and maintains NFT provenance.
Also applies to: 162-183
|
|
||
| // MsgNewClass represents a message to create a new NFT class. | ||
| message MsgNewClass { | ||
| option (cosmos.msg.v1.signer) = "creator"; | ||
|
|
||
| // class_id defines the unique identifier of the nft classification, similar to the contract address of ERC721 | ||
| string class_id = 1; | ||
|
|
||
| // name defines the human-readable name of the NFT classification. Optional | ||
| string name = 2; | ||
|
|
||
| // symbol is an abbreviated name for nft classification. Optional | ||
| string symbol = 3; | ||
|
|
||
| // description is a brief description of nft classification. Optional | ||
| string description = 4; | ||
|
|
||
| // uri for the class metadata stored off chain. It can define schema for Class and NFT `Data` attributes. Optional | ||
| string uri = 5; | ||
|
|
||
| // uri_hash is a hash of the document pointed by uri. Optional | ||
| string uri_hash = 6; | ||
|
|
||
| // data is the app specific metadata of the NFT class. Optional | ||
| google.protobuf.Any data = 7; | ||
|
|
||
| // creator is the address of the creator of the NFT class | ||
| string creator = 8 [(cosmos_proto.scalar) = "cosmos.AddressString"]; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add field validation constraints for MsgNewClass.
Consider adding protobuf field constraints to prevent abuse:
- Length limits for class_id, name, symbol, description
- URI format validation
- Size limits for metadata
Example:
// class_id defines the unique identifier of the nft classification
- string class_id = 1;
+ string class_id = 1 [(cosmos_proto.scalar) = "ClassID", (validate.rules).string = {
+ min_len: 3,
+ max_len: 64,
+ pattern: "^[a-zA-Z][a-zA-Z0-9_/-]*$"
+ }];Committable suggestion skipped: line range outside the PR's diff.
|
the |
Related to #20791
Add full NFT module functionality to Cosmos SDK.
MsgNewClass,MsgUpdateClass,MsgMintNFT,MsgBurnNFT, andMsgUpdateNFTmethods tox/nft/keeper/keeper.go.MsgNewClass,MsgUpdateClass,MsgMintNFT,MsgBurnNFT, andMsgUpdateNFTmessages tox/nft/proto/cosmos/nft/v1beta1/tx.proto.x/nft/keeper/msg_server.goto implement the new methods for creating, minting, burning, and updating NFTs.x/nft/proto/cosmos/nft/v1beta1/nft.prototo include the new methods for creating, minting, burning, and updating NFTs.x/nft/keeper/keeper_test.goandx/nft/keeper/msg_server_test.go.Summary by CodeRabbit
New Features
Tests
The update significantly expands the NFT ecosystem with robust management tools and comprehensive testing.