soroban-rpc: adding optional params breaks clients using params array format

[leighmcculloch]

What version are you using?

⁠ v20.1.0 ⁠

What did you do?

•⁠ ⁠Request the ⁠ simulateTransaction ⁠ endpoint using array format with a single argument in that params list:

{
  "jsonrpc": "2.0",
  "id": 0,
  "method": "simulateTransaction",
  "params": ["AAAAAgAAAADRjwIQ/2zB8tzxMB+71MMO4RoHWCBoTUcd+J0PEBHqKAAAAGQAAAF4AAAAAwAAAAAAAAAAAAAAAQAAAAEAAAAA0Y8CEP9swfLc8TAfu9TDDuEaB1ggaE1HHfidDxAR6igAAAAYAAAAAgAAAnMAYXNtAQAAAAEUBGABfgF+YAJ/fgBgAn5+AX5gAAACDQIBaQEwAAABaQFfAAADBgUBAgMDAwUDAQAQBhkDfwFBgIDAAAt/AEGAgMAAC38AQYCAwAALBy8FBm1lbW9yeQIAA2FkZAADAV8ABgpfX2RhdGFfZW5kAwELX19oZWFwX2Jhc2UDAgqMAgVdAgF/AX4CQAJAIAGnQf8BcSICQcAARg0AAkAgAkEGRg0AQgEhA0KDkICAgAEhAQwCCyABQgiIIQFCACEDDAELQgAhAyABEICAgIAAIQELIAAgATcDCCAAIAM3AwALmQEBAX8jgICAgABBIGsiAiSAgICAACACQRBqIAAQgoCAgAACQAJAIAIoAhANACACKQMYIQAgAiABEIKAgIAAIAIpAwCnDQAgACACKQMIfCIBIABUDQECQAJAIAFC//////////8AVg0AIAFCCIZCBoQhAAwBCyABEIGAgIAAIQALIAJBIGokgICAgAAgAA8LAAALEISAgIAAAAsJABCFgICAAAALBAAAAAsCAAsASw5jb250cmFjdHNwZWN2MAAAAAAAAAAAAAAAA2FkZAAAAAACAAAAAAAAAAFhAAAAAAAABgAAAAAAAAABYgAAAAAAAAYAAAABAAAABgAeEWNvbnRyYWN0ZW52bWV0YXYwAAAAAAAAABQAAAAAAG8OY29udHJhY3RtZXRhdjAAAAAAAAAABXJzdmVyAAAAAAAABjEuNzQuMQAAAAAAAAAAAAhyc3Nka3ZlcgAAAC8yMC4wLjMjOTNiMDllNDJlNGVmYTg0MWNiZDAzNGMwYmZmMGRjMzYyNzY1MDg2YwAAAAAAAAAAAAAAAAAA"]
}

What did you expect to see?

The request to be accepted as it is with soroban-rpc v20.0.2.

What did you see instead?

jsonrpc error: ErrorObject { code: InvalidParams, message: "invalid parameters", data: Some(RawValue("[-32602] got 1 parameters, want 2")) }

Also, if I make the same request, but with the params passed as a JSON objected with named fields, it is accepted:

