chore: improve the validations of messages

bnb-chain · Sep 15, 2023 · 8de6dd9 · 8de6dd9
1 parent 8d55b28
commit 8de6dd9
Show file tree

Hide file tree

Showing 6 changed files with 39 additions and 19 deletions.
diff --git a/x/permission/types/types.go b/x/permission/types/types.go
@@ -26,12 +26,13 @@ var (
 		ACTION_UPDATE_BUCKET_INFO: true,
 		ACTION_DELETE_BUCKET:      true,
 
-		ACTION_CREATE_OBJECT:  true,
-		ACTION_DELETE_OBJECT:  true,
-		ACTION_GET_OBJECT:     true,
-		ACTION_COPY_OBJECT:    true,
-		ACTION_EXECUTE_OBJECT: true,
-		ACTION_LIST_OBJECT:    true,
+		ACTION_CREATE_OBJECT:      true,
+		ACTION_DELETE_OBJECT:      true,
+		ACTION_GET_OBJECT:         true,
+		ACTION_COPY_OBJECT:        true,
+		ACTION_EXECUTE_OBJECT:     true,
+		ACTION_LIST_OBJECT:        true,
+		ACTION_UPDATE_OBJECT_INFO: true,
 
 		ACTION_TYPE_ALL: true,
 	}

