-
Notifications
You must be signed in to change notification settings - Fork 17
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
changed startLedger and endLedger of getEvents to be string types #36
changed startLedger and endLedger of getEvents to be string types #36
Conversation
… compatibility with rpc interface
…l was not working when params was sent as array with one element
let jsonParams = params; | ||
if (params.length === 1) { | ||
jsonParams = params[0]; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, is this trying to handle when params
is an object? Or, I'm not sure what this is doing...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@paulbellamy , yes, I cleaned up the code here, i ran into two errors when trying the client against rpc server:
- params.endLedger and params.startLedger need to be strings
- params needs to be object not array for getEvents, otherwise jsronrpc had unmarshal error on it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, but as it is, I suspect this change might break methods with a single parameter. For example, I pass [xdr.ScVal(...)]
, but then that get's converted to just xdr.ScVal(...)
, and the call fails. I expect object/array thing is because the pagination param at the end is required (not optional, bug in the docs). Was that what you were hitting?
Could we just do the startLedger/endLedger change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will still get error due to the params being an array, was getting error from rpc getEvents
server handler,
"error": {
"code": -32602,
"message": "invalid parameters: [-32602] invalid parameters"
}
the jrpc2 lib has json unmarsal errors with this params
json when it introspects and tries to marshal it into GetEventsRequest struct, both for number types on startLedger/endLedger, and the params being an array with a single object.
the jsonrpc spec allows array or object type for the params
, and i saw jrpc2 tries to handle both, but ends up in error anyways which I didn't step through deeper to see why. but it seems that params:[jsObj]
and params:jsObj
are both valid forms and should resolve to same jsObj
, which covers your example such as [xdr.ScVal(...)]
Looking at the other server.ts methods, none are passing arrays with single js objects yet, they are mostly passing arrays with single scalar value, and the change introduced here , would leave that as-is.
I would need to execute all the server.ts methods from this pr against current rpc server manually to validate your concern. We need an integration test between js client and rpc to verify these two are compliant with each other, maybe can automate by adding those calls to the existing integration client_headers_test.js
? In parallel, I think system-test will start to establish that coverage with stellar/system-test#5
How about i just create a jsonrpc.postObject()
method and have getEvents use that form, to isolate this for now to getEvents, which can confirm it will work in that scope.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I created issue #38 , has curl examples of the getEvents call with params
formats that error and work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh that's true it would only affect soroban-rpc methods with a single arg. I was mixing that up with contract fns with a single arg. Separate postObject
used by getEvents
to be more explicit (with a note about why it's needed) would be good, IMO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, added jsonrpc.postObject
- 4cb61f2
currently getting error response from rpc server for
server.getEvents
invocations:the rpc server jsonrpc handler expects
startLedger and
endLedgerin
getEvents:paramsto be integers expressed as string types, also saw the same error when sending
params:[{...}], getEvents wasn't able to unmarshal the params array, it only worked as
params:{}`Closes #38