{
  "jsonrpc": "2.0",
  "id": 0,
  "method": "simulateTransaction",
  "params": {"transaction":"AAAAAgAAAADRjwIQ/2zB8tzxMB+71MMO4RoHWCBoTUcd+J0PEBHqKAAAAGQAAAF4AAAAAgAAAAAAAAAAAAAAAQAAAAEAAAAA0Y8CEP9swfLc8TAfu9TDDuEaB1ggaE1HHfidDxAR6igAAAAYAAAAAgAAAnMAYXNtAQAAAAEUBGABfgF+YAJ/fgBgAn5+AX5gAAACDQIBaQEwAAABaQFfAAADBgUBAgMDAwUDAQAQBhkDfwFBgIDAAAt/AEGAgMAAC38AQYCAwAALBy8FBm1lbW9yeQIAA2FkZAADAV8ABgpfX2RhdGFfZW5kAwELX19oZWFwX2Jhc2UDAgqMAgVdAgF/AX4CQAJAIAGnQf8BcSICQcAARg0AAkAgAkEGRg0AQgEhA0KDkICAgAEhAQwCCyABQgiIIQFCACEDDAELQgAhAyABEICAgIAAIQELIAAgATcDCCAAIAM3AwALmQEBAX8jgICAgABBIGsiAiSAgICAACACQRBqIAAQgoCAgAACQAJAIAIoAhANACACKQMYIQAgAiABEIKAgIAAIAIpAwCnDQAgACACKQMIfCIBIABUDQECQAJAIAFC//////////8AVg0AIAFCCIZCBoQhAAwBCyABEIGAgIAAIQALIAJBIGokgICAgAAgAA8LAAALEISAgIAAAAsJABCFgICAAAALBAAAAAsCAAsASw5jb250cmFjdHNwZWN2MAAAAAAAAAAAAAAAA2FkZAAAAAACAAAAAAAAAAFhAAAAAAAABgAAAAAAAAABYgAAAAAAAAYAAAABAAAABgAeEWNvbnRyYWN0ZW52bWV0YXYwAAAAAAAAABQAAAAAAG8OY29udHJhY3RtZXRhdjAAAAAAAAAABXJzdmVyAAAAAAAABjEuNzQuMQAAAAAAAAAAAAhyc3Nka3ZlcgAAAC8yMC4wLjMjOTNiMDllNDJlNGVmYTg0MWNiZDAzNGMwYmZmMGRjMzYyNzY1MDg2YwAAAAAAAAAAAAAAAAAA"}
}

Discussion

This issue has been discussed in these places:

• Discord: https://discord.com/channels/897514728459468821/1187260422689132585
• Slack: https://stellarfoundation.slack.com/archives/C030Z9EHVQE/p1703122700787559

This issue arises because in v20.1.0 the soroban-rpc's simulateTransaction endpoint had a new optional parameter added to it's request object:

• soroban-rpc: Allow Budget Instruction Leeway to be Configured by Clients stellar-cli#1131

The jsonrpc specification doesn't say how optional parameters should behave, it is implementation specific, and the jsonrpc library in use by the soroban-rpc allows optional parameters when the params are defined as a JSON object, but requires them when the params are defined as a JSON array.

