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

Conversation

cmwaters
Copy link
Contributor

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

  • included the correct type prefix in the PR title
  • added ! to the type prefix if API or client breaking change
  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • followed the guidelines for building modules
  • included the necessary unit and integration tests
  • added a changelog entry to CHANGELOG.md
  • included comments for documenting Go code
  • 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 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)

Copy link
Contributor

@atheeshp atheeshp left a comment

Choose a reason for hiding this comment

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

lgtm, thanks @cmwaters !

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

Comment on lines +113 to +127
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
})
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

Copy link
Contributor

@amaury1093 amaury1093 left a comment

Choose a reason for hiding this comment

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

lgtm overall, can we add a changelog entry?

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


// IssuedAllowances returns all the grants given by an address
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

@cmwaters cmwaters merged commit eb01537 into master Jan 21, 2022
@cmwaters cmwaters deleted the callum/feegrant-query branch January 21, 2022 14:55
@julienrbrt
Copy link
Member

@Mergifyio backport release/v0.45.x

@mergify
Copy link
Contributor

mergify bot commented Apr 29, 2022

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
Copy link
Contributor

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

@julienrbrt
Copy link
Member

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
Copy link
Contributor

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.

randy75828 pushed a commit to Switcheo/cosmos-sdk that referenced this pull request Aug 10, 2022
Eengineer1 pushed a commit to cheqd/cosmos-sdk that referenced this pull request Aug 26, 2022
JeancarloBarrios pushed a commit to agoric-labs/cosmos-sdk that referenced this pull request Sep 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants