From 550bfcc3734890111ef1f953d0c2885983be3677 Mon Sep 17 00:00:00 2001 From: Julien Robert Date: Mon, 18 Apr 2022 13:00:35 +0200 Subject: [PATCH 1/5] implement first errros --- x/authz/authorization_grant.go | 8 ++++---- x/authz/errors.go | 9 +++++++++ x/authz/keeper/grpc_query.go | 2 +- x/authz/keeper/keeper.go | 17 ++++++++++------- x/authz/keeper/msg_server.go | 2 +- x/authz/msgs.go | 15 ++++++++------- x/authz/simulation/operations.go | 6 +++--- x/staking/types/authz.go | 4 ++-- 8 files changed, 38 insertions(+), 25 deletions(-) diff --git a/x/authz/authorization_grant.go b/x/authz/authorization_grant.go index bd94977ab67b..088ff40556f2 100644 --- a/x/authz/authorization_grant.go +++ b/x/authz/authorization_grant.go @@ -14,11 +14,11 @@ import ( // which is passed into the `blockTime` arg. func NewGrant(blockTime time.Time, a Authorization, expiration *time.Time) (Grant, error) { if expiration != nil && !expiration.After(blockTime) { - return Grant{}, sdkerrors.ErrInvalidRequest.Wrapf("expiration must be after the current block time (%v), got %v", blockTime.Format(time.RFC3339), expiration.Format(time.RFC3339)) + return Grant{}, sdkerrors.Wrapf(ErrInvalidExpirationTime, "expiration must be after the current block time (%v), got %v", blockTime.Format(time.RFC3339), expiration.Format(time.RFC3339)) } msg, ok := a.(proto.Message) if !ok { - return Grant{}, sdkerrors.Wrapf(sdkerrors.ErrPackAny, "cannot proto marshal %T", a) + return Grant{}, sdkerrors.ErrPackAny.Wrapf("cannot proto marshal %T", a) } any, err := cdctypes.NewAnyWithValue(msg) if err != nil { @@ -43,7 +43,7 @@ func (g Grant) UnpackInterfaces(unpacker cdctypes.AnyUnpacker) error { // GetAuthorization returns the cached value from the Grant.Authorization if present. func (g Grant) GetAuthorization() (Authorization, error) { if g.Authorization == nil { - return nil, sdkerrors.Wrap(sdkerrors.ErrInvalidType, "authorization is nil") + return nil, sdkerrors.ErrInvalidType.Wrap("authorization is nil") } a, ok := g.Authorization.GetCachedValue().(Authorization) if !ok { @@ -56,7 +56,7 @@ func (g Grant) ValidateBasic() error { av := g.Authorization.GetCachedValue() a, ok := av.(Authorization) if !ok { - return sdkerrors.Wrapf(sdkerrors.ErrInvalidType, "expected %T, got %T", (Authorization)(nil), av) + return sdkerrors.ErrInvalidType.Wrapf("expected %T, got %T", (Authorization)(nil), av) } return a.ValidateBasic() } diff --git a/x/authz/errors.go b/x/authz/errors.go index 02251c8d6f7e..dbcb4f544ae8 100644 --- a/x/authz/errors.go +++ b/x/authz/errors.go @@ -6,5 +6,14 @@ import ( // x/authz module sentinel errors var ( + //ErrNoAuthorizationFound error if there is no authorization found given a grant key + ErrNoAuthorizationFound = sdkerrors.Register(ModuleName, 2, "authorization not found") + // ErrInvalidExpirationTime error if the set expiration time is in the past ErrInvalidExpirationTime = sdkerrors.Register(ModuleName, 3, "expiration time of authorization should be more than current time") + // ErrUnknownAuthorizationType error for unknown authorization type + ErrUnknownAuthorizationType = sdkerrors.Register(ModuleName, 4, "unknown authorization type") + // ErrNoGrantKeyFound error if the requested grant key does not exist + ErrNoGrantKeyFound = sdkerrors.Register(ModuleName, 5, "grant key not found") + // ErrAuthorizationExpired error if the authorization has expired + ErrAuthorizationExpired = sdkerrors.Register(ModuleName, 6, "authorization expired") ) diff --git a/x/authz/keeper/grpc_query.go b/x/authz/keeper/grpc_query.go index 6f1947b1bbb7..11bf0715efe7 100644 --- a/x/authz/keeper/grpc_query.go +++ b/x/authz/keeper/grpc_query.go @@ -39,7 +39,7 @@ func (k Keeper) Grants(c context.Context, req *authz.QueryGrantsRequest) (*authz if req.MsgTypeUrl != "" { grant, found := k.getGrant(ctx, grantStoreKey(grantee, granter, req.MsgTypeUrl)) if !found { - return nil, sdkerrors.ErrNotFound.Wrapf("authorization not found for %s type", req.MsgTypeUrl) + return nil, sdkerrors.Wrapf(authz.ErrNoAuthorizationFound, "authorization not found for %s type", req.MsgTypeUrl) } authorization, err := grant.GetAuthorization() diff --git a/x/authz/keeper/keeper.go b/x/authz/keeper/keeper.go index a9917cf12119..9b4cbf94a786 100644 --- a/x/authz/keeper/keeper.go +++ b/x/authz/keeper/keeper.go @@ -60,12 +60,12 @@ func (k Keeper) update(ctx sdk.Context, grantee sdk.AccAddress, granter sdk.AccA skey := grantStoreKey(grantee, granter, updated.MsgTypeURL()) grant, found := k.getGrant(ctx, skey) if !found { - return sdkerrors.ErrNotFound.Wrap("authorization not found") + return authz.ErrNoAuthorizationFound } msg, ok := updated.(proto.Message) if !ok { - sdkerrors.ErrPackAny.Wrapf("cannot proto marshal %T", updated) + return sdkerrors.ErrPackAny.Wrapf("cannot proto marshal %T", updated) } any, err := codectypes.NewAnyWithValue(msg) @@ -76,6 +76,7 @@ func (k Keeper) update(ctx sdk.Context, grantee sdk.AccAddress, granter sdk.AccA grant.Authorization = any store := ctx.KVStore(k.storeKey) store.Set(skey, k.cdc.MustMarshal(&grant)) + return nil } @@ -93,13 +94,15 @@ func (k Keeper) DispatchActions(ctx sdk.Context, grantee sdk.AccAddress, msgs [] // if granter != grantee then check authorization.Accept, otherwise we implicitly accept. if !granter.Equals(grantee) { - grant, found := k.getGrant(ctx, grantStoreKey(grantee, granter, sdk.MsgTypeURL(msg))) + skey := grantStoreKey(grantee, granter, sdk.MsgTypeURL(msg)) + + grant, found := k.getGrant(ctx, skey) if !found { - return nil, sdkerrors.ErrUnauthorized.Wrap("authorization not found") + return nil, sdkerrors.Wrapf(authz.ErrNoAuthorizationFound, "failed to update grant with key %s", string(skey)) } if grant.Expiration != nil && grant.Expiration.Before(now) { - return nil, sdkerrors.ErrUnauthorized.Wrap("authorization expired") + return nil, authz.ErrAuthorizationExpired } authorization, err := grant.GetAuthorization() @@ -193,7 +196,7 @@ func (k Keeper) DeleteGrant(ctx sdk.Context, grantee sdk.AccAddress, granter sdk skey := grantStoreKey(grantee, granter, msgType) grant, found := k.getGrant(ctx, skey) if !found { - return sdkerrors.ErrNotFound.Wrap("authorization not found") + return sdkerrors.Wrapf(authz.ErrNoAuthorizationFound, "failed to delete grant with key %s", string(skey)) } store.Delete(skey) @@ -355,7 +358,7 @@ func (keeper Keeper) removeFromGrantQueue(ctx sdk.Context, grantKey []byte, gran key := GrantQueueKey(expiration, granter, grantee) bz := store.Get(key) if bz == nil { - return sdkerrors.ErrLogic.Wrap("can't remove grant from the expire queue, grant key not found") + return sdkerrors.Wrap(authz.ErrNoGrantKeyFound, "can't remove grant from the expire queue, grant key not found") } var queueItem authz.GrantQueueItem diff --git a/x/authz/keeper/msg_server.go b/x/authz/keeper/msg_server.go index ac81b60b6469..24047e921c8a 100644 --- a/x/authz/keeper/msg_server.go +++ b/x/authz/keeper/msg_server.go @@ -35,7 +35,7 @@ func (k Keeper) Grant(goCtx context.Context, msg *authz.MsgGrant) (*authz.MsgGra } t := authorization.MsgTypeURL() if k.router.HandlerByTypeURL(t) == nil { - return nil, sdkerrors.Wrapf(sdkerrors.ErrInvalidType, "%s doesn't exist.", t) + return nil, sdkerrors.ErrInvalidType.Wrapf("%s doesn't exist.", t) } err = k.SaveGrant(ctx, grantee, granter, authorization, msg.Grant.Expiration) diff --git a/x/authz/msgs.go b/x/authz/msgs.go index 877932b75e11..6b573bee0a3f 100644 --- a/x/authz/msgs.go +++ b/x/authz/msgs.go @@ -1,9 +1,10 @@ package authz import ( - "github.com/cosmos/cosmos-sdk/codec/legacy" "time" + "github.com/cosmos/cosmos-sdk/codec/legacy" + "github.com/gogo/protobuf/proto" cdctypes "github.com/cosmos/cosmos-sdk/codec/types" @@ -59,7 +60,7 @@ func (msg MsgGrant) ValidateBasic() error { } if granter.Equals(grantee) { - return sdkerrors.Wrap(sdkerrors.ErrInvalidRequest, "granter and grantee cannot be same") + return sdkerrors.ErrInvalidRequest.Wrap("granter and grantee cannot be same") } return msg.Grant.ValidateBasic() } @@ -88,7 +89,7 @@ func (msg *MsgGrant) GetAuthorization() (Authorization, error) { func (msg *MsgGrant) SetAuthorization(a Authorization) error { m, ok := a.(proto.Message) if !ok { - return sdkerrors.Wrapf(sdkerrors.ErrPackAny, "can't proto marshal %T", m) + return sdkerrors.ErrPackAny.Wrapf("can't proto marshal %T", m) } any, err := cdctypes.NewAnyWithValue(m) if err != nil { @@ -144,11 +145,11 @@ func (msg MsgRevoke) ValidateBasic() error { } if granter.Equals(grantee) { - return sdkerrors.Wrap(sdkerrors.ErrInvalidRequest, "granter and grantee cannot be same") + return sdkerrors.ErrInvalidRequest.Wrap("granter and grantee cannot be same") } if msg.MsgTypeUrl == "" { - return sdkerrors.Wrap(sdkerrors.ErrInvalidRequest, "missing method name") + return sdkerrors.ErrInvalidRequest.Wrap("missing method name") } return nil @@ -194,7 +195,7 @@ func (msg MsgExec) GetMessages() ([]sdk.Msg, error) { for i, msgAny := range msg.Msgs { msg, ok := msgAny.GetCachedValue().(sdk.Msg) if !ok { - return nil, sdkerrors.Wrapf(sdkerrors.ErrInvalidRequest, "messages contains %T which is not a sdk.MsgRequest", msgAny) + return nil, sdkerrors.ErrInvalidRequest.Wrapf("messages contains %T which is not a sdk.MsgRequest", msgAny) } msgs[i] = msg } @@ -215,7 +216,7 @@ func (msg MsgExec) ValidateBasic() error { } if len(msg.Msgs) == 0 { - return sdkerrors.Wrapf(sdkerrors.ErrInvalidRequest, "messages cannot be empty") + return sdkerrors.ErrInvalidRequest.Wrapf("messages cannot be empty") } return nil diff --git a/x/authz/simulation/operations.go b/x/authz/simulation/operations.go index 1879522b0ce1..ef5db52fdcc5 100644 --- a/x/authz/simulation/operations.go +++ b/x/authz/simulation/operations.go @@ -170,7 +170,7 @@ func SimulateMsgRevoke(ak authz.AccountKeeper, bk authz.BankKeeper, k keeper.Kee granterAcc, ok := simtypes.FindAccount(accs, granterAddr) if !ok { - return simtypes.NoOpMsg(authz.ModuleName, TypeMsgRevoke, "account not found"), nil, sdkerrors.Wrapf(sdkerrors.ErrNotFound, "account not found") + return simtypes.NoOpMsg(authz.ModuleName, TypeMsgRevoke, "account not found"), nil, sdkerrors.ErrNotFound.Wrapf("account not found") } spendableCoins := bk.SpendableCoins(ctx, granterAddr) @@ -241,11 +241,11 @@ func SimulateMsgExec(ak authz.AccountKeeper, bk authz.BankKeeper, k keeper.Keepe grantee, ok := simtypes.FindAccount(accs, granteeAddr) if !ok { - return simtypes.NoOpMsg(authz.ModuleName, TypeMsgRevoke, "Account not found"), nil, sdkerrors.Wrapf(sdkerrors.ErrNotFound, "grantee account not found") + return simtypes.NoOpMsg(authz.ModuleName, TypeMsgRevoke, "Account not found"), nil, sdkerrors.ErrNotFound.Wrapf("grantee account not found") } if _, ok := simtypes.FindAccount(accs, granterAddr); !ok { - return simtypes.NoOpMsg(authz.ModuleName, TypeMsgRevoke, "Account not found"), nil, sdkerrors.Wrapf(sdkerrors.ErrNotFound, "granter account not found") + return simtypes.NoOpMsg(authz.ModuleName, TypeMsgRevoke, "Account not found"), nil, sdkerrors.ErrNotFound.Wrapf("granter account not found") } granterspendableCoins := bk.SpendableCoins(ctx, granterAddr) diff --git a/x/staking/types/authz.go b/x/staking/types/authz.go index 6419ede7a702..01b265199f11 100644 --- a/x/staking/types/authz.go +++ b/x/staking/types/authz.go @@ -51,7 +51,7 @@ func (a StakeAuthorization) ValidateBasic() error { return sdkerrors.Wrapf(sdkerrors.ErrInvalidCoins, "negative coin amount: %v", a.MaxTokens) } if a.AuthorizationType == AuthorizationType_AUTHORIZATION_TYPE_UNSPECIFIED { - return sdkerrors.Wrapf(sdkerrors.ErrInvalidType, "unknown authorization type") + return authz.ErrUnknownAuthorizationType } return nil @@ -148,6 +148,6 @@ func normalizeAuthzType(authzType AuthorizationType) (string, error) { case AuthorizationType_AUTHORIZATION_TYPE_REDELEGATE: return sdk.MsgTypeURL(&MsgBeginRedelegate{}), nil default: - return "", sdkerrors.ErrInvalidType.Wrapf("unknown authorization type %T", authzType) + return "", sdkerrors.Wrapf(authz.ErrUnknownAuthorizationType, "cannot normalize authz type with %T", authzType) } } From 9f5936648976f1e5cebad4e27772cc963ef514b3 Mon Sep 17 00:00:00 2001 From: Julien Robert Date: Mon, 18 Apr 2022 13:26:32 +0200 Subject: [PATCH 2/5] add more errors --- x/authz/errors.go | 6 ++++++ x/authz/keeper/keeper.go | 2 +- x/authz/msgs.go | 2 +- x/staking/types/authz.go | 4 ++-- 4 files changed, 10 insertions(+), 4 deletions(-) diff --git a/x/authz/errors.go b/x/authz/errors.go index dbcb4f544ae8..8d104d0c920e 100644 --- a/x/authz/errors.go +++ b/x/authz/errors.go @@ -16,4 +16,10 @@ var ( ErrNoGrantKeyFound = sdkerrors.Register(ModuleName, 5, "grant key not found") // ErrAuthorizationExpired error if the authorization has expired ErrAuthorizationExpired = sdkerrors.Register(ModuleName, 6, "authorization expired") + // ErrGranteeIsGranter error if the grantee and the granter are the same + ErrGranteeIsGranter = sdkerrors.Register(ModuleName, 7, "grantee and granter should be different") + // ErrAuthorizationNumOfSigners error if an authorization message does not have only one signer + ErrAuthorizationNumOfSigners = sdkerrors.Register(ModuleName, 9, "authorization can be given to msg with only one signer") + // ErrNegativeMaxTokens error if the max tokens is negative + ErrNegativeMaxTokens = sdkerrors.Register(ModuleName, 12, "max tokens should be positive") ) diff --git a/x/authz/keeper/keeper.go b/x/authz/keeper/keeper.go index 9b4cbf94a786..a87375b044f1 100644 --- a/x/authz/keeper/keeper.go +++ b/x/authz/keeper/keeper.go @@ -88,7 +88,7 @@ func (k Keeper) DispatchActions(ctx sdk.Context, grantee sdk.AccAddress, msgs [] for i, msg := range msgs { signers := msg.GetSigners() if len(signers) != 1 { - return nil, sdkerrors.ErrInvalidRequest.Wrap("authorization can be given to msg with only one signer") + return nil, authz.ErrAuthorizationNumOfSigners } granter := signers[0] diff --git a/x/authz/msgs.go b/x/authz/msgs.go index 6b573bee0a3f..bdf871e427da 100644 --- a/x/authz/msgs.go +++ b/x/authz/msgs.go @@ -60,7 +60,7 @@ func (msg MsgGrant) ValidateBasic() error { } if granter.Equals(grantee) { - return sdkerrors.ErrInvalidRequest.Wrap("granter and grantee cannot be same") + return ErrGranteeIsGranter } return msg.Grant.ValidateBasic() } diff --git a/x/staking/types/authz.go b/x/staking/types/authz.go index 01b265199f11..4bd567c464c0 100644 --- a/x/staking/types/authz.go +++ b/x/staking/types/authz.go @@ -48,7 +48,7 @@ func (a StakeAuthorization) MsgTypeURL() string { func (a StakeAuthorization) ValidateBasic() error { if a.MaxTokens != nil && a.MaxTokens.IsNegative() { - return sdkerrors.Wrapf(sdkerrors.ErrInvalidCoins, "negative coin amount: %v", a.MaxTokens) + return sdkerrors.Wrapf(authz.ErrNegativeMaxTokens, "negative coin amount: %v", a.MaxTokens) } if a.AuthorizationType == AuthorizationType_AUTHORIZATION_TYPE_UNSPECIFIED { return authz.ErrUnknownAuthorizationType @@ -90,7 +90,7 @@ func (a StakeAuthorization) Accept(ctx sdk.Context, msg sdk.Msg) (authz.AcceptRe for _, validator := range denyList { ctx.GasMeter().ConsumeGas(gasCostPerIteration, "stake authorization") if validator == validatorAddress { - return authz.AcceptResponse{}, sdkerrors.ErrUnauthorized.Wrapf(" cannot delegate/undelegate to %s validator", validator) + return authz.AcceptResponse{}, sdkerrors.ErrUnauthorized.Wrapf("cannot delegate/undelegate to %s validator", validator) } } From 6137773f0cfd072591e14e4e6cfff6de5112195b Mon Sep 17 00:00:00 2001 From: Julien Robert Date: Tue, 19 Apr 2022 09:23:53 +0200 Subject: [PATCH 3/5] Use ErrGranteeIsGranter instead of ErrInvalidRequest --- x/authz/msgs.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x/authz/msgs.go b/x/authz/msgs.go index bdf871e427da..3aa145bd7d12 100644 --- a/x/authz/msgs.go +++ b/x/authz/msgs.go @@ -145,7 +145,7 @@ func (msg MsgRevoke) ValidateBasic() error { } if granter.Equals(grantee) { - return sdkerrors.ErrInvalidRequest.Wrap("granter and grantee cannot be same") + return ErrGranteeIsGranter } if msg.MsgTypeUrl == "" { From b71ed8edb40d40759b3b4a7cd92ea09686cffcda Mon Sep 17 00:00:00 2001 From: Julien Robert Date: Tue, 19 Apr 2022 18:11:39 +0200 Subject: [PATCH 4/5] update test --- x/authz/client/testutil/tx.go | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/x/authz/client/testutil/tx.go b/x/authz/client/testutil/tx.go index f7db4fe644be..70109e3e898a 100644 --- a/x/authz/client/testutil/tx.go +++ b/x/authz/client/testutil/tx.go @@ -14,6 +14,7 @@ import ( clitestutil "github.com/cosmos/cosmos-sdk/testutil/cli" "github.com/cosmos/cosmos-sdk/testutil/network" sdk "github.com/cosmos/cosmos-sdk/types" + "github.com/cosmos/cosmos-sdk/x/authz" "github.com/cosmos/cosmos-sdk/x/authz/client/cli" banktestutil "github.com/cosmos/cosmos-sdk/x/bank/client/testutil" bank "github.com/cosmos/cosmos-sdk/x/bank/types" @@ -563,7 +564,7 @@ func (s *IntegrationTestSuite) TestExecAuthorizationWithExpiration() { fmt.Sprintf("--%s=true", flags.FlagSkipConfirmation), }) s.Require().NoError(err) - s.Require().Contains(res.String(), "authorization not found") + s.Require().Contains(res.String(), authz.ErrNoAuthorizationFound.Error()) } func (s *IntegrationTestSuite) TestNewExecGenericAuthorized() { @@ -844,9 +845,9 @@ func (s *IntegrationTestSuite) TestExecDelegateAuthorization() { fmt.Sprintf("--%s=%s", flags.FlagFees, sdk.NewCoins(sdk.NewCoin(s.cfg.BondDenom, sdk.NewInt(10))).String()), fmt.Sprintf("--%s=true", flags.FlagSkipConfirmation), }, - 4, + authz.ErrNoAuthorizationFound.ABCICode(), false, - "authorization not found", + authz.ErrNoAuthorizationFound.Error(), }, } @@ -1065,9 +1066,9 @@ func (s *IntegrationTestSuite) TestExecUndelegateAuthorization() { fmt.Sprintf("--%s=%s", flags.FlagFees, sdk.NewCoins(sdk.NewCoin(s.cfg.BondDenom, sdk.NewInt(10))).String()), fmt.Sprintf("--%s=true", flags.FlagSkipConfirmation), }, - 4, + authz.ErrNoAuthorizationFound.ABCICode(), false, - "authorization not found", + authz.ErrNoAuthorizationFound.Error(), }, } From e39fd840a378a77d68f062890bfc1d72feb6bc45 Mon Sep 17 00:00:00 2001 From: Julien Robert Date: Tue, 19 Apr 2022 18:26:02 +0200 Subject: [PATCH 5/5] update test --- x/authz/client/testutil/tx.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x/authz/client/testutil/tx.go b/x/authz/client/testutil/tx.go index 70109e3e898a..da1cf903ed60 100644 --- a/x/authz/client/testutil/tx.go +++ b/x/authz/client/testutil/tx.go @@ -746,7 +746,7 @@ func (s *IntegrationTestSuite) TestNewExecGrantAuthorized() { fmt.Sprintf("--%s=%s", flags.FlagFees, sdk.NewCoins(sdk.NewCoin(s.cfg.BondDenom, sdk.NewInt(10))).String()), fmt.Sprintf("--%s=true", flags.FlagSkipConfirmation), }, - 4, + authz.ErrNoAuthorizationFound.ABCICode(), false, "", },