search

notadamking / RLTrader

A cryptocurrency trading environment using deep reinforcement learning and OpenAI's gym
https://discord.gg/ZZ7BGWh
GNU General Public License v3.0
1.73k stars 540 forks source link

Fixing precision and trade limit issues #79

Closed demirelo closed 5 years ago

demirelo commented 5 years ago
demirelo commented 5 years ago

Making the code more generic to handle different currency pairs (i.e., crypto/crypto & fiat/crypto, etc.) would require some changes in TradingEnv.py that will have a cascade effect in other parts of the code.

Currently, data_provider is of type BaseDataProvider class, which doesn't use ccxt and thus we cannot obtain precision values of the base and quote currency. Similarly, minimum (and maximum) price limits cannot be incorporated.

I believe ExchangeDataProvider should become the base class or the code needs to be modified such that def __init__(self, data_provider: BaseDataProvider, .... becomes def __init__(self, data_provider: ExchangeDataProvider, ....

This would then require some further changes. @notadamking , what do you think?

notadamking commented 5 years ago

@xeroquark you're on the right track, but I think a better path would be to add the necessary interface methods to BaseDataProvider, and then implement them differently in ExchangeDataProvider and StaticDataProvider. For example, something like get_precision(), get_max_price(), get_min_price() should do the trick.

demirelo commented 5 years ago

Yeah - I was thinking of the same actually. I'll give it a try.

notadamking commented 5 years ago

Great! Let me know when it's ready to review.

demirelo commented 5 years ago

@notadamking , the JSON structure of precision and min/max trading limits in CCXT package changes from exchange to exchange - based on the type of market (crypto-crypto or fiat-crypto etc.), which renders implementing this feature quite cumbersome.

Here are two JSON examples:

Bittrex (ETH/BTC)

... 'precision': {'amount': 8, 'price': 8}, 'limits': {'amount': {'min': 0.00740642, 'max': None}, 'price': {'min': 1e-08, 'max': None}}, ...

Binance (ETH/BTC)

... 'precision': {'base': 8, 'quote': 8, 'amount': 3, 'price': 6}, 'limits': {'amount': {'min': 0.001, 'max': 100000.0}, 'price': {'min': 1e-06, 'max': 100000.0}, 'cost': {'min': 0.001, 'max': None}}, ...

Bittrex has some additional elements while it doesn't have min/max cost for a trade. In case of fiat-crypto markets and their JSON structures a similar mismatch can be also seen...

I'm leaning towards modifying the code to work with selected exchanges that have identical JSON structure.

notadamking commented 5 years ago

@xeroquark the methodology for this library so far has been to provide the best defaults we can, but allow for extension wherever possible. I.e. by default implement Coinbase/Binance's min/max limits, and allow a min_precision_key, max_precision_key, min_limits_key, max_limits_key to be passed in as kwargs.

demirelo commented 5 years ago

@notadamking, PR is ready as far as I'm concerned. Fiat<->crypto pairs should work fine for almost all exchanges as I'm using CCXT's unified API. Coinbase Pro market values are used as default.

notadamking commented 5 years ago

Most of the suggestions in this PR have been implemented in the latest version of the repo, so I am going to close this for now.