From 7728ac8ccabf9efeecc16de96a4b657917861049 Mon Sep 17 00:00:00 2001 From: Bryan White Date: Mon, 25 Nov 2024 14:27:39 +0100 Subject: [PATCH 1/2] fix: outstanding service module non-status (gRPC) errors --- x/service/keeper/msg_server_add_service.go | 58 ++++++++++++------- x/service/keeper/msg_update_params.go | 18 +++++- .../keeper/query_relay_mining_difficulty.go | 6 +- x/service/keeper/query_service.go | 6 +- 4 files changed, 62 insertions(+), 26 deletions(-) diff --git a/x/service/keeper/msg_server_add_service.go b/x/service/keeper/msg_server_add_service.go index 52fe8365e..2d3dd69c0 100644 --- a/x/service/keeper/msg_server_add_service.go +++ b/x/service/keeper/msg_server_add_service.go @@ -5,6 +5,8 @@ import ( "fmt" sdk "github.com/cosmos/cosmos-sdk/types" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" "github.com/pokt-network/poktroll/telemetry" "github.com/pokt-network/poktroll/x/service/types" @@ -33,39 +35,48 @@ func (k msgServer) AddService( // Validate the message. if err := msg.ValidateBasic(); err != nil { logger.Error(fmt.Sprintf("Adding service failed basic validation: %v", err)) - return nil, err + return nil, status.Error(codes.InvalidArgument, err.Error()) } // Check if the service already exists or not. foundService, found := k.GetService(ctx, msg.Service.Id) if found { if foundService.OwnerAddress != msg.Service.OwnerAddress { - logger.Error(fmt.Sprintf("Owner address of existing service (%q) does not match the owner address %q", foundService.OwnerAddress, msg.OwnerAddress)) - return nil, types.ErrServiceInvalidOwnerAddress.Wrapf( - "existing owner address %q does not match the new owner address %q", - foundService.OwnerAddress, msg.Service.OwnerAddress, + return nil, status.Error( + codes.InvalidArgument, + types.ErrServiceInvalidOwnerAddress.Wrapf( + "existing owner address %q does not match the new owner address %q", + foundService.OwnerAddress, msg.Service.OwnerAddress, + ).Error(), ) } - return nil, types.ErrServiceAlreadyExists.Wrapf( - "TODO_MAINNET(@red-0ne): This is an ephemeral state of the code. Once we s/AddService/UpdateService/g, add the business logic here for updates here.", + return nil, status.Error( + codes.FailedPrecondition, + types.ErrServiceAlreadyExists.Wrapf( + "TODO_MAINNET(@red-0ne): This is an ephemeral state of the code. Once we s/AddService/UpdateService/g, add the business logic here for updates here.", + ).Error(), ) } // Retrieve the address of the actor adding the service; the owner of the service. serviceOwnerAddr, err := sdk.AccAddressFromBech32(msg.OwnerAddress) if err != nil { - logger.Error(fmt.Sprintf("could not parse address %s", msg.OwnerAddress)) - return nil, types.ErrServiceInvalidAddress.Wrapf( - "%s is not in Bech32 format", msg.OwnerAddress, + return nil, status.Error( + codes.InvalidArgument, + types.ErrServiceInvalidAddress.Wrapf( + "%s is not in Bech32 format", msg.OwnerAddress, + ).Error(), ) } // Check the actor has sufficient funds to pay for the add service fee. accCoins := k.bankKeeper.SpendableCoins(ctx, serviceOwnerAddr) if accCoins.Len() == 0 { - logger.Error(fmt.Sprintf("%s doesn't have any funds to add service: %v", serviceOwnerAddr, err)) - return nil, types.ErrServiceNotEnoughFunds.Wrapf( - "account has no spendable coins", + return nil, status.Error( + codes.FailedPrecondition, + types.ErrServiceNotEnoughFunds.Wrapf( + "account has no spendable coins", + ).Error(), ) } @@ -73,10 +84,12 @@ func (k msgServer) AddService( accBalance := accCoins.AmountOf("upokt") addServiceFee := k.GetParams(ctx).AddServiceFee if accBalance.LTE(addServiceFee.Amount) { - logger.Error(fmt.Sprintf("%s doesn't have enough funds to add service: %v", serviceOwnerAddr, err)) - return nil, types.ErrServiceNotEnoughFunds.Wrapf( - "account has %s, but the service fee is %s", - accBalance, k.GetParams(ctx).AddServiceFee, + return nil, status.Error( + codes.FailedPrecondition, + types.ErrServiceNotEnoughFunds.Wrapf( + "account has %s, but the service fee is %s", + accBalance, k.GetParams(ctx).AddServiceFee, + ).Error(), ) } @@ -84,10 +97,13 @@ func (k msgServer) AddService( serviceFee := sdk.NewCoins(*addServiceFee) err = k.bankKeeper.SendCoinsFromAccountToModule(ctx, serviceOwnerAddr, types.ModuleName, serviceFee) if err != nil { - logger.Error(fmt.Sprintf("Failed to deduct service fee from actor's balance: %v", err)) - return nil, types.ErrServiceFailedToDeductFee.Wrapf( - "account has %s, failed to deduct %s", - accBalance, k.GetParams(ctx).AddServiceFee, + logger.Error(fmt.Sprintf("Failed to deduct service fee from actor's balance: %+v", err)) + return nil, status.Error( + codes.Internal, + types.ErrServiceFailedToDeductFee.Wrapf( + "account has %s, failed to deduct %s", + accBalance, k.GetParams(ctx).AddServiceFee, + ).Error(), ) } diff --git a/x/service/keeper/msg_update_params.go b/x/service/keeper/msg_update_params.go index 964d1db26..e4222f390 100644 --- a/x/service/keeper/msg_update_params.go +++ b/x/service/keeper/msg_update_params.go @@ -2,20 +2,32 @@ package keeper import ( "context" + "fmt" + + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" "github.com/pokt-network/poktroll/x/service/types" ) func (k msgServer) UpdateParams(ctx context.Context, req *types.MsgUpdateParams) (*types.MsgUpdateParamsResponse, error) { + logger := k.Logger().With("method", "UpdateParams") + if err := req.ValidateBasic(); err != nil { - return nil, err + return nil, status.Error(codes.InvalidArgument, err.Error()) } if k.GetAuthority() != req.Authority { - return nil, types.ErrServiceInvalidSigner.Wrapf("invalid authority; expected %s, got %s", k.GetAuthority(), req.Authority) + return nil, status.Error( + codes.PermissionDenied, + types.ErrServiceInvalidSigner.Wrapf( + "invalid authority; expected %s, got %s", k.GetAuthority(), req.Authority, + ).Error(), + ) } if err := k.SetParams(ctx, req.Params); err != nil { - return nil, err + logger.Error(fmt.Sprintf("unable to set params: %+v", err)) + return nil, status.Error(codes.Internal, err.Error()) } return &types.MsgUpdateParamsResponse{}, nil diff --git a/x/service/keeper/query_relay_mining_difficulty.go b/x/service/keeper/query_relay_mining_difficulty.go index 17cef487a..e88e56286 100644 --- a/x/service/keeper/query_relay_mining_difficulty.go +++ b/x/service/keeper/query_relay_mining_difficulty.go @@ -2,6 +2,7 @@ package keeper import ( "context" + "fmt" "cosmossdk.io/store/prefix" "github.com/cosmos/cosmos-sdk/runtime" @@ -13,6 +14,8 @@ import ( ) func (k Keeper) RelayMiningDifficultyAll(ctx context.Context, req *types.QueryAllRelayMiningDifficultyRequest) (*types.QueryAllRelayMiningDifficultyResponse, error) { + logger := k.Logger().With("method", "RelayMiningDifficultyAll") + if req == nil { return nil, status.Error(codes.InvalidArgument, "invalid request") } @@ -25,7 +28,8 @@ func (k Keeper) RelayMiningDifficultyAll(ctx context.Context, req *types.QueryAl pageRes, err := query.Paginate(relayMiningDifficultyStore, req.Pagination, func(key []byte, value []byte) error { var relayMiningDifficulty types.RelayMiningDifficulty if err := k.cdc.Unmarshal(value, &relayMiningDifficulty); err != nil { - return err + logger.Error(fmt.Sprintf("unable to unmarshal relayMiningDifficulty with key (hex): %x: %+v", key, err)) + return status.Error(codes.Internal, err.Error()) } relayMiningDifficulties = append(relayMiningDifficulties, relayMiningDifficulty) diff --git a/x/service/keeper/query_service.go b/x/service/keeper/query_service.go index 0792fa688..6dbd6aa09 100644 --- a/x/service/keeper/query_service.go +++ b/x/service/keeper/query_service.go @@ -2,6 +2,7 @@ package keeper import ( "context" + "fmt" "cosmossdk.io/store/prefix" "github.com/cosmos/cosmos-sdk/runtime" @@ -15,6 +16,8 @@ import ( // AllServices queries all services. func (k Keeper) AllServices(ctx context.Context, req *types.QueryAllServicesRequest) (*types.QueryAllServicesResponse, error) { + logger := k.Logger().With("method", "AllServices") + if req == nil { return nil, status.Error(codes.InvalidArgument, "invalid request") } @@ -27,7 +30,8 @@ func (k Keeper) AllServices(ctx context.Context, req *types.QueryAllServicesRequ pageRes, err := query.Paginate(serviceStore, req.Pagination, func(key []byte, value []byte) error { var service sharedtypes.Service if err := k.cdc.Unmarshal(value, &service); err != nil { - return err + logger.Error(fmt.Sprintf("unable to unmarshal service with key (hex): %x: %+v", key, err)) + return status.Error(codes.Internal, err.Error()) } services = append(services, service) From 3188f8674d7f07d75acb9d38ef53b32395018860 Mon Sep 17 00:00:00 2001 From: Bryan White Date: Mon, 25 Nov 2024 15:24:32 +0100 Subject: [PATCH 2/2] chore: self-review improvements --- x/service/keeper/msg_update_params.go | 3 ++- x/service/keeper/query_relay_mining_difficulty.go | 3 ++- x/service/keeper/query_service.go | 3 ++- 3 files changed, 6 insertions(+), 3 deletions(-) diff --git a/x/service/keeper/msg_update_params.go b/x/service/keeper/msg_update_params.go index e4222f390..d8317c864 100644 --- a/x/service/keeper/msg_update_params.go +++ b/x/service/keeper/msg_update_params.go @@ -26,7 +26,8 @@ func (k msgServer) UpdateParams(ctx context.Context, req *types.MsgUpdateParams) } if err := k.SetParams(ctx, req.Params); err != nil { - logger.Error(fmt.Sprintf("unable to set params: %+v", err)) + err = fmt.Errorf("unable to set params: %w", err) + logger.Error(err.Error()) return nil, status.Error(codes.Internal, err.Error()) } diff --git a/x/service/keeper/query_relay_mining_difficulty.go b/x/service/keeper/query_relay_mining_difficulty.go index e88e56286..82403fd0a 100644 --- a/x/service/keeper/query_relay_mining_difficulty.go +++ b/x/service/keeper/query_relay_mining_difficulty.go @@ -28,7 +28,8 @@ func (k Keeper) RelayMiningDifficultyAll(ctx context.Context, req *types.QueryAl pageRes, err := query.Paginate(relayMiningDifficultyStore, req.Pagination, func(key []byte, value []byte) error { var relayMiningDifficulty types.RelayMiningDifficulty if err := k.cdc.Unmarshal(value, &relayMiningDifficulty); err != nil { - logger.Error(fmt.Sprintf("unable to unmarshal relayMiningDifficulty with key (hex): %x: %+v", key, err)) + err = fmt.Errorf("unable to unmarshal relayMiningDifficulty with key (hex): %x: %w", key, err) + logger.Error(err.Error()) return status.Error(codes.Internal, err.Error()) } diff --git a/x/service/keeper/query_service.go b/x/service/keeper/query_service.go index 6dbd6aa09..73f420edc 100644 --- a/x/service/keeper/query_service.go +++ b/x/service/keeper/query_service.go @@ -30,7 +30,8 @@ func (k Keeper) AllServices(ctx context.Context, req *types.QueryAllServicesRequ pageRes, err := query.Paginate(serviceStore, req.Pagination, func(key []byte, value []byte) error { var service sharedtypes.Service if err := k.cdc.Unmarshal(value, &service); err != nil { - logger.Error(fmt.Sprintf("unable to unmarshal service with key (hex): %x: %+v", key, err)) + err = fmt.Errorf("unable to unmarshal service with key (hex): %x: %w", key, err) + logger.Error(err.Error()) return status.Error(codes.Internal, err.Error()) }