knowm / XChange

XChange is a Java library providing a streamlined API for interacting with 60+ Bitcoin and Altcoin exchanges providing a consistent interface for trading and accessing market data.
http://knowm.org/open-source/xchange/
MIT License
3.85k stars 1.94k forks source link

The BitcoinCash BCC / BCH problem #2394

Open npomfret opened 6 years ago

npomfret commented 6 years ago

Does anyone have an elegant solution to the problem of different exchanges using different symbols for certain currencies (BitcoinCash being the obvious example)?

As the conversion isn't handled by xChange is the best (and only?) approach, to convert the symbol in the client code on it's way into, and out of, the exchange api?

ducttapedev commented 6 years ago

I have a not-so-elegant solution where I convert every Currency to a SafeCurrency (also CurrencyPair to SafeCurrencyPair), a wrapper class that I defined. The conversion looks something like this: SafeCurrency.new(Currency.BCC, binance). SafeCurrency then stores an identifier, which is the full name of the currency (BitcoinCash in this case). I have some brute force logic for every exchange I am using to convert a symbol like "BCH" along with its exchange into an identifier.

This is a many to many relationship (e.g. BCC can be BitcoinCash or BitConnect, and BCH can also be BitcoinCash). Before back converting from SafeCurrency to Currency, I also generate a mapping for each exchange, from Currency to SafeCurrency. That way, I can safely convert back from SafeCurrency to Currency with something like safeCurrency.unsafe(exchange).

Another proposal that may be more elegant is to use CoinMarketCap, which does a pretty good job of showing which exchanges use what symbols, and what currency that symbol refers to. Not sure if this can be obtained via their API. While their coverage is quite impressive, it does still miss a few cases.

On a related note, I've noticed a number of other inaccuracies as well. For instance, Binance has a currency pair VIBE/BTC, but this gets incorrectly parsed as VIB/EBTC. I'd be happy to see if any of my wrapping code I've written to fix these issues can be integrated directly into XChange, though like I have said, it is not very elegant.

npomfret commented 6 years ago

Hmm, thanks for the reply. Food for thought.

I can't help but think the currency constants should represent the currency rather than the symbol. So in the client java code you use something like BITCOIN_CASH instead of BCH, or BITCOIN instead of BTC. Then each exchange converts that into their own symbol. Most would use a standard mapping for almost every currency because almost all exchanges use the same names for almost all currencies. But, when an exchange uses BCC for bitcoin cash, or XBT for bitcoin it would convert in it's own non-standard way, both on the way into the http call, and on the way out.

ducttapedev commented 6 years ago

I agree that Currency should be represented with the full name rather than the symbol. That would avoid any ambiguities with symbols and make things a lot cleaner for those of us working with multiple exchanges. I've yet to come across two currencies with the same name, so it should be sufficient. Below are the mappings that I've found, written as an extension to XChange's Currency class (warning: Kotlin code). Note that this only covers {BinanceExchange, BittrexExchange, CryptopiaExchange, GDAXExchange, LiquiExchange, PoloniexExchange, KucoinExchange, KrakenExchange, GeminiExchange} and may not be exhaustive. The commented out code was a safer version that erred on the side of making currencies different rather than the same. SafeCurrency.knownCurrencies is instantiated at the beginning of the program with val knownCurrencies = Currency.getAvailableCurrencies(), which is not ideal because it could "fail silently" for currencies in Currency.getAvailableCurrencies() that are ambiguous.

/**
 * XChange's Currency class is insufficient because it considers currencies with the same symbol to be the same,
 * even though this is not the case. For instance, NET is NetCoin on Cryptopia but Nimiq on Liqui.
 * The opposite can also happen - BCH is BitcoinCash on Bitfinex but BCC is also BitcoinCash on Binance.
 * This function generates a unique identifier for each currency, typically the full name of the currency itself.
 */
