fix(x/data): return grpc error codes for queries #1579

aleem1314 · 2022-10-27T11:13:33Z

Description

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)

codecov · 2022-10-27T11:22:13Z

Codecov Report

Merging #1579 (d372692) into main (323a953) will not change coverage.
The diff coverage is 58.69%.

❗ Current head d372692 differs from pull request most recent head b1df863. Consider uploading reports for the commit b1df863 to get more accurate results

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1579   +/-   ##
=======================================
  Coverage   78.68%   78.68%           
=======================================
  Files         251      251           
  Lines       18849    18849           
=======================================
  Hits        14832    14832           
  Misses       3153     3153           
  Partials      864      864

Impacted Files	Coverage Δ
x/data/server/query_resolvers_by_url.go	`66.66% <33.33%> (ø)`
x/data/server/query_attestations_by_attestor.go	`66.66% <40.00%> (ø)`
x/data/server/query_resolvers_by_hash.go	`68.75% <42.85%> (ø)`
x/data/server/query_resolvers_by_iri.go	`68.75% <42.85%> (ø)`
x/data/server/query_attestations_by_hash.go	`73.33% <60.00%> (ø)`
x/data/server/query_attestations_by_iri.go	`73.33% <60.00%> (ø)`
x/data/server/query_anchor_by_hash.go	`86.95% <75.00%> (ø)`
x/data/server/query_anchor_by_iri.go	`86.95% <75.00%> (ø)`
x/data/server/query_convert_hash_to_iri.go	`100.00% <100.00%> (ø)`
x/data/server/query_convert_iri_to_hash.go	`100.00% <100.00%> (ø)`
... and 1 more

technicallyty

I think the preferred path forward with the new cosmossdk.io/errors package is to have consumer chains define their own common errors.

It might be worthwhile to open a new package errors and define some common errors, registered with the package above. we can avoid writing the status.Error(codes.InvalidArgument, "blah blah") over and over again, and instead just do ourErrorsPackage.InvalidArgument.Wrap("blah blah").

wdyt?

aleem1314 · 2022-10-31T13:50:10Z

I think the preferred path forward with the new cosmossdk.io/errors package is to have consumer chains define their own common errors.
It might be worthwhile to open a new package errors and define some common errors, registered with the package above. we can avoid writing the status.Error(codes.InvalidArgument, "blah blah") over and over again, and instead just do ourErrorsPackage.InvalidArgument.Wrap("blah blah").
wdyt?

Created errors package a532ffb.

technicallyty

great work! thanks for the changes 🙏🏻

ryanchristo

I think the preferred path forward with the new cosmossdk.io/errors package is to have consumer chains define their own common errors.

The goal of this issue was to use the standard gRPC code rather than defining custom errors with different codes. Developers wanting to use their own custom error messages will most likely be writing custom error messages based on the standard gRPC error codes (which is the case for the developers that originally reported this message).

Apologies for the delay on reviewing and the confusion. Maybe we can further discuss on our call tomorrow.

ryanchristo · 2022-10-31T23:33:35Z

errors/grpc.go

+var (
+	ErrCancelled        = errors.RegisterWithGRPCCode(moduleName, 1, codes.Canceled, "cancelled")
+	ErrUnkown           = errors.RegisterWithGRPCCode(moduleName, 2, codes.Unknown, "unknown")
+	ErrInvalidArgument  = errors.RegisterWithGRPCCode(moduleName, 3, codes.InvalidArgument, "invalid argument")
+	ErrNotFound         = errors.RegisterWithGRPCCode(moduleName, 4, codes.NotFound, "not found")
+	ErrAlreadyExists    = errors.RegisterWithGRPCCode(moduleName, 5, codes.AlreadyExists, "already exists")
+	ErrPermissionDenied = errors.RegisterWithGRPCCode(moduleName, 6, codes.PermissionDenied, "permission denied")
+	ErrInternal         = errors.RegisterWithGRPCCode(moduleName, 7, codes.Internal, "internal")
+	ErrUnavailable      = errors.RegisterWithGRPCCode(moduleName, 8, codes.Unavailable, "unavailable")
+	ErrUnauthenticated  = errors.RegisterWithGRPCCode(moduleName, 9, codes.Unauthenticated, "unauthenticated")
+	ErrUnimplemented    = errors.RegisterWithGRPCCode(moduleName, 10, codes.Unimplemented, "unimplemented")
+)


Where are these error codes coming from? It looks like the expected not found error should be status code 5 rather than 4 according to this: https://grpc.github.io/grpc/core/md_doc_statuscodes.html. This is also the expected error code that was reported by the user (in the issue summary).

Also see https://pkg.go.dev/google.golang.org/grpc/codes. I think we should be using these error codes rather than defining our own... Looks like there is some confusion around the issue per @technicallyty's above request.

Where are these error codes coming from? It looks like the expected not found error should be status code 5 rather than 4 according to this: https://grpc.github.io/grpc/core/md_doc_statuscodes.html. This is also the expected error code that was reported by the user (in the issue summary).

These error codes are just to make sure we don't have duplicate errors in each codespace. Nothing to do with gRPC status codes. For gRPC errors, we are using the third argument (codes.Canceled).

aleem1314 · 2022-11-01T08:53:52Z

I think the preferred path forward with the new cosmossdk.io/errors package is to have consumer chains define their own common errors.

