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

[Code Health] Ensure all message/query handler return gRPC status errors #860

Closed
1 of 7 tasks
bryanchriswhite opened this issue Oct 7, 2024 · 3 comments · Fixed by #955, #956, #959, #960 or #961
Closed
1 of 7 tasks

[Code Health] Ensure all message/query handler return gRPC status errors #860

bryanchriswhite opened this issue Oct 7, 2024 · 3 comments · Fixed by #955, #956, #959, #960 or #961
Assignees
@bryanchriswhite
Labels
code health Cleans up some code off-chain Off-chain business logic on-chain On-chain business logic
Milestone
Shannon Beta Test...

Comments

@bryanchriswhite
Copy link
Contributor

bryanchriswhite commented Oct 7, 2024 •
edited
Loading

Objective

Consistently encapsulate gRPC errors in gRPC status.Error() with appropriate status codes.

Origin Document

Observations made while working on #612.

While we already often wrap errors in custom cosmos-sdk error types, we SHOULD also be wrapping any of those errors which find their way back to a gRPC client in a gRPC status error. See: google.golang.org/grpc/status.

Goals

  • Align with best practices.
  • Improve gRPC client expectations and UX.
  • Improve consistency.

Deliverables

  • A PR for each module which:
    • wraps any non-status errors in all gRPC message/query handlers in an appropriately coded* gRPC status error.
    • updates status codes on existing status errors for which there is a more appropriate code*.
  • A PR which adds a CLI tool which detects msg/query handler methods with non-status return errors.
  • A PR which runs the CLI tool in CI to mitigate regressions on the main branch.

*(see: codes.Code)

Non-goals / Non-deliverables

  • Refactoring (moving / consolidating / renaming / etc.) error variables or types.
  • Refactoring message/query handler logic.

General deliverables

  • Comments: Add/update TODOs and comments alongside the source code so it is easier to follow.
  • Testing: Add new tests (unit and/or E2E) to the test suite.
  • Makefile: Add new targets to the Makefile to make the new functionality easier to use.
  • Documentation: Update architectural or development READMEs; use mermaid diagrams where appropriate.

Creator: @bryanchriswhite
@bryanchriswhite bryanchriswhite added off-chain Off-chain business logic on-chain On-chain business logic code health Cleans up some code labels Oct 7, 2024
@bryanchriswhite bryanchriswhite self-assigned this Oct 7, 2024
@bryanchriswhite bryanchriswhite moved this to 🔖 Ready in Shannon Oct 7, 2024
@Olshansk
Copy link
Member

Olshansk commented Nov 4, 2024

@bryanchriswhite Where are we on this?

@bryanchriswhite
Copy link
Contributor Author