fun Currency.getCurrencyIdentifier(exchange: Exchange): String {
    if(this.symbol == "BAT") {
        if(exchange is CryptopiaExchange) {
            return "BatCoin"
        }
        return "Basic Attention Token"
    }
    if(this.symbol == "BLZ") {
        if(exchange is BinanceExchange) {
            return "Bluzelle"
        }
        if(exchange is CryptopiaExchange) {
            return "BlazeCoin"
        }
        throw IllegalArgumentException("${symbol} not defined for exchange " + exchange.name())
    }
    if(this.symbol == "BTM") {
        if(exchange is PoloniexExchange) {
            return "Bitmark"
        }
        return "Bytom"
    }
    // typo on Binance's website...they're not the best with spelling and grammar
    if(this.symbol == "BXQ") {
        if(exchange is BinanceExchange) {
            return "BQX"
        }
        throw IllegalArgumentException("${symbol} not defined for exchange " + exchange.name())
    }
    if(this.symbol == "CMT") {
        if(exchange is CryptopiaExchange) {
            return "Comet"
        }
        return "CyberMiles"
    }
    if(this.symbol == "NET") {
        if(exchange is CryptopiaExchange) {
            return "NetCoin"
        }
        if(exchange is LiquiExchange) {
            return "Nimiq"
        }
        throw IllegalArgumentException("${symbol} not defined for exchange " + exchange.name())
    }
    if(this.symbol == "BTG") {
        if(exchange is CryptopiaExchange) {
            return "Bitgem"
        }
        if(
            exchange is BittrexExchange ||
            exchange is BinanceExchange ||
            exchange is BitfinexExchange ||
            exchange is KucoinExchange
        ) {
            return "Bitcoin Gold"
        }
        throw IllegalArgumentException("${symbol} not defined for exchange " + exchange.name())
    }
    if(this.symbol == "FUEL") {
        if(exchange is CryptopiaExchange) {
            return "FuelCoin"
        }
        if(exchange is BinanceExchange) {
            return "Etherparty"
        }
        throw IllegalArgumentException("${symbol} not defined for exchange " + exchange.name())
    }
    if(this.symbol == "BCC") {
        if(exchange is BinanceExchange || exchange is BittrexExchange || exchange is LiquiExchange) {
            return "BitcoinCash"
        } else {
            return "BitConnect"
        }
    }
    // Not really necessary since BCH already maps to Bitcoin Cash in XChange's Currency, but here to be safe
    if(this.symbol == "BCH") {
        if(exchange is BitstampExchange
            || exchange is GDAXExchange
            || exchange is CryptopiaExchange
            || exchange is KrakenExchange
            || exchange is QuadrigaCxExchange
            || exchange is BitfinexExchange
            || exchange is PoloniexExchange
            || exchange is KucoinExchange
        ) {
            return "BitcoinCash"
        }
        throw IllegalArgumentException("${symbol} not defined for exchange " + exchange.name())
    }

    if(this.symbol == "XBT") {
        return "Bitcoin"
    }
    if(this.symbol == "XLT") {
        return "Litecoin"
    }
    if(this.symbol == "XDC") {
        return "Dogecoin"
    }

    if(SafeCurrency.knownCurrencies.contains(this)) {
        return this.displayName
    }
//    println("Currency is not yet disambiguated (using default identifier): " + this)
//    return symbol + "-" + exchange.name()
    return symbol
}

By the way, it looks like CoinMarketCap's API will not help with this issue. The data is on their website, though. For instance: BitcoinCash disambiguation BitConnect disambiguation

ducttapedev commented 6 years ago

Perhaps the metadata JSON for each exchange can define a mapping between symbols and currency names?

archenroot commented 6 years ago

Just received email 38 minutes ago from dsx.uk:

Dear Ladislav,

This is an important message to let you know that we have changed the code for Bitcoin Cash from BCC to BCH. This also includes all symbol pairs. For example, BCCUSD is now BCHUSD.

We advise all API customers to check their trading solutions to ensure that this change does not disrupt their services.

If you have any questions, please don't hesitate to get in contact with us.

Best regards

Mike Rymanov CEO, DSX Regards, Mike Rymanov Chief Executive Officer https://dsx.uk

npomfret commented 6 years ago

A json file for per exchange mapping is a good idea. It would only need to hold any non-standard ones if we had a standard mapping file as well.

