-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Changes from all commits
782a918
e6c1d01
a375d7c
d8cd1ef
449d905
29b6bf4
5805809
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,4 @@ | ||
//go:build norace | ||
// +build norace | ||
|
||
package testutil | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -198,7 +198,7 @@ func (s *IntegrationTestSuite) TestCmdGetFeeGrant() { | |
} | ||
} | ||
|
||
func (s *IntegrationTestSuite) TestCmdGetFeeGrants() { | ||
func (s *IntegrationTestSuite) TestCmdGetFeeGrantsByGrantee() { | ||
val := s.network.Validators[0] | ||
grantee := s.addedGrantee | ||
clientCtx := val.ClientCtx | ||
|
@@ -219,7 +219,7 @@ func (s *IntegrationTestSuite) TestCmdGetFeeGrants() { | |
true, nil, 0, | ||
}, | ||
{ | ||
"non existed grantee", | ||
"non existent grantee", | ||
[]string{ | ||
"cosmos1nph3cfzk6trsmfxkeu943nvach5qw4vwstnvkl", | ||
fmt.Sprintf("--%s=json", tmcli.OutputFlag), | ||
|
@@ -240,7 +240,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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you also add integration tests for There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.QueryAllowancesByGranterResponse | ||
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.QueryAllowancesByGranterResponse{}, 0, | ||
}, | ||
{ | ||
"valid req", | ||
[]string{ | ||
granter.String(), | ||
fmt.Sprintf("--%s=json", tmcli.OutputFlag), | ||
}, | ||
false, &feegrant.QueryAllowancesByGranterResponse{}, 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 { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -93,3 +93,42 @@ func (q Keeper) Allowances(c context.Context, req *feegrant.QueryAllowancesReque | |
|
||
return &feegrant.QueryAllowancesResponse{Allowances: grants, Pagination: pageRes}, nil | ||
} | ||
|
||
// AllowancesByGranter queries all the allowances granted by the given granter | ||
func (q Keeper) AllowancesByGranter(c context.Context, req *feegrant.QueryAllowancesByGranterRequest) (*feegrant.QueryAllowancesByGranterResponse, 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
|
||
|
||
if err != nil { | ||
return nil, status.Error(codes.Internal, err.Error()) | ||
} | ||
|
||
return &feegrant.QueryAllowancesByGranterResponse{Allowances: grants, Pagination: pageRes}, nil | ||
} |
There was a problem hiding this comment.
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 withAllowances
.Maybe
AllowancesByGranter
to be more explicitThere was a problem hiding this comment.
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
toAllowancesByGrantee
or leave it as is?There was a problem hiding this comment.
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
overAllowances+IssuedAllowances
There was a problem hiding this comment.
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