Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add feegrant query to see allowances from a given granter #10947

Merged
merged 7 commits into from
Jan 21, 2022
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 22 additions & 0 deletions proto/cosmos/feegrant/v1beta1/query.proto
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,11 @@ service Query {
rpc Allowances(QueryAllowancesRequest) returns (QueryAllowancesResponse) {
option (google.api.http).get = "/cosmos/feegrant/v1beta1/allowances/{grantee}";
}

// IssuedAllowances returns all the grants given by an address
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's add a //Since comment. Any plans to backport these?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't mind backporting this. I'll defer the decision to others

rpc IssuedAllowances(QueryIssuedAllowancesRequest) returns (QueryIssuedAllowancesResponse) {
option (google.api.http).get = "/cosmos/feegrant/v1beta1/issued/{granter}";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I like the IssuedAllowances name. It's sounds confusing with Allowances.

Maybe AllowancesByGranter to be more explicit

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would we want to change Allowances to AllowancesByGrantee or leave it as is?

Copy link
Contributor

@amaury1093 amaury1093 Jan 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would be proto-breaking, so no, though it would be the ideal choice.

I still prefer Allowances+AllowancesByGranter over Allowances+IssuedAllowances

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok I will make the change

}
}

// QueryAllowanceRequest is the request type for the Query/Allowance RPC method.
Expand Down Expand Up @@ -54,3 +59,20 @@ message QueryAllowancesResponse {
// pagination defines an pagination for the response.
cosmos.base.query.v1beta1.PageResponse pagination = 2;
}

// QueryIssuedAllowancesRequest is the request type for the Query/IssuedAllowances RPC method.
message QueryIssuedAllowancesRequest {
string granter = 1 [(cosmos_proto.scalar) = "cosmos.AddressString"];

// pagination defines an pagination for the request.
cosmos.base.query.v1beta1.PageRequest pagination = 2;
}

// QueryIssuedAllowancesResponse is the response type for the Query/IssuedAllowances RPC method.
message QueryIssuedAllowancesResponse {
// allowances that have been issued by the granter.
repeated cosmos.feegrant.v1beta1.Grant allowances = 1;

// pagination defines an pagination for the response.
cosmos.base.query.v1beta1.PageResponse pagination = 2;
}
56 changes: 53 additions & 3 deletions x/feegrant/client/cli/query.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,8 @@ func GetQueryCmd() *cobra.Command {

feegrantQueryCmd.AddCommand(
GetCmdQueryFeeGrant(),
GetCmdQueryFeeGrants(),
GetCmdQueryFeeGrantsByGrantee(),
GetCmdQueryFeeGrantsByGranter(),
)

return feegrantQueryCmd
Expand Down Expand Up @@ -80,8 +81,8 @@ $ %s query feegrant grant [granter] [grantee]
return cmd
}