From discussions on the jsonrpc mailing list it sounds like it is common for jsonrpc implementations to allow optional parameters to be ommitted in the params (see https://groups.google.com/g/json-rpc/c/n4kiH9yKBww/m/0ssppnkGpv8J), but that behavior isn't supported by the jsonrpc library the soroban-rpc is using.

Until the soroban-rpc's jsonrpc implementation supports omitting optional parameters in the params array format, I don't see how we can ever add an optional parameter though, since doing so may break any client that is using array format, as what we saw with the above v20.1.0 release.

cc @Shaptic @mollykarcher

[2opremio]

@leighmcculloch do you have any idea of how prevalent is the usage of arrays? (if I am honest, I didn't even know it was allowed until I saw it in this ticket)

[leighmcculloch]

do you have any idea of how prevalent is the usage of arrays?

Given the inbound complaints when the params change went out, with folks discussing that the JS and CLI were broken it suggests some clients are using arrays, or were at least. It's difficult to know what the usage will be going forward or what other uses existed.

We selected JSON-RPC as the API format for this service, and with that we accepted both forms of interacting with it. I think it'd be difficult for us to say array params are unsupported since a JSON-RPC client may use that format and that could be out of control of the developer using the client.

To find the answer to this question one avenue might be for you to take a look at the JSON-RPC clients in different languages and see which use arrays in their requests.

[leighmcculloch]

If we were to discontinue support for array style requests or discontinue support for maintaining them in a backwards compatible way, it would be ideal to actually disable support for that request type, so that no one could use it in the first place. Again, this would probably require some research that there aren't clients out there that support only it.

[2opremio]

I think we have 3 options:

1. Disable support for array parameter requests (I don't like this one since it breaks compatibility with JSONRPC)
2. Allow support for optionals in arrays (e.g. by assuming that any missing suffix in the array is constituted by optionals). I have created an upstream ticket Optional parameter support in arrays creachadair/jrpc2#113.
3. Simply don't assume optional parameters will give us backwards compatibility.

I would go for (2), but it's a bit awkward. I wonder how other libraries deal with this (I haven't looked into this yet).

[leighmcculloch]

I wonder how other libraries deal with this (I haven't looked into this yet).

From discussions on the jsonrpc mailing list it sounds like it is common for jsonrpc implementations to allow optional parameters to be ommitted in the params (see https://groups.google.com/g/json-rpc/c/n4kiH9yKBww/m/0ssppnkGpv8J).

[2opremio]

The upstream library is about to merge a way to disable array parameters. creachadair/jrpc2#114

So, now it's becoming a real possibility.

[leighmcculloch]

We could do a little reach out in Discord and through folks we know are developing on the RPC API and query them if they are using the array format.

[2opremio]

Good idea!

[2opremio]

Another point of information is that our openrpc spec indicates that all methods are by-name.

So, in theory we could claim that by-position method-calls are not compliant (and we should probably disable array parameters to enforce it)

[leighmcculloch]

I think that's reasonable from a specification point-of-view, except that in practice the service has worked with arrays up until now, and even the js-stellar-sdk/soroban-client and soroban-cli have utilitized arrays. So if anyone else was using arrays, I would find that reasonable too.

Given how new the service is and that it would be better long to support only JSON objects for clarity of requests, I think it's reasonable for us to pursue turning off arrays. We should communicate it in the release notes, and probably also do some shout outs about it in the Discord before the release. (cc @tyvdh)

[creachadair]

One point that is probably worth noting here:

1. Disable support for array parameter requests (I don't like this one since it breaks compatibility with JSONRPC)

While JSON-RPC requires parameters to be specified as arrays or objects, that doesn't mean your API has to accept both if they don't make sense for your method. It only means they have to be one or the other to be a valid request message (or omitted entirely). The method is still free to reject one or the other (or both, if it doesn't want there to be any parameters) without violating the spec. That's what the -32602 (Invalid Params) error code is meant for.

I realize in this case you may need to work around some existing use, but you can freely tighten the rules on new API methods within the JSON-RPC rules. YMMV, but perhaps that will help simplify your decision making a bit.

[kalepail]

fwiw I'm not terribly concerned with disabling arrays from a backwards compatibility perspective. It'll be an easy tweak I believe and most folks are using SDKs anyway vs querying the RPC directly. Soroban by now has a long and colorful history of breaking things and as long as we're pre-mainnet we can afford a few more breaking changes to client software like this. It does need to be communicated to SDK developers but I don't foresee many issues here.

I will say the syntax here is really odd though. Can you simulate things other than a transaction? Why is this an object or an array at all in the first place? Or is this just a nuance of the JSON-RPC spec?

[2opremio]

OK, then we should probably disable array-based requests.

[mollykarcher]

If we disable all array-based requests, that is technically a breaking change. However, if we don't implement this change, then the next time that we want to add an optional parameter to a request, that change will be breaking (where normally adding optional params would not be). Given that with pubnet launch approaching we'll likely be starting to get more developer feedback/requests, it seems likely to be that this will be inevitable, and in that case it might just be better to do it sooner rather than later. So I'm also inclined to go forward with disabling it.

We could also just wait until we get a feature request that necessitates adding a new optional parameter and decide at that point. But regardless, we should probably file issues with ecosystem SDKs and queue up work to update the JS SDK now.

@janewang thoughts?

[creachadair]

Another option you could consider, if you're concerned about compatibility with existing use, is to write custom unmarshalers for your various parameter types that special-case the "old" (existing) usage, but require new fields to be specified as objects.

This way, old code that doesn't need the new fields will still work, but you can still enforce that optional parameters are plumbed via an object.

Example: https://go.dev/play/p/VBLyRI5td1J

[leighmcculloch]

I'm a +1 for simply turning off array requests. If we're going to do it, doing it sooner is better than later.

Writing custom unmarshalers to handle the old format also makes sense, but given the recent breakage probably pushed people to switching, hopefully it would be unnecessary to write the unmarshalers.

[janewang]

+1 on deprecating array format. I suggest we do this in the upcoming protocol 20 release as it is a breaking change

[2opremio]

It’s an easy change, but we first need our SDKs to only use objects (CC @Shaptic )

[mollykarcher]

@janewang should we make an announcement in discord about this upcoming change?

[janewang]

We will tee it up for protocol 21 announcements.