XRPLF / rippled

Decentralized cryptocurrency blockchain daemon implementing the XRP Ledger protocol in C++
https://xrpl.org
ISC License
4.64k stars 1.48k forks source link

Rippled API isn't compliant with the JSON RPC specification #3065

Closed hlb8122 closed 1 year ago

hlb8122 commented 5 years ago

Both JSON RPC 1.0 and 2.0 require that when a request invokes an error the "result" field is left empty and the error information is given in the "error" field.

This was noted in 2013 and the issue was closed in 2017. https://github.com/ripple/rippled/issues/214

intelliot commented 5 years ago

I believe that the issue you linked was resolved with https://github.com/ripple/rippled/pull/1907.

~Currently, when a request invokes an error, I think the "result" field is generally left empty and the error information is given in the "error", "error_code", and "error_message" fields. Which aspect of this behavior do you want changed?~ (This applies to the WebSocket API)

I think the rippled JSON-RPC API is actually compliant if you include "ripplerpc": "2.0" in the params. For example:

{
    "method": "account_info",
    "params": [{
        "id": 1,
        "jsonrpc": "2.0",
        "ripplerpc": "2.0",
        "account": "x"
    }]
}

Unfortunately, it doesn't look like this is documented anywhere. (Perhaps something for @mDuo13 to have a look at.)

hlb8122 commented 5 years ago

Is this what you meant?

{
    "method": "account_info",
    "jsonrpc": "2.0",
    "params": [
        {
            "ripplerpc": "2.0",
            ...
        }
    ],
    "id": 1,
}
intelliot commented 5 years ago

OK, I think you're saying that the rippled API is not compliant because it expects "id" and "jsonrpc" to be inside of the object in the params array, instead of at the top level. Is that right?

hlb8122 commented 5 years ago

I think you've just slipped up when writing your response?

As I've written it above that'd be compliant, but looking here it seems that

{
    "method": "account_info",
    "jsonrpc": "2.0",
    "ripplerpc": "2.0",
    "params": [
        {
            ...
        }
    ],
    "id": 1,
}

is the current case?

As stated here, the reference implementation written by the author of the JSON RPC 2.0 spec would reject this because extra fields in request objects aren't intended.

intelliot commented 5 years ago

It seems like that code path is only exercised when the commandline json2 feature is used, calling rippled from the command line. For example: ./rippled json2 '{ "jsonrpc" : "2.0", "ripplerpc" : "2.0", "id" : 5, "method": "server_info" }'

On the other hand, when calling the JSON-RPC API over HTTP, it's a different story. The example I posted earlier, with "id", "jsonrpc": "2.0", and "ripplerpc": "2.0" inside of the object in the params array, is the only variant that works for me in practice (using rippled version 1.3.1).

hlb8122 commented 5 years ago

hmmm, in that case you're right: if all but the method field are inside params then it certainly doesn't meet the JSON RPC 2.0 spec. Given that this isn't documented yet perhaps you could fix it without classing it as a breaking change.

I recommend against putting ripplerpc at the top-level with alongside id, method etc because: 1) The reference implementation would reject it, 2) The interface to most clients doesn't allow you to set top-level fields apart from the method and params.

I'm unsure of the correct way to do versioning in JSON RPC. Perhaps you're supposed place it on a new endpoint?

movitto commented 5 years ago

hmmm, in that case you're right: if all but the method field are inside params then it certainly doesn't meet the JSON RPC 2.0 spec. Given that this isn't documented yet perhaps you could fix it without classing it as a breaking change.

Existing clients would still need to be updated to be compatible. That being said a clean switch over (with no backwards compatibility between versions) would reduce code bloat and development complexity. Relevant docs would need to be updated of course.

I recommend against putting ripplerpc at the top-level with alongside id, method etc because:

1. The reference implementation would reject it,
2. The interface to most clients doesn't allow you to set top-level fields apart from the `method` and `params`.

I wrote an implementation a while back that permits extra 'header' fields, a feature I & others found useful in projects which depended on it (we used it for persistent 'session-id' & other authentication features).

I understand the desire to maintain compatibility with various JSON-RPC implementations but as mentioned in the stackoverflow posted above, this behaviour (extra fields) is not defined in the spec and thus it technically is permitted

hlb8122 commented 5 years ago

I understand the desire to maintain compatibility with various JSON-RPC implementations but as mentioned in the stackoverflow posted above, this behaviour (extra fields) is not defined in the spec and thus it technically is permitted

I understand. ripplerpc being a version field at the top-level is not a huge concern. The main problem I see is that the error field is never used in v1 responses and, as @intelliot pointed out, once v2 is enabled the id and jsonrpc jumps inside the params.

It's also worth noting that JSON RPC 2.0 does not require that params is an array of objects like in 1.0.

