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.

Sign up for GitHub

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

Jump to bottom

refactor: remove class&nft id validation #13836

Merged
merged 7 commits into from
Nov 15, 2022
Merged

refactor: remove class&nft id validation #13836

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
c5ccdfe
remove class&nft id validation
Nov 11, 2022
1a5b780
clean code
Nov 11, 2022
42a360f
fix e2e test&update adr-043 docs
Nov 11, 2022
191d3ea
refactor error code
Nov 14, 2022
e014265
remove invalid test cases
Nov 14, 2022
7108e16
update error code
Nov 15, 2022
7150dab
Merge branch 'main' into remove-nft-validation
julienrbrt Nov 15, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions docs/architecture/adr-043-nft-module.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
* 2021-05-01: Initial Draft
* 2021-07-02: Review updates
* 2022-06-15: Add batch operation
* 2022-11-11: Remove strict validation of classID and tokenID

## Status

Expand Down Expand Up @@ -75,7 +76,7 @@ message Class {
}
```

* `id` is an alphanumeric identifier of the NFT class; it is used as the primary index for storing the class; _required_
* `id` is used as the primary index for storing the class; _required_
* `name` is a descriptive name of the NFT class; _optional_
* `symbol` is the symbol usually shown on exchanges for the NFT class; _optional_
* `description` is a detailed description of the NFT class; _optional_
Expand All @@ -97,8 +98,8 @@ message NFT {
}
```

* `class_id` is the identifier of the NFT class where the NFT belongs; _required_,`[a-zA-Z][a-zA-Z0-9/:-]{2,100}`
* `id` is an alphanumeric identifier of the NFT, unique within the scope of its class. It is specified by the creator of the NFT and may be expanded to use DID in the future. `class_id` combined with `id` uniquely identifies an NFT and is used as the primary index for storing the NFT; _required_,`[a-zA-Z][a-zA-Z0-9/:-]{2,100}`
* `class_id` is the identifier of the NFT class where the NFT belongs; _required_
* `id` is an identifier of the NFT, unique within the scope of its class. It is specified by the creator of the NFT and may be expanded to use DID in the future. `class_id` combined with `id` uniquely identifies an NFT and is used as the primary index for storing the NFT; _required_

