Add client module #359
Add client module #359
Conversation
client/client.go
Outdated
| // Verify initial request matches server. | ||
| if !bytes.Equal(reqBody, resp.(vspResponse).Request) { | ||
| return fmt.Errorf("server response contains differing request") | ||
| } |
There was a problem hiding this comment.
currently in wallet, all of the POST methods do this check, but we do not perform it for GET /api/v3/vspinfo. So this looks fine as long as that is tested and works.
There was a problem hiding this comment.
Good catch. I had thoroughly tested all of the POST funcs using v3tool, but not the GET one. I've pulled that logic out of do() up into post() UPDATE: scrap that, I've just reverted back to the way it was being checked in the original code.
There was a problem hiding this comment.
My only other concern with moving this wallet code into this vspd package is it now becomes harder (impossible?) for wallet to ever save the signed response from the vsp. Granted we weren't doing that yet, but in the original vspd concept, these signed responses would be recorded and could be used as proof in case a vsp voted against a user's choices.
There was a problem hiding this comment.
Great point, ill work on that.
There was a problem hiding this comment.
Opened #364 to implement this.
|
|
||
| status := reply.StatusCode | ||
|
|
||
| if status != http.StatusOK { |
There was a problem hiding this comment.
Is there a reason why wallet's handling of non-400 errors is different? An issue we had run into before was where a reverse proxy was misconfigured, or vspd was not running/crashed/etc., and the 500 errors from the RP were difficult to distinguish from API errors.
There was a problem hiding this comment.
or rather, we still wanted to parse the response body if vspd returned a 400 level error.
There was a problem hiding this comment.
I'm not entirely sure what the original thinking for the different handling was, but afaict the errors returned by the new code will contain either the same or more information than those returned by the old code, so I think we are covered. I've added comments to explain the logic.
There was a problem hiding this comment.
I think it's probably a little more resilient to decide how to decode the response or return an error based on the HTTP status code first, rather than reading the response body first, but I think this code should work for most situations and we should still see 500 level errors in the error message as long as the reverse proxy's response body is not a JSON object.
There was a problem hiding this comment.
I don't think client should trust HTTP status for two reasons:
- Client will need to maintain its own list of statuses which it expects vspd to return, which is annoying and liable to fall out of date.
- Intermediary servers may return the same statuses as vspd returns, eg. RP might return 500 if vspd is down, vspd might return 500 if dcrd is down.
I just tried the "500 + JSON" scenario you described and I see that the code isnt working as I thought it would. I'll write some tests to make sure it works properly.
jholdstock
force-pushed
the
httpclient2
branch
from
5e92987
to
8573ea8
Compare
jholdstock
force-pushed
the
httpclient2
branch
from
8573ea8
to
3d70faf
Compare
Export a
clientmodule - a reusable HTTP client for vspd API consumers. This will be useful for dcrwallet, dcrwebapi, and testing tools such as v3tool which will soon be moved into this repo.The first commit on this PR is code copied directly from dcrwallet and the following commits are a few minor refactors.
@jrick - I'd appreciate a review from you on this because I'd like to see dcrwallet consume this module. I've already got a local diff for that which is working well.