fix: add status code if the body contain a code #93
Conversation
- Add relayOutputErr type - Update test
ricantar
force-pushed
the
add-status-code-response
branch
from
19acc9c
to
0dfe954
Compare
provider/provider.go
Outdated
| return matches[1] // Return the first captured group, which should be the status code. | ||
| } | ||
| } | ||
| return "200" |
There was a problem hiding this comment.
Are you sure you want to default to a success if none of the error cases we account for are matched?
I'm thinking one of two non mutually-exclusive ideas:
- Add a regex check for
200success - Update the method header to
func extractStatusFromResponse(response string) (statusCode string, known bool)
And default to "unknown", false as a default case signifying "def not an error" but also "relaying logic elsewhere on success`
There was a problem hiding this comment.
Yes, we want the default to be 200 because of normal chains (which should always get back to us with 200). Even tho it's really valuable to get those 200 cases, for now everything is "unknown" because the node are getting back to us with 200 no matter what
There was a problem hiding this comment.
Not going to block this because you're the primary user of the library so ultimately it's up to you, but I do still thinking that defaulting to success it not a good design decision. Defaulting to unknown (can be a constant in the SDK) and handling that appropriately prevents requiring "implicit knowledge" that "200" may not necessarily a "200".
[1] https://chat.openai.com/share/111342f3-0045-4e71-b24b-4e2b8822dd25
Side note: ChatGPT also suggested extracting the regex's outside of the function.
There was a problem hiding this comment.
@adshmh Bump on this one too. Need a 3rd data point.
There was a problem hiding this comment.
The regex it's already in a separate init, will be deleted later but for now can be there. But the default to unknown it's exactly what we have now and that's what we want to avoid. However since this is a temporary patch it's not really our intended design
There was a problem hiding this comment.
Regarding unknown vs. 200 , I think it is best to have "unknown" as the default return value, and return 200 only if it is explicitly identified in the core's response. Let the user of the library decide on what unknown means.
This makes the most sense to me because it makes sure the library does not mask any key detail: actual 200 vs. unknown is an important detail.
Although it is correctly pointed out that for now everything is "unknown" because the node are getting back to us with 200 no matter what, this library should not build on this missing/incorrect behavior and further complicate matters downstream. It should instead defer this decision to the user by returning all the available data pieces, which also removes the need for knowledge on the history of the matter when reading the library's code.
Also, "temporary" solutions may stay in-place for much longer than expected for a variety of reasons, and therefore still deserve the general application of best practices IMO.
There was a problem hiding this comment.
I have to disagree, the default status code for an HTTP call is 200 (which means OK I got your call), unknown is not a valid status code and should not be used. Here the definition of the RC 9110.
https://httpwg.org/specs/rfc9112.html#status.line
https://httpwg.org/specs/rfc9110.html#overview.of.status.codes
A status code must have 3 digit and we have to define a default ourself in the library (i.e: GO default is 200, if you forget to set one it will be set to 200 automatically)
There was a problem hiding this comment.
tl;dr ASSUMING we are building a long-term solution in this PR, I suggest defeaulting to 202 and having explicit checks for 200.
I did not have strong opinions about this under the assumption that it was a hack / temp / workaround in Morse, but given @ricantar messages that the same default will apply to Shannon (see the image below), will provide another argument.
- Regarding having a
3 digist status codeinstead ofunknown-> Agreed. - Regarding using
200-> Disagreed.
After asking perplexity (see search here), I looked into 202 and 204 and believe 202 may be the most appropriate, and least misleading, from the POV of an SDK..
What is the difference between a 200 status code and a 202 status code?:
When a server returns a 200 OK status code, it means that the server has successfully processed the client’s request and is returning the requested content in the response body.
- 202: The request went through but we don't know what happened
- 200: Explicit SUCCESS / OK
There was a problem hiding this comment.
I can agree with that, 202 it's better for a default
Co-authored-by: Daniel Olshansky <olshansky.daniel@gmail.com>
ricantar
force-pushed
the
add-status-code-response
branch
from
8671de0
to
2b71729
Compare
* fix: add status code if the body contain a code * feat: Include relayOutputErr - Add relayOutputErr type - Update test to handle new error type * fix: pre calculate regex Co-authored-by: Daniel Olshansky <olshansky.daniel@gmail.com> * Optimize errorCode extraction * update test to include the new status code behavior with 500 as default --------- Co-authored-by: Daniel Olshansky <olshansky.daniel@gmail.com>
* fix: add status code if the body contain a code * feat: Include relayOutputErr - Add relayOutputErr type - Update test to handle new error type * fix: pre calculate regex * Optimize errorCode extraction * update test to include the new status code behavior with 500 as default --------- Co-authored-by: Daniel Olshansky <olshansky.daniel@gmail.com>
No description provided.