rpc: support for named parameters in client #22656
Conversation
asutula
requested review from
fjl,
holiman,
rjl493456442 and
zsfelfoldi
as code owners
rpc/client.go
Outdated
| // CallArgsOpt is created by calling PositionalArgs, NamesArgs or NoArgs | ||
| // and controls the encoding of arguments in the RPC request. | ||
| type CallArgsOpt func(*callArgs) | ||
|
|
||
| // PositionalArgs is used to provide args in the server-expected order. | ||
| // They will be encoded as an array. | ||
| func PositionalArgs(args ...interface{}) CallArgsOpt { | ||
| return func(ca *callArgs) { | ||
| ca.list = args | ||
| } | ||
| } | ||
|
|
||
| // NamedArgs is used to provide args as a struct or map | ||
| // with member names that match the server-expected arg names. | ||
| // They will be encoded as an object. | ||
| func NamedArgs(arg interface{}) CallArgsOpt { | ||
| return func(ca *callArgs) { | ||
| ca.object = arg | ||
| } | ||
| } | ||
|
|
||
| // NoArgs is used to specify that there are no args. | ||
| func NoArgs() CallArgsOpt { | ||
| return func(ca *callArgs) {} | ||
| } | ||
|
|
There was a problem hiding this comment.
Here are the new "args options" to choose from. The two funcs below are updated to accept args in this way, and basically the rest of the changes in this PR are just refactoring to accommodate that change.
rpc/client_test.go
Outdated
| Result: new(int), | ||
| Error: &jsonError{Code: -32601, Message: "the method no_such_method does not exist/is not available"}, | ||
| }, | ||
| } | ||
| for i := range batch { | ||
| batch[i].ArgsOpt = nil | ||
| } |
There was a problem hiding this comment.
Tough decision here... was trying to understand the extent of what we're really trying to test, and decided that dropping the deep equal check for ArgsOpt was the easiest but still safe-ish way to go. Something like this is necessary because ArgsOpt is a func and non-nil funcs are never deeply equal. There would be other possibilities for testing this but they would require larger changes to this test, so I just stuck with simple for now.
|
We have discussed this today, and the question has come up a couple times before. For Ethereum itself, named args are not used and would not add much value. We would also need to standardize the arg names for the API, which is not done right now. However, I understand that the go-ethereum RPC implementation is generally useful even if you don't care about Ethereum at all, and in other APIs named args are more useful. The way you have implemented it here is not great because it is a backwards-incompatible change and requires writing |
asutula
force-pushed
the
asutula/rpc-object-support
branch
from
b3c07b1
to
9c653ff
Compare
Signed-off-by: Aaron Sutula <hi@asutula.com>
|
Yea that makes sense. I took a stab at that here, let me know if that is what you're thinking. |
| func TestClientNamedParams(t *testing.T) { | ||
| client := Client{} | ||
|
|
||
| type params struct { | ||
| Str string | ||
| Int int | ||
| Args *echoArgs | ||
| } | ||
|
|
||
| p := params{ | ||
| Str: "hello", | ||
| Int: 10, | ||
| Args: &echoArgs{"world"}, | ||
| } | ||
|
|
||
| method := "test_echo" | ||
| msg, err := client.newMessage(method, NewNamedParams(p)) | ||
| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| if msg.Method != method { | ||
| t.Fatalf("expect method %s but got %s", method, msg.Method) | ||
| } | ||
| var msgParams params | ||
| if err := json.Unmarshal(msg.Params, &msgParams); err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| if !reflect.DeepEqual(msgParams, p) { | ||
| t.Errorf("incorrect params %#v", msgParams) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
The goal of this test is to ensure that request params are correctly encoded when NamedParams are passed as the sole argument. Testing named params with an actual server would require that the server supports named params, which I don't think this package's server implementation does. Changing that would be beyond the scope of this PR since the goal here is just to allow the client to be able to communicate with servers that do support named params.
fjl
changed the title
Signed-off-by: Aaron Sutula <asutula@users.noreply.github.com>
Hey turns out this repo contains the best looking JSON RPC 2 library out there. I was trying to use its client code against a server that expects
paramsto be an object with member names that match what the server expects (this is supported by the JSON RPC 2 spec), but it seems thatparamsis always encoded as an array. This PR changes that, allowing the caller to specify params as an array or struct/map. This would make therpcpackage more generally usable... Not sure if that aligns with the goals of this code base (or maybe it could be it's own code base), but I know the larger community of Go developers would appreciate it.