Dev

[ajeep8]

Nils, my bitex fork on branch dev tag 2.0.0_180412 add following functions:

1. add bitfinex formatter
2. pair/ticker/orderbook/wallet of CoinOne/BitHumb/CEX.IO/CoinNest/Exmo/Gate.IO
3. proxy support on api.REST.base and api.REST.rest, this need all modules in api.REST add proxy, I just add in exmo

[firepol]

Thanks @ajeep8 for the PR, if I may add a little hint for next time, I think it would be easier for the maintainer to deal with separate PRs for separate exchanges.

In general: it's good to have separate PRs for separate features.

Personally, I created a different branch for each different exchange I was modifying (e.g. one for cryptopia, one for bittrex, one for bitstamp), and pushed a different PR for each one of them. Like this it's easier to review, also one PR gets merged faster while others may have still little issues to fix based on reviews.

Maybe now it's too late, but if you see an exchange is fixed and others need more work, you may do like this to have a sense that your PRs get merged faster (it's a good feeling) ;) cheers and thanks

[ajeep8]

Thanks @firepol. I'm newbie on github projects. I will seperate new exchange, but how about these exchanges? There should no bug, just testcase need add or modify, I'm busy these days, so can not complete the testcase, must I complete the testcase before merge?

[firepol]

@ajeep8, I think @nlsdfnbch is generally very busy, my last PR is also waiting for a while. Looking forward for your changes to be merged too, so I can test cexio since I use it.

[deepbrook]

Hey there! Sorry for the massive delay in reviewing this and other PRs. I am indeed swamped with work but will do my best to pick up the pace - you have both been doing excellent work. :)

As for the test cases, I'm currently migrating to pytest and a new mock server for verifying request generation and response processing. I'll see to it that I sort through the tests and disable outdated ones, since those make little sense to run and aren't great on morale (as was pointed out by @firepol). So for now, don't worry about them.

[ajeep8]

Oh......how can I remove the last push(cdd5c07)? The version before it is well-tested, and could be merge. cdd5c07 I add PairFetchError and modify some error raise, maybe different thought with @nlsdfnbch

[deepbrook]

you should be able to use git revert and push it to your branch, no?

[ajeep8]

Thanks nlsdfnbch.

The following need to discuss: 1) user_id: for convenience, I collect all key/secret/user_id into a dict, and maybe add addr/version later, and using a dict ALLREST={'Exmo':Exmo, 'Bitfinex':Bitfinex......}, so I can call all RESTs:

k = ALLREST[mkt](key=apiKeys[mkt][0], secret=apiKeys[mkt][1], user_id=apiKeys[mkt][2], proxies=PROXIES)

if there's no user_id(each exchange different), I must check if I need to assign user_id,

I think unity them even user_id=None is convenient.

2) endpointwithversion: it's ugly. and misleading variable name, should be "full_url". But I still not clear how to use the api version. your generate_uri using self.version, so how can call v1/endpoint when class init as v2? for example bitfinex, only v1 has symbols list endpoint.

3) _get_supported_pairs_formatted: in your code, user can fetch BTC_USD(formatted pair) by translate it to the exchange format, but cannot know what pair an exchange supply(not in same format). For example, I want to monitor 2 exchanges' same pair, compare the price, no matter what pair is, just the same pair on different exchanges.

4) PairFetchError & PublicDataFetchError: the last push I had reverted. I forgot why I add them, seem need distinguish network fetch error or format error

And others reply in review

[deepbrook]

Regarding 1): You raise a very good point! However, I think it's misleading to have user_id=None in the signature - this may look like we either forgot to handle its usage, or didnt update our code correctly if we simply ignore it in init. Instead, add **kwargs to __init__(). This way it's consistent with the way we handle unneeded parameters in the standardized methods.

Regarding 2): If the only endpoint we need from a version that isn't the version in API.version, is the symbol endpoint, using requests, as I did before, is the way to go. If there are several endpoints for standardized methods, which are only available on earlier versions of the api, it's best to pass version='' and add the endpoint version in the standardized method definition:

def ticker(pair, **kwargs):
     return self.request('v1/ticker/{}'.format(pair), params=kwargs)

..as a simple example.

Regarding 3): Again, an excellent point that I hadnt considered before. Again, I'm not quite sold on the solution, though. Since our formatter classes handle the formatting from unified to exchange specific, they should also handle it the other way around.

So, how about adding a parse() method for the exchange on the PairFormatter class that takes care of parsing the pairs ? like so:

# This is the exisiting formatter for Bitfinex
@staticmethod
def bitfinex_formatter(base, quote):
    """Format currencies for bitfinex.
    Edgecase: ``DASH``
        This symbol is shortened to ``DSH``.
    """
    base = 'DSH' if base == 'DASH' else base
    quote = 'DSH' if quote == 'DASH' else quote
    return base.lower() + quote.lower()

# And this is what a parser could look like:
@staticmethod
def bitfinex_parser(pair):
    base, quote = pair[:3], pair[3:]
    if base == 'DSH':
        base = 'DASH'
    if quote == 'DSH':
        quote = 'DASH'
    return PairFormatter(base.upper(), quote.upper())

I'll have a look into your other comments today!

Thanks for your vast contribution, it is much appreciated!

[firepol]

I wonder if this PR will be ever merged. No doubts it's a good contribution, just... it's not atomic, that's why it's still here waiting to be merged.

@ajeep8 If you have the time it would really be a good idea to make a few new branches.

A branch (based on the actual "dev" branch from origin, so to see conflicts and resolve them, e.g. binance I already pushed some fixes and they have been merged) for each different exchange.

Then cherry pick the various commits, or if (maybe easier) you know that only a few files belong to each exchange, maybe copy just those files from this branch to the specific one. Like this, maybe an exchange is good to go and can be merges, the other exchange implementation has a few issues to be discussed and corrected, easier to do. The other exchange maybe also a few fixes and can be processed easily. Like this each PR can be handled separately.

It seems more work but in the end it speeds up the process.

It's a pity to see some good contributions like this one waiting for months. To speed up the process it's really as easy as keeping the PRs small & atomic.

Just my 2 cents...