From 063d58a206a834ae451ddfca0b31998b02d1ab62 Mon Sep 17 00:00:00 2001 From: Bryan White Date: Mon, 25 Nov 2024 16:24:47 +0100 Subject: [PATCH 1/5] fix: outstanding supplier module non-status (gRPC) errors --- .../keeper/msg_server_unstake_supplier.go | 39 ++++++++++++------- x/supplier/keeper/msg_update_params.go | 18 +++++++-- x/supplier/keeper/query_supplier.go | 6 ++- 3 files changed, 46 insertions(+), 17 deletions(-) diff --git a/x/supplier/keeper/msg_server_unstake_supplier.go b/x/supplier/keeper/msg_server_unstake_supplier.go index 17cfda752..31c0272f4 100644 --- a/x/supplier/keeper/msg_server_unstake_supplier.go +++ b/x/supplier/keeper/msg_server_unstake_supplier.go @@ -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(fmt.Sprintf("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) diff --git a/x/supplier/keeper/msg_update_params.go b/x/supplier/keeper/msg_update_params.go index ed841b914..40aae6d96 100644 --- a/x/supplier/keeper/msg_update_params.go +++ b/x/supplier/keeper/msg_update_params.go @@ -3,6 +3,9 @@ package keeper import ( "context" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" + "github.com/pokt-network/poktroll/x/supplier/types" ) @@ -10,15 +13,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 = status.Error(codes.Internal, err.Error()) + logger.Error(err.Error()) + return nil, status.Error(codes.Internal, err.Error()) } return &types.MsgUpdateParamsResponse{}, nil diff --git a/x/supplier/keeper/query_supplier.go b/x/supplier/keeper/query_supplier.go index 92112827a..3d370a8fa 100644 --- a/x/supplier/keeper/query_supplier.go +++ b/x/supplier/keeper/query_supplier.go @@ -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") } @@ -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) From 66e246fa575881f51aa36930a81b26848fa6ca86 Mon Sep 17 00:00:00 2001 From: Bryan White Date: Mon, 25 Nov 2024 17:31:08 +0100 Subject: [PATCH 2/5] fix: linter error --- x/supplier/keeper/msg_server_unstake_supplier.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x/supplier/keeper/msg_server_unstake_supplier.go b/x/supplier/keeper/msg_server_unstake_supplier.go index 31c0272f4..2ac70a39b 100644 --- a/x/supplier/keeper/msg_server_unstake_supplier.go +++ b/x/supplier/keeper/msg_server_unstake_supplier.go @@ -45,7 +45,7 @@ func (k msgServer) UnstakeSupplier( // Ensure the singer address matches the owner address or the operator address. if !supplier.HasOperator(msg.GetSigner()) && !supplier.HasOwner(msg.GetSigner()) { - logger.Info(fmt.Sprintf("only the supplier owner or operator is allowed to unstake the supplier")) + logger.Info("only the supplier owner or operator is allowed to unstake the supplier") return nil, status.Error( codes.PermissionDenied, sharedtypes.ErrSharedUnauthorizedSupplierUpdate.Wrapf( From e19614059718f4a12a0f87b95558cff96173bd65 Mon Sep 17 00:00:00 2001 From: Bryan White Date: Tue, 26 Nov 2024 19:28:06 +0100 Subject: [PATCH 3/5] fix: failing tests --- x/supplier/keeper/msg_server_unstake_supplier_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/x/supplier/keeper/msg_server_unstake_supplier_test.go b/x/supplier/keeper/msg_server_unstake_supplier_test.go index fc844e4e0..d96a433fd 100644 --- a/x/supplier/keeper/msg_server_unstake_supplier_test.go +++ b/x/supplier/keeper/msg_server_unstake_supplier_test.go @@ -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) @@ -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) { From a2eb64a9ff0e3782b348884add028ce6de2d803d Mon Sep 17 00:00:00 2001 From: Bryan White Date: Mon, 2 Dec 2024 09:24:03 +0100 Subject: [PATCH 4/5] chore: review feedback improvements --- x/supplier/keeper/msg_update_params.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x/supplier/keeper/msg_update_params.go b/x/supplier/keeper/msg_update_params.go index 40aae6d96..bda2267cd 100644 --- a/x/supplier/keeper/msg_update_params.go +++ b/x/supplier/keeper/msg_update_params.go @@ -28,7 +28,7 @@ func (k msgServer) UpdateParams( } if err := k.SetParams(ctx, req.Params); err != nil { - err = status.Error(codes.Internal, err.Error()) + err = fmt.Errorf("unable to set params: %w", err) logger.Error(err.Error()) return nil, status.Error(codes.Internal, err.Error()) } From 83eb7e6c68cb2a70d705430169ad873e59d438ca Mon Sep 17 00:00:00 2001 From: Bryan White Date: Mon, 2 Dec 2024 09:29:07 +0100 Subject: [PATCH 5/5] fix: imports --- x/supplier/keeper/msg_update_params.go | 1 + 1 file changed, 1 insertion(+) diff --git a/x/supplier/keeper/msg_update_params.go b/x/supplier/keeper/msg_update_params.go index bda2267cd..4fa159dff 100644 --- a/x/supplier/keeper/msg_update_params.go +++ b/x/supplier/keeper/msg_update_params.go @@ -2,6 +2,7 @@ package keeper import ( "context" + "fmt" "google.golang.org/grpc/codes" "google.golang.org/grpc/status"