SteelBRS commented 4 years ago

Oh dear ... so I tried using http://software.dzhuvinov.com/json-rpc-2.0-client.html

    val serverURL = URL("http://s1.ripple.com:51234")
    val session = JSONRPC2Session(serverURL)
    val method = "account_offers"
    val namedParams = mapOf("account" to RIPPLE_ACCOUNT)
    val requestID = 1
    val request = JSONRPC2Request(method, namedParams, requestID)
    val response = session.send(request)
    if (response.indicatesSuccess())
        println(response.result)
    else
        println(response.error.message)

So I need to find an implementation of JSON-RPC 1.0? ... from 2005?

sigh

Any progress on this issue? Any idea when you'll support JSON-RPC 2.0?

SteelBRS commented 4 years ago

Nope ... I simply won't soil my modern Kotlin codebase with libraries from 15 years ago.

I'm just gonna have to wait until you get this fixed.

Starting down this path leads to the dark side - then I might as well abandon Event Sourcing and go back to relational databases from the 1970's (which is comparable to cars going from electric to coal engines)

MarkusTeufelberger commented 4 years ago

Well, cars went from electric to gas engines in the early 20th century... ;-)

Are there actual errors you encounter because the JSON-RPC 2.0 spec isn't 100% followed? You could also try the new gRPC interface by the way, this should be better typed and more modern than JSON-RPC.

SteelBRS commented 4 years ago

Yes, I've heard of this mad try from the gas companies to fight the future ... also gas powered radios to keep the electric companies out of the households.

Sorry again for the tone in my comments, but there's nothing that can get my blood up more than bad/legacy tech IT solutions.

I've tried adding all the parameters discussed earlier in this thread, but all tries ended up with an HTTP 400.

I'll see if I can get an elegant solution using gRPC :relieved: :thumbsup:

SteelBRS commented 4 years ago

Is gRPC enabled on the public servers? https://xrpl.org/configure-grpc.html

MarkusTeufelberger commented 4 years ago

Why public servers? Already in this issue it is stated that JSON-RPC 2.0 only works on command line, so you clearly have a local server running if you did your tests with the parameters discussed in this thread?

SteelBRS commented 4 years ago

I guess we're back to timelines then.

Any progress on this issue? Any idea when you'll support JSON-RPC 2.0?

On the public servers ... I write client software to the ripple network, I have no benefit in running my own rippled server.

MarkusTeufelberger commented 4 years ago

Then you're in the wrong place I'm afraid, since this is the repository for the code of this server, not the contact point for the operations team at Ripple who are running some public servers. I'm sure you can find someone to run a server for you if you need such special configurations and don't want to run it yourself for whatever reason.

The public servers operated by Ripple by the way are NOT intended (or fit...) to be used for production. You'll either need to operate your own or pay someone to do it for you anyways.

SteelBRS commented 4 years ago

Then I must admit I don't know how Ripple servers work. Because I thought the public servers were for production. And if you ran your own rippled server, it would maintain your own little test network i.e. for test usage only.

Quite embarrassing actually ... I've written quite an efficient trading robot, but don't understand the most basic concept about Rippled servers :upside_down_face:

MarkusTeufelberger commented 4 years ago

https://xrpl.org/get-started-with-the-rippled-api.html#public-servers

These public servers are not for sustained or business use, and they may become unavailable at any time. For regular use, you should run your own rippled server or contract someone you trust to do so.

The first sentence in the README of this repository sums it up nicely:

"The XRP Ledger is a decentralized cryptographic ledger powered by a network of peer-to-peer servers."

Peer-to-peer means that there are no differences between servers you run or that someone else runs, they are equals. You can just run a local instance, tell it to connect to the main network (it will even do so by default) and you'll be on your merry way. As this has nothing to do with the original issue any more though I just recommend you to read the documentation on https://xrpl.org in case there are more things unclear to you.

SteelBRS commented 4 years ago

