related to: #7274 and #7275
Still somewhat uncertain on two things that I'd appreciate more feedback on:
1. The optional temporary local overrides. Perhaps this is superfluous and we can simply make the transition without the override?
2. If this set of parameters seems to be large enough to allow application developers to create the chains they want but not so large as to be needlessly complex.
The parameters for RPC GET requests are parsed from query arguments in the
request URL. Rework this code to remove the need for tmjson. The structure of a
call still requires reflection, and still works the same way as before, but the
code structure has been simplified and cleaned up a bit.
Points of note:
- Consolidate handling of pointer types, so we only need to dereference once.
- Reduce the number of allocations of reflective types.
- Report errors for unsupported types rather than returning untyped nil.
Update the tests as well. There was one test case that checked for an error on
a behaviour the OpenAPI docs explicitly demonstrates as supported, so I fixed
that test case, and also added some new ones for cases that weren't checked.
Related:
* Update e2e base Go image to 1.17 (to match config).
Require that RPC functions take a context as their first argument, and return
an error as either their only result, or the second of two results.
This does not change how functions are dispatched, but will make it a little
easier to make more invasive changes in the near future.
Instead of taking a comma-separated string of parameter names, take each
parameter name as a separate argument. Now that we no longer have an extra flag
for caching, this fits nicely into a variadic trailer.
* Update all usage of NewRPCFunc and NewWSRPCFunc.
Define interfaces for the various methods a service may implement. This is
basically just the set of things on Environment that are exported as RPCs, but
these are also implemented by the light proxy.
* internal/rpc: use NewRoutesMap to construct routes on service start
* light/proxy: use NewRoutesMap to construct RPC routes
Rather than installing two separate panic handlers, defer the bookkeeping
separately from recovery, and lift the delegated handler call out to the top
level of the wrapper.
Also: Regularize the server middleware wrappers.
Add writeRPCResponse and writeHTTPResponse helpers, that handle the way RPC
responses are written to HTTP replies. These replace the exported helpers.
Visible effects:
- JSON results are now marshaled without indentation.
- HTTP status codes are now normalized.
- Cache control headers are no longer set.
Details:
- When writing a response to a URL (GET) request, do not marshal the whole
JSON-RPC object into the body, only encode the result or the error object.
This is a user-visible change.
- Do not change the HTTP status code for RPC errors. The RPC error already
reports what went wrong, the HTTP status should only report problems with the
HTTP transaction itself. This is a user-visible change.
- Encode JSON without indentation in POST response bodies. This is mainly cosmetic
but saves quite a bit of response data. Indent is still applied to GET responses to make
life easier for code examples.
- Remove an obsolete TODO about reporting an HTTP error on websocket upgrade.
Nothing needed to change; the upgrader already reports an error.
- Report an HTTP error when starting the server loop fails.
- Improve logging for encoding errors.
- Log less aggressively.
In two cases, we check for the content of an error right after asserting that
no error occurs. Fix the sense of those checks.
In one case, we check that there is no error with the diagnostic "expected
error". It's not clear whether this means "an error was expected" (which is
what I believe) or "we got the expected error". However, given the way the mock
plumbing is set up, the first interpretation seems right.
We should not set cache-control headers on RPC responses. HTTP caching
interacts poorly with resources that are expected to change frequently, or
whose rate of change is unpredictable.
More subtly, all calls to the POST endpoint use the same URL, which means a
cacheable response from one call may actually "hide" an uncacheable response
from a subsequent one. This is less of a problem for the GET endpoints, but
that means the behaviour of RPCs varies depending on which HTTP method your
client happens to use. Websocket requests were already marked statically
uncacheable, adding yet a third combination.
To address this:
- Stop setting cache-control headers.
- Update the tests that were checking for those headers.
- Remove the flags to request cache-control.
Apart from affecting the HTTP response headers, this change does not modify the
behaviour of any of the RPC methods.
* Rename rpctypes.Context to CallInfo.
Add methods to attach and recover this value from a context.Context.
* Rework RPC method handlers to accept "real" contexts.
- Replace *rpctypes.Context arguments with context.Context.
- Update usage of RPC context fields to use CallInfo.
No functional changes.
- Pull out a some helper code to simplify the control flow within the body of
the HTTP request handler.
- Front-load the URL path check so it does not get repeated for each request.
Instead of using anonymous maps, define tagged struct types for JSON argument
encoding. This allows us to have the encoding rules we want without tmjson.
This commit handles the "easy" cases. BroadcastEvidence is omitted here,
because it depends on the interface encoding rules from tmjson. I will address
that in a forthcoming change.
Defines a different concrete type that satisfies the service interface for a seed node.
update the seed node unit test to assert the new type.
Fixes#6775
* Update Caller interface and its documentation.
* Rework MapToRequest as ParamsToRequest.
The old interface returned the result as well as populating it. Nothing was
using this, so drop the duplicated value from the return signature. Clarify the
documentation on the Caller type.
Rework the MapToRequest helper to take an arbitrary value instead of only a
map. This is groundwork for getting rid of the custom marshaling code. For now,
however, the implementation preserves the existing behaviour for the map, until
we can replace those.
These two interfaces are identical, and besides HTTPClient being confusingly
named, all but one location uses Caller. Update that one location, and drop the
redundant interface.
Apart from the tests for the websocket client, positional parameters are not
used by RPC clients. The server supports both arrays and objects, but the
client only needs to provide one or the other.
Where possible, replace uses of the custom JSON library with the standard
library. The custom library treats interface and unnamed lteral types
differently, so this change avoids those even where it would probably be safe
to switch them.
Sam Kleinman
William Banfield
M. J. Fromberger
dependabot[bot]
Kene