diff --git a/x/sp/types/message.go b/x/sp/types/message.go
@@ -105,20 +105,20 @@ func (msg *MsgCreateStorageProvider) ValidateBasic() error {
 	if _, err := sdk.AccAddressFromHexUnsafe(msg.GcAddress); err != nil {
 		return errors.Wrapf(sdkerrors.ErrInvalidAddress, "invalid gc address (%s)", err)
 	}
+	//MaintenanceAddress is validated in msg server
 	if !msg.Deposit.IsValid() || !msg.Deposit.Amount.IsPositive() {
-		return errors.Wrap(sdkerrors.ErrInvalidRequest, "invalid deposit amount")
+		return errors.Wrap(sdkerrors.ErrInvalidCoins, "invalid deposit amount")
 	}
 	if msg.Description == (Description{}) {
 		return errors.Wrap(sdkerrors.ErrInvalidRequest, "empty description")
 	}
 	if err := validateBlsKeyAndProof(msg.BlsKey, msg.BlsProof); err != nil {
 		return err
 	}
-	err := IsValidEndpointURL(msg.Endpoint)
-	if err != nil {
+	if err := ValidateEndpointURL(msg.Endpoint); err != nil {
 		return errors.Wrapf(sdkerrors.ErrInvalidRequest, "invalid endpoint (%s)", err)
 	}
-	if msg.ReadPrice.IsNegative() || msg.StorePrice.IsNegative() {
+	if msg.ReadPrice.IsNil() || msg.ReadPrice.IsNegative() || msg.StorePrice.IsNil() || msg.StorePrice.IsNegative() {
 		return errors.Wrap(sdkerrors.ErrInvalidRequest, "invalid price")
 	}
 	return nil
@@ -177,7 +177,7 @@ func (msg *MsgEditStorageProvider) ValidateBasic() error {
 	}
 
 	if len(msg.Endpoint) != 0 {
-		err = IsValidEndpointURL(msg.Endpoint)
+		err = ValidateEndpointURL(msg.Endpoint)
 		if err != nil {
 			return errors.Wrapf(sdkerrors.ErrInvalidRequest, "invalid endpoint (%s)", err)
 		}
@@ -369,7 +369,7 @@ func (msg *MsgUpdateStorageProviderStatus) ValidateBasic() error {
 		return errors.Wrapf(sdkerrors.ErrInvalidRequest, "not allowed to update to status %s", msg.Status)
 	}
 	if msg.Status == STATUS_IN_MAINTENANCE && msg.Duration <= 0 {
-		return errors.Wrapf(sdkerrors.ErrInvalidRequest, "maintenanceDuration need to be set for %s", msg.Status)
+		return errors.Wrapf(sdkerrors.ErrInvalidRequest, "maintenance duration need to be set for %s", msg.Status)
 	}
 	return nil
 }

diff --git a/x/sp/types/message_test.go b/x/sp/types/message_test.go
@@ -30,7 +30,7 @@ func TestMsgCreateStorageProvider_ValidateBasic(t *testing.T) {
 	}{
 		{"basic", "a", "b", "c", "d", spAddr, spAddr, spAddr, spAddr, spAddr, spAddr, spAddr, blsPubKey, blsProof, coinPos, nil},
 		{"basic_empty", "a", "b", "c", "d", sdk.AccAddress{}, spAddr, spAddr, spAddr, spAddr, spAddr, spAddr, blsPubKey, blsProof, coinPos, sdkerrors.ErrInvalidAddress},
-		{"zero deposit", "a", "b", "c", "d", spAddr, spAddr, spAddr, spAddr, spAddr, spAddr, spAddr, blsPubKey, blsProof, coinZero, nil},
+		{"zero deposit", "a", "b", "c", "d", spAddr, spAddr, spAddr, spAddr, spAddr, spAddr, spAddr, blsPubKey, blsProof, coinZero, sdkerrors.ErrInvalidCoins},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
@@ -48,7 +48,7 @@ func TestMsgCreateStorageProvider_ValidateBasic(t *testing.T) {
 				Endpoint:           "http://127.0.0.1:9033",
 				StorePrice:         sdk.ZeroDec(),
 				ReadPrice:          sdk.ZeroDec(),
-				Deposit:            coinPos,
+				Deposit:            tt.deposit,
 			}
 			err := msg.ValidateBasic()
 			if tt.err != nil {

diff --git a/x/sp/types/util.go b/x/sp/types/util.go
@@ -7,7 +7,7 @@ import (
 )
 
 // Verify if input endpoint URL is valid.
-func IsValidEndpointURL(endpointURL string) error {
+func ValidateEndpointURL(endpointURL string) error {
 	if endpointURL == "" {
 		return errors.Wrap(ErrInvalidEndpointURL, "Endpoint url cannot be empty.")
 	}

diff --git a/x/storage/types/message.go b/x/storage/types/message.go
@@ -970,6 +970,11 @@ func (msg *MsgLeaveGroup) ValidateBasic() error {
 		return errors.Wrapf(sdkerrors.ErrInvalidAddress, "invalid creator address (%s)", err)
 	}
 
+	_, err = sdk.AccAddressFromHexUnsafe(msg.GroupOwner)
+	if err != nil {
+		return errors.Wrapf(sdkerrors.ErrInvalidAddress, "invalid group owner (%s)", err)
+	}
+
 	err = s3util.CheckValidGroupName(msg.GroupName)
 	if err != nil {
 		return err
@@ -1158,6 +1163,10 @@ func (msg *MsgPutPolicy) ValidateBasic() error {
 		return errors.Wrapf(gnfderrors.ErrInvalidGRN, "invalid greenfield resource name (%s)", err)
 	}
 
+	if msg.Principal == nil {
+		return gnfderrors.ErrInvalidPrincipal.Wrapf("principal cannot be empty")
+	}
+
 	if msg.Principal.Type == permtypes.PRINCIPAL_TYPE_GNFD_GROUP && grn.ResourceType() == resource.RESOURCE_TYPE_GROUP {
 		return gnfderrors.ErrInvalidPrincipal.Wrapf("Not allow grant group's permission to another group")
 	}
@@ -1218,9 +1227,19 @@ func (msg *MsgDeletePolicy) ValidateBasic() error {
 		return errors.Wrapf(gnfderrors.ErrInvalidGRN, "invalid greenfield resource name (%s)", err)
 	}
 
+	if msg.Principal == nil {
+		return gnfderrors.ErrInvalidPrincipal.Wrapf("principal cannot be empty")
+	}
+
 	if msg.Principal.Type == permtypes.PRINCIPAL_TYPE_GNFD_GROUP && grn.ResourceType() == resource.RESOURCE_TYPE_GROUP {
 		return gnfderrors.ErrInvalidPrincipal.Wrapf("Not allow grant group's permission to another group")
 	}
+
+	err = msg.Principal.ValidateBasic()
+	if err != nil {
+		return err
+	}
+
 	return nil
 }
 
@@ -1266,7 +1285,7 @@ func (msg *MsgMirrorBucket) ValidateBasic() error {
 		return errors.Wrapf(sdkerrors.ErrInvalidAddress, "invalid creator address (%s)", err)
 	}
 
-	if msg.Id.GT(sdk.NewUint(0)) {
+	if !msg.Id.IsNil() && msg.Id.GT(sdk.NewUint(0)) {
 		if msg.BucketName != "" {
 			return errors.Wrap(gnfderrors.ErrInvalidBucketName, "Bucket name should be empty")
 		}
@@ -1324,7 +1343,7 @@ func (msg *MsgMirrorObject) ValidateBasic() error {
 		return errors.Wrapf(sdkerrors.ErrInvalidAddress, "invalid creator address (%s)", err)
 	}
 
-	if msg.Id.GT(sdk.NewUint(0)) {
+	if !msg.Id.IsNil() && msg.Id.GT(sdk.NewUint(0)) {
 		if msg.BucketName != "" {
 			return errors.Wrap(gnfderrors.ErrInvalidBucketName, "Bucket name should be empty")
 		}
@@ -1389,7 +1408,7 @@ func (msg *MsgMirrorGroup) ValidateBasic() error {
 		return errors.Wrapf(sdkerrors.ErrInvalidAddress, "invalid creator address (%s)", err)
 	}
 
-	if msg.Id.GT(sdk.NewUint(0)) {
+	if !msg.Id.IsNil() && msg.Id.GT(sdk.NewUint(0)) {
 		if msg.GroupName != "" {
 			return errors.Wrap(gnfderrors.ErrInvalidGroupName, "Group name should be empty")
 		}

diff --git a/x/virtualgroup/types/message.go b/x/virtualgroup/types/message.go
@@ -192,7 +192,7 @@ func (msg *MsgWithdraw) ValidateBasic() error {
 	}
 
 	if !msg.Withdraw.IsValid() || !msg.Withdraw.Amount.IsPositive() {
-		return errors.Wrap(sdkerrors.ErrInvalidRequest, "invalid or non-positive deposit amount")
+		return errors.Wrap(sdkerrors.ErrInvalidRequest, "invalid or non-positive withdraw amount")
 	}
 	return nil
 }