From 5f4350d41859e5cbefd7514abe1b6461dc637c24 Mon Sep 17 00:00:00 2001 From: Dreamer <745124335@qq.com> Date: Tue, 15 Nov 2022 20:47:38 +0800 Subject: [PATCH] refactor(x/nft): remove class&nft id validation (#13836) --- docs/architecture/adr-043-nft-module.md | 7 ++- tests/e2e/nft/grpc.go | 80 +------------------------ tests/e2e/nft/query.go | 44 +------------- x/nft/client/cli/query.go | 6 -- x/nft/errors.go | 15 +++-- x/nft/genesis.go | 8 +-- x/nft/keeper/grpc_query.go | 33 +++++----- x/nft/keeper/grpc_query_test.go | 14 ++--- x/nft/msgs.go | 13 ++-- x/nft/validation.go | 35 ----------- 10 files changed, 49 insertions(+), 206 deletions(-) delete mode 100644 x/nft/validation.go diff --git a/docs/architecture/adr-043-nft-module.md b/docs/architecture/adr-043-nft-module.md index d3fb11dde6e9..87b4dbb5ee20 100644 --- 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 index 6b6d84943fc4..3175069e71d1 100644 --- 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 index 56e0265fbfe9..6b041632cce8 100644 --- 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 index 559a36f14e6e..52416d4a32a9 100644 --- a/x/nft/client/cli/query.go +++ b/x/nft/client/cli/query.go @@ -166,12 +166,6 @@ $ %s query %s nfts --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 index c7d6836596f7..771146f39e69 100644 --- 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 index 43be6ffbddd1..75124ea354e5 100644 --- 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 index e22fdf0b4a22..e409a78c0923 100644 --- 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) diff --git a/x/nft/keeper/grpc_query_test.go b/x/nft/keeper/grpc_query_test.go index 8afcf1c26e30..ae8e68d7b74b 100644 --- a/x/nft/keeper/grpc_query_test.go +++ b/x/nft/keeper/grpc_query_test.go @@ -29,7 +29,7 @@ func (s *TestSuite) TestBalance() { func(index int, require *require.Assertions) { req = &nft.QueryBalanceRequest{} }, - "invalid class id", + nft.ErrEmptyClassID.Error(), 0, func(index int, require *require.Assertions, res *nft.QueryBalanceResponse, expBalance uint64) {}, }, @@ -95,7 +95,7 @@ func (s *TestSuite) TestOwner() { Id: testID, } }, - "invalid class id", + nft.ErrEmptyClassID.Error(), func(index int, require *require.Assertions, res *nft.QueryOwnerResponse) {}, }, { @@ -105,7 +105,7 @@ func (s *TestSuite) TestOwner() { ClassId: testClassID, } }, - "invalid nft id", + nft.ErrEmptyNFTID.Error(), func(index int, require *require.Assertions, res *nft.QueryOwnerResponse) {}, }, { @@ -180,7 +180,7 @@ func (s *TestSuite) TestSupply() { func(index int, require *require.Assertions) { req = &nft.QuerySupplyRequest{} }, - "invalid class id", + nft.ErrEmptyClassID.Error(), 0, func(index int, require *require.Assertions, res *nft.QuerySupplyResponse, supply uint64) {}, }, @@ -393,7 +393,7 @@ func (s *TestSuite) TestNFT() { func(index int, require *require.Assertions) { req = &nft.QueryNFTRequest{} }, - "invalid class id", + nft.ErrEmptyClassID.Error(), func(index int, require *require.Assertions, res *nft.QueryNFTResponse) {}, }, { @@ -403,7 +403,7 @@ func (s *TestSuite) TestNFT() { ClassId: testClassID, } }, - "invalid nft id", + nft.ErrEmptyNFTID.Error(), func(index int, require *require.Assertions, res *nft.QueryNFTResponse) {}, }, { @@ -480,7 +480,7 @@ func (s *TestSuite) TestClass() { func(index int, require *require.Assertions) { req = &nft.QueryClassRequest{} }, - "invalid class id", + nft.ErrEmptyClassID.Error(), func(index int, require *require.Assertions, res *nft.QueryClassResponse) {}, }, { diff --git a/x/nft/msgs.go b/x/nft/msgs.go index ec648e5f498c..57926287bb1f 100644 --- a/x/nft/msgs.go +++ b/x/nft/msgs.go @@ -1,6 +1,7 @@ package nft import ( + "cosmossdk.io/errors" sdk "github.com/cosmos/cosmos-sdk/types" sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" ) @@ -14,22 +15,22 @@ var _ sdk.Msg = &MsgSend{} // ValidateBasic implements the Msg.ValidateBasic method. func (m MsgSend) ValidateBasic() error { - if err := ValidateClassID(m.ClassId); err != nil { - return sdkerrors.Wrapf(ErrInvalidID, "Invalid class id (%s)", m.ClassId) + if len(m.ClassId) == 0 { + return ErrEmptyClassID } - if err := ValidateNFTID(m.Id); err != nil { - return sdkerrors.Wrapf(ErrInvalidID, "Invalid nft id (%s)", m.Id) + if len(m.Id) == 0 { + return ErrEmptyNFTID } _, err := sdk.AccAddressFromBech32(m.Sender) if err != nil { - return sdkerrors.Wrapf(sdkerrors.ErrInvalidAddress, "Invalid sender address (%s)", m.Sender) + return errors.Wrapf(sdkerrors.ErrInvalidAddress, "Invalid sender address (%s)", m.Sender) } _, err = sdk.AccAddressFromBech32(m.Receiver) if err != nil { - return sdkerrors.Wrapf(sdkerrors.ErrInvalidAddress, "Invalid receiver address (%s)", m.Receiver) + return errors.Wrapf(sdkerrors.ErrInvalidAddress, "Invalid receiver address (%s)", m.Receiver) } return nil } diff --git a/x/nft/validation.go b/x/nft/validation.go deleted file mode 100644 index 65d684716608..000000000000 --- a/x/nft/validation.go +++ /dev/null @@ -1,35 +0,0 @@ -package nft - -import ( - "fmt" - "regexp" - - sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" -) - -var ( - // reClassIDString can be 3 ~ 100 characters long and support letters, followed by either - // a letter, a number or a slash ('/') or a colon (':') or ('-'). - reClassIDString = `[a-zA-Z][a-zA-Z0-9/:-]{2,100}` - reClassID = regexp.MustCompile(fmt.Sprintf(`^%s$`, reClassIDString)) - - // reNFTIDString can be 3 ~ 100 characters long and support letters, followed by either - // a letter, a number or a slash ('/') or a colon (':') or ('-'). - reNFTID = reClassID -) - -// ValidateClassID returns whether the class id is valid -func ValidateClassID(id string) error { - if !reClassID.MatchString(id) { - return sdkerrors.Wrapf(ErrInvalidClassID, "invalid class id: %s", id) - } - return nil -} - -// ValidateNFTID returns whether the nft id is valid -func ValidateNFTID(id string) error { - if !reNFTID.MatchString(id) { - return sdkerrors.Wrapf(ErrInvalidID, "invalid nft id: %s", id) - } - return nil -}