It's a very big breaking change though. All the signatures on the AccountService and TradeService and MarketDataService that take Currency would need to change. CurrencyPair would need to take the new object (which I guess should be called 'Currencyand rename the existing class toSymbol` or something if it's still needed). Ugh, it's a lot of work.

@timmolter do you have any thoughts?

walec51 commented 6 years ago

in my project I use a class like this + decorators on all XChanges Service and Exchange class which apply this for every method that receives or returns Currency objects

public class CurrencyOverrider {

    private final BiMap<String, String> currencyOverrides;

    public CurrencyOverrider(List<CurrencyOverride> currencyOverrides) {
        this.currencyOverrides = HashBiMap.create();
        currencyOverrides.forEach(
            override -> this.currencyOverrides.put(
                override.getCurrencyCode(),
                override.getOverrideCurrencyCode()
            )
        );
    }

    public Currency fromNormalToOverriden(Currency currency) {
        return new Currency(
            fromNormalToOverriden(currency.getCurrencyCode())
        );
    }

    public Currency fromOverridenToNormal(Currency currency) {
        return new Currency(
            fromOverridenToNormal(currency.getCurrencyCode())
        );
    }

    public String fromNormalToOverriden(String symbol) {
        return currencyOverrides.getOrDefault(symbol, symbol);
    }

    public String fromOverridenToNormal(String symbol) {
        return currencyOverrides.inverse().getOrDefault(symbol, symbol);
    }

    public CurrencyPair fromNormalToOverriden(CurrencyPair pair) {
        return new CurrencyPair(
            fromNormalToOverriden(pair.base),
            fromNormalToOverriden(pair.counter)
        );
    }

    public CurrencyPair fromOverridenToNormal(CurrencyPair pair) {
        return new CurrencyPair(
            fromOverridenToNormal(pair.base),
            fromOverridenToNormal(pair.counter)
        );
    }
}

I agree that a library level solution would be preferred but we should be allowed to turn it on/off. Some applications might prefer to pass currency codes that are actually used on a given exchange instead of the codes standardized by the library.

jnorthrup commented 6 years ago

@SunFrost @npomfret @michaelscheung @timmolter does this commit accurately demonstrate the problem in the unit test and a desirable first pass at moving past that problem in the same unit test?

https://github.com/knowm/XChange/pull/2534/commits/516c4ffc506e3eb7a922892578ecc00ff87b89df

jnorthrup commented 6 years ago

is the desired outcome:

+    assertEquals(avoidXBT,Currency.BTC );
+    assertNotEquals(Currency.XBT, avoidXBT);
walec51 commented 6 years ago

your example only shows an anonymous class that used the "BTC" code instead of "XBT" - by no means does this solve the problem, its like having and adapter that received "XBT" from the exchange API but changed it to "BTC"

the reason why this issue is hard are the following facts:

jnorthrup commented 6 years ago

@walec51 im not in disagreement with anything you've stated.

its like having and adapter that received "XBT" from the exchange API but changed it to "BTC"

that's the existing unit test and static behavior already. not in my PR, no.

I would take advantage of interfaces at the exchange level to pick and choose or even annotate into existence thru DI (and I almost never resort to DI) my favorite currency factory if the default was insufficient.

we're approaching 80 exchange modules now. it's hard to kludge a perfect solution together in this scope.

my own personal codebase already suffers from this issue cosmetically, and for this reason I use a hierarchical persistence id for reporting symbols based on CMC lookup, but this solution does not centralize on xchange-core it just synthesizes a hierarchy for the rest responses.

there are 70000 eth tokens so far.

last i bothered to look i think CMC had less than 2000 symbols, and there's new dead one's all the time which have ceased updates.

eth seems to have it's own namespace format for address synonms like a DNS system. presumably later generation dapp systems will provide this. maybe a hierarchical convention over IPFS or DHT is workable to cross reference markets, but if we are discussing XChange, I would not propose that as a default class behavior. I would say that the end user application will already perform the mapping woork it desires with the symbols given, and stay out of the way.