From 3cba149821dc301d8ba85f9521adfb4a16c44e01 Mon Sep 17 00:00:00 2001 From: Redouane Lakrache Date: Tue, 3 Sep 2024 09:47:26 +0200 Subject: [PATCH 1/7] chore: Session header and service basic validation --- x/proof/types/message_create_claim.go | 3 ++ x/proof/types/message_submit_proof.go | 21 ++++++-------- x/session/keeper/session_hydrator.go | 5 ++-- x/session/types/query_get_session_request.go | 5 ++-- x/session/types/session_header.go | 11 ++++---- x/shared/helpers/service_configs.go | 10 +++---- x/shared/helpers/service_test.go | 28 ++++++++++++++----- x/shared/types/errors.go | 1 + x/shared/{helpers => types}/service.go | 24 ++++++++-------- x/supplier/config/supplier_configs_reader.go | 3 +- .../keeper/msg_server_unstake_supplier.go | 1 - 11 files changed, 62 insertions(+), 50 deletions(-) rename x/shared/{helpers => types}/service.go (76%) diff --git a/x/proof/types/message_create_claim.go b/x/proof/types/message_create_claim.go index 3b375daf3..aeda406d1 100644 --- a/x/proof/types/message_create_claim.go +++ b/x/proof/types/message_create_claim.go @@ -22,6 +22,9 @@ func NewMsgCreateClaim( } } +// ValidateBasic performs basic stateless validation of a MsgCreateClaim by ensuring +// that each of the SupplierOperatorAddress, SessionHeader, and RootHash are non-empty +// and valid ones. func (msg *MsgCreateClaim) ValidateBasic() error { // Validate the supplier operator address if _, err := sdk.AccAddressFromBech32(msg.GetSupplierOperatorAddress()); err != nil { diff --git a/x/proof/types/message_submit_proof.go b/x/proof/types/message_submit_proof.go index b2ad02b2e..fb4736067 100644 --- a/x/proof/types/message_submit_proof.go +++ b/x/proof/types/message_submit_proof.go @@ -17,10 +17,9 @@ func NewMsgSubmitProof(supplierOperatorAddress string, sessionHeader *sessiontyp } } -// ValidateBasic ensures that the bech32 operator address strings for the supplier and -// application addresses are valid and that the proof and service ID are not empty. -// -// TODO_BETA: Call `msg.GetSessionHeader().ValidateBasic()` once its implemented +// ValidateBasic performs basic stateless validation of a MsgSubmitProof by ensuring +// that each of the SupplierOperatorAddress, SessionHeader, and Proof are non-empty +// and valid ones. func (msg *MsgSubmitProof) ValidateBasic() error { if _, err := sdk.AccAddressFromBech32(msg.GetSupplierOperatorAddress()); err != nil { return sdkerrors.ErrInvalidAddress.Wrapf( @@ -30,16 +29,14 @@ func (msg *MsgSubmitProof) ValidateBasic() error { ) } - if _, err := sdk.AccAddressFromBech32(msg.GetSessionHeader().GetApplicationAddress()); err != nil { - return sdkerrors.ErrInvalidAddress.Wrapf( - "application address: %q, error: %s", - msg.GetSessionHeader().GetApplicationAddress(), - err, - ) + // Retrieve & validate the session header + sessionHeader := msg.SessionHeader + if sessionHeader == nil { + return ErrProofInvalidSessionHeader.Wrapf("session header is nil") } - if msg.GetSessionHeader().GetService().GetId() == "" { - return ErrProofInvalidService.Wrap("proof service ID %q cannot be empty") + if err := sessionHeader.ValidateBasic(); err != nil { + return ErrProofInvalidSessionHeader.Wrapf("invalid session header: %v; %s", sessionHeader, err) } if len(msg.GetProof()) == 0 { diff --git a/x/session/keeper/session_hydrator.go b/x/session/keeper/session_hydrator.go index 52f377cc3..2d1ce934c 100644 --- a/x/session/keeper/session_hydrator.go +++ b/x/session/keeper/session_hydrator.go @@ -14,7 +14,6 @@ import ( "github.com/pokt-network/poktroll/x/session/types" "github.com/pokt-network/poktroll/x/shared" - sharedhelpers "github.com/pokt-network/poktroll/x/shared/helpers" sharedtypes "github.com/pokt-network/poktroll/x/shared/types" ) @@ -118,8 +117,8 @@ func (k Keeper) hydrateSessionID(ctx context.Context, sh *sessionHydrator) error // TODO_MAINNET: In the future, we will need to validate that the Service is // a valid service depending on whether or not its permissioned or permissionless - if !sharedhelpers.IsValidService(sh.sessionHeader.Service) { - return types.ErrSessionHydration.Wrapf("invalid service: %v", sh.sessionHeader.Service) + if err := sh.sessionHeader.Service.ValidateBasic(); err != nil { + return types.ErrSessionHydration.Wrapf("invalid service: %v; err %s", sh.sessionHeader.Service, err) } sh.sessionHeader.SessionId, sh.sessionIDBz = k.GetSessionId( diff --git a/x/session/types/query_get_session_request.go b/x/session/types/query_get_session_request.go index 3c7b754d3..9addcac33 100644 --- a/x/session/types/query_get_session_request.go +++ b/x/session/types/query_get_session_request.go @@ -3,7 +3,6 @@ package types import ( sdk "github.com/cosmos/cosmos-sdk/types" - sharedhelpers "github.com/pokt-network/poktroll/x/shared/helpers" sharedtypes "github.com/pokt-network/poktroll/x/shared/types" ) @@ -27,8 +26,8 @@ func (query *QueryGetSessionRequest) ValidateBasic() error { } // Validate the Service ID - if !sharedhelpers.IsValidService(query.Service) { - return ErrSessionInvalidService.Wrapf("invalid service for session being retrieved %s;", query.Service) + if err := query.Service.ValidateBasic(); err != nil { + return ErrSessionInvalidService.Wrapf("invalid service for session being retrieved %s; %v", query.Service, err) } // Validate the height for which a session is being retrieved diff --git a/x/session/types/session_header.go b/x/session/types/session_header.go index ff322b938..ccce220fd 100644 --- a/x/session/types/session_header.go +++ b/x/session/types/session_header.go @@ -2,11 +2,8 @@ package types import ( sdk "github.com/cosmos/cosmos-sdk/types" - - sharedhelpers "github.com/pokt-network/poktroll/x/shared/helpers" ) -// TODO_BETA: Ensure this is used everywhere a SessionHeader is validated. // ValidateBasic performs basic stateless validation of a SessionHeader. func (sh *SessionHeader) ValidateBasic() error { // Validate the application address @@ -20,8 +17,12 @@ func (sh *SessionHeader) ValidateBasic() error { } // Validate the service - if sh.Service == nil || !sharedhelpers.IsValidService(sh.Service) { - return ErrSessionInvalidService.Wrapf("invalid service: %s", sh.Service) + if sh.Service == nil { + return ErrSessionInvalidService.Wrapf("missing service: %s", sh.Service) + } + + if err := sh.Service.ValidateBasic(); err != nil { + return ErrSessionInvalidService.Wrapf("invalid service: %v; %s", sh.Service, err) } // Sessions can only start at height 1 diff --git a/x/shared/helpers/service_configs.go b/x/shared/helpers/service_configs.go index 5462ca1fa..a0be741bc 100644 --- a/x/shared/helpers/service_configs.go +++ b/x/shared/helpers/service_configs.go @@ -22,8 +22,8 @@ func ValidateAppServiceConfigs(services []*sharedtypes.ApplicationServiceConfig) return fmt.Errorf("serviceConfig cannot be nil: %v", services) } // Check the Service - if !IsValidService(serviceConfig.Service) { - return fmt.Errorf("invalid service: %v", serviceConfig.Service) + if err := serviceConfig.Service.ValidateBasic(); err != nil { + return sharedtypes.ErrSharedInvalidService.Wrapf("invalid service: %v; %s", serviceConfig.Service, err) } } return nil @@ -40,8 +40,8 @@ func ValidateSupplierServiceConfigs(services []*sharedtypes.SupplierServiceConfi } // Check the Service - if !IsValidService(serviceConfig.Service) { - return fmt.Errorf("invalid service: %v", serviceConfig.Service) + if err := serviceConfig.Service.ValidateBasic(); err != nil { + return sharedtypes.ErrSharedInvalidService.Wrapf("invalid service: %v; %s", serviceConfig.Service, err) } // Check the Endpoints @@ -62,7 +62,7 @@ func ValidateSupplierServiceConfigs(services []*sharedtypes.SupplierServiceConfi if endpoint.Url == "" { return fmt.Errorf("endpoint.Url cannot be empty: %v", serviceConfig) } - if !IsValidEndpointUrl(endpoint.Url) { + if !sharedtypes.IsValidEndpointUrl(endpoint.Url) { return fmt.Errorf("invalid endpoint.Url: %v", serviceConfig) } diff --git a/x/shared/helpers/service_test.go b/x/shared/helpers/service_test.go index 349c4ee1b..b7f70c384 100644 --- a/x/shared/helpers/service_test.go +++ b/x/shared/helpers/service_test.go @@ -73,8 +73,12 @@ func TestIsValidService(t *testing.T) { Id: test.serviceId, Name: test.serviceName, } - isValid := IsValidService(service) - require.Equal(t, test.expectedIsValid, isValid) + err := service.ValidateBasic() + if test.expectedIsValid { + require.NoError(t, err) + } else { + require.Error(t, err) + } }) } } @@ -124,8 +128,13 @@ func TestIsValidServiceName(t *testing.T) { for _, test := range tests { t.Run(test.desc, func(t *testing.T) { - isValid := IsValidServiceName(test.serviceName) - require.Equal(t, test.expectedIsValid, isValid) + service := &sharedtypes.Service{Id: "svc", Name: test.serviceName} + err := service.ValidateBasic() + if test.expectedIsValid { + require.NoError(t, err) + } else { + require.ErrorIs(t, err, sharedtypes.ErrSharedInvalidService.Wrapf("invalid service name: %s", test.serviceName)) + } }) } } @@ -195,8 +204,13 @@ func TestIsValidServiceId(t *testing.T) { for _, test := range tests { t.Run(test.desc, func(t *testing.T) { - isValid := IsValidServiceId(test.serviceId) - require.Equal(t, test.expectedIsValid, isValid) + service := &sharedtypes.Service{Id: test.serviceId} + err := service.ValidateBasic() + if test.expectedIsValid { + require.NoError(t, err) + } else { + require.ErrorIs(t, err, sharedtypes.ErrSharedInvalidService.Wrapf("invalid service ID: %s", test.serviceId)) + } }) } } @@ -254,7 +268,7 @@ func TestIsValidEndpointUrl(t *testing.T) { for _, test := range tests { t.Run(test.desc, func(t *testing.T) { - isValid := IsValidEndpointUrl(test.endpointURL) + isValid := sharedtypes.IsValidEndpointUrl(test.endpointURL) require.Equal(t, test.expectedIsValid, isValid) }) } diff --git a/x/shared/types/errors.go b/x/shared/types/errors.go index f823d6893..0ef96d4f5 100644 --- a/x/shared/types/errors.go +++ b/x/shared/types/errors.go @@ -15,4 +15,5 @@ var ( ErrSharedEmitEvent = sdkerrors.Register(ModuleName, 1104, "failed to emit event") ErrSharedUnauthorizedSupplierUpdate = sdkerrors.Register(ModuleName, 1105, "unauthorized supplier update") ErrSharedInvalidRevShare = sdkerrors.Register(ModuleName, 1106, "invalid revenue share configuration") + ErrSharedInvalidService = sdkerrors.Register(ModuleName, 1107, "invalid service") ) diff --git a/x/shared/helpers/service.go b/x/shared/types/service.go similarity index 76% rename from x/shared/helpers/service.go rename to x/shared/types/service.go index 396dee124..e1c866255 100644 --- a/x/shared/helpers/service.go +++ b/x/shared/types/service.go @@ -1,10 +1,8 @@ -package helpers +package types import ( "net/url" "regexp" - - sharedtypes "github.com/pokt-network/poktroll/x/shared/types" ) const ( @@ -24,20 +22,22 @@ func init() { // Compile the regex pattern regexExprServiceId = regexp.MustCompile(regexServiceId) regexExprServiceName = regexp.MustCompile(regexServiceName) - } -// IsValidService checks if the provided ServiceId struct has valid fields -// TODO_TECHDEBT(BETA): Refactor to a `Service#ValidateBasic` method. -func IsValidService(service *sharedtypes.Service) bool { - // Check if service Id and Name are valid using the provided helper functions - return service != nil && - IsValidServiceId(service.Id) && - IsValidServiceName(service.Name) +// ValidateBasic performs basic stateless validation of a SessionHeader. +func (s *Service) ValidateBasic() error { + if !IsValidServiceId(s.Id) { + return ErrSharedInvalidService.Wrapf("invalid service ID: %s", s.Id) + } + + if !IsValidServiceName(s.Name) { + return ErrSharedInvalidService.Wrapf("invalid service name: %s", s.Name) + } + + return nil } // IsValidServiceId checks if the input string is a valid serviceId -// TODO_TECHDEBT(BETA): Refactor to a `ServiceId#ValidateBasic` method. func IsValidServiceId(serviceId string) bool { // ServiceId CANNOT be empty if len(serviceId) == 0 { diff --git a/x/supplier/config/supplier_configs_reader.go b/x/supplier/config/supplier_configs_reader.go index d8adfb227..fab3f8a0d 100644 --- a/x/supplier/config/supplier_configs_reader.go +++ b/x/supplier/config/supplier_configs_reader.go @@ -11,7 +11,6 @@ import ( "github.com/pokt-network/poktroll/pkg/polylog" _ "github.com/pokt-network/poktroll/pkg/polylog/polyzero" "github.com/pokt-network/poktroll/x/shared/helpers" - sharedhelpers "github.com/pokt-network/poktroll/x/shared/helpers" sharedtypes "github.com/pokt-network/poktroll/x/shared/types" ) @@ -127,7 +126,7 @@ func ParseSupplierConfigs(ctx context.Context, configContent []byte) (*SupplierS // Populate the services slice for _, svc := range stakeConfig.Services { // Validate the serviceId - if !sharedhelpers.IsValidServiceId(svc.ServiceId) { + if !sharedtypes.IsValidServiceId(svc.ServiceId) { return nil, ErrSupplierConfigInvalidServiceId.Wrapf("%s", svc.ServiceId) } diff --git a/x/supplier/keeper/msg_server_unstake_supplier.go b/x/supplier/keeper/msg_server_unstake_supplier.go index 877134b2a..510368baa 100644 --- a/x/supplier/keeper/msg_server_unstake_supplier.go +++ b/x/supplier/keeper/msg_server_unstake_supplier.go @@ -12,7 +12,6 @@ import ( "github.com/pokt-network/poktroll/x/supplier/types" ) -// TODO_BETA(#489): Determine if an application needs an unbonding period after unstaking. func (k msgServer) UnstakeSupplier( ctx context.Context, msg *types.MsgUnstakeSupplier, From 124b660674da80a400e9ed67e226c9c7acd6a0f1 Mon Sep 17 00:00:00 2001 From: Redouane Lakrache Date: Tue, 3 Sep 2024 18:55:22 +0200 Subject: [PATCH 2/7] fix: Adapt test after refactor --- api/poktroll/application/types.pulsar.go | 9 +- .../config/application_configs_reader.go | 3 +- x/application/types/types.pb.go | 8 +- .../keeper/msg_server_submit_proof_test.go | 89 +++++++++++-------- x/proof/types/message_submit_proof.go | 2 +- x/proof/types/message_submit_proof_test.go | 44 +++++---- x/session/types/session_header.go | 2 +- 7 files changed, 94 insertions(+), 63 deletions(-) diff --git a/api/poktroll/application/types.pulsar.go b/api/poktroll/application/types.pulsar.go index 48571c524..60824543a 100644 --- a/api/poktroll/application/types.pulsar.go +++ b/api/poktroll/application/types.pulsar.go @@ -1641,8 +1641,13 @@ type Application struct { sizeCache protoimpl.SizeCache unknownFields protoimpl.UnknownFields - Address string `protobuf:"bytes,1,opt,name=address,proto3" json:"address,omitempty"` // The Bech32 address of the application. - Stake *v1beta1.Coin `protobuf:"bytes,2,opt,name=stake,proto3" json:"stake,omitempty"` // The total amount of uPOKT the application has staked + Address string `protobuf:"bytes,1,opt,name=address,proto3" json:"address,omitempty"` // The Bech32 address of the application. + Stake *v1beta1.Coin `protobuf:"bytes,2,opt,name=stake,proto3" json:"stake,omitempty"` // The total amount of uPOKT the application has staked + // TODO_BETA(@red-0ne, @olshansk): Limit this to one service_config. + // + // Remove `repeated`, drop the `s` from service_configs and document why + // this is the case in the app config (and here) per this discussion: + // https://github.com/pokt-network/poktroll/pull/750#discussion_r1735025033 ServiceConfigs []*shared.ApplicationServiceConfig `protobuf:"bytes,3,rep,name=service_configs,json=serviceConfigs,proto3" json:"service_configs,omitempty"` // The list of services this appliccation is configured to request service for // TODO_BETA: Rename `delegatee_gateway_addresses` to `gateway_addresses_delegated_to`. // Ensure to rename all relevant configs, comments, variables, function names, etc as well. diff --git a/x/application/module/config/application_configs_reader.go b/x/application/module/config/application_configs_reader.go index 871ac8709..143dff694 100644 --- a/x/application/module/config/application_configs_reader.go +++ b/x/application/module/config/application_configs_reader.go @@ -5,7 +5,6 @@ import ( sdk "github.com/cosmos/cosmos-sdk/types" - sharedhelpers "github.com/pokt-network/poktroll/x/shared/helpers" sharedtypes "github.com/pokt-network/poktroll/x/shared/types" ) @@ -73,7 +72,7 @@ func ParseApplicationConfigs(configContent []byte) (*ApplicationStakeConfig, err for _, serviceId := range parsedAppConfig.ServiceIds { // Validate serviceId - if !sharedhelpers.IsValidServiceId(serviceId) { + if !sharedtypes.IsValidServiceId(serviceId) { return nil, ErrApplicationConfigInvalidServiceId.Wrapf("%s", serviceId) } diff --git a/x/application/types/types.pb.go b/x/application/types/types.pb.go index 2e6001391..1b90dba76 100644 --- a/x/application/types/types.pb.go +++ b/x/application/types/types.pb.go @@ -29,8 +29,12 @@ const _ = proto.GoGoProtoPackageIsVersion3 // please upgrade the proto package // Application defines the type used to store an on-chain definition and state for an application type Application struct { - Address string `protobuf:"bytes,1,opt,name=address,proto3" json:"address,omitempty"` - Stake *types.Coin `protobuf:"bytes,2,opt,name=stake,proto3" json:"stake,omitempty"` + Address string `protobuf:"bytes,1,opt,name=address,proto3" json:"address,omitempty"` + Stake *types.Coin `protobuf:"bytes,2,opt,name=stake,proto3" json:"stake,omitempty"` + // TODO_BETA(@red-0ne, @olshansk): Limit this to one service_config. + // Remove `repeated`, drop the `s` from service_configs and document why + // this is the case in the app config (and here) per this discussion: + // https://github.com/pokt-network/poktroll/pull/750#discussion_r1735025033 ServiceConfigs []*types1.ApplicationServiceConfig `protobuf:"bytes,3,rep,name=service_configs,json=serviceConfigs,proto3" json:"service_configs,omitempty"` // TODO_BETA: Rename `delegatee_gateway_addresses` to `gateway_addresses_delegated_to`. // Ensure to rename all relevant configs, comments, variables, function names, etc as well. diff --git a/x/proof/keeper/msg_server_submit_proof_test.go b/x/proof/keeper/msg_server_submit_proof_test.go index 66b317707..91b9de7b5 100644 --- a/x/proof/keeper/msg_server_submit_proof_test.go +++ b/x/proof/keeper/msg_server_submit_proof_test.go @@ -528,9 +528,9 @@ func TestMsgServer_SubmitProof_Error(t *testing.T) { ) tests := []struct { - desc string - newProofMsg func(t *testing.T) *prooftypes.MsgSubmitProof - expectedErr error + desc string + newProofMsg func(t *testing.T) *prooftypes.MsgSubmitProof + proofToExpectedError func(*prooftypes.MsgSubmitProof) error }{ { desc: "proof service ID cannot be empty", @@ -547,14 +547,19 @@ func TestMsgServer_SubmitProof_Error(t *testing.T) { expectedMerkleProofPath, ) }, - expectedErr: status.Error( - codes.InvalidArgument, - prooftypes.ErrProofInvalidSessionId.Wrapf( - "session ID does not match on-chain session ID; expected %q, got %q", - validSessionHeader.GetSessionId(), - "", - ).Error(), - ), + proofToExpectedError: func(msgSubmitProof *prooftypes.MsgSubmitProof) error { + sessionError := sessiontypes.ErrSessionInvalidSessionId.Wrapf( + "invalid session ID: %s", + msgSubmitProof.GetSessionHeader().GetSessionId(), + ) + return status.Error( + codes.InvalidArgument, + prooftypes.ErrProofInvalidSessionHeader.Wrapf( + "invalid session header: %s", + sessionError, + ).Error(), + ) + }, }, { desc: "merkle proof cannot be empty", @@ -571,12 +576,14 @@ func TestMsgServer_SubmitProof_Error(t *testing.T) { proof.Proof = []byte{} return proof }, - expectedErr: status.Error( - codes.InvalidArgument, - prooftypes.ErrProofInvalidProof.Wrap( - "proof cannot be empty", - ).Error(), - ), + proofToExpectedError: func(_ *prooftypes.MsgSubmitProof) error { + return status.Error( + codes.InvalidArgument, + prooftypes.ErrProofInvalidProof.Wrap( + "proof cannot be empty", + ).Error(), + ) + }, }, { desc: "proof session ID must match on-chain session ID", @@ -589,14 +596,16 @@ func TestMsgServer_SubmitProof_Error(t *testing.T) { expectedMerkleProofPath, ) }, - expectedErr: status.Error( - codes.InvalidArgument, - prooftypes.ErrProofInvalidSessionId.Wrapf( - "session ID does not match on-chain session ID; expected %q, got %q", - validSessionHeader.GetSessionId(), - wrongSessionIdHeader.GetSessionId(), - ).Error(), - ), + proofToExpectedError: func(msgSubmitProof *prooftypes.MsgSubmitProof) error { + return status.Error( + codes.InvalidArgument, + prooftypes.ErrProofInvalidSessionId.Wrapf( + "session ID does not match on-chain session ID; expected %q, got %q", + validSessionHeader.GetSessionId(), + msgSubmitProof.GetSessionHeader().GetSessionId(), + ).Error(), + ) + }, }, { desc: "proof supplier must be in on-chain session", @@ -609,28 +618,30 @@ func TestMsgServer_SubmitProof_Error(t *testing.T) { expectedMerkleProofPath, ) }, - expectedErr: status.Error( - codes.InvalidArgument, - prooftypes.ErrProofNotFound.Wrapf( - "supplier operator address %q not found in session ID %q", - wrongSupplierOperatorAddr, - validSessionHeader.GetSessionId(), - ).Error(), - ), + proofToExpectedError: func(msgSubmitProof *prooftypes.MsgSubmitProof) error { + return status.Error( + codes.InvalidArgument, + prooftypes.ErrProofNotFound.Wrapf( + "supplier operator address %q not found in session ID %q", + wrongSupplierOperatorAddr, + msgSubmitProof.GetSessionHeader().GetSessionId(), + ).Error(), + ) + }, }, } // Submit the corresponding proof. for _, test := range tests { t.Run(test.desc, func(t *testing.T) { - proofMsg := test.newProofMsg(t) + msgSubmitProof := test.newProofMsg(t) // Advance the block height to the proof path seed height. earliestSupplierProofCommitHeight := shared.GetEarliestSupplierProofCommitHeight( &sharedParams, - proofMsg.GetSessionHeader().GetSessionEndBlockHeight(), + msgSubmitProof.GetSessionHeader().GetSessionEndBlockHeight(), blockHeaderHash, - proofMsg.GetSupplierOperatorAddress(), + msgSubmitProof.GetSupplierOperatorAddress(), ) ctx = keepertest.SetBlockHeight(ctx, earliestSupplierProofCommitHeight-1) @@ -641,9 +652,11 @@ func TestMsgServer_SubmitProof_Error(t *testing.T) { // Advance the block height to the earliest proof commit height. ctx = keepertest.SetBlockHeight(ctx, earliestSupplierProofCommitHeight) - submitProofRes, err := srv.SubmitProof(ctx, proofMsg) + submitProofRes, err := srv.SubmitProof(ctx, msgSubmitProof) - require.ErrorContains(t, err, test.expectedErr.Error()) + expectedErr := test.proofToExpectedError(msgSubmitProof) + require.ErrorIs(t, err, expectedErr) + require.ErrorContains(t, err, expectedErr.Error()) require.Nil(t, submitProofRes) proofRes, err := keepers.AllProofs(ctx, &prooftypes.QueryAllProofsRequest{}) diff --git a/x/proof/types/message_submit_proof.go b/x/proof/types/message_submit_proof.go index fb4736067..828d8d337 100644 --- a/x/proof/types/message_submit_proof.go +++ b/x/proof/types/message_submit_proof.go @@ -36,7 +36,7 @@ func (msg *MsgSubmitProof) ValidateBasic() error { } if err := sessionHeader.ValidateBasic(); err != nil { - return ErrProofInvalidSessionHeader.Wrapf("invalid session header: %v; %s", sessionHeader, err) + return ErrProofInvalidSessionHeader.Wrapf("invalid session header: %s", err) } if len(msg.GetProof()) == 0 { diff --git a/x/proof/types/message_submit_proof_test.go b/x/proof/types/message_submit_proof_test.go index aaef1c1e3..a60ccf3ad 100644 --- a/x/proof/types/message_submit_proof_test.go +++ b/x/proof/types/message_submit_proof_test.go @@ -17,9 +17,9 @@ func TestMsgSubmitProof_ValidateBasic(t *testing.T) { testClosestMerkleProof := []byte{1, 2, 3, 4} tests := []struct { - desc string - msg MsgSubmitProof - expectedErr error + desc string + msg MsgSubmitProof + msgSubmitProofExpectedErr func(sessiontypes.SessionHeader) error }{ { desc: "application bech32 address is invalid", @@ -34,11 +34,14 @@ func TestMsgSubmitProof_ValidateBasic(t *testing.T) { }, Proof: testClosestMerkleProof, }, - expectedErr: sdkerrors.ErrInvalidAddress.Wrapf( - "application address: %q, error: %s", - "not_a_bech32_address", - "decoding bech32 failed: invalid separator index -1", - ), + msgSubmitProofExpectedErr: func(sh sessiontypes.SessionHeader) error { + sessionError := sessiontypes.ErrSessionInvalidAppAddress.Wrapf( + "invalid application address: %s; (%s)", + sh.ApplicationAddress, + "decoding bech32 failed: invalid separator index -1", + ) + return ErrProofInvalidSessionHeader.Wrapf("invalid session header: %s", sessionError) + }, }, { desc: "supplier operator bech32 address is invalid", @@ -53,11 +56,13 @@ func TestMsgSubmitProof_ValidateBasic(t *testing.T) { }, Proof: testClosestMerkleProof, }, - expectedErr: sdkerrors.ErrInvalidAddress.Wrapf( - "supplier operator address %q, error: %s", - "not_a_bech32_address", - "decoding bech32 failed: invalid separator index -1", - ), + msgSubmitProofExpectedErr: func(sh sessiontypes.SessionHeader) error { + return sdkerrors.ErrInvalidAddress.Wrapf( + "supplier operator address %q, error: %s", + "not_a_bech32_address", + "decoding bech32 failed: invalid separator index -1", + ) + }, }, { desc: "session service ID is empty", @@ -72,7 +77,11 @@ func TestMsgSubmitProof_ValidateBasic(t *testing.T) { }, Proof: testClosestMerkleProof, }, - expectedErr: ErrProofInvalidService.Wrap("proof service ID %q cannot be empty"), + msgSubmitProofExpectedErr: func(sh sessiontypes.SessionHeader) error { + serviceError := sharedtypes.ErrSharedInvalidService.Wrapf("invalid service ID: %s", sh.Service.Id) + sessionError := sessiontypes.ErrSessionInvalidService.Wrapf("invalid service: %v; %s", sh.Service, serviceError) + return ErrProofInvalidSessionHeader.Wrapf("invalid session header: %s", sessionError) + }, }, { desc: "valid message metadata", @@ -92,9 +101,10 @@ func TestMsgSubmitProof_ValidateBasic(t *testing.T) { for _, test := range tests { t.Run(test.desc, func(t *testing.T) { err := test.msg.ValidateBasic() - if test.expectedErr != nil { - require.ErrorIs(t, err, test.expectedErr) - require.ErrorContains(t, err, test.expectedErr.Error()) + if test.msgSubmitProofExpectedErr != nil { + expectedErr := test.msgSubmitProofExpectedErr(*test.msg.SessionHeader) + require.ErrorIs(t, err, expectedErr) + require.ErrorContains(t, err, expectedErr.Error()) return } require.NoError(t, err) diff --git a/x/session/types/session_header.go b/x/session/types/session_header.go index ccce220fd..d5f715e71 100644 --- a/x/session/types/session_header.go +++ b/x/session/types/session_header.go @@ -8,7 +8,7 @@ import ( func (sh *SessionHeader) ValidateBasic() error { // Validate the application address if _, err := sdk.AccAddressFromBech32(sh.ApplicationAddress); err != nil { - return ErrSessionInvalidAppAddress.Wrapf("invalid application address: %s; (%v)", sh.ApplicationAddress, err) + return ErrSessionInvalidAppAddress.Wrapf("invalid application address: %s; (%s)", sh.ApplicationAddress, err) } // Validate the session ID From 29944153df072d743132796b620e226ce52133f8 Mon Sep 17 00:00:00 2001 From: Redouane Lakrache Date: Tue, 3 Sep 2024 19:52:34 +0200 Subject: [PATCH 3/7] chore: Fix godoc comment --- x/shared/types/service.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x/shared/types/service.go b/x/shared/types/service.go index e1c866255..f5db0f0ed 100644 --- a/x/shared/types/service.go +++ b/x/shared/types/service.go @@ -24,7 +24,7 @@ func init() { regexExprServiceName = regexp.MustCompile(regexServiceName) } -// ValidateBasic performs basic stateless validation of a SessionHeader. +// ValidateBasic performs basic stateless validation of a Service. func (s *Service) ValidateBasic() error { if !IsValidServiceId(s.Id) { return ErrSharedInvalidService.Wrapf("invalid service ID: %s", s.Id) From 9bc4442eeea9f908ab3024057fbf8d4405223744 Mon Sep 17 00:00:00 2001 From: Redouane Lakrache Date: Fri, 6 Sep 2024 22:08:09 +0200 Subject: [PATCH 4/7] WIP: ValidteBasic --- .../keeper/msg_server_submit_proof_test.go | 16 +-- x/proof/types/message_create_claim.go | 4 +- x/proof/types/message_submit_proof.go | 4 +- x/proof/types/message_submit_proof_test.go | 16 +-- .../keeper/msg_server_add_service_test.go | 2 +- x/service/module/tx_add_service.go | 3 +- x/service/types/errors.go | 27 ++-- x/service/types/message_add_service.go | 23 +--- x/service/types/message_add_service_test.go | 120 ++++++++++-------- x/shared/types/errors.go | 17 +-- x/shared/types/service.go | 25 ++++ 11 files changed, 140 insertions(+), 117 deletions(-) diff --git a/x/proof/keeper/msg_server_submit_proof_test.go b/x/proof/keeper/msg_server_submit_proof_test.go index 91b9de7b5..cc291e5e4 100644 --- a/x/proof/keeper/msg_server_submit_proof_test.go +++ b/x/proof/keeper/msg_server_submit_proof_test.go @@ -528,9 +528,9 @@ func TestMsgServer_SubmitProof_Error(t *testing.T) { ) tests := []struct { - desc string - newProofMsg func(t *testing.T) *prooftypes.MsgSubmitProof - proofToExpectedError func(*prooftypes.MsgSubmitProof) error + desc string + newProofMsg func(t *testing.T) *prooftypes.MsgSubmitProof + msgSubmitProofToExpectedErrorFn func(*prooftypes.MsgSubmitProof) error }{ { desc: "proof service ID cannot be empty", @@ -547,7 +547,7 @@ func TestMsgServer_SubmitProof_Error(t *testing.T) { expectedMerkleProofPath, ) }, - proofToExpectedError: func(msgSubmitProof *prooftypes.MsgSubmitProof) error { + msgSubmitProofToExpectedErrorFn: func(msgSubmitProof *prooftypes.MsgSubmitProof) error { sessionError := sessiontypes.ErrSessionInvalidSessionId.Wrapf( "invalid session ID: %s", msgSubmitProof.GetSessionHeader().GetSessionId(), @@ -576,7 +576,7 @@ func TestMsgServer_SubmitProof_Error(t *testing.T) { proof.Proof = []byte{} return proof }, - proofToExpectedError: func(_ *prooftypes.MsgSubmitProof) error { + msgSubmitProofToExpectedErrorFn: func(_ *prooftypes.MsgSubmitProof) error { return status.Error( codes.InvalidArgument, prooftypes.ErrProofInvalidProof.Wrap( @@ -596,7 +596,7 @@ func TestMsgServer_SubmitProof_Error(t *testing.T) { expectedMerkleProofPath, ) }, - proofToExpectedError: func(msgSubmitProof *prooftypes.MsgSubmitProof) error { + msgSubmitProofToExpectedErrorFn: func(msgSubmitProof *prooftypes.MsgSubmitProof) error { return status.Error( codes.InvalidArgument, prooftypes.ErrProofInvalidSessionId.Wrapf( @@ -618,7 +618,7 @@ func TestMsgServer_SubmitProof_Error(t *testing.T) { expectedMerkleProofPath, ) }, - proofToExpectedError: func(msgSubmitProof *prooftypes.MsgSubmitProof) error { + msgSubmitProofToExpectedErrorFn: func(msgSubmitProof *prooftypes.MsgSubmitProof) error { return status.Error( codes.InvalidArgument, prooftypes.ErrProofNotFound.Wrapf( @@ -654,7 +654,7 @@ func TestMsgServer_SubmitProof_Error(t *testing.T) { submitProofRes, err := srv.SubmitProof(ctx, msgSubmitProof) - expectedErr := test.proofToExpectedError(msgSubmitProof) + expectedErr := test.msgSubmitProofToExpectedErrorFn(msgSubmitProof) require.ErrorIs(t, err, expectedErr) require.ErrorContains(t, err, expectedErr.Error()) require.Nil(t, submitProofRes) diff --git a/x/proof/types/message_create_claim.go b/x/proof/types/message_create_claim.go index aeda406d1..deae434ee 100644 --- a/x/proof/types/message_create_claim.go +++ b/x/proof/types/message_create_claim.go @@ -22,9 +22,7 @@ func NewMsgCreateClaim( } } -// ValidateBasic performs basic stateless validation of a MsgCreateClaim by ensuring -// that each of the SupplierOperatorAddress, SessionHeader, and RootHash are non-empty -// and valid ones. +// ValidateBasic performs basic stateless validation of a MsgCreateClaim. func (msg *MsgCreateClaim) ValidateBasic() error { // Validate the supplier operator address if _, err := sdk.AccAddressFromBech32(msg.GetSupplierOperatorAddress()); err != nil { diff --git a/x/proof/types/message_submit_proof.go b/x/proof/types/message_submit_proof.go index 828d8d337..b4d6b312b 100644 --- a/x/proof/types/message_submit_proof.go +++ b/x/proof/types/message_submit_proof.go @@ -17,9 +17,7 @@ func NewMsgSubmitProof(supplierOperatorAddress string, sessionHeader *sessiontyp } } -// ValidateBasic performs basic stateless validation of a MsgSubmitProof by ensuring -// that each of the SupplierOperatorAddress, SessionHeader, and Proof are non-empty -// and valid ones. +// ValidateBasic performs basic stateless validation of a MsgSubmitProof. func (msg *MsgSubmitProof) ValidateBasic() error { if _, err := sdk.AccAddressFromBech32(msg.GetSupplierOperatorAddress()); err != nil { return sdkerrors.ErrInvalidAddress.Wrapf( diff --git a/x/proof/types/message_submit_proof_test.go b/x/proof/types/message_submit_proof_test.go index a60ccf3ad..3404ff0c0 100644 --- a/x/proof/types/message_submit_proof_test.go +++ b/x/proof/types/message_submit_proof_test.go @@ -17,9 +17,9 @@ func TestMsgSubmitProof_ValidateBasic(t *testing.T) { testClosestMerkleProof := []byte{1, 2, 3, 4} tests := []struct { - desc string - msg MsgSubmitProof - msgSubmitProofExpectedErr func(sessiontypes.SessionHeader) error + desc string + msg MsgSubmitProof + sessionHeaderToExpectedErrorFn func(sessiontypes.SessionHeader) error }{ { desc: "application bech32 address is invalid", @@ -34,7 +34,7 @@ func TestMsgSubmitProof_ValidateBasic(t *testing.T) { }, Proof: testClosestMerkleProof, }, - msgSubmitProofExpectedErr: func(sh sessiontypes.SessionHeader) error { + sessionHeaderToExpectedErrorFn: func(sh sessiontypes.SessionHeader) error { sessionError := sessiontypes.ErrSessionInvalidAppAddress.Wrapf( "invalid application address: %s; (%s)", sh.ApplicationAddress, @@ -56,7 +56,7 @@ func TestMsgSubmitProof_ValidateBasic(t *testing.T) { }, Proof: testClosestMerkleProof, }, - msgSubmitProofExpectedErr: func(sh sessiontypes.SessionHeader) error { + sessionHeaderToExpectedErrorFn: func(sh sessiontypes.SessionHeader) error { return sdkerrors.ErrInvalidAddress.Wrapf( "supplier operator address %q, error: %s", "not_a_bech32_address", @@ -77,7 +77,7 @@ func TestMsgSubmitProof_ValidateBasic(t *testing.T) { }, Proof: testClosestMerkleProof, }, - msgSubmitProofExpectedErr: func(sh sessiontypes.SessionHeader) error { + sessionHeaderToExpectedErrorFn: func(sh sessiontypes.SessionHeader) error { serviceError := sharedtypes.ErrSharedInvalidService.Wrapf("invalid service ID: %s", sh.Service.Id) sessionError := sessiontypes.ErrSessionInvalidService.Wrapf("invalid service: %v; %s", sh.Service, serviceError) return ErrProofInvalidSessionHeader.Wrapf("invalid session header: %s", sessionError) @@ -101,8 +101,8 @@ func TestMsgSubmitProof_ValidateBasic(t *testing.T) { for _, test := range tests { t.Run(test.desc, func(t *testing.T) { err := test.msg.ValidateBasic() - if test.msgSubmitProofExpectedErr != nil { - expectedErr := test.msgSubmitProofExpectedErr(*test.msg.SessionHeader) + if test.sessionHeaderToExpectedErrorFn != nil { + expectedErr := test.sessionHeaderToExpectedErrorFn(*test.msg.SessionHeader) require.ErrorIs(t, err, expectedErr) require.ErrorContains(t, err, expectedErr.Error()) return diff --git a/x/service/keeper/msg_server_add_service_test.go b/x/service/keeper/msg_server_add_service_test.go index 962689710..495ad29bd 100644 --- a/x/service/keeper/msg_server_add_service_test.go +++ b/x/service/keeper/msg_server_add_service_test.go @@ -140,7 +140,7 @@ func TestMsgServer_AddService(t *testing.T) { Name: "service 1", ComputeUnitsPerRelay: 0, }, - expectedErr: types.ErrServiceInvalidComputeUnitsPerRelay, + expectedErr: sharedtypes.ErrSharedInvalidComputeUnitsPerRelay, }, { desc: "invalid - no spendable coins", diff --git a/x/service/module/tx_add_service.go b/x/service/module/tx_add_service.go index 4e51bdad5..dc5b6c911 100644 --- a/x/service/module/tx_add_service.go +++ b/x/service/module/tx_add_service.go @@ -13,6 +13,7 @@ import ( "github.com/spf13/cobra" "github.com/pokt-network/poktroll/x/service/types" + sharedtypes "github.com/pokt-network/poktroll/x/shared/types" ) var _ = strconv.Itoa(0) @@ -46,7 +47,7 @@ $ poktrolld tx service add-service "svc1" "service_one" 1 --keyring-backend test if len(args) > 2 { computeUnitsPerRelay, err = strconv.ParseUint(args[2], 10, 64) if err != nil { - return types.ErrServiceInvalidComputeUnitsPerRelay.Wrapf("unable to parse as uint64: %s", args[2]) + return sharedtypes.ErrSharedInvalidComputeUnitsPerRelay.Wrapf("unable to parse as uint64: %s", args[2]) } } else { fmt.Printf("Using default compute_units_per_relay: %d\n", types.DefaultComputeUnitsPerRelay) diff --git a/x/service/types/errors.go b/x/service/types/errors.go index d0c52965a..8c72b9f63 100644 --- a/x/service/types/errors.go +++ b/x/service/types/errors.go @@ -6,18 +6,17 @@ import sdkerrors "cosmossdk.io/errors" // x/service module sentinel errors var ( - ErrServiceInvalidSigner = sdkerrors.Register(ModuleName, 1100, "expected gov account as only signer for proposal message") - ErrServiceDuplicateIndex = sdkerrors.Register(ModuleName, 1101, "duplicate index when adding a new service") - ErrServiceInvalidAddress = sdkerrors.Register(ModuleName, 1102, "invalid address when adding a new service") - ErrServiceMissingID = sdkerrors.Register(ModuleName, 1103, "missing service ID") - ErrServiceMissingName = sdkerrors.Register(ModuleName, 1104, "missing service name") - ErrServiceAlreadyExists = sdkerrors.Register(ModuleName, 1105, "service already exists") - ErrServiceInvalidServiceFee = sdkerrors.Register(ModuleName, 1106, "invalid ServiceFee") - ErrServiceAccountNotFound = sdkerrors.Register(ModuleName, 1107, "account not found") - ErrServiceNotEnoughFunds = sdkerrors.Register(ModuleName, 1108, "not enough funds to add service") - ErrServiceFailedToDeductFee = sdkerrors.Register(ModuleName, 1109, "failed to deduct fee") - ErrServiceInvalidRelayResponse = sdkerrors.Register(ModuleName, 1110, "invalid relay response") - ErrServiceInvalidRelayRequest = sdkerrors.Register(ModuleName, 1111, "invalid relay request") - ErrServiceInvalidComputeUnitsPerRelay = sdkerrors.Register(ModuleName, 1112, "invalid compute units per relay") - ErrServiceInvalidOwnerAddress = sdkerrors.Register(ModuleName, 1113, "invalid owner address") + ErrServiceInvalidSigner = sdkerrors.Register(ModuleName, 1100, "expected gov account as only signer for proposal message") + ErrServiceDuplicateIndex = sdkerrors.Register(ModuleName, 1101, "duplicate index when adding a new service") + ErrServiceInvalidAddress = sdkerrors.Register(ModuleName, 1102, "invalid address when adding a new service") + ErrServiceMissingID = sdkerrors.Register(ModuleName, 1103, "missing service ID") + ErrServiceMissingName = sdkerrors.Register(ModuleName, 1104, "missing service name") + ErrServiceAlreadyExists = sdkerrors.Register(ModuleName, 1105, "service already exists") + ErrServiceInvalidServiceFee = sdkerrors.Register(ModuleName, 1106, "invalid ServiceFee") + ErrServiceAccountNotFound = sdkerrors.Register(ModuleName, 1107, "account not found") + ErrServiceNotEnoughFunds = sdkerrors.Register(ModuleName, 1108, "not enough funds to add service") + ErrServiceFailedToDeductFee = sdkerrors.Register(ModuleName, 1109, "failed to deduct fee") + ErrServiceInvalidRelayResponse = sdkerrors.Register(ModuleName, 1110, "invalid relay response") + ErrServiceInvalidRelayRequest = sdkerrors.Register(ModuleName, 1111, "invalid relay request") + ErrServiceInvalidOwnerAddress = sdkerrors.Register(ModuleName, 1112, "invalid owner address") ) diff --git a/x/service/types/message_add_service.go b/x/service/types/message_add_service.go index 599409519..bdcbe4b1e 100644 --- a/x/service/types/message_add_service.go +++ b/x/service/types/message_add_service.go @@ -8,10 +8,6 @@ import ( const ( DefaultComputeUnitsPerRelay uint64 = 1 - // ComputeUnitsPerRelayMax is the maximum allowed compute_units_per_relay value when adding or updating a service. - // TODO_MAINNET: The reason we have a maximum is to account for potential integer overflows. - // Should we revisit all uint64 and convert them to BigInts? - ComputeUnitsPerRelayMax uint64 = 2 ^ 16 ) var _ sdk.Msg = (*MsgAddService)(nil) @@ -37,25 +33,10 @@ func (msg *MsgAddService) ValidateBasic() error { if msg.Service.OwnerAddress != msg.OwnerAddress { return ErrServiceInvalidOwnerAddress.Wrapf("service owner address %q does not match the signer address %q", msg.Service.OwnerAddress, msg.OwnerAddress) } - // TODO_BETA: Add a validate basic function to the `Service` object - if msg.Service.Id == "" { - return ErrServiceMissingID - } - if msg.Service.Name == "" { - return ErrServiceMissingName - } - if err := ValidateComputeUnitsPerRelay(msg.Service.ComputeUnitsPerRelay); err != nil { + + if err := msg.Service.ValidateBasic(); err != nil { return err } - return nil -} -// ValidateComputeUnitsPerRelay makes sure the compute units per relay is a valid value -func ValidateComputeUnitsPerRelay(computeUnitsPerRelay uint64) error { - if computeUnitsPerRelay == 0 { - return ErrServiceInvalidComputeUnitsPerRelay.Wrap("compute units per relay must be greater than 0") - } else if computeUnitsPerRelay > ComputeUnitsPerRelayMax { - return ErrServiceInvalidComputeUnitsPerRelay.Wrapf("compute units per relay must be less than %d", ComputeUnitsPerRelayMax) - } return nil } diff --git a/x/service/types/message_add_service_test.go b/x/service/types/message_add_service_test.go index fc4a7b107..62bb9cdaf 100644 --- a/x/service/types/message_add_service_test.go +++ b/x/service/types/message_add_service_test.go @@ -23,33 +23,47 @@ func TestMsgAddService_ValidateBasic(t *testing.T) { // Service: intentionally omitted, }, expectedErr: ErrServiceInvalidAddress, - }, { - desc: "valid service owner address - no service ID", + }, + { + desc: "no service ID", msg: MsgAddService{ OwnerAddress: serviceOwnerAddress, - Service: sharedtypes.Service{Name: "service name", OwnerAddress: serviceOwnerAddress}, // ID intentionally omitted + Service: sharedtypes.Service{ + // ID intentionally omitted + Name: "service name", + OwnerAddress: serviceOwnerAddress, + ComputeUnitsPerRelay: 1, + }, }, - expectedErr: ErrServiceMissingID, - }, { - desc: "valid service owner address - no service name", + expectedErr: sharedtypes.ErrSharedInvalidService.Wrapf("invalid service ID: %s", ""), + }, + { + desc: "no service name", msg: MsgAddService{ OwnerAddress: serviceOwnerAddress, - Service: sharedtypes.Service{Id: "svc1", OwnerAddress: serviceOwnerAddress}, // Name intentionally omitted + Service: sharedtypes.Service{ + Id: "svc1", + // Name intentionally omitted + OwnerAddress: serviceOwnerAddress, + ComputeUnitsPerRelay: 1, + }, }, - expectedErr: ErrServiceMissingName, - }, { - desc: "valid service owner address - zero compute units per relay", + expectedErr: nil, + }, + { + desc: "invalid service name", msg: MsgAddService{ OwnerAddress: serviceOwnerAddress, Service: sharedtypes.Service{ Id: "svc1", - Name: "service name", - ComputeUnitsPerRelay: 0, + Name: "service&name", OwnerAddress: serviceOwnerAddress, + ComputeUnitsPerRelay: 1, }, }, - expectedErr: ErrServiceInvalidComputeUnitsPerRelay, - }, { + expectedErr: sharedtypes.ErrSharedInvalidService.Wrapf("invalid service name: %s", "service&name"), + }, + { desc: "signer address does not equal service owner address", msg: MsgAddService{ OwnerAddress: serviceOwnerAddress, @@ -63,7 +77,35 @@ func TestMsgAddService_ValidateBasic(t *testing.T) { expectedErr: ErrServiceInvalidOwnerAddress, }, { - desc: "valid msg add service", + desc: "zero compute units per relay", + msg: MsgAddService{ + OwnerAddress: serviceOwnerAddress, + Service: sharedtypes.Service{ + Id: "svc1", + Name: "service name", + ComputeUnitsPerRelay: 0, + OwnerAddress: serviceOwnerAddress, + }, + }, + expectedErr: sharedtypes.ErrSharedInvalidService. + Wrapf("%s", sharedtypes.ErrSharedInvalidComputeUnitsPerRelay), + }, + { + desc: "compute units per relay greater than max", + msg: MsgAddService{ + OwnerAddress: serviceOwnerAddress, + Service: sharedtypes.Service{ + Id: "svc1", + Name: "service name", + ComputeUnitsPerRelay: sharedtypes.ComputeUnitsPerRelayMax + 1, + OwnerAddress: serviceOwnerAddress, + }, + }, + expectedErr: sharedtypes.ErrSharedInvalidService. + Wrapf("%s", sharedtypes.ErrSharedInvalidComputeUnitsPerRelay), + }, + { + desc: "min compute units per relay", msg: MsgAddService{ OwnerAddress: serviceOwnerAddress, Service: sharedtypes.Service{ @@ -75,48 +117,26 @@ func TestMsgAddService_ValidateBasic(t *testing.T) { }, expectedErr: nil, }, - } - for _, test := range tests { - t.Run(test.desc, func(t *testing.T) { - err := test.msg.ValidateBasic() - if test.expectedErr != nil { - require.ErrorIs(t, err, test.expectedErr) - return - } - require.NoError(t, err) - }) - } -} - -func TestValidateComputeUnitsPerRelay(t *testing.T) { - tests := []struct { - desc string - computeUnitsPerRelay uint64 - expectedErr error - }{ { - desc: "zero compute units per relay", - computeUnitsPerRelay: 0, - expectedErr: ErrServiceInvalidComputeUnitsPerRelay, - }, { - desc: "valid compute units per relay", - computeUnitsPerRelay: 1, - expectedErr: nil, - }, { - desc: "max compute units per relay", - computeUnitsPerRelay: ComputeUnitsPerRelayMax, - expectedErr: nil, - }, { - desc: "compute units per relay greater than max", - computeUnitsPerRelay: ComputeUnitsPerRelayMax + 1, - expectedErr: ErrServiceInvalidComputeUnitsPerRelay, + desc: "max compute units per relay", + msg: MsgAddService{ + OwnerAddress: serviceOwnerAddress, + Service: sharedtypes.Service{ + Id: "svc1", + Name: "service name", + ComputeUnitsPerRelay: sharedtypes.ComputeUnitsPerRelayMax, + OwnerAddress: serviceOwnerAddress, + }, + }, + expectedErr: nil, }, } for _, test := range tests { t.Run(test.desc, func(t *testing.T) { - err := ValidateComputeUnitsPerRelay(test.computeUnitsPerRelay) + err := test.msg.ValidateBasic() if test.expectedErr != nil { require.ErrorIs(t, err, test.expectedErr) + require.ErrorContains(t, err, test.expectedErr.Error()) return } require.NoError(t, err) diff --git a/x/shared/types/errors.go b/x/shared/types/errors.go index 0ef96d4f5..576ccd7dd 100644 --- a/x/shared/types/errors.go +++ b/x/shared/types/errors.go @@ -8,12 +8,13 @@ import ( // x/shared module sentinel errors var ( - ErrSharedInvalidSigner = sdkerrors.Register(ModuleName, 1100, "expected gov account as only signer for proposal message") - ErrSharedInvalidAddress = sdkerrors.Register(ModuleName, 1101, "invalid address") - ErrSharedParamNameInvalid = sdkerrors.Register(ModuleName, 1102, "the provided param name is invalid") - ErrSharedParamInvalid = sdkerrors.Register(ModuleName, 1103, "the provided param is invalid") - ErrSharedEmitEvent = sdkerrors.Register(ModuleName, 1104, "failed to emit event") - ErrSharedUnauthorizedSupplierUpdate = sdkerrors.Register(ModuleName, 1105, "unauthorized supplier update") - ErrSharedInvalidRevShare = sdkerrors.Register(ModuleName, 1106, "invalid revenue share configuration") - ErrSharedInvalidService = sdkerrors.Register(ModuleName, 1107, "invalid service") + ErrSharedInvalidSigner = sdkerrors.Register(ModuleName, 1100, "expected gov account as only signer for proposal message") + ErrSharedInvalidAddress = sdkerrors.Register(ModuleName, 1101, "invalid address") + ErrSharedParamNameInvalid = sdkerrors.Register(ModuleName, 1102, "the provided param name is invalid") + ErrSharedParamInvalid = sdkerrors.Register(ModuleName, 1103, "the provided param is invalid") + ErrSharedEmitEvent = sdkerrors.Register(ModuleName, 1104, "failed to emit event") + ErrSharedUnauthorizedSupplierUpdate = sdkerrors.Register(ModuleName, 1105, "unauthorized supplier update") + ErrSharedInvalidRevShare = sdkerrors.Register(ModuleName, 1106, "invalid revenue share configuration") + ErrSharedInvalidService = sdkerrors.Register(ModuleName, 1107, "invalid service") + ErrSharedInvalidComputeUnitsPerRelay = sdkerrors.Register(ModuleName, 1108, "invalid compute units per relay") ) diff --git a/x/shared/types/service.go b/x/shared/types/service.go index f5db0f0ed..3a62a0093 100644 --- a/x/shared/types/service.go +++ b/x/shared/types/service.go @@ -3,9 +3,16 @@ package types import ( "net/url" "regexp" + + sdk "github.com/cosmos/cosmos-sdk/types" ) const ( + // ComputeUnitsPerRelayMax is the maximum allowed compute_units_per_relay value when adding or updating a service. + // TODO_MAINNET: The reason we have a maximum is to account for potential integer overflows. + // Should we revisit all uint64 and convert them to BigInts? + ComputeUnitsPerRelayMax uint64 = 2 ^ 16 + maxServiceIdLength = 16 // Limiting all serviceIds to 16 characters maxServiceIdName = 42 // Limit the name of the service name to 42 characters @@ -34,6 +41,14 @@ func (s *Service) ValidateBasic() error { return ErrSharedInvalidService.Wrapf("invalid service name: %s", s.Name) } + if _, err := sdk.AccAddressFromBech32(s.OwnerAddress); err != nil { + return ErrSharedInvalidService.Wrapf("invalid owner address: %s", s.OwnerAddress) + } + + if err := ValidateComputeUnitsPerRelay(s.ComputeUnitsPerRelay); err != nil { + return ErrSharedInvalidService.Wrapf("%s", err) + } + return nil } @@ -86,3 +101,13 @@ func IsValidEndpointUrl(endpoint string) bool { return true } + +// ValidateComputeUnitsPerRelay makes sure the compute units per relay is a valid value +func ValidateComputeUnitsPerRelay(computeUnitsPerRelay uint64) error { + if computeUnitsPerRelay == 0 { + return ErrSharedInvalidComputeUnitsPerRelay.Wrap("compute units per relay must be greater than 0") + } else if computeUnitsPerRelay > ComputeUnitsPerRelayMax { + return ErrSharedInvalidComputeUnitsPerRelay.Wrapf("compute units per relay must be less than %d", ComputeUnitsPerRelayMax) + } + return nil +} From 281edd557779225c287819b41d1d3dd94916b1c9 Mon Sep 17 00:00:00 2001 From: Redouane Lakrache Date: Sat, 7 Sep 2024 02:34:02 +0200 Subject: [PATCH 5/7] fix: Revert service validate basic --- .../keeper/msg_server_add_service_test.go | 2 +- x/service/module/tx_add_service.go | 3 +- x/service/types/message_add_service.go | 25 +++- x/service/types/message_add_service_test.go | 120 ++++++++---------- x/shared/types/errors.go | 17 ++- x/shared/types/service.go | 32 ++--- 6 files changed, 92 insertions(+), 107 deletions(-) diff --git a/x/service/keeper/msg_server_add_service_test.go b/x/service/keeper/msg_server_add_service_test.go index 1e5ea62d3..1f587a931 100644 --- a/x/service/keeper/msg_server_add_service_test.go +++ b/x/service/keeper/msg_server_add_service_test.go @@ -141,7 +141,7 @@ func TestMsgServer_AddService(t *testing.T) { Name: "service 1", ComputeUnitsPerRelay: 0, }, - expectedErr: sharedtypes.ErrSharedInvalidComputeUnitsPerRelay, + expectedErr: types.ErrServiceInvalidComputeUnitsPerRelay, }, { desc: "invalid - no spendable coins", diff --git a/x/service/module/tx_add_service.go b/x/service/module/tx_add_service.go index dc5b6c911..4e51bdad5 100644 --- a/x/service/module/tx_add_service.go +++ b/x/service/module/tx_add_service.go @@ -13,7 +13,6 @@ import ( "github.com/spf13/cobra" "github.com/pokt-network/poktroll/x/service/types" - sharedtypes "github.com/pokt-network/poktroll/x/shared/types" ) var _ = strconv.Itoa(0) @@ -47,7 +46,7 @@ $ poktrolld tx service add-service "svc1" "service_one" 1 --keyring-backend test if len(args) > 2 { computeUnitsPerRelay, err = strconv.ParseUint(args[2], 10, 64) if err != nil { - return sharedtypes.ErrSharedInvalidComputeUnitsPerRelay.Wrapf("unable to parse as uint64: %s", args[2]) + return types.ErrServiceInvalidComputeUnitsPerRelay.Wrapf("unable to parse as uint64: %s", args[2]) } } else { fmt.Printf("Using default compute_units_per_relay: %d\n", types.DefaultComputeUnitsPerRelay) diff --git a/x/service/types/message_add_service.go b/x/service/types/message_add_service.go index bdcbe4b1e..289e95ba5 100644 --- a/x/service/types/message_add_service.go +++ b/x/service/types/message_add_service.go @@ -8,6 +8,10 @@ import ( const ( DefaultComputeUnitsPerRelay uint64 = 1 + // ComputeUnitsPerRelayMax is the maximum allowed compute_units_per_relay value when adding or updating a service. + // TODO_MAINNET: The reason we have a maximum is to account for potential integer overflows. + // Should we revisit all uint64 and convert them to BigInts? + ComputeUnitsPerRelayMax uint64 = 2 ^ 16 ) var _ sdk.Msg = (*MsgAddService)(nil) @@ -33,10 +37,27 @@ func (msg *MsgAddService) ValidateBasic() error { if msg.Service.OwnerAddress != msg.OwnerAddress { return ErrServiceInvalidOwnerAddress.Wrapf("service owner address %q does not match the signer address %q", msg.Service.OwnerAddress, msg.OwnerAddress) } - - if err := msg.Service.ValidateBasic(); err != nil { + // TODO_BETA: Add a validate basic function to the `Service` object + if msg.Service.Id == "" { + return ErrServiceMissingID + } + if msg.Service.Name == "" { + return ErrServiceMissingName + } + if err := ValidateComputeUnitsPerRelay(msg.Service.ComputeUnitsPerRelay); err != nil { return err } return nil } + +// ValidateComputeUnitsPerRelay makes sure the compute units per relay is a valid value +func ValidateComputeUnitsPerRelay(computeUnitsPerRelay uint64) error { + if computeUnitsPerRelay == 0 { + return ErrServiceInvalidComputeUnitsPerRelay.Wrap("compute units per relay must be greater than 0") + } else if computeUnitsPerRelay > ComputeUnitsPerRelayMax { + return ErrServiceInvalidComputeUnitsPerRelay.Wrapf("compute units per relay must be less than %d", ComputeUnitsPerRelayMax) + } + + return nil +} diff --git a/x/service/types/message_add_service_test.go b/x/service/types/message_add_service_test.go index 62bb9cdaf..fc4a7b107 100644 --- a/x/service/types/message_add_service_test.go +++ b/x/service/types/message_add_service_test.go @@ -23,47 +23,33 @@ func TestMsgAddService_ValidateBasic(t *testing.T) { // Service: intentionally omitted, }, expectedErr: ErrServiceInvalidAddress, - }, - { - desc: "no service ID", + }, { + desc: "valid service owner address - no service ID", msg: MsgAddService{ OwnerAddress: serviceOwnerAddress, - Service: sharedtypes.Service{ - // ID intentionally omitted - Name: "service name", - OwnerAddress: serviceOwnerAddress, - ComputeUnitsPerRelay: 1, - }, + Service: sharedtypes.Service{Name: "service name", OwnerAddress: serviceOwnerAddress}, // ID intentionally omitted }, - expectedErr: sharedtypes.ErrSharedInvalidService.Wrapf("invalid service ID: %s", ""), - }, - { - desc: "no service name", + expectedErr: ErrServiceMissingID, + }, { + desc: "valid service owner address - no service name", msg: MsgAddService{ OwnerAddress: serviceOwnerAddress, - Service: sharedtypes.Service{ - Id: "svc1", - // Name intentionally omitted - OwnerAddress: serviceOwnerAddress, - ComputeUnitsPerRelay: 1, - }, + Service: sharedtypes.Service{Id: "svc1", OwnerAddress: serviceOwnerAddress}, // Name intentionally omitted }, - expectedErr: nil, - }, - { - desc: "invalid service name", + expectedErr: ErrServiceMissingName, + }, { + desc: "valid service owner address - zero compute units per relay", msg: MsgAddService{ OwnerAddress: serviceOwnerAddress, Service: sharedtypes.Service{ Id: "svc1", - Name: "service&name", + Name: "service name", + ComputeUnitsPerRelay: 0, OwnerAddress: serviceOwnerAddress, - ComputeUnitsPerRelay: 1, }, }, - expectedErr: sharedtypes.ErrSharedInvalidService.Wrapf("invalid service name: %s", "service&name"), - }, - { + expectedErr: ErrServiceInvalidComputeUnitsPerRelay, + }, { desc: "signer address does not equal service owner address", msg: MsgAddService{ OwnerAddress: serviceOwnerAddress, @@ -77,35 +63,7 @@ func TestMsgAddService_ValidateBasic(t *testing.T) { expectedErr: ErrServiceInvalidOwnerAddress, }, { - desc: "zero compute units per relay", - msg: MsgAddService{ - OwnerAddress: serviceOwnerAddress, - Service: sharedtypes.Service{ - Id: "svc1", - Name: "service name", - ComputeUnitsPerRelay: 0, - OwnerAddress: serviceOwnerAddress, - }, - }, - expectedErr: sharedtypes.ErrSharedInvalidService. - Wrapf("%s", sharedtypes.ErrSharedInvalidComputeUnitsPerRelay), - }, - { - desc: "compute units per relay greater than max", - msg: MsgAddService{ - OwnerAddress: serviceOwnerAddress, - Service: sharedtypes.Service{ - Id: "svc1", - Name: "service name", - ComputeUnitsPerRelay: sharedtypes.ComputeUnitsPerRelayMax + 1, - OwnerAddress: serviceOwnerAddress, - }, - }, - expectedErr: sharedtypes.ErrSharedInvalidService. - Wrapf("%s", sharedtypes.ErrSharedInvalidComputeUnitsPerRelay), - }, - { - desc: "min compute units per relay", + desc: "valid msg add service", msg: MsgAddService{ OwnerAddress: serviceOwnerAddress, Service: sharedtypes.Service{ @@ -117,26 +75,48 @@ func TestMsgAddService_ValidateBasic(t *testing.T) { }, expectedErr: nil, }, + } + for _, test := range tests { + t.Run(test.desc, func(t *testing.T) { + err := test.msg.ValidateBasic() + if test.expectedErr != nil { + require.ErrorIs(t, err, test.expectedErr) + return + } + require.NoError(t, err) + }) + } +} + +func TestValidateComputeUnitsPerRelay(t *testing.T) { + tests := []struct { + desc string + computeUnitsPerRelay uint64 + expectedErr error + }{ { - desc: "max compute units per relay", - msg: MsgAddService{ - OwnerAddress: serviceOwnerAddress, - Service: sharedtypes.Service{ - Id: "svc1", - Name: "service name", - ComputeUnitsPerRelay: sharedtypes.ComputeUnitsPerRelayMax, - OwnerAddress: serviceOwnerAddress, - }, - }, - expectedErr: nil, + desc: "zero compute units per relay", + computeUnitsPerRelay: 0, + expectedErr: ErrServiceInvalidComputeUnitsPerRelay, + }, { + desc: "valid compute units per relay", + computeUnitsPerRelay: 1, + expectedErr: nil, + }, { + desc: "max compute units per relay", + computeUnitsPerRelay: ComputeUnitsPerRelayMax, + expectedErr: nil, + }, { + desc: "compute units per relay greater than max", + computeUnitsPerRelay: ComputeUnitsPerRelayMax + 1, + expectedErr: ErrServiceInvalidComputeUnitsPerRelay, }, } for _, test := range tests { t.Run(test.desc, func(t *testing.T) { - err := test.msg.ValidateBasic() + err := ValidateComputeUnitsPerRelay(test.computeUnitsPerRelay) if test.expectedErr != nil { require.ErrorIs(t, err, test.expectedErr) - require.ErrorContains(t, err, test.expectedErr.Error()) return } require.NoError(t, err) diff --git a/x/shared/types/errors.go b/x/shared/types/errors.go index 576ccd7dd..0ef96d4f5 100644 --- a/x/shared/types/errors.go +++ b/x/shared/types/errors.go @@ -8,13 +8,12 @@ import ( // x/shared module sentinel errors var ( - ErrSharedInvalidSigner = sdkerrors.Register(ModuleName, 1100, "expected gov account as only signer for proposal message") - ErrSharedInvalidAddress = sdkerrors.Register(ModuleName, 1101, "invalid address") - ErrSharedParamNameInvalid = sdkerrors.Register(ModuleName, 1102, "the provided param name is invalid") - ErrSharedParamInvalid = sdkerrors.Register(ModuleName, 1103, "the provided param is invalid") - ErrSharedEmitEvent = sdkerrors.Register(ModuleName, 1104, "failed to emit event") - ErrSharedUnauthorizedSupplierUpdate = sdkerrors.Register(ModuleName, 1105, "unauthorized supplier update") - ErrSharedInvalidRevShare = sdkerrors.Register(ModuleName, 1106, "invalid revenue share configuration") - ErrSharedInvalidService = sdkerrors.Register(ModuleName, 1107, "invalid service") - ErrSharedInvalidComputeUnitsPerRelay = sdkerrors.Register(ModuleName, 1108, "invalid compute units per relay") + ErrSharedInvalidSigner = sdkerrors.Register(ModuleName, 1100, "expected gov account as only signer for proposal message") + ErrSharedInvalidAddress = sdkerrors.Register(ModuleName, 1101, "invalid address") + ErrSharedParamNameInvalid = sdkerrors.Register(ModuleName, 1102, "the provided param name is invalid") + ErrSharedParamInvalid = sdkerrors.Register(ModuleName, 1103, "the provided param is invalid") + ErrSharedEmitEvent = sdkerrors.Register(ModuleName, 1104, "failed to emit event") + ErrSharedUnauthorizedSupplierUpdate = sdkerrors.Register(ModuleName, 1105, "unauthorized supplier update") + ErrSharedInvalidRevShare = sdkerrors.Register(ModuleName, 1106, "invalid revenue share configuration") + ErrSharedInvalidService = sdkerrors.Register(ModuleName, 1107, "invalid service") ) diff --git a/x/shared/types/service.go b/x/shared/types/service.go index 3a62a0093..be25d92d6 100644 --- a/x/shared/types/service.go +++ b/x/shared/types/service.go @@ -3,16 +3,9 @@ package types import ( "net/url" "regexp" - - sdk "github.com/cosmos/cosmos-sdk/types" ) const ( - // ComputeUnitsPerRelayMax is the maximum allowed compute_units_per_relay value when adding or updating a service. - // TODO_MAINNET: The reason we have a maximum is to account for potential integer overflows. - // Should we revisit all uint64 and convert them to BigInts? - ComputeUnitsPerRelayMax uint64 = 2 ^ 16 - maxServiceIdLength = 16 // Limiting all serviceIds to 16 characters maxServiceIdName = 42 // Limit the name of the service name to 42 characters @@ -41,13 +34,16 @@ func (s *Service) ValidateBasic() error { return ErrSharedInvalidService.Wrapf("invalid service name: %s", s.Name) } - if _, err := sdk.AccAddressFromBech32(s.OwnerAddress); err != nil { - return ErrSharedInvalidService.Wrapf("invalid owner address: %s", s.OwnerAddress) - } + // TODO_FOLLOWUP: Enabling these checks will break code where Service is created + // without OwnerAddress or ComputeUnitsPerRelay. + // Remove the comments in a follow-up PR where embedded Service is replaced by ServiceId. + //if _, err := sdk.AccAddressFromBech32(s.OwnerAddress); err != nil { + // return ErrSharedInvalidService.Wrapf("invalid owner address: %s", s.OwnerAddress) + //} - if err := ValidateComputeUnitsPerRelay(s.ComputeUnitsPerRelay); err != nil { - return ErrSharedInvalidService.Wrapf("%s", err) - } + //if err := ValidateComputeUnitsPerRelay(s.ComputeUnitsPerRelay); err != nil { + // return ErrSharedInvalidService.Wrapf("%s", err) + //} return nil } @@ -101,13 +97,3 @@ func IsValidEndpointUrl(endpoint string) bool { return true } - -// ValidateComputeUnitsPerRelay makes sure the compute units per relay is a valid value -func ValidateComputeUnitsPerRelay(computeUnitsPerRelay uint64) error { - if computeUnitsPerRelay == 0 { - return ErrSharedInvalidComputeUnitsPerRelay.Wrap("compute units per relay must be greater than 0") - } else if computeUnitsPerRelay > ComputeUnitsPerRelayMax { - return ErrSharedInvalidComputeUnitsPerRelay.Wrapf("compute units per relay must be less than %d", ComputeUnitsPerRelayMax) - } - return nil -} From 0a7f927647d30ab53d6a129c9ff15f1890f78e2e Mon Sep 17 00:00:00 2001 From: Redouane Lakrache Date: Mon, 9 Sep 2024 16:49:15 +0200 Subject: [PATCH 6/7] chore: Address review change requests --- x/proof/keeper/msg_server_submit_proof_test.go | 7 ++----- x/proof/types/message_submit_proof.go | 2 +- x/proof/types/message_submit_proof_test.go | 10 +++++----- x/session/keeper/session_hydrator.go | 2 +- x/session/types/query_get_session_request.go | 4 ++-- x/session/types/session_header.go | 8 ++++---- x/shared/helpers/service_configs.go | 4 ++-- x/shared/types/service.go | 4 ++-- 8 files changed, 19 insertions(+), 22 deletions(-) diff --git a/x/proof/keeper/msg_server_submit_proof_test.go b/x/proof/keeper/msg_server_submit_proof_test.go index abcda25a0..7827a701e 100644 --- a/x/proof/keeper/msg_server_submit_proof_test.go +++ b/x/proof/keeper/msg_server_submit_proof_test.go @@ -570,15 +570,12 @@ func TestMsgServer_SubmitProof_Error(t *testing.T) { }, msgSubmitProofToExpectedErrorFn: func(msgSubmitProof *prooftypes.MsgSubmitProof) error { sessionError := sessiontypes.ErrSessionInvalidSessionId.Wrapf( - "invalid session ID: %s", + "%q", msgSubmitProof.GetSessionHeader().GetSessionId(), ) return status.Error( codes.InvalidArgument, - prooftypes.ErrProofInvalidSessionHeader.Wrapf( - "invalid session header: %s", - sessionError, - ).Error(), + prooftypes.ErrProofInvalidSessionHeader.Wrapf("%s", sessionError).Error(), ) }, }, diff --git a/x/proof/types/message_submit_proof.go b/x/proof/types/message_submit_proof.go index b4d6b312b..154d3b15f 100644 --- a/x/proof/types/message_submit_proof.go +++ b/x/proof/types/message_submit_proof.go @@ -34,7 +34,7 @@ func (msg *MsgSubmitProof) ValidateBasic() error { } if err := sessionHeader.ValidateBasic(); err != nil { - return ErrProofInvalidSessionHeader.Wrapf("invalid session header: %s", err) + return ErrProofInvalidSessionHeader.Wrapf("%s", err) } if len(msg.GetProof()) == 0 { diff --git a/x/proof/types/message_submit_proof_test.go b/x/proof/types/message_submit_proof_test.go index 3404ff0c0..972576b85 100644 --- a/x/proof/types/message_submit_proof_test.go +++ b/x/proof/types/message_submit_proof_test.go @@ -36,11 +36,11 @@ func TestMsgSubmitProof_ValidateBasic(t *testing.T) { }, sessionHeaderToExpectedErrorFn: func(sh sessiontypes.SessionHeader) error { sessionError := sessiontypes.ErrSessionInvalidAppAddress.Wrapf( - "invalid application address: %s; (%s)", + "%q; (%s)", sh.ApplicationAddress, "decoding bech32 failed: invalid separator index -1", ) - return ErrProofInvalidSessionHeader.Wrapf("invalid session header: %s", sessionError) + return ErrProofInvalidSessionHeader.Wrapf("%s", sessionError) }, }, { @@ -78,9 +78,9 @@ func TestMsgSubmitProof_ValidateBasic(t *testing.T) { Proof: testClosestMerkleProof, }, sessionHeaderToExpectedErrorFn: func(sh sessiontypes.SessionHeader) error { - serviceError := sharedtypes.ErrSharedInvalidService.Wrapf("invalid service ID: %s", sh.Service.Id) - sessionError := sessiontypes.ErrSessionInvalidService.Wrapf("invalid service: %v; %s", sh.Service, serviceError) - return ErrProofInvalidSessionHeader.Wrapf("invalid session header: %s", sessionError) + serviceError := sharedtypes.ErrSharedInvalidService.Wrapf("ID: %q", sh.Service.Id) + sessionError := sessiontypes.ErrSessionInvalidService.Wrapf("%s", serviceError) + return ErrProofInvalidSessionHeader.Wrapf("%s", sessionError) }, }, { diff --git a/x/session/keeper/session_hydrator.go b/x/session/keeper/session_hydrator.go index 2d1ce934c..05f33ac15 100644 --- a/x/session/keeper/session_hydrator.go +++ b/x/session/keeper/session_hydrator.go @@ -118,7 +118,7 @@ func (k Keeper) hydrateSessionID(ctx context.Context, sh *sessionHydrator) error // a valid service depending on whether or not its permissioned or permissionless if err := sh.sessionHeader.Service.ValidateBasic(); err != nil { - return types.ErrSessionHydration.Wrapf("invalid service: %v; err %s", sh.sessionHeader.Service, err) + return types.ErrSessionHydration.Wrapf("%s", err) } sh.sessionHeader.SessionId, sh.sessionIDBz = k.GetSessionId( diff --git a/x/session/types/query_get_session_request.go b/x/session/types/query_get_session_request.go index 9addcac33..da610ddfe 100644 --- a/x/session/types/query_get_session_request.go +++ b/x/session/types/query_get_session_request.go @@ -22,12 +22,12 @@ func NewQueryGetSessionRequest(appAddress, serviceId string, blockHeight int64) func (query *QueryGetSessionRequest) ValidateBasic() error { // Validate the application address if _, err := sdk.AccAddressFromBech32(query.ApplicationAddress); err != nil { - return ErrSessionInvalidAppAddress.Wrapf("invalid app address for session being retrieved %s; (%v)", query.ApplicationAddress, err) + return ErrSessionInvalidAppAddress.Wrapf("%s", err) } // Validate the Service ID if err := query.Service.ValidateBasic(); err != nil { - return ErrSessionInvalidService.Wrapf("invalid service for session being retrieved %s; %v", query.Service, err) + return ErrSessionInvalidService.Wrapf("invalid service for session being retrieved %s; %s", query.Service, err) } // Validate the height for which a session is being retrieved diff --git a/x/session/types/session_header.go b/x/session/types/session_header.go index d5f715e71..e0f842492 100644 --- a/x/session/types/session_header.go +++ b/x/session/types/session_header.go @@ -8,21 +8,21 @@ import ( func (sh *SessionHeader) ValidateBasic() error { // Validate the application address if _, err := sdk.AccAddressFromBech32(sh.ApplicationAddress); err != nil { - return ErrSessionInvalidAppAddress.Wrapf("invalid application address: %s; (%s)", sh.ApplicationAddress, err) + return ErrSessionInvalidAppAddress.Wrapf("%q; (%s)", sh.ApplicationAddress, err) } // Validate the session ID if len(sh.SessionId) == 0 { - return ErrSessionInvalidSessionId.Wrapf("invalid session ID: %s", sh.SessionId) + return ErrSessionInvalidSessionId.Wrapf("%q", sh.SessionId) } // Validate the service if sh.Service == nil { - return ErrSessionInvalidService.Wrapf("missing service: %s", sh.Service) + return ErrSessionInvalidService.Wrapf("missing service") } if err := sh.Service.ValidateBasic(); err != nil { - return ErrSessionInvalidService.Wrapf("invalid service: %v; %s", sh.Service, err) + return ErrSessionInvalidService.Wrapf("%s", err) } // Sessions can only start at height 1 diff --git a/x/shared/helpers/service_configs.go b/x/shared/helpers/service_configs.go index a0be741bc..6d8dfe9bf 100644 --- a/x/shared/helpers/service_configs.go +++ b/x/shared/helpers/service_configs.go @@ -23,7 +23,7 @@ func ValidateAppServiceConfigs(services []*sharedtypes.ApplicationServiceConfig) } // Check the Service if err := serviceConfig.Service.ValidateBasic(); err != nil { - return sharedtypes.ErrSharedInvalidService.Wrapf("invalid service: %v; %s", serviceConfig.Service, err) + return sharedtypes.ErrSharedInvalidService.Wrapf("%s", err) } } return nil @@ -41,7 +41,7 @@ func ValidateSupplierServiceConfigs(services []*sharedtypes.SupplierServiceConfi // Check the Service if err := serviceConfig.Service.ValidateBasic(); err != nil { - return sharedtypes.ErrSharedInvalidService.Wrapf("invalid service: %v; %s", serviceConfig.Service, err) + return sharedtypes.ErrSharedInvalidService.Wrapf("%s", err) } // Check the Endpoints diff --git a/x/shared/types/service.go b/x/shared/types/service.go index be25d92d6..46e1081e0 100644 --- a/x/shared/types/service.go +++ b/x/shared/types/service.go @@ -27,11 +27,11 @@ func init() { // ValidateBasic performs basic stateless validation of a Service. func (s *Service) ValidateBasic() error { if !IsValidServiceId(s.Id) { - return ErrSharedInvalidService.Wrapf("invalid service ID: %s", s.Id) + return ErrSharedInvalidService.Wrapf("ID: %q", s.Id) } if !IsValidServiceName(s.Name) { - return ErrSharedInvalidService.Wrapf("invalid service name: %s", s.Name) + return ErrSharedInvalidService.Wrapf("name: %q", s.Name) } // TODO_FOLLOWUP: Enabling these checks will break code where Service is created From 6ba8e4141af0c3b2b34d6ba9949973e72ecfd80b Mon Sep 17 00:00:00 2001 From: Redouane Lakrache Date: Mon, 9 Sep 2024 17:38:03 +0200 Subject: [PATCH 7/7] Empty commit