```text
{class_id}/{id} --> NFT (bytes)
Expand Down
80 changes: 3 additions & 77 deletions tests/e2e/nft/grpc.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,19 +19,6 @@ func (s *IntegrationTestSuite) TestQueryBalanceGRPC() {
errMsg string
expectValue uint64
}{
{
name: "fail not exist class id",
args: struct {
ClassID string
Owner string
}{
ClassID: "invalid_class_id",
Owner: s.owner.String(),
},
expectErr: true,
errMsg: "invalid class id",
expectValue: 0,
},
{
name: "fail not exist owner",
args: struct {
Expand Down Expand Up @@ -87,19 +74,6 @@ func (s *IntegrationTestSuite) TestQueryOwnerGRPC() {
errMsg string
expectResult string
}{
{
name: "class id is invalid",
args: struct {
ClassID string
ID string
}{
ClassID: "invalid_class_id",
ID: ExpNFT.Id,
},
expectErr: true,
errMsg: "invalid class id",
expectResult: "",
},
{
name: "class id does not exist",
args: struct {
Expand All @@ -112,18 +86,6 @@ func (s *IntegrationTestSuite) TestQueryOwnerGRPC() {
expectErr: false,
expectResult: "",
},
{
name: "nft id is invalid",
args: struct {
ClassID string
ID string
}{
ClassID: ExpNFT.ClassId,
ID: "invalid_nft_id",
},
expectErr: true,
expectResult: "",
},
{
name: "nft id does not exist",
args: struct {
Expand Down Expand Up @@ -180,14 +142,14 @@ func (s *IntegrationTestSuite) TestQuerySupplyGRPC() {
expectResult uint64
}{
{
name: "class id is invalid",
name: "class id is empty",
args: struct {
ClassID string
}{
ClassID: "invalid_class_id",
ClassID: "",
},
expectErr: true,
errMsg: "invalid class id",
errMsg: nft.ErrEmptyClassID.Error(),
expectResult: 0,
},
{
Expand Down Expand Up @@ -337,42 +299,6 @@ func (s *IntegrationTestSuite) TestQueryNFTGRPC() {
expectErr bool
errorMsg string
}{
{
name: "class id is invalid",
args: struct {
ClassID string
ID string
}{
ClassID: "invalid_class_id",
ID: ExpNFT.Id,
},
expectErr: true,
errorMsg: "invalid class id",
},
{
name: "class id does not exist",
args: struct {
ClassID string
ID string
}{
ClassID: "class",
ID: ExpNFT.Id,
},
expectErr: true,
errorMsg: "not found nft",
},
Comment on lines -352 to -363
Copy link
Member

Choose a reason for hiding this comment

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

nit: I think this one can be put back

Copy link
Member

Choose a reason for hiding this comment

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

Just tested locally and it can be put back, but we can do a follow-up PR :)

Copy link
Member

Choose a reason for hiding this comment

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

Merging then, I'll update the docs in a quick follow-up and re-add this test.

{
name: "nft id is invalid",
args: struct {
ClassID string
ID string
}{
ClassID: ExpNFT.ClassId,
ID: "invalid_nft_id",
},
expectErr: true,
errorMsg: "invalid nft id",
},
{
name: "nft id does not exist",
args: struct {
Expand Down
44 changes: 3 additions & 41 deletions tests/e2e/nft/query.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -221,19 +221,6 @@ func (s *IntegrationTestSuite) TestQueryOwner() {
errorMsg string
expectResult string
}{
{
name: "class id is invalid",
args: struct {
ClassID string
ID string
}{
ClassID: "invalid_class_id",
ID: testID,
},
expectErr: true,
errorMsg: "invalid class id",
expectResult: "",
},
{
name: "class id does not exist",
args: struct {
Expand All @@ -246,18 +233,6 @@ func (s *IntegrationTestSuite) TestQueryOwner() {
expectErr: false,
expectResult: "",
},
{
name: "nft id is invalid",
args: struct {
ClassID string
ID string
}{
ClassID: testClassID,
ID: "invalid_nft_id",
},
expectErr: true,
expectResult: "",
},
{
name: "nft id does not exist",
args: struct {
Expand Down Expand Up @@ -311,19 +286,6 @@ func (s *IntegrationTestSuite) TestQueryBalance() {
errorMsg string
expectResult uint64
}{
{
name: "class id is invalid",
args: struct {
ClassID string
Owner string
}{
ClassID: "invalid_class_id",
Owner: val.Address.String(),
},
expectErr: true,
errorMsg: "invalid class id",
expectResult: 0,
},
{
name: "class id does not exist",
args: struct {
Expand Down Expand Up @@ -389,14 +351,14 @@ func (s *IntegrationTestSuite) TestQuerySupply() {
expectResult uint64
}{
{
name: "class id is invalid",
name: "class id is empty",
args: struct {
ClassID string
}{
ClassID: "invalid_class_id",
ClassID: "",
},
expectErr: true,
errorMsg: "invalid class id",
errorMsg: nft.ErrEmptyClassID.Error(),
expectResult: 0,
},
{
Expand Down
6 changes: 0 additions & 6 deletions x/nft/client/cli/query.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,12 +166,6 @@ $ %s query %s nfts <class-id> --owner=<owner>
return err
}

if len(classID) > 0 {
if err := nft.ValidateClassID(classID); err != nil {
return err
}
}

if len(owner) == 0 && len(classID) == 0 {
return errors.ErrInvalidRequest.Wrap("must provide at least one of classID or owner")
}
Expand Down
15 changes: 7 additions & 8 deletions x/nft/errors.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,16 +1,15 @@
package nft

import (
sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
"cosmossdk.io/errors"
)

// x/nft module sentinel errors
var (
ErrInvalidNFT = sdkerrors.Register(ModuleName, 2, "invalid nft")
ErrClassExists = sdkerrors.Register(ModuleName, 3, "nft class already exist")
ErrClassNotExists = sdkerrors.Register(ModuleName, 4, "nft class does not exist")
ErrNFTExists = sdkerrors.Register(ModuleName, 5, "nft already exist")
ErrNFTNotExists = sdkerrors.Register(ModuleName, 6, "nft does not exist")
ErrInvalidID = sdkerrors.Register(ModuleName, 7, "invalid id")
ErrInvalidClassID = sdkerrors.Register(ModuleName, 8, "invalid class id")
ErrClassExists = errors.Register(ModuleName, 3, "nft class already exist")
ErrClassNotExists = errors.Register(ModuleName, 4, "nft class does not exist")
ErrNFTExists = errors.Register(ModuleName, 5, "nft already exist")
ErrNFTNotExists = errors.Register(ModuleName, 6, "nft does not exist")
ErrEmptyClassID = errors.Register(ModuleName, 7, "empty class id")
ErrEmptyNFTID = errors.Register(ModuleName, 8, "empty nft id")
)
8 changes: 4 additions & 4 deletions x/nft/genesis.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,14 +7,14 @@ import (
// ValidateGenesis check the given genesis state has no integrity issues
func ValidateGenesis(data GenesisState) error {
for _, class := range data.Classes {
if err := ValidateClassID(class.Id); err != nil {
return err
if len(class.Id) == 0 {
return ErrEmptyClassID
}
}
for _, entry := range data.Entries {
for _, nft := range entry.Nfts {
if err := ValidateNFTID(nft.Id); err != nil {
return err
if len(nft.Id) == 0 {
return ErrEmptyNFTID
}
if _, err := sdk.AccAddressFromBech32(entry.Owner); err != nil {
return err
Expand Down
33 changes: 14 additions & 19 deletions x/nft/keeper/grpc_query.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,8 @@ func (k Keeper) Balance(goCtx context.Context, r *nft.QueryBalanceRequest) (*nft
return nil, sdkerrors.ErrInvalidRequest.Wrap("empty request")
}

if err := nft.ValidateClassID(r.ClassId); err != nil {
return nil, err
if len(r.ClassId) == 0 {
return nil, nft.ErrEmptyClassID
}

owner, err := sdk.AccAddressFromBech32(r.Owner)
Expand All @@ -38,12 +38,12 @@ func (k Keeper) Owner(goCtx context.Context, r *nft.QueryOwnerRequest) (*nft.Que
return nil, sdkerrors.ErrInvalidRequest.Wrap("empty request")
}

if err := nft.ValidateClassID(r.ClassId); err != nil {
return nil, err
if len(r.ClassId) == 0 {
return nil, nft.ErrEmptyClassID
}

if err := nft.ValidateNFTID(r.Id); err != nil {
return nil, err
if len(r.Id) == 0 {
return nil, nft.ErrEmptyNFTID
}

ctx := sdk.UnwrapSDKContext(goCtx)
Expand All @@ -57,8 +57,8 @@ func (k Keeper) Supply(goCtx context.Context, r *nft.QuerySupplyRequest) (*nft.Q
return nil, sdkerrors.ErrInvalidRequest.Wrap("empty request")
}

if err := nft.ValidateClassID(r.ClassId); err != nil {
return nil, err
if len(r.ClassId) == 0 {
return nil, nft.ErrEmptyClassID
}
ctx := sdk.UnwrapSDKContext(goCtx)
supply := k.GetTotalSupply(ctx, r.ClassId)
Expand All @@ -73,11 +73,6 @@ func (k Keeper) NFTs(goCtx context.Context, r *nft.QueryNFTsRequest) (*nft.Query

var err error
var owner sdk.AccAddress
if len(r.ClassId) > 0 {
if err := nft.ValidateClassID(r.ClassId); err != nil {
return nil, err
}
}

if len(r.Owner) > 0 {
owner, err = sdk.AccAddressFromBech32(r.Owner)
Expand Down Expand Up @@ -138,11 +133,11 @@ func (k Keeper) NFT(goCtx context.Context, r *nft.QueryNFTRequest) (*nft.QueryNF
return nil, sdkerrors.ErrInvalidRequest.Wrap("empty request")
}

if err := nft.ValidateClassID(r.ClassId); err != nil {
return nil, err
if len(r.ClassId) == 0 {
return nil, nft.ErrEmptyClassID
}
if err := nft.ValidateNFTID(r.Id); err != nil {
return nil, err
if len(r.Id) == 0 {
return nil, nft.ErrEmptyNFTID
}

ctx := sdk.UnwrapSDKContext(goCtx)
Expand All @@ -159,8 +154,8 @@ func (k Keeper) Class(goCtx context.Context, r *nft.QueryClassRequest) (*nft.Que
return nil, sdkerrors.ErrInvalidRequest.Wrap("empty request")
}

if err := nft.ValidateClassID(r.ClassId); err != nil {
return nil, err
if len(r.ClassId) == 0 {
return nil, nft.ErrEmptyClassID
}

ctx := sdk.UnwrapSDKContext(goCtx)
Expand Down
Loading