Coinbase rate limits impact calls for arbitrarily large number of pages#389
Open
angelaponte wants to merge 31 commits intodanpaquin:masterfrom
Open
Coinbase rate limits impact calls for arbitrarily large number of pages#389angelaponte wants to merge 31 commits intodanpaquin:masterfrom
angelaponte wants to merge 31 commits intodanpaquin:masterfrom
Conversation
When you invoke _send_paginated_message, it will reach the Coinbase Pro API's rate limit, when the results have too many pages. This patch adds a sleep interval after each page that will work for both public and private endpoints, regardless of the number of pages. This sleep will inevitably slow down paginated calls, but that's required as the number of pages requested grows without bound, per the Coinbase Pro API's spec's rate limits : https://docs.pro.coinbase.com/#rate-limits An additional improvement would be for the private endpoints to send the new sleep_interval parameter at .2 (as opposed to the default .34).
Added optimization for authenticated calls to reduce the sleep_interval from .34 to .2 for authenticated requests, per the Coinbase API request limit spec at: https://docs.pro.coinbase.com/#rate-limits
The location of the time.sleep call in the previous commit caused a huge performance hit. I moved the sleep call to a place where it will only be invoked immediately before the next page is requested.
Owner
|
Great consideration, but maybe we ought to pass the |
Coinbase has added separate portfolio functionality within a single account. These two new functions add that functionality to CBPro.
The sleep_interval parameter is now a keyword argument
Now uses **kwargs to pass the sleep_interval parameter
mcardillo55
reviewed
Nov 22, 2020
Collaborator
mcardillo55
left a comment
mcardillo55
left a comment
There was a problem hiding this comment.
It seems like this PR went from forcing paginated requests to obey rate limits, to having it be an optional feature. Is that correct?
I'm wondering, should we make adhering to the rate limits the default mode, but give users the option to override this. I think that was what @danpaquin was alluding to. Thoughts?
| } | ||
| ] | ||
| """ | ||
| return self._send_message('get', '/profiles') |
Collaborator
There was a problem hiding this comment.
GET /profile has a parameter active so we should add some logic in case this parameter is provided (pass as keyword argument data to _send_message()
For some reason, sometimes the response is empty.
I'm not sure that the internal exceptions are correct
I want to see the result.
Except needed its indentation fixed.
Is the Coinbase API returning empty results?
This time, I'm using an empty string.
Still troubleshooting
I need output
Let's see if this takes
Looks like the kwarg isn't coming in . . .
Let's see what else crops up . . .
Am I getting warmer?
Maybe that will resolve the issue?
Trying to tighten up the code
It doesn't seem to be sleeping long enough . . .
The bug should be gone now.
Trying to get the sleep_interval to work correctly
Trying to make this work on a Mac from both Python and R
This is the bug that never dies
Not sure if this will help . . .
At least it works in Python . . . might need to change my R code
My syntax is still shaky
This is impressively annoying
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I ran into what looks like a Coinbase API rate limit issue when calling AuthenticatedClient.get_account_history, read the source, and didn't see how API rate limits were being handled in PublicClient._send_paginated_message .
Here's the Coinbase Pro API rate limits page:
https://docs.pro.coinbase.com/#rate-limits
It says:
"PUBLIC ENDPOINTS
We throttle public endpoints by IP: 3 requests per second, up to 6 requests per second in bursts.
PRIVATE ENDPOINTS
We throttle private endpoints by profile ID: 5 requests per second, up to 10 requests per second in bursts."
This patch adds a time.sleep of .2 seconds to paginated calls in the AuthenticatedClient (to meet the 5 request per second limit) and sets a default time.sleep of .34 seconds for all other paginated calls (e.g. the PublicClient).
The changes are simple and easy to read, but I only tested them for performance. I have not tested all of the paginated calls. The changes necessarily make the pulling of multiple pages of data slower.