refactor(x/nft): remove class&nft id validation (#13836) (#13870)

(cherry picked from commit 5f4350d) Co-authored-by: Dreamer <745124335@qq.com>
cosmos · Nov 15, 2022 · 1725103 · 1725103
1 parent 3f2266a
commit 1725103
Show file tree

Hide file tree

Showing 10 changed files with 49 additions and 206 deletions.
diff --git a/docs/architecture/adr-043-nft-module.md b/docs/architecture/adr-043-nft-module.md
@@ -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
 
@@ -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_
@@ -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)

diff --git a/tests/e2e/nft/grpc.go b/tests/e2e/nft/grpc.go
@@ -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 {
@@ -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 {
@@ -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 {
@@ -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,
 		},
 		{
@@ -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",
-		},
-		{
-			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 {

diff --git a/tests/e2e/nft/query.go b/tests/e2e/nft/query.go
@@ -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 {
@@ -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 {
@@ -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 {
@@ -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,
 		},
 		{

diff --git a/x/nft/client/cli/query.go b/x/nft/client/cli/query.go
@@ -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")
 			}

diff --git a/x/nft/errors.go b/x/nft/errors.go
@@ -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")
 )
diff --git a/x/nft/genesis.go b/x/nft/genesis.go
@@ -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

diff --git a/x/nft/keeper/grpc_query.go b/x/nft/keeper/grpc_query.go
@@ -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)
@@ -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)
@@ -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)
@@ -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)
@@ -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)
@@ -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)