Standardize API Responses/HTTP Codes

[alexanderbez]

Summary

Currently when querying for a resource(s) from a querier, in the non-happy path we naively return a 500 HTTP status code with the error:

res, height, err := cliCtx.QueryWithData(endpoint, bz) // or cliCtx.Query(endpoint)
if err != nil {
	rest.WriteErrorResponse(w, http.StatusInternalServerError, err.Error())
	return
}

This can happen for any of the following reasons:

1. Proof verification failure (client-side)
2. Response is not OK (server-side)
   2a. Bad querier path
   2b. Invalid query height
   2c. Invalid query height with proof
   2d. Querying against pruned state
   2e. The resource(s) does not exist

Problem Definition

We should not be returning a 500 naively for all these conditions.

• (2a-2c) Should result in 4xx
• (2d) Should remain as a 5xx
• (2e) Should most likely return a 2xx with an empty/zero-value result and either the current or provided height.

Proposal

Based on the error, use a proper code in the ResponseQuery object to effectively return the correct type of error. Then, the CLIContext (or whatever upstream client) can appropriately handle this by returning a typed error. Finally, the desired HTTP status code and body can be returned based on the concrete type of the error.

e.g.

res, err := cliCtx.Query(endpoint)
if err != nil {
  switch err:
  case BadInput:
       // 4xx...
  case ResourceDoesNotExist:
        // 2xx...
  default:
       rest.WriteErrorResponse(w, http.StatusInternalServerError, err.Error())
       return
}

Note: I'm not sure if this will be a trivial or small PR but can be broken down into multiple PRs. Essentially each REST handler (CLI too?) and module querier method will have to be adjusted.

/cc @jackzampolin @fedekunze

---

For Admin Use

• Not duplicate issue
• Appropriate labels applied
• Appropriate contributors tagged
• Contributor assigned/self-assigned

[tnachen]

How about the case of 1)? If it's a client side issue we should send invalid request then right?
Overall this looks right to me.

[alexanderbez]

@tnachen in the case of (1) this means proof verification failed; i.e. the proof Tendermint sent us in the ABCI query response is invalid. Not sure what else we can do apart from what we currently do.

[tnachen]

Should we just send a bad request back?

[alexanderbez]

hmmm, maybe we should return 4xx for proof verification failure. Good idea.

[alexanderbez]

We should probably tackle #4844 first and then this issue.

[github-actions]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[tac0turtle]

grpc error codes brings this to us, we just need to do a little grooming of what error is needed for each response