x/authz module suggestions for minor code improvements

[dalmirel]

As a part of model based testing audit of the authz module, code inspection has been done in order to find issues or possible improvements in code.
The following are recommendations for slightly neater and maybe better organized code (as far as recommended module structure). from a perspective of someone who is just getting introduced to authz implementation and Cosmos SDK itself.

Version:
master (commit: 8800d2e)
auditing is performed for tag: v0.46.0-alpha4 (commit: 354faa8)

Code improvement suggestions:

1. validateAndBech32fy – possibly validateAllowAndDenyValidators as the name of the function or something similar would be more appropriate taking into account the logic of the func?
   

   cosmos-sdk/x/staking/types/authz.go

   Line 119 in 8800d2e

   func validateAndBech32fy(allowed []sdk.ValAddress, denied []sdk.ValAddress) ([]string, []string, error) {

2. Move the line outside of /bellow of the switch statement, in order not to duplicate code:

   cosmos-sdk/x/staking/types/authz.go

   Line 68 in 8800d2e

   amount = msg.Amount

3. This line here finished here probably by merge mistake, this comment should be on top of this

   cosmos-sdk/x/staking/types/authz.go

   Line 144 in 8800d2e

   func normalizeAuthzType(authzType AuthorizationType) (string, error) {

4. Define variable for cached value av := g.Authorization.GetCachedValue() in
   

   cosmos-sdk/x/authz/authorization_grant.go

   Line 50 in 6d1525f

   a, ok := g.Authorization.GetCachedValue().(Authorization)

   
   And return this value in

   cosmos-sdk/x/authz/authorization_grant.go

   Line 52 in 6d1525f

   return nil, sdkerrors.ErrInvalidType.Wrapf("expected %T, got %T", (Authorization)(nil), g.Authorization.GetCachedValue())

   the same way as in ValidateBasic func

5. Adding some comments for query Grants function in order to help understanding since there are two return branches and different logics in one method - depending on MsgTypeUrl - as stated in

   cosmos-sdk/proto/cosmos/authz/v1beta1/query.proto

   Lines 38 to 39 in 8800d2e

   	// Optional, msg_type_url, when set, will query only grants matching given msg type.
   	string msg_type_url = 3;

   Or perhaps define authorizations var authorizations []*authz.Grant and pagination var pageRes *query.PageResponse variables, assign them values in both cases (grants for certain MsgType and in case of querying all MsgTypes grants) and have only one return from Grants func.

6. Even though this is happening in tx - and no store changes will be committed in the end if err is returned, I think it is cleaner to move store.deletion: after the successful deletion of grant queue value and before emitting event.

   cosmos-sdk/x/authz/keeper/keeper.go

   Lines 199 to 208 in fdd3d07

   	store.Delete(skey)
   	if grant.Expiration != nil {
   	err := k.removeFromGrantQueue(ctx, skey, granter, grantee, *grant.Expiration)
   	if err != nil {
   	return err
   	}
   	}
   	return ctx.EventManager().EmitTypedEvent(&authz.EventRevoke{

7. if you wish to follow the guide for building modules and recommended module structure, since there is genesis_test.go with unit tests for these functions, I suggest you create keeper/genesis.go file with InitGenesis and ExportGenesis functions (move them from keeper.go file .

8. Note all Grant Queue key parts in comments that should explain key format here and/or here.

9. NewCmdGrantAuthorization() improvements:

• add info that spending limit is entered from CLI as max token amount for staking authorizations, because currently, we have:
  

  cosmos-sdk/x/authz/client/cli/tx.go

  Line 173 in ea8b0f7

  cmd.Flags().String(FlagSpendLimit, "", "SpendLimit for Send Authorization, an array of Coins allowed spend")

• since spendLimit could be an array of coins for send authorizations, return an error for stake authorizations in case that more than one spending limit has been entered. Current implementation is:

  cosmos-sdk/x/authz/client/cli/tx.go

  Line 129 in ea8b0f7

  delegateLimit = &spendLimit[0]

  
  so, only first limit would be considered.
• currently stake authorizations are created for denom that could differ from bond_denom. There is no check, but on the other hand I could not find a way to access this information. I got a link from @AmauryM to an existing issue: Expose chain information via new RPC route #11582 and an explanation that currently, we are creating "invalid" stake authorizations and leave them be until, that concrete authorized stake transactions hit the node and get rejected because of invalid denom.