Dave-Vallance / bt-ccxt-store

Fork of Ed Bartosh's CCXT Store Work
MIT License
422 stars 185 forks source link

Please add bt-ccxt-store to pypi.org #1

Closed sandroboehme closed 5 years ago

sandroboehme commented 5 years ago

To make installation easier please add bt-ccxt-store to pypi.org. I've added an initial setup.py in this pull request to make that easier. You just might want to replace the comment with your actual email address. BTW: Your integration of ccxt will make updating backtrader so much easier. Thanks for making that public!

sandroboehme commented 5 years ago

I've added an other commit that fixes an issue with get_wallet_balance() on Binance. When called there returns the following error:

Traceback (most recent call last): File "/Users/sandro/developing/projekte/bt-ccxt-store/samples/example_binance.py", line 103, in cerebro.run() File "/Users/sandro/developing/projekte/bt-ccxt-store/env/lib/python3.7/site-packages/backtrader/cerebro.py", line 1127, in run runstrat = self.runstrategies(iterstrat) File "/Users/sandro/developing/projekte/bt-ccxt-store/env/lib/python3.7/site-packages/backtrader/cerebro.py", line 1298, in runstrategies self._runnext(runstrats) File "/Users/sandro/developing/projekte/bt-ccxt-store/env/lib/python3.7/site-packages/backtrader/cerebro.py", line 1630, in _runnext strat._next() File "/Users/sandro/developing/projekte/bt-ccxt-store/env/lib/python3.7/site-packages/backtrader/strategy.py", line 347, in _next super(Strategy, self)._next() File "/Users/sandro/developing/projekte/bt-ccxt-store/env/lib/python3.7/site-packages/backtrader/lineiterator.py", line 266, in _next self.next() File "/Users/sandro/developing/projekte/bt-ccxt-store/samples/example_binance.py", line 22, in next cash, value = self.broker.get_wallet_balance('BNB') File "/Users/sandro/developing/projekte/bt-ccxt-store/ccxtbt/ccxtbroker.py", line 142, in get_wallet_balance balance = self.store.get_wallet_balance(currency, params=params) File "/Users/sandro/developing/projekte/bt-ccxt-store/ccxtbt/ccxtstore.py", line 132, in retry_method return method(self, *args, **kwargs) File "/Users/sandro/developing/projekte/bt-ccxt-store/ccxtbt/ccxtstore.py", line 142, in get_wallet_balance balance = self.exchange.fetch_balance(params) File "/Users/sandro/developing/projekte/bt-ccxt-store/env/lib/python3.7/site-packages/ccxt/binance.py", line 410, in fetch_balance response = self.privateGetAccount(params) File "/Users/sandro/developing/projekte/bt-ccxt-store/env/lib/python3.7/site-packages/ccxt/binance.py", line 1129, in request response = self.fetch2(path, api, method, params, headers, body) File "/Users/sandro/developing/projekte/bt-ccxt-store/env/lib/python3.7/site-packages/ccxt/base/exchange.py", line 362, in fetch2 request = self.sign(path, api, method, params, headers, body) File "/Users/sandro/developing/projekte/bt-ccxt-store/env/lib/python3.7/site-packages/ccxt/binance.py", line 1060, in sign }, params)) File "/Users/sandro/developing/projekte/bt-ccxt-store/env/lib/python3.7/site-packages/ccxt/base/exchange.py", line 598, in extend result.update(arg) TypeError: 'NoneType' object is not iterable

I've also made a few smaller changes for convenience. Please be aware that I've only tested that change with Binance. If you have a Kraken account you might want to test it there as well. Thanks!

Dave-Vallance commented 5 years ago

Hi @sandroboehme

Thank you very much for the pull request and suggestions. I will try and take a look at this at some point next week.

I am generally testing on Kraken, Bitfinex and Bitmex so it is good that someone else is looking at Binance. I generally find that each exchange has something unique about it that requires a tweak in the code.

sandroboehme commented 5 years ago

Hi @Dave-Vallance,