Not started but expected to be low effort. I intended to do this in parallel with a higher effort issue (like #543).

@bryanchriswhite bryanchriswhite moved this from 🔖 Ready to 🏗 In progress in Shannon Nov 17, 2024
@bryanchriswhite bryanchriswhite mentioned this issue Nov 18, 2024
Draft
15 tasks
@bryanchriswhite bryanchriswhite linked a pull request Nov 18, 2024 that will close this issue
[Tooling] Add protocheck status-errors subcmd to check for non-gRPC status error returns #929
Draft
15 tasks
This was referenced Nov 22, 2024
Merged
Merged
Merged
Merged
Merged
Merged
@bryanchriswhite bryanchriswhite moved this from 🏗 In progress to 👀 In review in Shannon Dec 2, 2024
bryanchriswhite added a commit that referenced this issue Dec 2, 2024
@bryanchriswhite
[Code Health] fix: service module gRPC status error returns (#959)
1c490ce 
## Summary

Ensure all service module message and query handlers return gRPC status
errors.

## Issue

- #860 

## Type of change

Select one or more from the following:

- [ ] New feature, functionality or library
- [x] Consensus breaking; add the `consensus-breaking` label if so. See
#791 for details
- [ ] Bug fix
- [x] Code health or cleanup
- [ ] Documentation
- [ ] Other (specify)

## Testing

- [ ] **Documentation**: `make docusaurus_start`; only needed if you
make doc changes
- [x] **Unit Tests**: `make go_develop_and_test`
- [ ] **LocalNet E2E Tests**: `make test_e2e`
- [x] **DevNet E2E Tests**: Add the `devnet-test-e2e` label to the PR.

## Sanity Checklist

- [x] I have tested my changes using the available tooling
- [ ] I have commented my code
- [x] I have performed a self-review of my own code; both comments &
source code
- [ ] I create and reference any new tickets, if applicable
- [ ] I have left TODOs throughout the codebase, if applicable
bryanchriswhite added a commit that referenced this issue Dec 2, 2024
@bryanchriswhite
[Code Health] fix: session module gRPC status return errors (#960)
41a373b 
## Summary

Ensure all session message and query handlers return gRPC status errors.

## Issue

- #860 

## Type of change

Select one or more from the following:

- [ ] New feature, functionality or library
- [x] Consensus breaking; add the `consensus-breaking` label if so. See
#791 for details
- [ ] Bug fix
- [x] Code health or cleanup
- [ ] Documentation
- [ ] Other (specify)

## Testing

- [ ] **Documentation**: `make docusaurus_start`; only needed if you
make doc changes
- [x] **Unit Tests**: `make go_develop_and_test`
- [ ] **LocalNet E2E Tests**: `make test_e2e`
- [x] **DevNet E2E Tests**: Add the `devnet-test-e2e` label to the PR.

## Sanity Checklist

- [x] I have tested my changes using the available tooling
- [ ] I have commented my code
- [x] I have performed a self-review of my own code; both comments &
source code
- [ ] I create and reference any new tickets, if applicable
- [ ] I have left TODOs throughout the codebase, if applicable
bryanchriswhite added a commit that referenced this issue Dec 2, 2024
@bryanchriswhite
[Code Health] fix: shared module gRPC return errors (#961)
9678997 
## Summary

Ensure all shared message and query handlers return gRPC status errors.

## Issue

- #860 

## Type of change

Select one or more from the following:

- [ ] New feature, functionality or library
- [x] Consensus breaking; add the `consensus-breaking` label if so. See
#791 for details
- [ ] Bug fix
- [x] Code health or cleanup
- [ ] Documentation
- [ ] Other (specify)

## Testing

- [ ] **Documentation**: `make docusaurus_start`; only needed if you
make doc changes
- [x] **Unit Tests**: `make go_develop_and_test`
- [ ] **LocalNet E2E Tests**: `make test_e2e`
- [x] **DevNet E2E Tests**: Add the `devnet-test-e2e` label to the PR.

## Sanity Checklist

- [x] I have tested my changes using the available tooling
- [ ] I have commented my code
- [x] I have performed a self-review of my own code; both comments &
source code
- [ ] I create and reference any new tickets, if applicable
- [ ] I have left TODOs throughout the codebase, if applicable
bryanchriswhite added a commit that referenced this issue Dec 2, 2024
@bryanchriswhite
[Code Health] fix: application module gRPC status error returns (#954)
f35e879 
## Summary

Ensure all application message and query handlers return gRPC status
errors.

## Issue

- #860 

## Type of change

Select one or more from the following:

- [ ] New feature, functionality or library
- [x] Consensus breaking; add the `consensus-breaking` label if so. See
#791 for details
- [ ] Bug fix
- [x] Code health or cleanup
- [ ] Documentation
- [ ] Other (specify)

## Testing

- [ ] **Documentation**: `make docusaurus_start`; only needed if you
make doc changes
- [x] **Unit Tests**: `make go_develop_and_test`
- [ ] **LocalNet E2E Tests**: `make test_e2e`
- [x] **DevNet E2E Tests**: Add the `devnet-test-e2e` label to the PR.

## Sanity Checklist

- [x] I have tested my changes using the available tooling
- [ ] I have commented my code
- [x] I have performed a self-review of my own code; both comments &
source code
- [ ] I create and reference any new tickets, if applicable
- [ ] I have left TODOs throughout the codebase, if applicable
@bryanchriswhite bryanchriswhite mentioned this issue Dec 2, 2024
Merged
15 tasks
This was linked to pull requests Dec 2, 2024
[Code Health] fix: gateway module gRPC status error returns #955
Merged
[Code Health] fix: proof module gRPC status error returns #956
Merged
@bryanchriswhite bryanchriswhite mentioned this issue Dec 2, 2024
Merged
15 tasks
This was linked to pull requests Dec 2, 2024
[Code Health] fix: service module gRPC status error returns #959
Merged
[Code Health] fix: session module gRPC status return errors #960
Merged
[Code Health] fix: shared module gRPC return errors #961
Merged
This was linked to pull requests Dec 2, 2024
[Code Health] fix: application module gRPC status error returns #954
Merged
[Code Health] fix: supplier module gRPC return errors #962
Merged
[Code Health] fix: tokenomics module gRPC return errors #963
Merged
bryanchriswhite added a commit that referenced this issue Dec 2, 2024
@bryanchriswhite
[Code Health] fix: supplier module gRPC return errors (#962)
1a6cabd 
## Summary

Ensure all supplier message and query handlers return gRPC status
errors.

## Issue

- #860

## Type of change

Select one or more from the following:

- [ ] New feature, functionality or library
- [x] Consensus breaking; add the `consensus-breaking` label if so. See
#791 for details
- [ ] Bug fix
- [x] Code health or cleanup
- [ ] Documentation
- [ ] Other (specify)

## Testing

- [ ] **Documentation**: `make docusaurus_start`; only needed if you
make doc changes
- [x] **Unit Tests**: `make go_develop_and_test`
- [ ] **LocalNet E2E Tests**: `make test_e2e`
- [x] **DevNet E2E Tests**: Add the `devnet-test-e2e` label to the PR.

## Sanity Checklist

- [x] I have tested my changes using the available tooling
- [ ] I have commented my code
- [x] I have performed a self-review of my own code; both comments &
source code
- [ ] I create and reference any new tickets, if applicable
- [ ] I have left TODOs throughout the codebase, if applicable
@github-project-automation github-project-automation bot moved this from 👀 In review to ✅ Done in Shannon Dec 2, 2024
bryanchriswhite added a commit that referenced this issue Dec 2, 2024
@bryanchriswhite @red-0ne
[Code Health] fix: tokenomics module gRPC return errors (#963)
74a23eb 
## Summary

Ensure all supplier message and query handlers return gRPC status
errors.

## Issue

- #860

## Type of change

Select one or more from the following:

- [ ] New feature, functionality or library
- [ ] Consensus breaking; add the `consensus-breaking` label if so. See
#791 for details
- [ ] Bug fix
- [x] Code health or cleanup
- [ ] Documentation
- [ ] Other (specify)

## Testing

- [ ] **Documentation**: `make docusaurus_start`; only needed if you
make doc changes
- [x] **Unit Tests**: `make go_develop_and_test`
- [ ] **LocalNet E2E Tests**: `make test_e2e`
- [x] **DevNet E2E Tests**: Add the `devnet-test-e2e` label to the PR.

## Sanity Checklist

- [x] I have tested my changes using the available tooling
- [ ] I have commented my code
- [x] I have performed a self-review of my own code; both comments &
source code
- [ ] I create and reference any new tickets, if applicable
- [ ] I have left TODOs throughout the codebase, if applicable

---------

Co-authored-by: red-0ne <[email protected]>
@bryanchriswhite bryanchriswhite moved this from ✅ Done to 🏗 In progress in Shannon Dec 2, 2024
@bryanchriswhite bryanchriswhite linked a pull request Dec 9, 2024 that will close this issue
[Code Health] Ensure supplier module msg responses are non-empty #971
Merged
15 tasks
@bryanchriswhite bryanchriswhite removed a link to a pull request Dec 9, 2024
[Code Health] Ensure supplier module msg responses are non-empty #971
Merged
15 tasks
@bryanchriswhite
Copy link
Contributor Author

Closing as the fixes have all been merged. NOTE: #929 is still in progress and is what automates the mitigation of these issues going forward, once integrated into CI.

@github-project-automation github-project-automation bot moved this from 🏗 In progress to ✅ Done in Shannon Dec 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@bryanchriswhite bryanchriswhite

Labels
code health Cleans up some code off-chain Off-chain business logic on-chain On-chain business logic
Projects
Shannon
Status: ✅ Done
Milestone
Shannon Beta TestNet Support
Development

Successfully merging a pull request may close this issue.

[Code Health] fix: gateway module gRPC status error returns pokt-network/poktroll
[Code Health] fix: proof module gRPC status error returns pokt-network/poktroll
[Code Health] fix: service module gRPC status error returns pokt-network/poktroll
[Code Health] fix: session module gRPC status return errors pokt-network/poktroll
[Code Health] fix: shared module gRPC return errors pokt-network/poktroll
[Code Health] fix: application module gRPC status error returns pokt-network/poktroll
[Code Health] fix: supplier module gRPC return errors pokt-network/poktroll
[Code Health] fix: tokenomics module gRPC return errors pokt-network/poktroll
[Tooling] Add protocheck status-errors subcmd to check for non-gRPC status error returns pokt-network/poktroll
2 participants
@bryanchriswhite @Olshansk