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

cmwaters · 2022-01-13T14:47:34Z

Description

This PR adds a new query to the feegrant query server: IssuedAllowances which takes an address and returns all the allowances that the address has granted. This is a helpful query for UI tools so that users can more easily manage their feegrants (create, modify, delete).

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...

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 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)

atheeshp

lgtm, thanks @cmwaters !

atheeshp · 2022-01-18T05:21:09Z

x/feegrant/client/testutil/suite.go

@@ -239,7 +239,7 @@ 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)


Can you also add integration tests for GetCmdQueryFeeGrantsByGranter

atheeshp · 2022-01-18T05:53:13Z

x/feegrant/keeper/grpc_query.go

+	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
+	})


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

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

amaury1093

lgtm overall, can we add a changelog entry?

amaury1093 · 2022-01-18T15:59:11Z

proto/cosmos/feegrant/v1beta1/query.proto

@@ -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


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

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

amaury1093 · 2022-01-18T17:27:37Z

proto/cosmos/feegrant/v1beta1/query.proto

+
+  // IssuedAllowances returns all the grants given by an address
+  rpc IssuedAllowances(QueryIssuedAllowancesRequest) returns (QueryIssuedAllowancesResponse) {
+    option (google.api.http).get = "/cosmos/feegrant/v1beta1/issued/{granter}";


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

Maybe AllowancesByGranter to be more explicit

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

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

I still prefer Allowances+AllowancesByGranter over Allowances+IssuedAllowances

Ok I will make the change

julienrbrt · 2022-04-29T14:10:02Z

@Mergifyio backport release/v0.45.x

mergify · 2022-04-29T14:10:06Z

backport release/v0.45.x

❌ No backport have been created

Backport to branch release/v0.45.x failed: Git reported the following error:

fatal: couldn't find remote ref master

alexanderbez · 2022-04-29T14:12:58Z

We might need to do this one manually @julienrbrt. Maybe just checkout the commit and manually create a PR

julienrbrt · 2022-05-02T15:24:38Z

I have tried to cherry-pick the commit, but I wonder if the api/** changes should be ignored: this path does not exist in v0.45.

alexanderbez · 2022-05-02T17:18:38Z

if it does not exist in 0.45, then yes, it can be ignored most likely. Double check with me if you're not sure though.

…port #10947) (#11885)

…port cosmos#10947) (cosmos#11885)

add feegrant query to see allowances from a given granter

782a918

cmwaters requested review from aaronc and alexanderbez as code owners January 13, 2022 14:47

github-actions bot added C:CLI C:x/feegrant labels Jan 13, 2022

amaury1093 assigned amaury1093 and atheeshp Jan 13, 2022

fix build error

e6c1d01

atheeshp approved these changes Jan 18, 2022

View reviewed changes

atheeshp reviewed Jan 18, 2022

View reviewed changes

cmwaters added 2 commits January 18, 2022 11:38

add client integration tests

a375d7c

Merge branch 'master' into callum/feegrant-query

d8cd1ef

tac0turtle requested a review from amaury1093 January 18, 2022 11:15

amaury1093 reviewed Jan 18, 2022

View reviewed changes

cmwaters added 3 commits January 21, 2022 15:17

Merge branch 'master' into callum/feegrant-query

449d905

rename to allowances by granter

29b6bf4

add changelog

5805809

cmwaters merged commit eb01537 into master Jan 21, 2022

cmwaters deleted the callum/feegrant-query branch January 21, 2022 14:55

julienrbrt mentioned this pull request Apr 29, 2022

fix: grants by granter pagination total (backport #11813) #11828

Merged

julienrbrt pushed a commit that referenced this pull request May 6, 2022

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

4b7fb5c

julienrbrt mentioned this pull request May 6, 2022

feat: add feegrant query to see allowances from a given granter (backport #10947) #11885

Merged

19 tasks

alexanderbez pushed a commit that referenced this pull request May 6, 2022

feat: add feegrant query to see allowances from a given granter (back…

8bbeba5

…port #10947) (#11885)

likhita-809 mentioned this pull request May 20, 2022

Manual Testing for v0.46 #11880

Closed

72 tasks

atheeshp mentioned this pull request May 24, 2022

fix: cli grants-by-grantee, grants-by-granter cmds #11983

Merged

19 tasks

randy75828 pushed a commit to Switcheo/cosmos-sdk that referenced this pull request Aug 10, 2022

feat: add feegrant query to see allowances from a given granter (back…

b90c151

…port cosmos#10947) (cosmos#11885)

Eengineer1 pushed a commit to cheqd/cosmos-sdk that referenced this pull request Aug 26, 2022

feat: add feegrant query to see allowances from a given granter (back…

d849bb7

…port cosmos#10947) (cosmos#11885)

faddat mentioned this pull request Dec 11, 2023

chore: update to v0.45.16 cudoventures/cosmos-sdk#16

Draft

19 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

cmwaters commented Jan 13, 2022

atheeshp left a comment

atheeshp Jan 18, 2022

cmwaters Jan 18, 2022

atheeshp Jan 18, 2022 •

edited

Loading

cmwaters Jan 18, 2022

amaury1093 left a comment

amaury1093 Jan 18, 2022

cmwaters Jan 21, 2022

amaury1093 Jan 18, 2022

cmwaters Jan 20, 2022

amaury1093 Jan 21, 2022 •

edited

Loading

cmwaters Jan 21, 2022

julienrbrt commented Apr 29, 2022

mergify bot commented Apr 29, 2022

alexanderbez commented Apr 29, 2022

julienrbrt commented May 2, 2022

alexanderbez commented May 2, 2022

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

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

Conversation

cmwaters commented Jan 13, 2022

Description

Author Checklist

Reviewers Checklist

atheeshp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

atheeshp Jan 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amaury1093 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amaury1093 Jan 21, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

julienrbrt commented Apr 29, 2022

mergify bot commented Apr 29, 2022

❌ No backport have been created

alexanderbez commented Apr 29, 2022

julienrbrt commented May 2, 2022

alexanderbez commented May 2, 2022

atheeshp Jan 18, 2022 •

edited

Loading

amaury1093 Jan 21, 2022 •

edited

Loading