thanks for your quick answer. That sounds like your code is actively maintained and I'm very happy about that! I've already found an other problem with Binance that I've already fixed locally. I'll send a separate PR about that next. Just a quick background: The method annotated with @Retry makes two calls to Binance. One that creates an order and an other one that fetches the result. The first one is successful but the second one needs the symbol at Binance. This is why it leads to an error which leads to retries which leads to up to 5 orders where I only wanted to create one :-).

Dave-Vallance commented 5 years ago

Hi @sandroboehme

Nice - Sounds good.

The code is maintained but a little slowly. I use it as a base for projects I work on with clients and update it as new work comes and I find a new issue.

Much appreciated that you are contributing. I need to have a little think of a good way to regression test Kraken, Bitmex and Bitfinex when we make updates.

As you are finding each exchange has a nuance or two that means we often need to tweak things. I am trying to account for by making certain options customize-able like the broker mapping option and private end point support.

sandroboehme commented 5 years ago

I think the only way to have helpful regression tests is to send the requests to the actual exchange. There we can get the feedback e.g. from Binance that it needs the symbol in the fetch request which other exchanges don't need.

But for Binance for example I don't see a reliable way to automate that. I've just created a buy order with the minimum amount of value that was way below the current market price. Then I stepped through the code with the debugger to see what's happening and control the execution of the code. As nobody would sell at the price of my order I had time to just cancel the order. This way it was ok for me but I would not be comfortable to try that the first time without a debugger.

Even though I don't think it would be a problem with the small order size I used but cancelling many orders could be treated as the "Machine Learning Limit" "You spam order creation and cancellation very quickly without executing trades." which can result in being banned for up to three days.

At Binance there is an endpoint for testing the order creation. But that doesn't return an order id that could be used to test subsequent fetch calls.

Binance does offer sub accounts where one can place a limited amount of money. But unfortunately only for corporate accounts. I might try to get a better verification status and ask the support to get sub accounts even though it would not be a corporate accounts. I could be lucky and get sub accounts anyways.

Considering these aspects in my opinion the only way to have regression tests is to create a suite of tests that matters to our use cases and execute them in a debugger if e.g. the CCXT version or the Backtrader version changes. Or maybe even before we deploy a new version of our code to production to make sure it doesn't call untested Backtrader or CCXT code.

Dave-Vallance commented 5 years ago

Thanks, good info and suggestions!

Yes, regression is tricky because every exchange is different and very few have test API's.

Currently, what I do is run a simple script (specific to each exchange) every once in a while that will just test placing stop orders, limit orders, cancel them and then finally a very small market order. The last one requires a small number/fraction of coins on the exchange. Obviously not ideal.

Bitmex does have a testnet - So that is useful.

Speaking of changes to CCXT and Backtrader, after upgrading my CCXT package from 17.x to 18.x recently, I noticed that the value returned when an order filled changed. Fortunately, the broker mapping option allowed me to update to the new value but it did leave me scratching my head for a while. :)

Dave-Vallance commented 5 years ago

Hi @sandroboehme, Just reviewing this now. Quick question, the 'testorders' parameter in the params.json sample are for this bit you mentioned:

At Binance there is an endpoint for testing the order creation. But that doesn't return an order id that could be used to test subsequent fetch calls.

Correct? I don't see it used in the example scripts you submitted. (maybe I am going blind!)

I like the idea of having exchange specific options all neatly contained in a parameters json. Broker mappings etc could eventually be moved over.

Dave-Vallance commented 5 years ago

tested on Kitkraken and Bitfinex - No issues.

Merged.

sandroboehme commented 5 years ago

Thats great, thanks for reviewing and merging!

Just reviewing this now. Quick question, the 'testorders' parameter in the params.json sample are for this bit you mentioned:

At Binance there is an endpoint for testing the order creation. But that doesn't return an order id that could be used to test subsequent fetch calls.

Correct? I don't see it used in the example scripts you submitted. (maybe I am going blind!)

You are absolutely right. It could be used later but it's not used yet. If you want, you can remove it.

sandroboehme commented 5 years ago

@Dave-Vallance did you submit bt-ccxt-store to pypi.org already? That would make it way easier to consume it.

Dave-Vallance commented 5 years ago

@sandroboehme Not yet! It is on my todo list but I am moving a little slow lately. (Busy weeks)

I still need to merge your other pull request too!