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

getOrderbook - return raw order data #886

Merged
merged 2 commits into from
May 8, 2018
Merged

Conversation

intelliot
Copy link
Collaborator

Include the full BookOffer data under data

Fix #799

@intelliot intelliot requested a review from FredKSchott April 24, 2018 23:26
Copy link
Contributor

@FredKSchott FredKSchott left a comment

Choose a reason for hiding this comment

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

Code looks good, but isn't this a better use case for:

await api._requestAll('book_offers', { ... });

Or is the idea that a public consumer wants this sooner rather than later? Maybe in addition to merging this PR, I could spend some of my sprint this week finalizing request()/requestAll() so that we can get closer to making them public.

@intelliot what do you think?

@sublimator
Copy link
Contributor

@FredKSchott

spend some of my sprint
Is that some of the fabled 20% time at Google?

@intelliot
Copy link
Collaborator Author

@FredKSchott I don't think we should transparently make multiple requests behind-the-scenes. Pagination exists to prevent overloading the rippled server. If we wanted to request all at once, we'd make a change in rippled. However, rippled already supports this: developers already get all of the results when they're on an 'admin' connection.

Yes, we should finalize request(). Here's one proposal for doing it: #887

@FredKSchott
Copy link
Contributor

@intelliot +1, I like the approach in #887 even better. I'll try to take a look this week

Copy link
Collaborator

@wilsonianb wilsonianb left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@intelliot intelliot merged commit f28ec27 into develop May 8, 2018
@intelliot intelliot deleted the getOrderbook-raw-data branch May 8, 2018 22:06
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