From 1c490ce2577991b21188b6a8625d8421d7c8ba16 Mon Sep 17 00:00:00 2001 From: Bryan White Date: Mon, 2 Dec 2024 10:15:31 +0100 Subject: [PATCH] [Code Health] fix: service module gRPC status error returns (#959) ## Summary Ensure all service module message and query handlers return gRPC status errors. ## Issue - #860 ## Type of change Select one or more from the following: - [ ] New feature, functionality or library - [x] Consensus breaking; add the `consensus-breaking` label if so. See #791 for details - [ ] Bug fix - [x] Code health or cleanup - [ ] Documentation - [ ] Other (specify) ## Testing - [ ] **Documentation**: `make docusaurus_start`; only needed if you make doc changes - [x] **Unit Tests**: `make go_develop_and_test` - [ ] **LocalNet E2E Tests**: `make test_e2e` - [x] **DevNet E2E Tests**: Add the `devnet-test-e2e` label to the PR. ## Sanity Checklist - [x] I have tested my changes using the available tooling - [ ] I have commented my code - [x] I have performed a self-review of my own code; both comments & source code - [ ] I create and reference any new tickets, if applicable - [ ] I have left TODOs throughout the codebase, if applicable --- x/service/keeper/msg_server_add_service.go | 58 ++++++++++++------- x/service/keeper/msg_update_params.go | 19 +++++- .../keeper/query_relay_mining_difficulty.go | 7 ++- x/service/keeper/query_service.go | 7 ++- 4 files changed, 65 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..d8317c864 100644 --- a/x/service/keeper/msg_update_params.go +++ b/x/service/keeper/msg_update_params.go @@ -2,20 +2,33 @@ 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 + err = fmt.Errorf("unable to set params: %w", err) + logger.Error(err.Error()) + 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..82403fd0a 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,9 @@ 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 + 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()) } relayMiningDifficulties = append(relayMiningDifficulties, relayMiningDifficulty) diff --git a/x/service/keeper/query_service.go b/x/service/keeper/query_service.go index 0792fa688..73f420edc 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,9 @@ 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 + 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()) } services = append(services, service)