Add book_changes rpc #4212
Add book_changes rpc #4212
Conversation
|
|
||
| template <class L> | ||
| Json::Value | ||
| computeBookChanges(std::shared_ptr<L const> const& lpAccepted) |
There was a problem hiding this comment.
Is it possible to compute these changes outside of rippled?
There was a problem hiding this comment.
It is possible to compute everything outside of rippled, given that it is a public ledger with a public mesh and well defined data formats.
There was a problem hiding this comment.
While theoretically true, that's not practically true. For instance, if I wanted to compute the state of each new ledger based on that ledgers transactions, outside of rippled, I'd have to copy the transaction application code and keep it up to date with amendments. While theoretically possible, this would be a huge lift, and I'd argue not practically possible. It would make sense to just rely on rippled for such a thing.
But this code just seems like parsing metadata. Is there a reason this can't be done in a simple Python script, outside of rippled?
There was a problem hiding this comment.
You would first need to request all the txn meta data for a given ledger from rippled then compute the result. This is a much heavier rpc call because you're pushing [potentially a lot] more data to an external service for aggregation, instead of just computing it in place where it already exists. It's equivalent to dumping every row in an SQL table because you want to find the SUM() of a column.
|
I'm fine with this and agree that APIs that are "established" and in serious use could move inside May I suggest a config tunable that allows specific RPC endpoints to be disabled? This could make sense for other commands or command categories. |
|
@nbougalis While I agree with you in principle (that RPC permissions should be granular) I don't think it's really the time or place to add those changes. The {
"command": "ledger",
"ledger_index": "validated",
"transactions": true,
"expand": true
}@cjcobb23 There's no reason to pick on an RPC call that:
I realise Ripple apparently now dislikes RPC (although not enough to avoid new calls for NFTs?) but Ripple's internal preferences are not a reason to hamper basic use by other users of the network. Ripple can disable RPC on their own public facing nodes if that is what they wish to do, no one is stopping them/you from doing that. Computing charts over the DEX is a basic use of the network, and currently there is no reasonable way for users to do it. This is purely a courtesy commit for people who would rather not trust third party nodes. We will continue to provide this API on our own servers regardless of whether the PR is accepted. |
|
@RichardAH I am just trying to understand the functionality better, as to whether this could be implemented in a second layer. Not saying it should, not trying to block the PR. Just trying to understand 🙂 |
|
@RichardAH we would happily accept this functionality in Clio as well. |
I never suggested that it was.
No argument from me there.
I don't think anyone was "picking" on this RPC call. I don't want to speak for CJ, but I suspect his question was more aimed at understanding whether this is an API that
It's not that "Ripple apparently now dislikes RPC"; it's simply that our own experience (and others, too, including people who operate servers on xrplcluster.com and whom you know well) that RPC calls against Would I prefer it if all RPC API calls were eliminated from Of course, I don't have a magic wand and can't just eliminate API calls, even if a better alternative to them existed, because (a) I have no ability to control what software people run; and (b) I value backwards compatibility.
I doubt that Ripple will ever run "custom" code which removes "baseline" functionality that has been merged into mainline. Frankly, I doubt it ever would run "custom" code with the possible exception of code used for debugging, load-testing and/or to test non-breaking changes that we are looking to propose.
Nobody suggested this PR wouldn't be accepted; to my knowledge, no PR against this repo has been rejected on a whim or because someone at Ripple doesn't like it. And to start now, with this PR seems dumb and shortsighted: first, as you know, the repo will soon be under the XRPLF's control, and they would be able to merge this. And second, as you put it, you would provide this API on your servers regardless of whether it were merged or not. 🖖 |
|
For context this is the second time this has been PRed and pointlessly argued over, wasting everyone’s time. |
There was a problem hiding this comment.
For context this is the second time this has been PRed and pointlessly argued over, wasting everyone’s time.
I don't want to have an argument with you @RichardAH, but the PR you link, had a grand total of 7 comments; perhaps some of those were pointless, but I'd hardly call this "arguing" and you were the one that closed it.
Removing RPC is not a solution to a lack of sensible locking inside rippled, which is the ultimate cause of the issues you speak of, not RPC itself.
I don't think that entirely removing the RPC layer makes sense either, but I do believe that the best path forward for high-volume RPC service is a separate engine. For small servers, I think native functoinality within rippled is fine, even if the cost we all have to bear (compile time, maintenance, potential for bugs, etc) is ongoing.
Should there be an external workhorse RPC engine? Yes. Should small independent node runners still be able to use the existing RPC functionality? Yes. They are not mutually exclusive,
Nobody suggested otherwise.
and mostly I am annoyed that CJ jumped immediately back on the second version of this PR with the same type of attack.
Come on... nobody "attacked" this PR, Richard.
Ripple don’t get to unilaterally decide RPC is being removed from the codebase.
If you had read my previous post you'd see that I agree with you. As I wrote, although I would pesonally remove some parts of the RPC API, I can't because "[...] I don't have a magic wand and can't just eliminate API calls, even if a better alternative to them existed, because (a) I have no ability to control what software people run; and (b) I value backwards compatibility."
I left a couple of comments for you to consider and there's a few further opportunities to simplify the code, but I don't particularly care to argue further about it. The PR looks fine to me. 👍
| { | ||
| std::map< | ||
| std::string, | ||
| std::tuple< |
There was a problem hiding this comment.
Recommend using struct here to make it easier to identify which field is which.
|
|
||
| std::optional<uint32_t> offerCancel; | ||
| uint16_t tt = tx.first->getFieldU16(sfTransactionType); | ||
| switch (tt) |
There was a problem hiding this comment.
Avoid introducing an unnecessary variable:
switch(tx.first->getFieldU16(sfTransactionType))
| else | ||
| ss << p << "|" << g; | ||
|
|
||
| std::string key{ss.str()}; |
There was a problem hiding this comment.
Recommend you avoid std::stringstream and use a std::string directly.
The above are what I was referring to, not your comments @nbougalis. These were posted on #4144 and lead to the original PR being closed. This is a fairly clear position, that I do not think I have misinterpreted. Thank you for approving the PR. |
@RichardAH I do think, and am not alone, within Ripple and outside of it, that it would be better to remove as much of the API layer from rippled as possible, and move it into a separate service. Some API access needs to remain in rippled, but ideally this would be minimal. This is not going to happen over night though, and needs community support, and a migration process. Generally, the community has been supportive of this. Clio needs to be adopted and battle tested though before anything is removed from rippled. Removing as much of the API as possible would improve the performance, robustness and maintainability. Do you not agree? Also, its pretty apparent that RPC via rippled does not scale well for high traffic services, like s2 or xrplcluster. So we definitely need a more scalable RPC service. Given the existence of said service, it would be a huge burden to also maintain the RPC layer of rippled as well. This is another reason to remove as much of the RPC layer from rippled as possible. I blocked your prior PR because I didn't see a reason it needed to be in rippled as opposed to clio, or some other service. I believe every new RPC bloats the rippled code, and given the presence of clio, I will give pushback on any new RPC commands being added to rippled. I think this is good for rippled. You are welcome to make your case for why it should be added, as you have here. I'm sorry you feel attacked. It's nothing personal; I'm just trying to keep the code as healthy and robust as possible. I still would prefer this functionality be added to clio or some other service instead, as it easily could be. But, this is an open source project, and as you say, people are already using this functionality. I'm also happy that a developer outside of Ripple is contributing to rippled. So I'm ok letting this in, pending review. |
I can't say I do. Personally I'd rather just fix the locking issues in rippled and make RPC work for most work loads without unnecessary additional copying of ledger data. |
While I understand and agree with what you say (over time!)... Then what it comes down to is that until Clio is battle tested and implemented, the XRPL is at a total standstill when it comes to adding features. Like this one, aiming at providing better data for DEX use cases. This is not all rippled node bloat: this is something for every sysadmin to decide for themselves if they want to enable this or not. If the decision is to block everything (we are supportive but also know we're not there in a few weeks or months) I think this hurts the XRPL more than it solves later during the migration away from rippled RPC. |
I agree that clio needs to be more battle tested. Since this is being used currently, I'm ok with adding it. I do think this could have been done in a python script outside of rippled, via parsing transaction metadata. But, I'm ok with it going in. Just would like to see a better name for a few of the fields in the response. |
RichardAH
force-pushed
the
BookChanges191
branch
from
27846df to
56923e1
Compare
|
@cjcobb23 I've made the requested changes |
High Level Overview of Change
Add
book_changesRPC api and subscription type.Context of Change
book_changesis an API that provides candlesick information for building charts at a per-ledger level.Every XRPL DEX order-book that had at least one change in the queried ledger is iterated and the following information is provided as JSON:
{open, high, low, close, volume A, volume B}The pairs are ordered with XRP always being the first currency in the pair, or the lowest issuer-currency pair by string comparison.
E.g.
{ "result" : { "changes" : [ { "close" : "60.76767128172011", "cur_a" : "XRP_drops", "cur_b" : "rLqUC2eCPohYvJCEBJ77eCCqVL2uEiczjA/5852646F67650000000000000000000000000000", "high" : "60.76767128172011", "low" : "60.76159531559148", "open" : "60.76159531559148", "vol_a" : "520687381", "vol_b" : "8568683" }, { "close" : "0.0007781040000000079", "cur_a" : "XRP_drops", "cur_b" : "rhLr8bGvHvBgYXAHNPyXrQAcKGrQ2X5nU4/5854726976694100000000000000000000000000", "high" : "0.0007781040000000079", "low" : "0.0007781040000000079", "open" : "0.0007781040000000079", "vol_a" : "3852", "vol_b" : "4950495.0495049" }, { "close" : "0.1", "cur_a" : "rKWH2NPtXn636uodWJ3dLLM7wHmyG2qpz6/KWH", "cur_b" : "rTGoNeK6vpu28U2oE5tiqhH3x8z2jBhxv/TGO", "high" : "0.1", "low" : "0.1", "open" : "0.1", "vol_a" : "0.8", "vol_b" : "8" } ], "ledger_hash" : "33C53CF838DEE3C1F74E0BA8BEDEE28D2527E11B1C53846A22E9DBB33EC048C2", "ledger_index" : 72381103, "ledger_time" : 708708711, "status" : "success", "type" : "bookChanges" } }Type of Change
This API is being used by multiple people in production, and it doesn't make sense for it to continue to live outside of the main codebase.