This repository was archived by the owner on Mar 11, 2025. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 17
changed startLedger and endLedger of getEvents to be string types #36
Merged
paulbellamy
merged 4 commits into
stellar:main
from
sreuland:fix_getevents_ledger_params
Jan 23, 2023
Merged
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
b565dcc
changed startLedger and endLedger of getEvents to be string types for…
sreuland c6ed5a4
removed params array usage if only one param value, rpc json unmarsha…
sreuland 46b61c1
clean up the fix for passing string ledgers and object as params
sreuland 4cb61f2
#38: new jsonrpc.postObject method for getEvents to use
sreuland File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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:
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 justxdr.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,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 thatparams:[jsObj]
andparams:jsObj
are both valid forms and should resolve to samejsObj
, 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#5How 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 bygetEvents
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