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

Add book_changes subscription type #4144

Closed
wants to merge 1 commit into from

Conversation

RichardAH
Copy link
Collaborator

@RichardAH RichardAH commented Apr 12, 2022

High Level Overview of Change

Context of Change

With Ripple's data API becoming unreliable and/or going offline, reliable DEX trading data is now in demand.

This new subscription type produces candle stick data per altered book per closed ledger, meaning anyone can be their own source for this data.

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Refactor (non-breaking change that only restructures code)
  • Tests (You added tests for code that already exists, or your new feature included in this PR)
  • Documentation Updates
  • Release

Before / After

A new stream type is added:
{"command":"subscribe","streams":["book_changes"]}

Test Plan

I tested this manually. Unit tests are possible but presently I don't have time to write them.

Since this is an RPC call and is completely opt-in, any bug is unlikely to affect anyone except those using it.

Copy link
Contributor

@cjcobb23 cjcobb23 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not at all appropriate to put in rippled at this time. All new API changes should go into clio. PR there and we can merge it fast. I'll help you with anything that needs to be done. It's under the XRPLF org now.

@cjcobb23
Copy link
Contributor

Just to be clear, the entire rippled API layer is moving out of rippled and into Clio. This is for the health of the network, rippled and the XRPL ecosystem. Adding any new things to the API in rippled will delay that transition, and thus hurt the network, rippled and the XRPL ecosystem.

@RichardAH
Copy link
Collaborator Author

RichardAH commented Apr 13, 2022

Just to be clear, the entire rippled API layer is moving out of rippled and into Clio. This is for the health of the network, rippled and the XRPL ecosystem. Adding any new things to the API in rippled will delay that transition, and thus hurt the network, rippled and the XRPL ecosystem.

Hi CJ

I think offering admins Clio as a work horse for heavy query loads is indeed advisable. However this is a lightweight aggregator which only runs when ledgers are closed. I don’t think restricting access to it by requiring admins to install and maintain 3 processes instead of one helps them or the XRPL.

@wojake
Copy link
Collaborator

wojake commented Apr 13, 2022

Just to be clear, the entire rippled API layer is moving out of rippled and into Clio. This is for the health of the network, rippled and the XRPL ecosystem. Adding any new things to the API in rippled will delay that transition, and thus hurt the network, rippled and the XRPL ecosystem.

How does moving the API layer from rippled to clio improve the XRPL & rippled? Interested to learn more.

@cjcobb23
Copy link
Contributor

Just to be clear, the entire rippled API layer is moving out of rippled and into Clio. This is for the health of the network, rippled and the XRPL ecosystem. Adding any new things to the API in rippled will delay that transition, and thus hurt the network, rippled and the XRPL ecosystem.

Hi CJ

I think offering admins Clio as a work horse for heavy query loads is indeed advisable. However this is a lightweight aggregator which only runs when ledgers are closed. I don’t think restricting access to it by requiring admins to install and maintain 3 processes instead of one helps them or the XRPL.

Hi Richard,
We are removing the entire RPC layer from rippled, and replacing it with a minimal binary API, with all client requests flowing through clio. We are also working to make the process of running both processes as seamless as possible. When exactly this all goes to prod is up in the air though; so my question to you is, how desperately do you need this functionality (that you've coded this PR) right now?

@RichardAH
Copy link
Collaborator Author

Hi Richard, We are removing the entire RPC layer from rippled, and replacing it with a minimal binary API, with all client requests flowing through clio. We are also working to make the process of running both processes as seamless as possible. When exactly this all goes to prod is up in the air though; so my question to you is, how desperately do you need this functionality (that you've coded this PR) right now?

I think removing the direct API needs to be proposed as an XLS for discussion rather than be “just done”. There are a lot of stake holders this will significantly affect.

We are already running our own builds of the above code. I PR’d it for the benefit of others who need dex candle stick data in the absence of ripple data API. How urgently do they need it? Well there is no other way presently to get proper candle stick data from the XRPL DEX. So I imagine fairly urgently.

@cjcobb23
Copy link
Contributor

Hi Richard, We are removing the entire RPC layer from rippled, and replacing it with a minimal binary API, with all client requests flowing through clio. We are also working to make the process of running both processes as seamless as possible. When exactly this all goes to prod is up in the air though; so my question to you is, how desperately do you need this functionality (that you've coded this PR) right now?

I think removing the direct API needs to be proposed as an XLS for discussion rather than be “just done”. There are a lot of stake holders this will significantly affect.

We are already running our own builds of the above code. I PR’d it for the benefit of others who need dex candle stick data in the absence of ripple data API. How urgently do they need it? Well there is no other way presently to get proper candle stick data from the XRPL DEX. So I imagine fairly urgently.

Yeah we can propose an XLS. Would be happy to discuss. It's also not just being done. This has been decently out in the open since I gave the clio presentation, and presented all this to the community at large. Maybe I wasn't clear enough though.

Can xrpintel.com do this? If not I'm ok adding this in (contingent on review obviously); just means more stuff we have to copy to clio, but that's not so bad.

@RichardAH RichardAH closed this Apr 22, 2022
@RichardAH RichardAH mentioned this pull request Jun 19, 2022
7 tasks
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.

4 participants