// GetCmdQueryFeeGrants returns cmd to query for all grants for a grantee.
func GetCmdQueryFeeGrants() *cobra.Command {
// GetCmdQueryFeeGrantsByGrantee returns cmd to query for all grants for a grantee.
func GetCmdQueryFeeGrantsByGrantee() *cobra.Command {
cmd := &cobra.Command{
Use: "grants [grantee]",
Args: cobra.ExactArgs(1),
Expand Down Expand Up @@ -128,3 +129,52 @@ $ %s query feegrant grants [grantee]

return cmd
}

// GetCmdQueryFeeGrantsByGranter returns cmd to query for all grants by a granter.
func GetCmdQueryFeeGrantsByGranter() *cobra.Command {
cmd := &cobra.Command{
Use: "grants [granter]",
Args: cobra.ExactArgs(1),
Short: "Query all grants by a granter",
Long: strings.TrimSpace(
fmt.Sprintf(`Queries all the grants issued for a granter address.

Example:
$ %s query feegrant grants [granter]
`, version.AppName),
),
RunE: func(cmd *cobra.Command, args []string) error {
clientCtx := client.GetClientContextFromCmd(cmd)
queryClient := feegrant.NewQueryClient(clientCtx)

granterAddr, err := sdk.AccAddressFromBech32(args[0])
if err != nil {
return err
}

pageReq, err := client.ReadPageRequest(cmd.Flags())
if err != nil {
return err
}

res, err := queryClient.IssuedAllowances(
cmd.Context(),
&feegrant.QueryIssuedAllowancesRequest{
Granter: granterAddr.String(),
Pagination: pageReq,
},
)

if err != nil {
return err
}

return clientCtx.PrintProto(res)
},
}

flags.AddQueryFlagsToCmd(cmd)
flags.AddPaginationFlagsToCmd(cmd, "grants")

return cmd
}
1 change: 1 addition & 0 deletions x/feegrant/client/testutil/cli_test.go
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
//go:build norace
// +build norace

package testutil
Expand Down
62 changes: 59 additions & 3 deletions x/feegrant/client/testutil/suite.go
Original file line number Diff line number Diff line change
Expand Up @@ -197,7 +197,7 @@ func (s *IntegrationTestSuite) TestCmdGetFeeGrant() {
}
}

func (s *IntegrationTestSuite) TestCmdGetFeeGrants() {
func (s *IntegrationTestSuite) TestCmdGetFeeGrantsByGrantee() {
val := s.network.Validators[0]
grantee := s.addedGrantee
clientCtx := val.ClientCtx
Expand All @@ -218,7 +218,7 @@ func (s *IntegrationTestSuite) TestCmdGetFeeGrants() {
true, nil, 0,
},
{
"non existed grantee",
"non existent grantee",
[]string{
"cosmos1nph3cfzk6trsmfxkeu943nvach5qw4vwstnvkl",
fmt.Sprintf("--%s=json", tmcli.OutputFlag),
Expand All @@ -239,7 +239,63 @@ func (s *IntegrationTestSuite) TestCmdGetFeeGrants() {
tc := tc

s.Run(tc.name, func() {
cmd := cli.GetCmdQueryFeeGrants()
cmd := cli.GetCmdQueryFeeGrantsByGrantee()
out, err := clitestutil.ExecTestCLICmd(clientCtx, cmd, tc.args)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you also add integration tests for GetCmdQueryFeeGrantsByGranter

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done 👍


if tc.expectErr {
s.Require().Error(err)
} else {
s.Require().NoError(err)
s.Require().NoError(clientCtx.Codec.UnmarshalJSON(out.Bytes(), tc.resp), out.String())
s.Require().Len(tc.resp.Allowances, tc.expectLength)
}
})
}
}

func (s *IntegrationTestSuite) TestCmdGetFeeGrantsByGranter() {
val := s.network.Validators[0]
granter := s.addedGranter
clientCtx := val.ClientCtx

testCases := []struct {
name string
args []string
expectErr bool
resp *feegrant.QueryIssuedAllowancesResponse
expectLength int
}{
{
"wrong grantee",
[]string{
"wrong_grantee",
fmt.Sprintf("--%s=json", tmcli.OutputFlag),
},
true, nil, 0,
},
{
"non existent grantee",
[]string{
"cosmos1nph3cfzk6trsmfxkeu943nvach5qw4vwstnvkl",
fmt.Sprintf("--%s=json", tmcli.OutputFlag),
},
false, &feegrant.QueryIssuedAllowancesResponse{}, 0,
},
{
"valid req",
[]string{
granter.String(),
fmt.Sprintf("--%s=json", tmcli.OutputFlag),
},
false, &feegrant.QueryIssuedAllowancesResponse{}, 1,
},
}

for _, tc := range testCases {
tc := tc

s.Run(tc.name, func() {
cmd := cli.GetCmdQueryFeeGrantsByGranter()
out, err := clitestutil.ExecTestCLICmd(clientCtx, cmd, tc.args)

if tc.expectErr {
Expand Down
2 changes: 1 addition & 1 deletion x/feegrant/filtered_fee_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,9 @@ import (
"time"

"github.com/cosmos/cosmos-sdk/x/feegrant"
ocproto "github.com/tendermint/tendermint/proto/tendermint/types"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
ocproto "github.com/tendermint/tendermint/proto/tendermint/types"

"github.com/cosmos/cosmos-sdk/simapp"
sdk "github.com/cosmos/cosmos-sdk/types"
Expand Down
39 changes: 39 additions & 0 deletions x/feegrant/keeper/grpc_query.go
Original file line number Diff line number Diff line change
Expand Up @@ -93,3 +93,42 @@ func (q Keeper) Allowances(c context.Context, req *feegrant.QueryAllowancesReque

return &feegrant.QueryAllowancesResponse{Allowances: grants, Pagination: pageRes}, nil
}

// IssuedAllowances queries all the allowances granted by the given granter
func (q Keeper) IssuedAllowances(c context.Context, req *feegrant.QueryIssuedAllowancesRequest) (*feegrant.QueryIssuedAllowancesResponse, error) {
if req == nil {
return nil, status.Error(codes.InvalidArgument, "invalid request")
}

granterAddr, err := sdk.AccAddressFromBech32(req.Granter)
if err != nil {
return nil, err
}

ctx := sdk.UnwrapSDKContext(c)

var grants []*feegrant.Grant

store := ctx.KVStore(q.storeKey)
pageRes, err := query.Paginate(store, req.Pagination, func(key []byte, value []byte) error {
var grant feegrant.Grant

granter, _ := feegrant.ParseAddressesFromFeeAllowanceKey(key)
if !granter.Equals(granterAddr) {
return nil
}

if err := q.cdc.Unmarshal(value, &grant); err != nil {
return err
}

grants = append(grants, &grant)
return nil
})
Comment on lines +113 to +127
Copy link
Contributor

@atheeshp atheeshp Jan 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think iterating all the store keys might not be a better idea (it can be possible that this store can be big).
cc: @aaronc , @AmauryM

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes the more efficient solution would be to have a secondary index and I think in the future we may want to have the feegrant module use orm. For now I think this suffices due to the following reasons:

  • The range scan is only parsing the keys and not the values to determine whether the allowance comes from the granter
  • I can't imagine chains having a state size greater than a few hundred entries
  • This is just a query endpoint (only affects individual nodes) and can be turned off from the public if need be.
  • Most use cases will be for individuals checking grants on their own machines. Most "commercial" level products will be using something like a postgres indexer for serving feegrant information


if err != nil {
return nil, status.Error(codes.Internal, err.Error())
}

return &feegrant.QueryIssuedAllowancesResponse{Allowances: grants, Pagination: pageRes}, nil
}
66 changes: 66 additions & 0 deletions x/feegrant/keeper/grpc_query_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,72 @@ func (suite *KeeperTestSuite) TestFeeAllowances() {
}
}

func (suite *KeeperTestSuite) TestFeeIssuedAllowances() {
testCases := []struct {
name string
req *feegrant.QueryIssuedAllowancesRequest
expectErr bool
preRun func()
postRun func(_ *feegrant.QueryIssuedAllowancesResponse)
}{
{
"nil request",
nil,
true,
func() {},
func(*feegrant.QueryIssuedAllowancesResponse) {},
},
{
"fail: invalid grantee",
&feegrant.QueryIssuedAllowancesRequest{
Granter: "invalid_grantee",
},
true,
func() {},
func(*feegrant.QueryIssuedAllowancesResponse) {},
},
{
"no grants",
&feegrant.QueryIssuedAllowancesRequest{
Granter: suite.addrs[0].String(),
},
false,
func() {},
func(resp *feegrant.QueryIssuedAllowancesResponse) {
suite.Require().Equal(len(resp.Allowances), 0)
},
},
{
"valid query: expect single grant",
&feegrant.QueryIssuedAllowancesRequest{
Granter: suite.addrs[0].String(),
},
false,
func() {
grantFeeAllowance(suite)
},
func(resp *feegrant.QueryIssuedAllowancesResponse) {
suite.Require().Equal(len(resp.Allowances), 1)
suite.Require().Equal(resp.Allowances[0].Granter, suite.addrs[0].String())
suite.Require().Equal(resp.Allowances[0].Grantee, suite.addrs[1].String())
},
},
}

for _, tc := range testCases {
suite.Run(tc.name, func() {
tc.preRun()
resp, err := suite.keeper.IssuedAllowances(suite.ctx, tc.req)
if tc.expectErr {
suite.Require().Error(err)
} else {
suite.Require().NoError(err)
tc.postRun(resp)
}
})
}
}

func grantFeeAllowance(suite *KeeperTestSuite) {
exp := suite.sdkCtx.BlockTime().AddDate(1, 0, 0)
err := suite.app.FeeGrantKeeper.GrantAllowance(suite.sdkCtx, suite.addrs[0], suite.addrs[1], &feegrant.BasicAllowance{
Expand Down
15 changes: 15 additions & 0 deletions x/feegrant/key.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package feegrant
import (
sdk "github.com/cosmos/cosmos-sdk/types"
"github.com/cosmos/cosmos-sdk/types/address"
"github.com/cosmos/cosmos-sdk/types/kv"
)

const (
Expand Down Expand Up @@ -34,3 +35,17 @@ func FeeAllowanceKey(granter sdk.AccAddress, grantee sdk.AccAddress) []byte {
func FeeAllowancePrefixByGrantee(grantee sdk.AccAddress) []byte {
return append(FeeAllowanceKeyPrefix, address.MustLengthPrefix(grantee.Bytes())...)
}

func ParseAddressesFromFeeAllowanceKey(key []byte) (granter, grantee sdk.AccAddress) {
// key is of format:
// 0x00<granteeAddressLen (1 Byte)><granteeAddress_Bytes><granterAddressLen (1 Byte)><granterAddress_Bytes><msgType_Bytes>
kv.AssertKeyAtLeastLength(key, 2)
granteeAddrLen := key[1] // remove prefix key
kv.AssertKeyAtLeastLength(key, int(2+granteeAddrLen))
grantee = sdk.AccAddress(key[2 : 2+granteeAddrLen])
granterAddrLen := int(key[2+granteeAddrLen])
kv.AssertKeyAtLeastLength(key, 3+int(granteeAddrLen+byte(granterAddrLen)))
granter = sdk.AccAddress(key[3+granterAddrLen : 3+granteeAddrLen+byte(granterAddrLen)])

return granter, grantee
}
25 changes: 25 additions & 0 deletions x/feegrant/key_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
package feegrant_test

import (
"testing"

"github.com/stretchr/testify/require"

sdk "github.com/cosmos/cosmos-sdk/types"
"github.com/cosmos/cosmos-sdk/x/feegrant"
)

func TestMarshalAndUnmarshalFeegrantKey(t *testing.T) {
grantee, err := sdk.AccAddressFromBech32("cosmos1qk93t4j0yyzgqgt6k5qf8deh8fq6smpn3ntu3x")
require.NoError(t, err)
granter, err := sdk.AccAddressFromBech32("cosmos1p9qh4ldfd6n0qehujsal4k7g0e37kel90rc4ts")
require.NoError(t, err)

key := feegrant.FeeAllowanceKey(granter, grantee)
require.Len(t, key, len(grantee.Bytes())+len(granter.Bytes())+3)
require.Equal(t, feegrant.FeeAllowancePrefixByGrantee(grantee), key[:len(grantee.Bytes())+2])

g1, g2 := feegrant.ParseAddressesFromFeeAllowanceKey(key)
require.Equal(t, granter, g1)
require.Equal(t, grantee, g2)
}
Loading