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.

Sign up for GitHub

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

Jump to bottom

Check error for RegisterQueryHandlerClient in all modules RegisterGRPCGatewayRoutes #10437

Closed
4 tasks
ValarDragon opened this issue Oct 26, 2021 · 4 comments · Fixed by #10439
Closed
4 tasks

Check error for RegisterQueryHandlerClient in all modules RegisterGRPCGatewayRoutes #10437

ValarDragon opened this issue Oct 26, 2021 · 4 comments · Fixed by #10439
Assignees
@likhita-809
Labels
good first issue

Comments

@ValarDragon
Copy link
Contributor

ValarDragon commented Oct 26, 2021 •
edited
Loading

Summary of Bug

Every modules RegisterGRPCGatewayRoutes calls RegisterQueryHandlerClient but doesn't check the error. This should instead be oneof:

  • The error should be checked and panicked upon
  • types.RegisterGRPCGatewayRoutes should be types.MustRegisterGRPCGatewayRoutes and internally check the error
  • a // nolint:errcheck should be applied.

I am confused why errcheck isn't getting ran / applied on the SDK and erroring this on all the PRs, but should be addressed as downstream libraries follow the template set in the SDK modules.

See: https://github.com/cosmos/cosmos-sdk/blob/master/x/authz/module/module.go#L78-L80

For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged
  • Contributor assigned/self-assigned
@ValarDragon ValarDragon mentioned this issue Oct 26, 2021
Merged
@likhita-809 likhita-809 mentioned this issue Oct 26, 2021
Merged
19 tasks
@likhita-809
Copy link
Contributor

  • types.RegisterGRPCGatewayRoutes should be types.MustRegisterGRPCGatewayRoutes and internally check the error

@ValarDragon do you mean to change RegisterGRPCGatewayRoutes to MustRegisterGRPCGatewayRoutes or types.RegisterQueryHandlerClient to types.MustRegisterQueryHandlerClient ?

@likhita-809 likhita-809 self-assigned this Oct 26, 2021
@ValarDragon
Copy link
Contributor Author

ValarDragon commented Oct 26, 2021 •
edited
Loading

types.RegisterQueryHandlerClient to types.MustRegisterQueryHandlerClient is whats intended for that.

@likhita-809
Copy link
Contributor

likhita-809 commented Oct 27, 2021 •
edited
Loading

types.RegisterQueryHandlerClient to types.MustRegisterQueryHandlerClient is whats intended for that.

It seems like RegisterQueryHandlerClient comes from auto-generated code. Not sure, if we could change this. Any other suggestions @ValarDragon ?

@ValarDragon
Copy link
Contributor Author

your PR seems good to me then@

@mergify mergify bot closed this as completed in #10439 Oct 28, 2021
mergify bot pushed a commit that referenced this issue Oct 28, 2021
@likhita-809
fix: Check error for RegisterQueryHandlerClient in all modules Regist…
b5ca8e0 
…erGRPCGatewayRoutes (#10439)

<!--
The default pull request template is for types feat, fix, or refactor.
For other templates, add one of the following parameters to the url:
- template=docs.md
- template=other.md
-->

## Description

Closes: #10437 

Checks the error for `RegisterQueryHandlerClient` in all modules `RegisterGRPCGatewayRoutes`

---

### 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](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [ ] added `!` to the type prefix if API or client breaking change
- [ ] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#pr-targeting))
- [ ] provided a link to the relevant issue or specification
- [ ] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/master/docs/building-modules)
- [ ] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#testing)
- [ ] added a changelog entry to `CHANGELOG.md`
- [ ] included comments for [documenting Go code](https://blog.golang.org/godoc)
- [ ] 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](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) 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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@likhita-809 likhita-809

Labels
good first issue
Projects
None yet
Milestone
No milestone
3 participants
@ValarDragon @barriebyron @likhita-809