Skip to content
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

soroban-rpc: add jrpc2 batch compliance test #965

Closed
wants to merge 2 commits into from

Conversation

tsachiherman
Copy link
Contributor

What

add jrpc2 batch compliance test

Why

Add a test to verify the output of the jrpc library.

Known limitations

n/a

@tsachiherman tsachiherman self-assigned this Sep 14, 2023
@2opremio
Copy link
Contributor

Doesn't this belong to the library itself?

@tsachiherman
Copy link
Contributor Author

Doesn't this belong to the library itself?

Yes.. but I wanted to confirm that on our end first.

I agree that this test need to move to the library.

@tsachiherman
Copy link
Contributor Author

tsachiherman commented Sep 18, 2023

I spent some time looking into this issue. According to the jrpc2 specification:

	//
	//   The Server SHOULD respond with an Array containing the corresponding
	//   Response objects
	//

where the word SHOULD is interpreted as per RPC 2119:

3. SHOULD   This word, or the adjective "RECOMMENDED", mean that there
   may exist valid reasons in particular circumstances to ignore a
   particular item, but the full implications must be understood and
   carefully weighed before choosing a different course.

which means that the responsibility for decoding this either as an array or a single element falls on the client, which MUST be capable of supporting both, given that it knows that the server has that flexibility.

Would I implement it in that way ? most likely not. But any client being unable to decode the response is a non-compliant client, whereas the server is protocol compliant.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants