Skip to content

Commit

Permalink
[Code Health] fix: supplier module gRPC return errors (#962)
Browse files Browse the repository at this point in the history
## Summary

Ensure all supplier 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
  • Loading branch information
bryanchriswhite authored Dec 2, 2024
1 parent acd7fc8 commit 1a6cabd
Show file tree
Hide file tree
Showing 4 changed files with 49 additions and 19 deletions.
39 changes: 26 additions & 13 deletions x/supplier/keeper/msg_server_unstake_supplier.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,32 +28,45 @@ func (k msgServer) UnstakeSupplier(
logger.Info(fmt.Sprintf("About to unstake supplier with msg: %v", msg))

if err := msg.ValidateBasic(); err != nil {
return nil, err
return nil, status.Error(codes.InvalidArgument, err.Error())
}

// Check if the supplier already exists or not
supplier, isSupplierFound := k.GetSupplier(ctx, msg.OperatorAddress)
supplier, isSupplierFound := k.GetSupplier(ctx, msg.GetOperatorAddress())
if !isSupplierFound {
logger.Info(fmt.Sprintf("Supplier not found. Cannot unstake address %s", msg.OperatorAddress))
return nil, suppliertypes.ErrSupplierNotFound
logger.Info(fmt.Sprintf("Supplier not found. Cannot unstake address %s", msg.GetOperatorAddress()))
return nil, status.Error(
codes.NotFound,
suppliertypes.ErrSupplierNotFound.Wrapf(
"supplier with operator address %q", msg.GetOperatorAddress(),
).Error(),
)
}

// Ensure the singer address matches the owner address or the operator address.
if !supplier.HasOperator(msg.Signer) && !supplier.HasOwner(msg.Signer) {
logger.Error("only the supplier owner or operator is allowed to unstake the supplier")
return nil, sharedtypes.ErrSharedUnauthorizedSupplierUpdate.Wrapf(
"signer %q is not allowed to unstake supplier %v",
msg.Signer,
supplier,
if !supplier.HasOperator(msg.GetSigner()) && !supplier.HasOwner(msg.GetSigner()) {
logger.Info("only the supplier owner or operator is allowed to unstake the supplier")
return nil, status.Error(
codes.PermissionDenied,
sharedtypes.ErrSharedUnauthorizedSupplierUpdate.Wrapf(
"signer %q is not allowed to unstake supplier %+v",
msg.Signer,
supplier,
).Error(),
)
}

logger.Info(fmt.Sprintf("Supplier found. Unstaking supplier for address %s", msg.OperatorAddress))
logger.Info(fmt.Sprintf("Supplier found. Unstaking supplier with operating address %s", msg.GetOperatorAddress()))

// Check if the supplier has already initiated the unstake action.
if supplier.IsUnbonding() {
logger.Warn(fmt.Sprintf("Supplier %s still unbonding from previous unstaking", msg.OperatorAddress))
return nil, suppliertypes.ErrSupplierIsUnstaking
logger.Info(fmt.Sprintf("Supplier %s still unbonding from previous unstaking", msg.GetOperatorAddress()))
return nil, status.Error(
codes.FailedPrecondition,
suppliertypes.ErrSupplierIsUnstaking.Wrapf(
"supplier with operator address %q", msg.GetOperatorAddress(),
).Error(),
)
}

sdkCtx := sdk.UnwrapSDKContext(ctx)
Expand Down
4 changes: 2 additions & 2 deletions x/supplier/keeper/msg_server_unstake_supplier_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -280,7 +280,7 @@ func TestMsgServer_UnstakeSupplier_FailIfNotStaked(t *testing.T) {
}
_, err := srv.UnstakeSupplier(ctx, unstakeMsg)
require.Error(t, err)
require.ErrorIs(t, err, suppliertypes.ErrSupplierNotFound)
require.ErrorContains(t, err, suppliertypes.ErrSupplierNotFound.Error())

_, isSupplierFound = supplierModuleKeepers.GetSupplier(ctx, supplierOperatorAddr)
require.False(t, isSupplierFound)
Expand Down Expand Up @@ -311,7 +311,7 @@ func TestMsgServer_UnstakeSupplier_FailIfCurrentlyUnstaking(t *testing.T) {
ctx = keepertest.SetBlockHeight(ctx, sdkCtx.BlockHeight()+1)

_, err = srv.UnstakeSupplier(ctx, unstakeMsg)
require.ErrorIs(t, err, suppliertypes.ErrSupplierIsUnstaking)
require.ErrorContains(t, err, suppliertypes.ErrSupplierIsUnstaking.Error())
}

func TestMsgServer_UnstakeSupplier_OperatorCanUnstake(t *testing.T) {
Expand Down
19 changes: 16 additions & 3 deletions x/supplier/keeper/msg_update_params.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,10 @@ package keeper

import (
"context"
"fmt"

"google.golang.org/grpc/codes"
"google.golang.org/grpc/status"

"github.com/pokt-network/poktroll/x/supplier/types"
)
Expand All @@ -10,15 +14,24 @@ 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.ErrSupplierInvalidSigner.Wrapf("invalid authority; expected %s, got %s", k.GetAuthority(), req.Authority)
return nil, status.Error(
codes.PermissionDenied,
types.ErrSupplierInvalidSigner.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
Expand Down
6 changes: 5 additions & 1 deletion x/supplier/keeper/query_supplier.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@ func (k Keeper) AllSuppliers(
ctx context.Context,
req *types.QueryAllSuppliersRequest,
) (*types.QueryAllSuppliersResponse, error) {
logger := k.Logger().With("method", "AllSuppliers")

if req == nil {
return nil, status.Error(codes.InvalidArgument, "invalid request")
}
Expand All @@ -33,7 +35,9 @@ func (k Keeper) AllSuppliers(
func(key []byte, value []byte) error {
var supplier sharedtypes.Supplier
if err := k.cdc.Unmarshal(value, &supplier); err != nil {
return err
err = fmt.Errorf("unmarshaling supplier with key (hex): %x: %+v", key, err)
logger.Error(err.Error())
return status.Error(codes.Internal, err.Error())
}

suppliers = append(suppliers, supplier)
Expand Down

0 comments on commit 1a6cabd

Please sign in to comment.