The goal of this issue was to use the standard gRPC code rather than defining custom errors with different codes. Developers wanting to use their own custom error messages will most likely be writing custom error messages based on the standard gRPC error codes (which is the case for the developers that originally reported this message).

The current PR fixes the issue related to gRPC status codes. We were getting Unknown status because queries are not returning valid gRPC errors.

response on main and v4.1.x branch

$ grpcurl -plaintext -d '{"iri":"invalid-iri.rdf"}' redwood.regen.network:9090 regen.data.v1.Query/AnchorByIRI

ERROR:
  Code: Unknown
  Message: failed to parse IRI invalid-iri.rdf: regen: prefix required: invalid IRI

In this PR

$ grpcurl -plaintext -d '{"iri":"invalid-iri.rdf"}' localhost:9090 regen.data.v1.Query/AnchorByIRI

ERROR:
  Code: InvalidArgument
  Message: codespace grpc code 3: invalid argument: failed to parse IRI: failed to parse IRI invalid-iri.rdf: regen: prefix required: invalid IRI

ryanchristo · 2022-11-01T16:07:43Z

I'm still a bit confused why we are registering different error codes. I realize the correct error code is returned in the following example but we are displaying the custom registered error code in the message (a different error code):

$ grpcurl -plaintext -d '{"iri":"regen:13toVgf5aZqSVSeJQv562xkkeoe3rr3bJWa29PHVKVf77VAkVMcDvVd.rdf"}' localhost:9090 regen.data.v1.Query/AnchorByIRI

ERROR:
  Code: NotFound
  Message: codespace grpc code 4: not found: data record with IRI: regen:13toVgf5aZqSVSeJQv562xkkeoe3rr3bJWa29PHVKVf77VAkVMcDvVd.rdf

ryanchristo · 2022-11-01T16:10:01Z

It would make more sense to me if we were using the same error codes but I'll try to do some digging on best practices.

aleem1314 · 2022-11-02T09:33:09Z

It would make more sense to me if we were using the same error codes but I'll try to do some digging on best practices.

Updated error codes to match gRPC error.

ryanchristo

LGTM

errors/grpc.go

fix: return grpc error codes

214f718

aleem1314 added 2 commits October 27, 2022 18:26

fix: return grpc error codes

f2c3115

chore: add changelog

b8d558e

aleem1314 marked this pull request as ready for review October 27, 2022 13:12

aleem1314 requested a review from a team October 27, 2022 13:12

technicallyty reviewed Oct 27, 2022

View reviewed changes

aleem1314 added 2 commits October 31, 2022 19:08

feat: add error module

516cd01

chore: fix imports

4a6ee5f

chore: add changelog

7dbbb38

technicallyty approved these changes Oct 31, 2022

View reviewed changes

ryanchristo requested changes Oct 31, 2022

View reviewed changes

Merge branch 'main' into aleem/1577-grpc-error-codes

fdd0ad2

chore: fix grpc error codes

d372692

ryanchristo approved these changes Nov 2, 2022

View reviewed changes

errors/grpc.go Outdated Show resolved Hide resolved

Update errors/grpc.go

b1df863

ryanchristo changed the title ~~fix(data): return grpc error codes for queries~~ fix(x/data): return grpc error codes for queries Nov 2, 2022

ryanchristo merged commit f1cbc55 into main Nov 2, 2022

ryanchristo deleted the aleem/1577-grpc-error-codes branch November 2, 2022 21:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(x/data): return grpc error codes for queries #1579

fix(x/data): return grpc error codes for queries #1579

aleem1314 commented Oct 27, 2022 •

edited

Loading

codecov bot commented Oct 27, 2022 •

edited

Loading

technicallyty left a comment

aleem1314 commented Oct 31, 2022

technicallyty left a comment

ryanchristo left a comment

ryanchristo Oct 31, 2022

ryanchristo Oct 31, 2022

aleem1314 Nov 1, 2022

aleem1314 commented Nov 1, 2022 •

edited

Loading

ryanchristo commented Nov 1, 2022

ryanchristo commented Nov 1, 2022

aleem1314 commented Nov 2, 2022

ryanchristo left a comment

fix(x/data): return grpc error codes for queries #1579

fix(x/data): return grpc error codes for queries #1579

Conversation

aleem1314 commented Oct 27, 2022 • edited Loading

Description

Author Checklist

Reviewers Checklist

codecov bot commented Oct 27, 2022 • edited Loading

Codecov Report

technicallyty left a comment

Choose a reason for hiding this comment

aleem1314 commented Oct 31, 2022

technicallyty left a comment

Choose a reason for hiding this comment

ryanchristo left a comment

Choose a reason for hiding this comment

ryanchristo Oct 31, 2022

Choose a reason for hiding this comment

ryanchristo Oct 31, 2022

Choose a reason for hiding this comment

aleem1314 Nov 1, 2022

Choose a reason for hiding this comment

aleem1314 commented Nov 1, 2022 • edited Loading

ryanchristo commented Nov 1, 2022

ryanchristo commented Nov 1, 2022

aleem1314 commented Nov 2, 2022

ryanchristo left a comment

Choose a reason for hiding this comment

aleem1314 commented Oct 27, 2022 •

edited

Loading

codecov bot commented Oct 27, 2022 •

edited

Loading

aleem1314 commented Nov 1, 2022 •

edited

Loading