search

danpaquin / coinbasepro-python

The unofficial Python client for the Coinbase Pro API
MIT License
1.82k stars 740 forks source link

revert request. please take out the #389 rate limits fix #394

Open noah222 opened 4 years ago

noah222 commented 4 years ago

It is the end users job to measure and limit their requests, not a task that a simple api should be trying to do.

Please do not ever include code with sleep() in it. Due to outer code, it may have already been .2 seconds. For time critical code, including sleep messages will not be wise long term. Although a bit more work, coding an actual measure of requests, or allowing the end user of this code to perform these limits would be better. From a design standpoint, the code should be as open ended as possible. Imposing sleep commands inside of this code is a handycap for long term complex implementations that already use multi-threading. It essentially breaks any time critical code that has already been developed. There is no reason to use sleep in coinbase api code ever!!!! Sorry to be a drag, but I do not think this is a good thing to do for many technical reasons, including time sensitive applications that are already developed and working well.

angelaponte commented 4 years ago

This change has not been accepted into the master branch, but consider the case below where you need to pull the full trade history: import pandas import time import cbpro

set your coinbaseKey, coinbaseSecret, coinbasePassphrase, and accountId

auth_client = cbpro.AuthenticatedClient(coinbaseKey, coinbaseSecret, coinbasePassphrase) time.sleep(.34) history_gen = auth_client.get_account_history(account_id=accountId) time.sleep(.2) accountHistory = pandas.DataFrame(history_gen)

Using the generator will try to pull all of the pages, but no matter how much sleep the end user puts in place, the paginated calls made by the history_gen behind the scenes will exceed the request limit.

An end user won't need the added sleep when the number of pages they pull is small, but any real world implementation will run into the rate limit issue when many pages of history need to be pulled.

get_fills has the same issue.

mcardillo55 commented 3 years ago

@noah222 Would you find it acceptable if the library defaulted to obeying rate limits, with the option to disable? The major problem is in cases like @angelaponte pointed out. I believe this is even seen on the issue tracker in #403. While we certainly don't want to hold back users, calls like get_orders() or get_fills() can become unusable if we do not play nicely with the enforced rate limits.