algod: Add endpoints for devmode timestamps, sync, and ready #468
Conversation
.test-env
Outdated
| @@ -1,6 +1,6 @@ | |||
| # Configs for testing repo download: | |||
| SDK_TESTING_URL="https://github.com/algorand/algorand-sdk-testing" | |||
| SDK_TESTING_BRANCH="master" | |||
| SDK_TESTING_BRANCH="timestamp-tests" | |||
There was a problem hiding this comment.
TODO: revert back to master
algochoi
changed the title
algosdk/v2client/algod.py
Outdated
| # Some algod responses currently return a 200 OK | ||
| # but have an empty response. | ||
| # Do not return an error, and just return an empty response. | ||
| if resp.status == 200: |
There was a problem hiding this comment.
nit. If at all possible, prefer to explicitly anticipate the "happy path" without catching an exception. I.e., even though its a bit more convoluted to explicitly check for an empty response along with a 200 then to try/catch as here, it'll be a bit more robust if there's a bug whereby algod returns a 200 errantly and has an unparseable response that is alerting of an error. Alternately, if json.load() gives a special error when an empty body is provided, you can selectively catch that specific Exception-type instead of the generic type.
There was a problem hiding this comment.
Another approach would be to have an explict "white list" of acceptable endpoints that we know shouldn't return a body... But that seems like a maintenance hassle.
There was a problem hiding this comment.
A little less annoying would be to provide a new parameter such as empty_response defaulting to False.
There was a problem hiding this comment.
Alternately, if json.load() gives a special error when an empty body is provided, you can selectively catch that specific Exception-type instead of the generic type.
Unfortunately, I don't think there is a separate exception for empty bodies vs bad bodies (parsing error).
A little less annoying would be to provide a new parameter such as empty_response defaulting to False.
I like this approach at the moment - Ideally the server should return a different response (204) if there is an empty body, but that seems like a bigger hill to climb.
There was a problem hiding this comment.
I jumped the gun here, it looks like there are two ways an empty response is returned from algod:
- We return a
NoContentresponse in the v2 endpoints: https://github.com/algorand/go-algorand/blob/master/daemon/algod/api/server/v2/handlers.go#L1114 - We encode a
nilinto the response in the common endpoints: https://github.com/algorand/go-algorand/blob/master/daemon/algod/api/server/common/handlers.go#L92
For 1, the server returns a length 0 response which causes an exception when parsing in Python, but for 2, the response is of length 5 for some reason and it parses to a None object in Python (without raising an exception). To handle case 1, it seems sufficient to just check the response length and ensure that it is 0.
| @@ -905,6 +905,11 @@ def expect_path(context, path): | |||
| assert exp_query == actual_query, f"{exp_query} != {actual_query}" | |||
|
|
|||
|
|
|||
| @then('expect the request to be "{method}" "{path}"') | |||
| def expect_request(context, method, path): | |||
| return expect_path(context, path) | |||
There was a problem hiding this comment.
From my check, only the path is guaranteed to be returned in the response for the Python SDK
| @@ -14,4 +14,3 @@ | |||
| @rekey_v1 | |||
| @send | |||
| @send.keyregtxn | |||
| @simulate | |||
There was a problem hiding this comment.
@ahangsu I decided to take this tag out until the tests are fixed
This PR implements the devmode timestamp offset, sync, and ready endpoints in the Python SDK.
Tested locally and added some cucumber tests.