Workaround for successful 202s on REST chains #98
Conversation
| @@ -942,15 +946,24 @@ func parseRelaySuccesfulOutput(bodyBytes []byte, requestStatusCode int) (*RelayO | |||
| return &output, err | |||
| } | |||
|
|
|||
| // Check if there's explicitly a result field, if there's on mark it as success, otherwise check what's the potential status. | |||
| // for REST chain that doesn't return result in any of the call will be defaulted to 202 in extractStatusFromResponse | |||
| isValidJson := json.Valid([]byte(output.Response)) | |||
There was a problem hiding this comment.
My only question is an open one for @adshmh : does this impact the Portal logic for handling Pocket Mainnet/Archival and if so, how much of an impact is this?
There was a problem hiding this comment.
+1 to any insight you have.
I tried to be as backwards compatible as possible with the if/else below, but it only uses logic and not any sort of real testing.
There was a problem hiding this comment.
I believe there are cases where Pocket returns a non-json response, but those may have the text result in them (we need to verify this)
There was a problem hiding this comment.
@adshmh so in terms of next step with this PR, which is it?
- Merge it in -> Update portal-middleware -> deploy -> test in prod
OR - Do some sort of investigation for different request/response types?
OR - 👍 that this is backwards compatible enough and go with option (1) above
I'm opting for option (3) but lmk if you have a better idea for (2).
Please approve/request changes to the PR as needed.
There was a problem hiding this comment.
I agree that option [3] makes the most sense here.
To verify backward compatibility of the change we need to a) review the API docs and b) run a few tests against a Morse node.
Other than potential compatibility issues, the code changes look OK.
There was a problem hiding this comment.
review the API docs
This is correct: https://docs.pokt.network/reference/api-definition
Here is the source of truth: https://github.com/pokt-network/pocket-core/blob/staging/doc/specs/rpc-spec.yaml
run a few tests against a Morse node.
Sounds good.
- Where are the instructions to do this?
- Is this something you can do?
- Happy to dive into it if (1) does not exist or you don't have the capacity given (2).
There was a problem hiding this comment.
- I don't think this has been done/documented before.
- Yes: I can build a doc and run the tests.
|
Note to team: AFTER (if?) this PR is merged, we'll also need to:
cc @fredteumer |
There was a problem hiding this comment.
i prefer this approach since the middleware should just be a passthrough and shouldn't manipulate the return status of a message. ( i believe we should maintain this principle ).
With this change to pocket-go i think this is the best happy medium where the protocol can continue to do its thing but then the SDK can handle the translation. Lmk if this works @adshmh
Customer support request:
New business logic:
resultis present)resultpresent -> 200 (success)resultnot present -> check the status (if available)resultfield)resultnot present && json is valid -> 200resultnot present && json is invalid -> extract from response