You threw the book at me! (a legalese term, but I'm taking it back to IT for when people tell you to read the manual)

SteelBRS commented 4 years ago

Running rippled-1.5.0 on Debian Buster @ Scaleway

Unfortunately the workaround @intelliot talked about doesn't work:

image

Maybe if I downgrade to rippled version 1.3.1?

SteelBRS commented 4 years ago

An ambitious solution here might be to throw out both JSON-RPC 1.0 and gRPC ... and replace them with GraphQL. That way the clients can get exactly what they need and not waste any processing or networking resources on - to the client - irrelevant data.

MarkusTeufelberger commented 4 years ago

I definitely do NOT want GraphQL exclusively. Feel free to open an issue + PRs for a GraphQL interface in rippled though. I have a feeling however that getting GraphQL into a C++/boost code base will be a challenge.

The following seems to work by the way (I don't know how a 100% valid response would look like):

{
  "id": 1337,
  "command": "account_info",
  "account": "r9cZA1mLK5R5Am25ArfXFmqgNwjZgnfk59",
  "ledger_index": "current",
  "jsonrpc": "2.0",
  "ripplerpc": "2.0"
}

or with ping:

{
  "id": 1,
  "command": "ping",
  "jsonrpc": "2.0",
  "ripplerpc": "2.0"
}

Copy-paste into https://xrpl.org/websocket-api-tool.html#account_info and send the request.

SteelBRS commented 4 years ago

Thank you for your time finding a solution @MarkusTeufelberger Those requests however are not compliant JSON-RPC 2.0 requests https://www.jsonrpc.org/specification#request_object

SteelBRS commented 4 years ago

Well ... I can see the writing on the wall ... this issue was opened almost a year ago and nothing has happened. To get any progress within a reasonable amount of time I guess I'll just have to soil my codebase with oldschool JSON-RPC 1.0 code :frowning_face: I really hope that you get funding to modernize Ripple's tech ... seems like the owners just pad their own fortunes rather than fixing the company.

MarkusTeufelberger commented 4 years ago

Well, I got it to run on my local node now using the command line, with a correct RPC call and response (though it still requires adding that extra field that was already criticized in the original PR - which was merged about half a decade ago btw.). I also got it to work against s1.ripple.com, but again the workarounds described in here apply. Since they only apply to parameters you already have full control over, I guess you should be able to manage.

SteelBRS commented 4 years ago

I did a solution with a simple REST call. Yes your requests are valid JSON-RPC 1.0, version 2.0 just formalizes which elements are allowed in the root document: jsonrpc="2.0", method, params (optional) and id

MarkusTeufelberger commented 4 years ago

Yes, I managed with jsonrpc 2.0 (the calls look different than the ones I posted).

SteelBRS commented 4 years ago

Cheers, dude ... sorry for nagging, but ... being a programmer for about 34 years, I still try to challenge myself to create the most elegant, easily readable & maintainable solutions - and these are often obtained using the latest tech. If you're not trying to be the best possible programmer, then it's time to change profession. I'm sure your goals are the same.

Sometimes to create better IT you have to look back though ... origins of Event Sourcing 😅 image

MarkusTeufelberger commented 4 years ago

Sure, looking forward to your contributions.

SteelBRS commented 4 years ago

Erhhh ... Ripple pays their developers, right? 😄

MarkusTeufelberger commented 4 years ago

I guess? I'm not employed there.

cjcobb23 commented 4 years ago

@SteelBRS just out of curiousity, why don't you implement the JSON 2.0 functionality that you desire and create a pull request? This is an open source project, so anyone can contribute.

Or you could use the gRPC API. The gRPC API only has a subset of the functionality of the JSON API, but extending that functionality should be straightforward. The infrastructure is already there.

SteelBRS commented 4 years ago

I don't work for free. Well I did at Red Cross Codeathon 2017 ... but I won't for a company that has plenty of money to pay employees to get their tech in order.

MarkusTeufelberger commented 4 years ago

@cjcobb23 - JSON-RPC 2.0 was already implemented and added to rippled years ago, but incorrectly/not according to spec and the functionality remains to this day undocumented. It is also really hard to hit that code path and definitely not obvious. This is the issue that tracks the workarounds to convince rippled to actually give a JSON-RPC 2.0 answer e.g. via web sockets or the RPC port.

GRPC functionality in rippled is only experimental as far as I know and doesn't seem to be actively extended to match the existing methods any further. Especially with RPC interfaces I would expect them to be used by people that are not comfortable coding in C++ by the way (otherwise they could just use libXRPL), so the "straightforward extension of functionality" might be a bigger task than you imagine.

cjcobb23 commented 4 years ago

@MarkusTeufelberger good points. Just want to mention, we would welcome an extension to the gRPC API. The gRPC API is currently being used by Xpring to build SDK's in various languages; we implemented the minimal subset that Xpring needed for the SDKs. It would be ideal to continue to extend the gRPC API to match the existing methods; it's just a low priority task. However, the gRPC API is somewhat tangential to this issue.

@SteelBRS - how you look at the work is your own perspective and I can't force a perspective upon you. However, if you need this functionality as part of a side project, you could consider the development as part of said side project. If you need this functionality for work you are doing for an employer, you could view implementing this functionality as part of your job, and implement during your normal work hours, that you are getting paid for. It's a matter of perspective though, and I'm not asserting this perspective I laid out is any more correct than your perspective.

intelliot commented 1 year ago

Closing as we have no specific plans to make changes here, but y'all please feel free to open a new issue with any particular requests or needs.