From ead6e6f94847e46e4e6270d54d18a546e39a2b93 Mon Sep 17 00:00:00 2001 From: likhita-809 <78951027+likhita-809@users.noreply.github.com> Date: Mon, 9 May 2022 17:30:16 +0530 Subject: [PATCH] refactor: x/authz minor code improvements (#11840) ## Description Closes: #11569 --- ### Author Checklist *All items are required. Please add a note to the item if the item is not applicable and please add links to any relevant follow up issues.* I have... - [ ] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title - [ ] added `!` to the type prefix if API or client breaking change - [ ] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/main/CONTRIBUTING.md#pr-targeting)) - [ ] provided a link to the relevant issue or specification - [ ] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/main/docs/building-modules) - [ ] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/main/CONTRIBUTING.md#testing) - [ ] added a changelog entry to `CHANGELOG.md` - [ ] included comments for [documenting Go code](https://blog.golang.org/godoc) - [ ] updated the relevant documentation or specification - [ ] reviewed "Files changed" and left comments if necessary - [ ] confirmed all CI checks have passed ### Reviewers Checklist *All items are required. Please add a note if the item is not applicable and please add your handle next to the items reviewed if you only reviewed selected items.* I have... - [ ] confirmed the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title - [ ] confirmed `!` in the type prefix if API or client breaking change - [ ] confirmed all author checklist items have been addressed - [ ] reviewed state machine logic - [ ] reviewed API design and naming - [ ] reviewed documentation is accurate - [ ] reviewed tests and test coverage - [ ] manually tested (if applicable) --- x/authz/authorization_grant.go | 5 +- x/authz/client/cli/tx.go | 16 ++++- x/authz/client/testutil/query.go | 6 +- x/authz/client/testutil/tx.go | 114 +++++++++++++++++++++++++++++-- x/authz/keeper/grpc_query.go | 1 + x/authz/keeper/keeper.go | 4 +- x/staking/types/authz.go | 6 +- 7 files changed, 132 insertions(+), 20 deletions(-) diff --git a/x/authz/authorization_grant.go b/x/authz/authorization_grant.go index 088ff40556f2..b364ce8bb591 100644 --- a/x/authz/authorization_grant.go +++ b/x/authz/authorization_grant.go @@ -45,9 +45,10 @@ func (g Grant) GetAuthorization() (Authorization, error) { if g.Authorization == nil { return nil, sdkerrors.ErrInvalidType.Wrap("authorization is nil") } - a, ok := g.Authorization.GetCachedValue().(Authorization) + av := g.Authorization.GetCachedValue() + a, ok := av.(Authorization) if !ok { - return nil, sdkerrors.ErrInvalidType.Wrapf("expected %T, got %T", (Authorization)(nil), g.Authorization.GetCachedValue()) + return nil, sdkerrors.ErrInvalidType.Wrapf("expected %T, got %T", (Authorization)(nil), av) } return a, nil } diff --git a/x/authz/client/cli/tx.go b/x/authz/client/cli/tx.go index e6f37343ffbc..088eac05ac27 100644 --- a/x/authz/client/cli/tx.go +++ b/x/authz/client/cli/tx.go @@ -118,15 +118,25 @@ Examples: var delegateLimit *sdk.Coin if limit != "" { - spendLimit, err := sdk.ParseCoinsNormalized(limit) + spendLimit, err := sdk.ParseCoinNormalized(limit) if err != nil { return err } + queryClient := staking.NewQueryClient(clientCtx) - if !spendLimit.IsAllPositive() { + res, err := queryClient.Params(cmd.Context(), &staking.QueryParamsRequest{}) + if err != nil { + return err + } + + if spendLimit.Denom != res.Params.BondDenom { + return fmt.Errorf("invalid denom %s; coin denom should match the current bond denom %s", spendLimit.Denom, res.Params.BondDenom) + } + + if !spendLimit.IsPositive() { return fmt.Errorf("spend-limit should be greater than zero") } - delegateLimit = &spendLimit[0] + delegateLimit = &spendLimit } allowed, err := bech32toValidatorAddresses(allowValidators) diff --git a/x/authz/client/testutil/query.go b/x/authz/client/testutil/query.go index 56296576507b..45921a054de6 100644 --- a/x/authz/client/testutil/query.go +++ b/x/authz/client/testutil/query.go @@ -25,7 +25,7 @@ func (s *IntegrationTestSuite) TestQueryAuthorizations() { []string{ grantee.String(), "send", - fmt.Sprintf("--%s=100steak", cli.FlagSpendLimit), + fmt.Sprintf("--%s=100stake", cli.FlagSpendLimit), fmt.Sprintf("--%s=true", flags.FlagSkipConfirmation), fmt.Sprintf("--%s=%s", flags.FlagFrom, val.Address), fmt.Sprintf("--%s=%s", flags.FlagBroadcastMode, flags.BroadcastBlock), @@ -103,7 +103,7 @@ func (s *IntegrationTestSuite) TestQueryAuthorization() { []string{ grantee.String(), "send", - fmt.Sprintf("--%s=100steak", cli.FlagSpendLimit), + fmt.Sprintf("--%s=100stake", cli.FlagSpendLimit), fmt.Sprintf("--%s=true", flags.FlagSkipConfirmation), fmt.Sprintf("--%s=%s", flags.FlagFrom, val.Address), fmt.Sprintf("--%s=%s", flags.FlagBroadcastMode, flags.BroadcastBlock), @@ -161,7 +161,7 @@ func (s *IntegrationTestSuite) TestQueryAuthorization() { fmt.Sprintf("--%s=json", tmcli.OutputFlag), }, false, - `{"@type":"/cosmos.bank.v1beta1.SendAuthorization","spend_limit":[{"denom":"steak","amount":"100"}]}`, + `{"@type":"/cosmos.bank.v1beta1.SendAuthorization","spend_limit":[{"denom":"stake","amount":"100"}]}`, }, } for _, tc := range testCases { diff --git a/x/authz/client/testutil/tx.go b/x/authz/client/testutil/tx.go index da1cf903ed60..e0a2ff95c2f5 100644 --- a/x/authz/client/testutil/tx.go +++ b/x/authz/client/testutil/tx.go @@ -69,7 +69,7 @@ func (s *IntegrationTestSuite) SetupSuite() { out, err := CreateGrant(val, []string{ s.grantee[1].String(), "send", - fmt.Sprintf("--%s=100steak", cli.FlagSpendLimit), + fmt.Sprintf("--%s=100stake", cli.FlagSpendLimit), fmt.Sprintf("--%s=%s", flags.FlagFrom, val.Address.String()), fmt.Sprintf("--%s=true", flags.FlagSkipConfirmation), fmt.Sprintf("--%s=%s", flags.FlagBroadcastMode, flags.BroadcastBlock), @@ -89,7 +89,7 @@ func (s *IntegrationTestSuite) SetupSuite() { out, err = CreateGrant(val, []string{ s.grantee[2].String(), "send", - fmt.Sprintf("--%s=100steak", cli.FlagSpendLimit), + fmt.Sprintf("--%s=100stake", cli.FlagSpendLimit), fmt.Sprintf("--%s=%s", flags.FlagFrom, val.Address.String()), fmt.Sprintf("--%s=true", flags.FlagSkipConfirmation), fmt.Sprintf("--%s=%s", flags.FlagBroadcastMode, flags.BroadcastBlock), @@ -156,45 +156,49 @@ func (s *IntegrationTestSuite) TestCLITxGrantAuthorization() { args []string expectedCode uint32 expectErr bool + expErrMsg string }{ { "Invalid granter Address", []string{ "grantee_addr", "send", - fmt.Sprintf("--%s=100steak", cli.FlagSpendLimit), + fmt.Sprintf("--%s=100stake", cli.FlagSpendLimit), fmt.Sprintf("--%s=%s", flags.FlagFrom, "granter"), fmt.Sprintf("--%s=true", flags.FlagGenerateOnly), fmt.Sprintf("--%s=%d", cli.FlagExpiration, twoHours), }, 0, true, + "key not found", }, { "Invalid grantee Address", []string{ "grantee_addr", "send", - fmt.Sprintf("--%s=100steak", cli.FlagSpendLimit), + fmt.Sprintf("--%s=100stake", cli.FlagSpendLimit), fmt.Sprintf("--%s=%s", flags.FlagFrom, val.Address.String()), fmt.Sprintf("--%s=true", flags.FlagGenerateOnly), fmt.Sprintf("--%s=%d", cli.FlagExpiration, twoHours), }, 0, true, + "invalid separator index", }, { "Invalid expiration time", []string{ grantee.String(), "send", - fmt.Sprintf("--%s=100steak", cli.FlagSpendLimit), + fmt.Sprintf("--%s=100stake", cli.FlagSpendLimit), fmt.Sprintf("--%s=%s", flags.FlagFrom, val.Address.String()), fmt.Sprintf("--%s=true", flags.FlagBroadcastMode), fmt.Sprintf("--%s=%d", cli.FlagExpiration, pastHour), }, 0, true, + "", }, { "fail with error invalid msg-type", @@ -210,6 +214,7 @@ func (s *IntegrationTestSuite) TestCLITxGrantAuthorization() { }, 0x1d, false, + "", }, { "failed with error both validators not allowed", @@ -227,6 +232,92 @@ func (s *IntegrationTestSuite) TestCLITxGrantAuthorization() { }, 0, true, + "cannot set both allowed & deny list", + }, + { + "invalid bond denom for tx delegate authorization allowed validators", + []string{ + grantee.String(), + "delegate", + fmt.Sprintf("--%s=100xyz", cli.FlagSpendLimit), + fmt.Sprintf("--%s=true", flags.FlagSkipConfirmation), + fmt.Sprintf("--%s=%s", flags.FlagFrom, val.Address.String()), + fmt.Sprintf("--%s=%s", flags.FlagBroadcastMode, flags.BroadcastBlock), + fmt.Sprintf("--%s=%d", cli.FlagExpiration, twoHours), + fmt.Sprintf("--%s=%s", cli.FlagAllowedValidators, val.ValAddress.String()), + fmt.Sprintf("--%s=%s", flags.FlagFees, sdk.NewCoins(sdk.NewCoin(s.cfg.BondDenom, sdk.NewInt(10))).String()), + }, + 0, + true, + "invalid denom", + }, + { + "invalid bond denom for tx delegate authorization deny validators", + []string{ + grantee.String(), + "delegate", + fmt.Sprintf("--%s=100xyz", cli.FlagSpendLimit), + fmt.Sprintf("--%s=true", flags.FlagSkipConfirmation), + fmt.Sprintf("--%s=%s", flags.FlagFrom, val.Address.String()), + fmt.Sprintf("--%s=%s", flags.FlagBroadcastMode, flags.BroadcastBlock), + fmt.Sprintf("--%s=%d", cli.FlagExpiration, twoHours), + fmt.Sprintf("--%s=%s", cli.FlagDenyValidators, val.ValAddress.String()), + fmt.Sprintf("--%s=%s", flags.FlagFees, sdk.NewCoins(sdk.NewCoin(s.cfg.BondDenom, sdk.NewInt(10))).String()), + }, + 0, + true, + "invalid denom", + }, + { + "invalid bond denom for tx undelegate authorization", + []string{ + grantee.String(), + "unbond", + fmt.Sprintf("--%s=100xyz", cli.FlagSpendLimit), + fmt.Sprintf("--%s=true", flags.FlagSkipConfirmation), + fmt.Sprintf("--%s=%s", flags.FlagFrom, val.Address.String()), + fmt.Sprintf("--%s=%s", flags.FlagBroadcastMode, flags.BroadcastBlock), + fmt.Sprintf("--%s=%d", cli.FlagExpiration, twoHours), + fmt.Sprintf("--%s=%s", cli.FlagAllowedValidators, val.ValAddress.String()), + fmt.Sprintf("--%s=%s", flags.FlagFees, sdk.NewCoins(sdk.NewCoin(s.cfg.BondDenom, sdk.NewInt(10))).String()), + }, + 0, + true, + "invalid denom", + }, + { + "invalid bond denon for tx redelegate authorization", + []string{ + grantee.String(), + "redelegate", + fmt.Sprintf("--%s=100xyz", cli.FlagSpendLimit), + fmt.Sprintf("--%s=true", flags.FlagSkipConfirmation), + fmt.Sprintf("--%s=%s", flags.FlagFrom, val.Address.String()), + fmt.Sprintf("--%s=%s", flags.FlagBroadcastMode, flags.BroadcastBlock), + fmt.Sprintf("--%s=%d", cli.FlagExpiration, twoHours), + fmt.Sprintf("--%s=%s", cli.FlagAllowedValidators, val.ValAddress.String()), + fmt.Sprintf("--%s=%s", flags.FlagFees, sdk.NewCoins(sdk.NewCoin(s.cfg.BondDenom, sdk.NewInt(10))).String()), + }, + 0, + true, + "invalid denom", + }, + { + "invalid decimal coin expression with more than single coin", + []string{ + grantee.String(), + "delegate", + fmt.Sprintf("--%s=100stake,20xyz", cli.FlagSpendLimit), + fmt.Sprintf("--%s=true", flags.FlagSkipConfirmation), + fmt.Sprintf("--%s=%s", flags.FlagFrom, val.Address.String()), + fmt.Sprintf("--%s=%s", flags.FlagBroadcastMode, flags.BroadcastBlock), + fmt.Sprintf("--%s=%d", cli.FlagExpiration, twoHours), + fmt.Sprintf("--%s=%s", cli.FlagAllowedValidators, val.ValAddress.String()), + fmt.Sprintf("--%s=%s", flags.FlagFees, sdk.NewCoins(sdk.NewCoin(s.cfg.BondDenom, sdk.NewInt(10))).String()), + }, + 0, + true, + "invalid decimal coin expression", }, { "valid tx delegate authorization allowed validators", @@ -243,6 +334,7 @@ func (s *IntegrationTestSuite) TestCLITxGrantAuthorization() { }, 0, false, + "", }, { "valid tx delegate authorization deny validators", @@ -259,6 +351,7 @@ func (s *IntegrationTestSuite) TestCLITxGrantAuthorization() { }, 0, false, + "", }, { "valid tx undelegate authorization", @@ -275,6 +368,7 @@ func (s *IntegrationTestSuite) TestCLITxGrantAuthorization() { }, 0, false, + "", }, { "valid tx redelegate authorization", @@ -291,13 +385,14 @@ func (s *IntegrationTestSuite) TestCLITxGrantAuthorization() { }, 0, false, + "", }, { "Valid tx send authorization", []string{ grantee.String(), "send", - fmt.Sprintf("--%s=100steak", cli.FlagSpendLimit), + fmt.Sprintf("--%s=100stake", cli.FlagSpendLimit), fmt.Sprintf("--%s=%s", flags.FlagFrom, val.Address.String()), fmt.Sprintf("--%s=%s", flags.FlagBroadcastMode, flags.BroadcastBlock), fmt.Sprintf("--%s=%d", cli.FlagExpiration, twoHours), @@ -306,6 +401,7 @@ func (s *IntegrationTestSuite) TestCLITxGrantAuthorization() { }, 0, false, + "", }, { "Valid tx generic authorization", @@ -321,6 +417,7 @@ func (s *IntegrationTestSuite) TestCLITxGrantAuthorization() { }, 0, false, + "", }, { "fail when granter = grantee", @@ -336,6 +433,7 @@ func (s *IntegrationTestSuite) TestCLITxGrantAuthorization() { }, 0, true, + "grantee and granter should be different", }, { "Valid tx with amino", @@ -352,6 +450,7 @@ func (s *IntegrationTestSuite) TestCLITxGrantAuthorization() { }, 0, false, + "", }, } @@ -364,6 +463,7 @@ func (s *IntegrationTestSuite) TestCLITxGrantAuthorization() { ) if tc.expectErr { s.Require().Error(err, out) + s.Require().Contains(err.Error(), tc.expErrMsg) } else { var txResp sdk.TxResponse s.Require().NoError(err) @@ -392,7 +492,7 @@ func (s *IntegrationTestSuite) TestCmdRevokeAuthorizations() { []string{ grantee.String(), "send", - fmt.Sprintf("--%s=100steak", cli.FlagSpendLimit), + fmt.Sprintf("--%s=100stake", cli.FlagSpendLimit), fmt.Sprintf("--%s=true", flags.FlagSkipConfirmation), fmt.Sprintf("--%s=%s", flags.FlagFrom, val.Address), fmt.Sprintf("--%s=%s", flags.FlagBroadcastMode, flags.BroadcastBlock), diff --git a/x/authz/keeper/grpc_query.go b/x/authz/keeper/grpc_query.go index 11bf0715efe7..634a0f6e2313 100644 --- a/x/authz/keeper/grpc_query.go +++ b/x/authz/keeper/grpc_query.go @@ -20,6 +20,7 @@ import ( var _ authz.QueryServer = Keeper{} // Authorizations implements the Query/Grants gRPC method. +// It returns grants for a granter-grantee pair. If msg type URL is set, it returns grants only for that msg type. func (k Keeper) Grants(c context.Context, req *authz.QueryGrantsRequest) (*authz.QueryGrantsResponse, error) { if req == nil { return nil, status.Errorf(codes.InvalidArgument, "empty request") diff --git a/x/authz/keeper/keeper.go b/x/authz/keeper/keeper.go index b879f091d246..0a5f28234133 100644 --- a/x/authz/keeper/keeper.go +++ b/x/authz/keeper/keeper.go @@ -199,8 +199,6 @@ func (k Keeper) DeleteGrant(ctx sdk.Context, grantee sdk.AccAddress, granter sdk return sdkerrors.Wrapf(authz.ErrNoAuthorizationFound, "failed to delete grant with key %s", string(skey)) } - store.Delete(skey) - if grant.Expiration != nil { err := k.removeFromGrantQueue(ctx, skey, granter, grantee, *grant.Expiration) if err != nil { @@ -208,6 +206,8 @@ func (k Keeper) DeleteGrant(ctx sdk.Context, grantee sdk.AccAddress, granter sdk } } + store.Delete(skey) + return ctx.EventManager().EmitTypedEvent(&authz.EventRevoke{ MsgTypeUrl: msgType, Granter: granter.String(), diff --git a/x/staking/types/authz.go b/x/staking/types/authz.go index 4bd567c464c0..3a6d60c87544 100644 --- a/x/staking/types/authz.go +++ b/x/staking/types/authz.go @@ -10,14 +10,13 @@ import ( // Tracking issues https://github.com/cosmos/cosmos-sdk/issues/9054, https://github.com/cosmos/cosmos-sdk/discussions/9072 const gasCostPerIteration = uint64(10) -// Normalized Msg type URLs var ( _ authz.Authorization = &StakeAuthorization{} ) // NewStakeAuthorization creates a new StakeAuthorization object. func NewStakeAuthorization(allowed []sdk.ValAddress, denied []sdk.ValAddress, authzType AuthorizationType, amount *sdk.Coin) (*StakeAuthorization, error) { - allowedValidators, deniedValidators, err := validateAndBech32fy(allowed, denied) + allowedValidators, deniedValidators, err := validateAllowAndDenyValidators(allowed, denied) if err != nil { return nil, err } @@ -114,7 +113,7 @@ func (a StakeAuthorization) Accept(ctx sdk.Context, msg sdk.Msg) (authz.AcceptRe Updated: &StakeAuthorization{Validators: a.GetValidators(), AuthorizationType: a.GetAuthorizationType(), MaxTokens: &limitLeft}}, nil } -func validateAndBech32fy(allowed []sdk.ValAddress, denied []sdk.ValAddress) ([]string, []string, error) { +func validateAllowAndDenyValidators(allowed []sdk.ValAddress, denied []sdk.ValAddress) ([]string, []string, error) { if len(allowed) == 0 && len(denied) == 0 { return nil, nil, sdkerrors.ErrInvalidRequest.Wrap("both allowed & deny list cannot be empty") } @@ -139,6 +138,7 @@ func validateAndBech32fy(allowed []sdk.ValAddress, denied []sdk.ValAddress) ([]s return nil, deniedValidators, nil } +// Normalized Msg type URLs func normalizeAuthzType(authzType AuthorizationType) (string, error) { switch authzType { case AuthorizationType_AUTHORIZATION_TYPE_DELEGATE: