Ticker missing transactionCurrency

[jheusser]

I just noticed now that the core Ticker only provides the tradableIdentifier and is lacking the transactionCurrency. This is inconsistent with Trade and Order which have the whole currency pair information. This is an issue if you have multiple different Ticker events in your system with different base currencies, so you can't assign a Ticker unambiguously to a currency pair. Imo this is a big issue.

One possible solution would be providing an additional constructor in TickerBuilder such that individual exchange implementation could add that information, without having to touch every single exchange implementation right now. What do you guys think?

[timmolter]

The currency is of course built into the BgMoney fields of Ticker, but you point out an inconsistency between Ticker and Trade / Order. So what's better? To have the transactionCurrency in all of them or none of them?

[jheusser]

Sorry that's an oversight, of course it's in BigMoney! Personally I'm not a fan of BigMoney as it only binds one currency to a price, which on its own is not too useful, e.g. Ticker still needs to have tradableIdentifier to make it a meaningful currency pair, yet we have 5 BigMoney instances in Ticker all with the same base currency.

What speaks against using CurrencyPair as type inside market data POJOs and all price fields are by definition denominated in the base currency? Also avoids confusing the two string arguments for tradableIdentifier and transactionCurrency.

Anyway, thanks for pointing this out.

[timmolter]

I'm not really a big fan of BigMoney either. I prefer your idea of having a CurrencyPair to represent the tradableIdentifier and transactionCurrency. Originally, we designed the library that way because we envisioned making modules for traditional financial services where tradeableIdentifier could be somthing like GOOG or AAPL. I think now, I'd rather keep XChange focused on crypto currencies.

Also, if we get rid of BigMoney, we will be able to avoid the 3-char constraint that the BigMoney lib enforces.

If we do this refactor, now would be the time to do it before the 2.0.0 release. Many methods would have to be refactored though, representing a lot of work.

[gary-rowe]

@timmolter BigMoney has its place when working with fiat currencies but really isn't suitable for cryptocurrencies. Now that XChange has firmly established itself in the cryptocurrency world perhaps the time has come to represent cryptocurrencies with BigInteger (satoshis) and provide a collection of standard utilities within MoneyUtils for converting from BigDecimal representations as provided by the exchanges.

I've written a few for MultiBit HD that I can push into XChange if you like.

[jheusser]

@timmolter Agreed, if we do this rather big change then better before a 2.0.0 release as it changes the API etc. It's not a difficult refactor but a lot of work. I'm happy to help, but it probably requires more people to touch every exchange.

[timmolter]

OK, let's do the refactor. @jheusser Can we coordinate a time to split the work and get it done? I could update the interfaces, push, and we could both tackle the refactoring.

@gary-rowe I see the refactor to BigInteger as a separate refactor, and I'd like to do the BigMoney to BigDecimal + CurrencyPair first. Thanks for chiming in, and I'll get back to you on the matter.

[jheusser]

@timmolter OK, I have some time tonight and tomorrow night for the refactor. Ok, so you would update core first? Perhaps would be good to work on this on a new branch and merge it back into develop when it's all ready.

[timmolter]

@jheusser sorry I was MIA for the whole day. I had another project to tackle. I'll create another branch and update core first.

[jheusser]

OK. Will have a look now. So e.g. Wallet will just have a string for the currency? Probably makes sense.

[timmolter]

yes.

FYI, I just pushed a new branch and made some commits already. I was thinking I could start from the top (alphabetically) and you from the bottom.

[jheusser]

Thanks. I'll start at the bottom at xchange-vircurex

[timmolter]

Excellent! I've notice that this refactor cleans the code up a lot too.

[jheusser]

How do you handle that: MoneyUtils.parse(vircurexOpenOrder.getCounterCurrency() + " " + vircurexOpenOrder.getUnitPrice()) pattern now. Does it just reduce to getting the unitPrice?

[timmolter]

correct.

[timmolter]

I added you as a Collaborator. Feel free to push directly...

[jheusser]

Do you also change the names of tradableIdentifier and transactionCurrency names in the Rescu interfaces of the individual exchanges?

[jheusser]

Thanks just wanted to ask that. Just pushed first, will reference #337 in next commit

[timmolter]

That's a good question. No. I think the *Raw classes should still take strings and NOT CurrencyPair. Same goes for the ResCU interfaces.

Only the XChange Services classes should take CurrencyPair args. Then pass either currencyPair.baseCurrency or currencyPair.counterCurrency or both down to the *Raw classes as needed.

[timmolter]

As far as the names, what do you think would be better? We can always do a global search and replace for those later. Same for currencyPair.baseCurrency and currencyPair.counterCurrency.

[gary-rowe]

Perhaps consider a rename to currencyPair.base and currencyPair.counter since it's obvious that they are currencies?

[jheusser]

So to avoid any doubt BTC/USD is baseCurrency/counterCurrency, right? :)

[jheusser]

However, in our old terminology, transactionCurrency is confusingly "USD" in BTC/USD. So the translation is now: tradableIdentifier -> baseCurrency transactionCurrency -> counterCurrency

correct?

[jheusser]

@gary-rowe good point. But yeah we can do a search/replace afterwards

[timmolter]

@jheusser correct. correct. We'll have a better name in the end. This will all clear up a lot of confusion for sure.

[timmolter]

BTW, I got rid of Wallet.createInstance too for consistency, since no other DTO provided that.

[jheusser]

What do we do with mtgox?

[timmolter]

i already removed it. :)

[jheusser]

Great. This refactor shows how confusing the two strings actually were. Often order of arguments is flipped even within the same exchange implementation.

[timmolter]

yup, very BAD! This is a welcome change.

[jheusser]

Doing justcoin and cryptotrade now.

[jheusser]

@timmolter JustCoin requires a currency for withdrawFunds which the interface does not provide. hrm...

[timmolter]

We'll need to add a String... args to the interface method.

[jpe42]

why not just change it to withdrawFunds(String currency, BigDecimal amount, String address)

[jheusser]

Yeah I would prefer that actually the varargs are not very nice..

[timmolter]

Ah yes, of course. It was BigMoney before. I'll change it and push ASAP.

[timmolter]

pushed

[jheusser]

Thanks!

[jheusser]

Very glad that there are tests for most of the exchanges at the moment :)

[jheusser]

where are we at? I see you touched coinbase, which is the exchange I would fix next?

[jheusser]

also examples needs adjustments

[timmolter]

I'm on BTC-E.

[jheusser]

Ok, I start coinbase

[timmolter]

yeah, without unit tests, this would be a disaster. I wish there was 100% coverage, but that's a lot to ask for.

[jheusser]

@jamespedwards42 unless you would like to chip in, I see you wrote coinbase and it seems quite elaborate :)

[jheusser]

Cex has no unit tests

[jheusser]

working on Cavirtex now

[jpe42]

I can sit down and do Coinbase later tonight ... Is it okay if I leave BigMoney in the raw classes? Redoing the deserialization would take some time, plus I prefer the safety that the BigMoney class provides.

[jheusser]

@jamespedwards42 that's great thanks! For the deserialisation it looks like CoinbaseMoney is actually kind of a wrapper for BigMoney. It would be best if you would just store the currency and amount which you deserialise anyway directly in CoinbaseMoney. Like that you don't need the BigMoney dependency (which I think @timmolter removed)

[timmolter]

@jheusser I'm attempting to keep an updated list of exchanges implemented and what's missing, such as unit tests.

https://github.com/timmolter/XChange/wiki/Exchange-Support

[timmolter]

@jheusser I'm finishing up campbx right now