From 6291d22f46f4c4f9121375af700dbdafa51577e7 Mon Sep 17 00:00:00 2001 From: "M. J. Fromberger" Date: Mon, 10 Jan 2022 13:56:43 -0800 Subject: [PATCH] rpc: simplify the JSON-RPC client Caller interface (#7549) * 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. --- rpc/client/http/http.go | 91 +++++++++----------------- rpc/jsonrpc/client/decode.go | 18 ++--- rpc/jsonrpc/client/http_json_client.go | 42 ++++++------ rpc/jsonrpc/client/ws_client.go | 2 +- rpc/jsonrpc/jsonrpc_test.go | 8 +-- rpc/jsonrpc/server/ws_handler_test.go | 2 +- rpc/jsonrpc/types/types.go | 28 +++++--- rpc/test/helpers.go | 2 +- 8 files changed, 81 insertions(+), 112 deletions(-) diff --git a/rpc/client/http/http.go b/rpc/client/http/http.go index d18671fd0..de4269179 100644 --- a/rpc/client/http/http.go +++ b/rpc/client/http/http.go @@ -203,21 +203,17 @@ func (b *BatchHTTP) Count() int { func (c *baseRPCClient) Status(ctx context.Context) (*coretypes.ResultStatus, error) { result := new(coretypes.ResultStatus) - _, err := c.caller.Call(ctx, "status", map[string]interface{}{}, result) - if err != nil { + if err := c.caller.Call(ctx, "status", map[string]interface{}{}, result); err != nil { return nil, err } - return result, nil } func (c *baseRPCClient) ABCIInfo(ctx context.Context) (*coretypes.ResultABCIInfo, error) { result := new(coretypes.ResultABCIInfo) - _, err := c.caller.Call(ctx, "abci_info", map[string]interface{}{}, result) - if err != nil { + if err := c.caller.Call(ctx, "abci_info", map[string]interface{}{}, result); err != nil { return nil, err } - return result, nil } @@ -235,13 +231,11 @@ func (c *baseRPCClient) ABCIQueryWithOptions( data bytes.HexBytes, opts rpcclient.ABCIQueryOptions) (*coretypes.ResultABCIQuery, error) { result := new(coretypes.ResultABCIQuery) - _, err := c.caller.Call(ctx, "abci_query", + if err := c.caller.Call(ctx, "abci_query", map[string]interface{}{"path": path, "data": data, "height": opts.Height, "prove": opts.Prove}, - result) - if err != nil { + result); err != nil { return nil, err } - return result, nil } @@ -250,8 +244,7 @@ func (c *baseRPCClient) BroadcastTxCommit( tx types.Tx, ) (*coretypes.ResultBroadcastTxCommit, error) { result := new(coretypes.ResultBroadcastTxCommit) - _, err := c.caller.Call(ctx, "broadcast_tx_commit", map[string]interface{}{"tx": tx}, result) - if err != nil { + if err := c.caller.Call(ctx, "broadcast_tx_commit", map[string]interface{}{"tx": tx}, result); err != nil { return nil, err } return result, nil @@ -277,8 +270,7 @@ func (c *baseRPCClient) broadcastTX( tx types.Tx, ) (*coretypes.ResultBroadcastTx, error) { result := new(coretypes.ResultBroadcastTx) - _, err := c.caller.Call(ctx, route, map[string]interface{}{"tx": tx}, result) - if err != nil { + if err := c.caller.Call(ctx, route, map[string]interface{}{"tx": tx}, result); err != nil { return nil, err } return result, nil @@ -293,8 +285,7 @@ func (c *baseRPCClient) UnconfirmedTxs( if limit != nil { params["limit"] = limit } - _, err := c.caller.Call(ctx, "unconfirmed_txs", params, result) - if err != nil { + if err := c.caller.Call(ctx, "unconfirmed_txs", params, result); err != nil { return nil, err } return result, nil @@ -302,8 +293,7 @@ func (c *baseRPCClient) UnconfirmedTxs( func (c *baseRPCClient) NumUnconfirmedTxs(ctx context.Context) (*coretypes.ResultUnconfirmedTxs, error) { result := new(coretypes.ResultUnconfirmedTxs) - _, err := c.caller.Call(ctx, "num_unconfirmed_txs", map[string]interface{}{}, result) - if err != nil { + if err := c.caller.Call(ctx, "num_unconfirmed_txs", map[string]interface{}{}, result); err != nil { return nil, err } return result, nil @@ -311,16 +301,14 @@ func (c *baseRPCClient) NumUnconfirmedTxs(ctx context.Context) (*coretypes.Resul func (c *baseRPCClient) CheckTx(ctx context.Context, tx types.Tx) (*coretypes.ResultCheckTx, error) { result := new(coretypes.ResultCheckTx) - _, err := c.caller.Call(ctx, "check_tx", map[string]interface{}{"tx": tx}, result) - if err != nil { + if err := c.caller.Call(ctx, "check_tx", map[string]interface{}{"tx": tx}, result); err != nil { return nil, err } return result, nil } func (c *baseRPCClient) RemoveTx(ctx context.Context, txKey types.TxKey) error { - _, err := c.caller.Call(ctx, "remove_tx", map[string]interface{}{"tx_key": txKey}, nil) - if err != nil { + if err := c.caller.Call(ctx, "remove_tx", map[string]interface{}{"tx_key": txKey}, nil); err != nil { return err } return nil @@ -328,8 +316,7 @@ func (c *baseRPCClient) RemoveTx(ctx context.Context, txKey types.TxKey) error { func (c *baseRPCClient) NetInfo(ctx context.Context) (*coretypes.ResultNetInfo, error) { result := new(coretypes.ResultNetInfo) - _, err := c.caller.Call(ctx, "net_info", map[string]interface{}{}, result) - if err != nil { + if err := c.caller.Call(ctx, "net_info", map[string]interface{}{}, result); err != nil { return nil, err } return result, nil @@ -337,8 +324,7 @@ func (c *baseRPCClient) NetInfo(ctx context.Context) (*coretypes.ResultNetInfo, func (c *baseRPCClient) DumpConsensusState(ctx context.Context) (*coretypes.ResultDumpConsensusState, error) { result := new(coretypes.ResultDumpConsensusState) - _, err := c.caller.Call(ctx, "dump_consensus_state", map[string]interface{}{}, result) - if err != nil { + if err := c.caller.Call(ctx, "dump_consensus_state", map[string]interface{}{}, result); err != nil { return nil, err } return result, nil @@ -346,8 +332,7 @@ func (c *baseRPCClient) DumpConsensusState(ctx context.Context) (*coretypes.Resu func (c *baseRPCClient) ConsensusState(ctx context.Context) (*coretypes.ResultConsensusState, error) { result := new(coretypes.ResultConsensusState) - _, err := c.caller.Call(ctx, "consensus_state", map[string]interface{}{}, result) - if err != nil { + if err := c.caller.Call(ctx, "consensus_state", map[string]interface{}{}, result); err != nil { return nil, err } return result, nil @@ -362,8 +347,7 @@ func (c *baseRPCClient) ConsensusParams( if height != nil { params["height"] = height } - _, err := c.caller.Call(ctx, "consensus_params", params, result) - if err != nil { + if err := c.caller.Call(ctx, "consensus_params", params, result); err != nil { return nil, err } return result, nil @@ -371,8 +355,7 @@ func (c *baseRPCClient) ConsensusParams( func (c *baseRPCClient) Health(ctx context.Context) (*coretypes.ResultHealth, error) { result := new(coretypes.ResultHealth) - _, err := c.caller.Call(ctx, "health", map[string]interface{}{}, result) - if err != nil { + if err := c.caller.Call(ctx, "health", map[string]interface{}{}, result); err != nil { return nil, err } return result, nil @@ -384,10 +367,9 @@ func (c *baseRPCClient) BlockchainInfo( maxHeight int64, ) (*coretypes.ResultBlockchainInfo, error) { result := new(coretypes.ResultBlockchainInfo) - _, err := c.caller.Call(ctx, "blockchain", + if err := c.caller.Call(ctx, "blockchain", map[string]interface{}{"minHeight": minHeight, "maxHeight": maxHeight}, - result) - if err != nil { + result); err != nil { return nil, err } return result, nil @@ -395,8 +377,7 @@ func (c *baseRPCClient) BlockchainInfo( func (c *baseRPCClient) Genesis(ctx context.Context) (*coretypes.ResultGenesis, error) { result := new(coretypes.ResultGenesis) - _, err := c.caller.Call(ctx, "genesis", map[string]interface{}{}, result) - if err != nil { + if err := c.caller.Call(ctx, "genesis", map[string]interface{}{}, result); err != nil { return nil, err } return result, nil @@ -404,8 +385,7 @@ func (c *baseRPCClient) Genesis(ctx context.Context) (*coretypes.ResultGenesis, func (c *baseRPCClient) GenesisChunked(ctx context.Context, id uint) (*coretypes.ResultGenesisChunk, error) { result := new(coretypes.ResultGenesisChunk) - _, err := c.caller.Call(ctx, "genesis_chunked", map[string]interface{}{"chunk": id}, result) - if err != nil { + if err := c.caller.Call(ctx, "genesis_chunked", map[string]interface{}{"chunk": id}, result); err != nil { return nil, err } return result, nil @@ -417,8 +397,7 @@ func (c *baseRPCClient) Block(ctx context.Context, height *int64) (*coretypes.Re if height != nil { params["height"] = height } - _, err := c.caller.Call(ctx, "block", params, result) - if err != nil { + if err := c.caller.Call(ctx, "block", params, result); err != nil { return nil, err } return result, nil @@ -429,8 +408,7 @@ func (c *baseRPCClient) BlockByHash(ctx context.Context, hash bytes.HexBytes) (* params := map[string]interface{}{ "hash": hash, } - _, err := c.caller.Call(ctx, "block_by_hash", params, result) - if err != nil { + if err := c.caller.Call(ctx, "block_by_hash", params, result); err != nil { return nil, err } return result, nil @@ -445,8 +423,7 @@ func (c *baseRPCClient) BlockResults( if height != nil { params["height"] = height } - _, err := c.caller.Call(ctx, "block_results", params, result) - if err != nil { + if err := c.caller.Call(ctx, "block_results", params, result); err != nil { return nil, err } return result, nil @@ -458,8 +435,7 @@ func (c *baseRPCClient) Header(ctx context.Context, height *int64) (*coretypes.R if height != nil { params["height"] = height } - _, err := c.caller.Call(ctx, "header", params, result) - if err != nil { + if err := c.caller.Call(ctx, "header", params, result); err != nil { return nil, err } return result, nil @@ -470,8 +446,7 @@ func (c *baseRPCClient) HeaderByHash(ctx context.Context, hash bytes.HexBytes) ( params := map[string]interface{}{ "hash": hash, } - _, err := c.caller.Call(ctx, "header_by_hash", params, result) - if err != nil { + if err := c.caller.Call(ctx, "header_by_hash", params, result); err != nil { return nil, err } return result, nil @@ -483,8 +458,7 @@ func (c *baseRPCClient) Commit(ctx context.Context, height *int64) (*coretypes.R if height != nil { params["height"] = height } - _, err := c.caller.Call(ctx, "commit", params, result) - if err != nil { + if err := c.caller.Call(ctx, "commit", params, result); err != nil { return nil, err } return result, nil @@ -496,8 +470,7 @@ func (c *baseRPCClient) Tx(ctx context.Context, hash bytes.HexBytes, prove bool) "hash": hash, "prove": prove, } - _, err := c.caller.Call(ctx, "tx", params, result) - if err != nil { + if err := c.caller.Call(ctx, "tx", params, result); err != nil { return nil, err } return result, nil @@ -526,8 +499,7 @@ func (c *baseRPCClient) TxSearch( params["per_page"] = perPage } - _, err := c.caller.Call(ctx, "tx_search", params, result) - if err != nil { + if err := c.caller.Call(ctx, "tx_search", params, result); err != nil { return nil, err } @@ -554,8 +526,7 @@ func (c *baseRPCClient) BlockSearch( params["per_page"] = perPage } - _, err := c.caller.Call(ctx, "block_search", params, result) - if err != nil { + if err := c.caller.Call(ctx, "block_search", params, result); err != nil { return nil, err } @@ -579,8 +550,7 @@ func (c *baseRPCClient) Validators( if height != nil { params["height"] = height } - _, err := c.caller.Call(ctx, "validators", params, result) - if err != nil { + if err := c.caller.Call(ctx, "validators", params, result); err != nil { return nil, err } return result, nil @@ -591,8 +561,7 @@ func (c *baseRPCClient) BroadcastEvidence( ev types.Evidence, ) (*coretypes.ResultBroadcastEvidence, error) { result := new(coretypes.ResultBroadcastEvidence) - _, err := c.caller.Call(ctx, "broadcast_evidence", map[string]interface{}{"evidence": ev}, result) - if err != nil { + if err := c.caller.Call(ctx, "broadcast_evidence", map[string]interface{}{"evidence": ev}, result); err != nil { return nil, err } return result, nil diff --git a/rpc/jsonrpc/client/decode.go b/rpc/jsonrpc/client/decode.go index f69926cb7..5d41a94c0 100644 --- a/rpc/jsonrpc/client/decode.go +++ b/rpc/jsonrpc/client/decode.go @@ -9,33 +9,27 @@ import ( rpctypes "github.com/tendermint/tendermint/rpc/jsonrpc/types" ) -func unmarshalResponseBytes( - responseBytes []byte, - expectedID rpctypes.JSONRPCIntID, - result interface{}, -) (interface{}, error) { - +func unmarshalResponseBytes(responseBytes []byte, expectedID rpctypes.JSONRPCIntID, result interface{}) error { // Read response. If rpc/core/types is imported, the result will unmarshal // into the correct type. response := &rpctypes.RPCResponse{} if err := json.Unmarshal(responseBytes, response); err != nil { - return nil, fmt.Errorf("error unmarshaling: %w", err) + return fmt.Errorf("error unmarshaling: %w", err) } if response.Error != nil { - return nil, response.Error + return response.Error } if err := validateAndVerifyID(response, expectedID); err != nil { - return nil, fmt.Errorf("wrong ID: %w", err) + return fmt.Errorf("wrong ID: %w", err) } // Unmarshal the RawMessage into the result. if err := tmjson.Unmarshal(response.Result, result); err != nil { - return nil, fmt.Errorf("error unmarshaling result: %w", err) + return fmt.Errorf("error unmarshaling result: %w", err) } - - return result, nil + return nil } func unmarshalResponseBytesArray( diff --git a/rpc/jsonrpc/client/http_json_client.go b/rpc/jsonrpc/client/http_json_client.go index 029ec3b34..b2823d5e7 100644 --- a/rpc/jsonrpc/client/http_json_client.go +++ b/rpc/jsonrpc/client/http_json_client.go @@ -106,9 +106,15 @@ func (u parsedURL) GetTrimmedURL() string { //------------------------------------------------------------- -// Caller implementers can facilitate calling the JSON-RPC endpoint. +// A Caller handles the round trip of a single JSON-RPC request. The +// implementation is responsible for assigning request IDs, marshaling +// parameters, and unmarshaling results. type Caller interface { - Call(ctx context.Context, method string, params map[string]interface{}, result interface{}) (interface{}, error) + // Call sends a new request for method to the server with the given + // parameters. If params == nil, the request has empty parameters. + // If result == nil, any result value must be discarded without error. + // Otherwise the concrete value of result must be a pointer. + Call(ctx context.Context, method string, params, result interface{}) error } //------------------------------------------------------------- @@ -174,28 +180,23 @@ func NewWithHTTPClient(remote string, c *http.Client) (*Client, error) { // Call issues a POST HTTP request. Requests are JSON encoded. Content-Type: // application/json. -func (c *Client) Call( - ctx context.Context, - method string, - params map[string]interface{}, - result interface{}, -) (interface{}, error) { +func (c *Client) Call(ctx context.Context, method string, params, result interface{}) error { id := c.nextRequestID() - request, err := rpctypes.MapToRequest(id, method, params) + request, err := rpctypes.ParamsToRequest(id, method, params) if err != nil { - return nil, fmt.Errorf("failed to encode params: %w", err) + return fmt.Errorf("failed to encode params: %w", err) } requestBytes, err := json.Marshal(request) if err != nil { - return nil, fmt.Errorf("failed to marshal request: %w", err) + return fmt.Errorf("failed to marshal request: %w", err) } requestBuf := bytes.NewBuffer(requestBytes) httpRequest, err := http.NewRequestWithContext(ctx, http.MethodPost, c.address, requestBuf) if err != nil { - return nil, fmt.Errorf("request setup failed: %w", err) + return fmt.Errorf("request setup failed: %w", err) } httpRequest.Header.Set("Content-Type", "application/json") @@ -206,14 +207,14 @@ func (c *Client) Call( httpResponse, err := c.client.Do(httpRequest) if err != nil { - return nil, err + return err } defer httpResponse.Body.Close() responseBytes, err := io.ReadAll(httpResponse.Body) if err != nil { - return nil, fmt.Errorf("failed to read response body: %w", err) + return fmt.Errorf("failed to read response body: %w", err) } return unmarshalResponseBytes(responseBytes, id, result) @@ -340,19 +341,14 @@ func (b *RequestBatch) Send(ctx context.Context) ([]interface{}, error) { // Call enqueues a request to call the given RPC method with the specified // parameters, in the same way that the `Client.Call` function would. -func (b *RequestBatch) Call( - _ context.Context, - method string, - params map[string]interface{}, - result interface{}, -) (interface{}, error) { +func (b *RequestBatch) Call(_ context.Context, method string, params, result interface{}) error { id := b.client.nextRequestID() - request, err := rpctypes.MapToRequest(id, method, params) + request, err := rpctypes.ParamsToRequest(id, method, params) if err != nil { - return nil, err + return err } b.enqueue(&jsonRPCBufferedRequest{request: request, result: result}) - return result, nil + return nil } //------------------------------------------------------------- diff --git a/rpc/jsonrpc/client/ws_client.go b/rpc/jsonrpc/client/ws_client.go index 624b08776..80520e2a3 100644 --- a/rpc/jsonrpc/client/ws_client.go +++ b/rpc/jsonrpc/client/ws_client.go @@ -217,7 +217,7 @@ func (c *WSClient) Send(ctx context.Context, request rpctypes.RPCRequest) error // Call enqueues a call request onto the Send queue. Requests are JSON encoded. func (c *WSClient) Call(ctx context.Context, method string, params map[string]interface{}) error { - request, err := rpctypes.MapToRequest(c.nextRequestID(), method, params) + request, err := rpctypes.ParamsToRequest(c.nextRequestID(), method, params) if err != nil { return err } diff --git a/rpc/jsonrpc/jsonrpc_test.go b/rpc/jsonrpc/jsonrpc_test.go index b22dfa3d7..2c353e508 100644 --- a/rpc/jsonrpc/jsonrpc_test.go +++ b/rpc/jsonrpc/jsonrpc_test.go @@ -148,7 +148,7 @@ func echoViaHTTP(ctx context.Context, cl client.Caller, val string) (string, err "arg": val, } result := new(ResultEcho) - if _, err := cl.Call(ctx, "echo", params, result); err != nil { + if err := cl.Call(ctx, "echo", params, result); err != nil { return "", err } return result.Value, nil @@ -159,7 +159,7 @@ func echoIntViaHTTP(ctx context.Context, cl client.Caller, val int) (int, error) "arg": val, } result := new(ResultEchoInt) - if _, err := cl.Call(ctx, "echo_int", params, result); err != nil { + if err := cl.Call(ctx, "echo_int", params, result); err != nil { return 0, err } return result.Value, nil @@ -170,7 +170,7 @@ func echoBytesViaHTTP(ctx context.Context, cl client.Caller, bytes []byte) ([]by "arg": bytes, } result := new(ResultEchoBytes) - if _, err := cl.Call(ctx, "echo_bytes", params, result); err != nil { + if err := cl.Call(ctx, "echo_bytes", params, result); err != nil { return []byte{}, err } return result.Value, nil @@ -181,7 +181,7 @@ func echoDataBytesViaHTTP(ctx context.Context, cl client.Caller, bytes tmbytes.H "arg": bytes, } result := new(ResultEchoDataBytes) - if _, err := cl.Call(ctx, "echo_data_bytes", params, result); err != nil { + if err := cl.Call(ctx, "echo_data_bytes", params, result); err != nil { return []byte{}, err } return result.Value, nil diff --git a/rpc/jsonrpc/server/ws_handler_test.go b/rpc/jsonrpc/server/ws_handler_test.go index 11c97ade0..00d6a18a5 100644 --- a/rpc/jsonrpc/server/ws_handler_test.go +++ b/rpc/jsonrpc/server/ws_handler_test.go @@ -28,7 +28,7 @@ func TestWebsocketManagerHandler(t *testing.T) { } // check basic functionality works - req, err := rpctypes.MapToRequest( + req, err := rpctypes.ParamsToRequest( rpctypes.JSONRPCStringID("TestWebsocketManager"), "c", map[string]interface{}{"s": "a", "i": 10}, diff --git a/rpc/jsonrpc/types/types.go b/rpc/jsonrpc/types/types.go index 617be7ebd..a44faf92f 100644 --- a/rpc/jsonrpc/types/types.go +++ b/rpc/jsonrpc/types/types.go @@ -98,17 +98,27 @@ func (req RPCRequest) String() string { return fmt.Sprintf("RPCRequest{%s %s/%X}", req.ID, req.Method, req.Params) } -func MapToRequest(id jsonrpcid, method string, params map[string]interface{}) (RPCRequest, error) { - var paramsMap = make(map[string]json.RawMessage, len(params)) - for name, value := range params { - valueJSON, err := tmjson.Marshal(value) - if err != nil { - return RPCRequest{}, err +// ParamsToRequest constructs a new RPCRequest with the given ID, method, and parameters. +func ParamsToRequest(id jsonrpcid, method string, params interface{}) (RPCRequest, error) { + var payload json.RawMessage + var err error + switch t := params.(type) { + case map[string]interface{}: + // TODO(creachadair): This special case preserves existing behavior that + // relies on the custom JSON encoding library. Remove it once that + // requirement has been removed. + paramsMap := make(map[string]json.RawMessage, len(t)) + for name, value := range t { + valueJSON, err := tmjson.Marshal(value) + if err != nil { + return RPCRequest{}, err + } + paramsMap[name] = valueJSON } - paramsMap[name] = valueJSON + payload, err = json.Marshal(paramsMap) + default: + payload, err = json.Marshal(params) } - - payload, err := json.Marshal(paramsMap) if err != nil { return RPCRequest{}, err } diff --git a/rpc/test/helpers.go b/rpc/test/helpers.go index b7db50f6c..42f78c01f 100644 --- a/rpc/test/helpers.go +++ b/rpc/test/helpers.go @@ -32,7 +32,7 @@ func waitForRPC(ctx context.Context, conf *config.Config) { } result := new(coretypes.ResultStatus) for { - _, err := client.Call(ctx, "status", map[string]interface{}{}, result) + err := client.Call(ctx, "status", map[string]interface{}{}, result) if err == nil { return }