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.87k stars 1.94k forks source link

Verify Order tradableSize for new orders #379

Closed jsCoin closed 9 years ago

jsCoin commented 10 years ago

Some exchanges have a restriction on how small of orders can be made. Often the size restriction is pair dependent. Throwing an exception before sending for orders that would be rejected by the exchange for being too small would be helpful. For automated trading it would be helpful to be able to get the minimum exchange/pair size when programmatically creating orders to avoid the exception condition entirely and allow early return if there aren't enough funds or available volume for a trade.

timmolter commented 10 years ago

This could be done like we call the verify method in most service classes where a CurrencyPair is an argument. You could add an additional verifyOrderAmount method that works similarly. If you want to submit a PR for that, I'll accept it.

rafalkrupinski commented 10 years ago

-1 I don't like the idea of implementing exchange-specific verify(LimitOrder) method. Exchanges should expose requirements for orders - minimum volume, price, and steps (or scale). Once that's done, verify could be implemented in a generic way, externally to exchanges or their services. Also, bots could use this information as well

timmolter commented 10 years ago

I think it's asking too much for exchanges to expose requirements like that. They never will. However the developer of an XChange module (usually) knows these requiremenst from the API docs or other experience and itd be helpful to build some checks into XChange. So plugging some verify logic into the *Raw layer would be fine, but there doesn't need to be a generic interface to support this. Additionally, if you submit something unacceptable in a request, that error should be propagated back to the client code as an exception and it should be handled.

rafalkrupinski commented 10 years ago

@timmolter Sorry, what I meant by saying that exchanges should expose this information, was actually that the clients implemented by the Xchange should provide it, either from the server or hardcoded.

I know adding a generic function across all the clients could be a daunting task, but we already have a simple and working solution to that: NotYetImplementedException; let's implement it for those we currently develop and use, and if someone misses it in his client, it'll be easy enough to add it.

Also, fees would be nice to have.

-1 to closing the issue, we're still discussing it, aren't we. :)

jheusser commented 10 years ago

I would be worried that hardcoded rules would get horribly outdated unless the exchanges provided that information. I rather have the exchange tell me directly my order failed due to wrong order size, than XChange falsely rejecting an order due to being out of date.

rafalkrupinski commented 10 years ago

I think that these rules are already hardcoded in some client implementations, only implicitly, just the way that worries you, @jheusser. What I'm proposing is not to reject orders, but expose the information required to make a proper LimitOrder - minimum trade amount and scale, and price scale. Then the user may use a generic tool to verify his orders.

@jheusser, you say that you rather get your order rejected by the exchange. What if exchange rejects it without saying why?

Besides, it's implemented in cointrader, my code, who knows where else. The duplication itself makes it a candidate for inclusion in xchange.

jheusser commented 10 years ago

@rafalkrupinski Ok, that sounds very reasonable, to have it as option for building proper LimitOrders.

That's fine, I think it's easier to figure out what's wrong (knowing that it's definitely a real error), fixing my code than fixing XChange and then depend on the latest SNAPSHOT (currently I do depend on the latest snapshot unfortunately, but I guess in the future this will stabilise a bit more)

rafalkrupinski commented 10 years ago

https://github.com/rafalkrupinski/XChange/commit/7245dbb9ff6f44632dd89e80a37c9a6f1dbf680e Comments more than welcome.

I'd want to add getFeeFactor(CurrencyPair) but I remember more than one of the exchanges implements maker-taker fee strategy. I think it only checked Limit vs Market orders, so it should be easy.

Could someone please reopen the issue?

timmolter commented 10 years ago

Comments and consideration below. I'm still not sure what's best.

  1. @rafalkrupinski 's code returns a boolean, but not information about why
  2. The requirements will likely change in time, making relying on hard-coded verify code incorrect
  3. Different exchanges may or may not provide this info in their API
  4. Creating a bot that operates over many exchanges will have it's challenges given that fact that these parameters are a moving target.
  5. Should XChange provide hardcoded values and/or verifying code? Or should it be a "dumb" communication layer between client code and exchanges?
  6. NotYetImplementedException should be used very sparingly.

My feeling is that given the complexity and fragility of verifying these order parameters, it should be left out of XChange. On the other hand, for a bot builder, the code is an absolute necessity, and I can appreciate that it is absolutely needed and XChange would be a logical "home" for that code. I'm still not sure what is best.

Here are some other options:

  1. Add methods in the *Utils class in each module
  2. Create an OrderBuilder class where inside the build method, verification code is run, which throws specific exceptions that bots can process and adjust their logic. Somehow make it generic.
rafalkrupinski commented 10 years ago

On 01.10.2014 10:03, Tim Molter wrote:

Comments and consideration below. I'm still not sure what's best.

  1. @rafalkrupinski https://github.com/rafalkrupinski 's code returns a boolean, but not information about why

A valid remark, lets throw a descriptive exception instead.

  1. The requirements will likely change in time, making relying on hard-coded verify code incorrect
  2. Different exchanges may or may not provide this info in their API
  3. Creating a bot that operates over many exchanges will have it's challenges given that fact that these parameters are a moving target.

OK, some exchanges provide the info, others don't. In either case, currently application developer or end user must track these requirements independently. Keeping it here, even hard coded, would just make the effort shared.

  1. Should XChange provide hardcoded values and/or verifying code? Or should it be a "dumb" communication layer between client code and exchanges?

For example, Hitbtc provides lot multipliers and accepts board lots (integers), quietly discarding anything after the decimal point. It would neither be hardcoding, nor would it be "dumb". We'll use the data indirectly, so it would be smart :)

Given the OrderRequirements, validation is a common code.

  1. NotYetImplementedException should be used very sparingly.

Actually my code just returns null as the default PollingMetadataService, keeping to the convention.

Here are some other options:

  1. Add methods in the *Utils class in each module
  2. Create an OrderBuilder class where inside the |build| method, verification code is run, which throws specific exceptions that bots can process and adjust their logic. Somehow make it generic.

Example process of building and maintaining a client with only the validation available:

  1. Write the code, hardcode order requirements for each exchange or make the users remember it, or else the XChange throws an exception.
  2. At some point both your hardcoded values and the validation code become outdated
  3. Someone updates the validation code in XChange
  4. You have to update hardcoded values in your code

Example process of building a client with requirements available:

  1. Write a generic code using the OrderRequirements
  2. At some point the hardcoded values become outdated
  3. Someone updates OrderRequirements
  4. Profit
sutra commented 10 years ago

This is a snippet of the Account(the Exchange account) interface of one of my projects, paste here for you information:

    /**
     * Returns the API trade fee rate.
     *
     * @return the API trade fee rate.
     */
    BigDecimal getApiTradeFeeRate();

    /**
     * Returns the minimum amount of one order that can be placed on the exchange.
     *
     * @return the minimum amount of one order.
     */
    BigDecimal getMinAmountPerOrder();

    /**
     * Returns the rounded bid amount.
     *
     * @param bidAmount the unrounded bid amount.
     * @return the rounded bid amount.
     */
    BigDecimal roundBidAmount(BigDecimal bidAmount);

    /**
     * Returns the rounded ask amount.
     *
     * @param askAmount the unrounded ask amount.
     * @return the rounded ask amount.
     */
    BigDecimal roundAskAmount(BigDecimal askAmount);

    /**
     * Returns the scale of bid/ask price that allowed.
     *
     * @return the scale of bid/ask price.
     */
    int getPriceScale();

the getMinAmountPerOrder() contains 2 information, the min amount and the min scale, for example 0.010 means the the min amount is 0.01 and it could be 0.013, but 0.009 is not allowed.

rafalkrupinski commented 10 years ago

Thanks, @sutra. Is Account the same as cointrader's Market?

The fee on ANX is different for limit and market orders, and I think one other exchange employs maker-taker fee policy. I was thinking about getLimitOrderFee(CPair) and getMarketOrderFee(CPair) On the other hand, 1 or 2 exchanges out of 17 could be treated in special way.

the getMinAmountPerOrder() contains 2 information, the min amount and the min scale

I like the idea.

sutra commented 10 years ago

@rafalkrupinski I put the fee configuration in Account, because in some exchange, different account has different fee rate.

sutra commented 10 years ago

This is really a complex thing, some exchange has a min money amount of trade(the order price * the order quantity).

jheusser commented 10 years ago

The whole exchange configuration is very tricky. I keep track of per currency pair / per exchange:

rafalkrupinski commented 10 years ago

@sutra, I haven't encountered exchanges with minimum order value, but of course I only know a few. How many of them limits the min order value?

@jheusser exchanges actually limit precision, not just scale?

sutra commented 10 years ago

@rafalkrupinski BtcTrade is 0.1. If place a 9@0.01 = 0.09(quantity@price=GrossTradeAmt), it will say "Turnover is too small, please raise your purchase quantity". If we raise to 10@0.01=0.1, it will be placed successfully.

rafalkrupinski commented 10 years ago

@sutra I understand the problem, but what's its scale? If single BTCTrade limits the volume, I wouldn't bother adjusting the common interface. Lets expose it in exchange specific way and put it in the validation.

I've previously said that validation could be a common code, but clearly it can't. It could be a common interface though.

sutra commented 10 years ago

@rafalkrupinski the min price of BTC/CNY in BtcTrade should be 0.01 and the minAmountPerOrder should be 0.010(0.001, 0.002, ... 0.009 are not allowed, but 0.011 are allowed).

List all restrictions of BTC/CNY in BtcTrade here(these restrictions are got by trying on its web page):

nimrood commented 10 years ago

I agree with @rafalkrupinski -- I believe creating an interface makes sense. The XChange project could provide a common implementation of the interface for the handful of exchanges that provide all the necessary information.

@timmolter, I feel this largely a client-side issue. I do not feel it is the XChange project's responsibility to inform the client that it is making proper order. XChange is to provide the conduit for sending a request to the exchange -- it shouldn't be error-checking your request unless it simply doesn't match the API.

Like @jheusser, I have associated all the fees/commissions, min/max price, min order amount, and price/amount scale information to the CurrencyPair. Maybe XChange could benefit from expanding the CurrencyPair or deriving a common Market class from it -- providing the information necessary to validate an order?

timmolter commented 10 years ago

OK, thanks for the discussion everybody!

After hearing what you guys have to say, it makes a lot of sense for XChange to provide this detailed information that bots need to make valid orders. Sometimes the info can be pulled from the exchange itself and other times it needs to be hardcoded. In any case, if one person puts in the effort to get it all figured out, everyone else can benefit. It also makes sense that XChange does not do any automatic validation, but only provides the necessary info to clients (bots) to correctly create and validate orders.

So now the question is: What's the best way to add this to XChange? I heard a few ideas, but what do you guys think now after hearing alternative ways?

nimrood commented 10 years ago

@timmolter: This is a tough question to answer. The exchange API's are all over the place in terms of information availability and in what manner (public v. authenticated v. some exchanges provide an API method to calculate orders/quotes).

I'm leaning toward an interface (or interfaces) which largely put the validation responsibility back on the client. For example, if an exchange doesn't provide fee info at all -- the client must implement XChange's fee interface. The fee implementation class is registered with XChange by the cilent, then XChange's order validation methods can be safely invoked.

I really do not know -- I'm still braining storming the idea. As has already been stated by several developers, this issue is a tricky one to address.

rafalkrupinski commented 10 years ago

Here is the second iteration https://github.com/rafalkrupinski/XChange/commit/29795234aa67df18045799e2c6ad9b1cc9844cfd It includes:

What's skipped (what I think should go to exchange specific API and validation impl)

to do:

rafalkrupinski commented 10 years ago

I forgot about fees

timmolter commented 10 years ago

@nimrood Could you take a look at @rafalkrupinski proposal? You're in a better position than I to judge the usefulness.

sutra commented 10 years ago

Let me make things more complex, for BitVC/HUOBI, there are:

Error Code Message
50 Buy price cannot be higher 10% than the market price
54 Selling amount cannot be lower 10% than the market price
55 Buy price cannot be higher 5% than the market price
56 Sell price cannot be lower 5% than the market price
87 Minimum amount is 0.1 , buy price cannot be higher 1% than market price
88 Minimum amount is 0.1 , sell price cannot be lower 1% than market price
89 Minimum amount is 0.1 , buy price cannot be higher 1% than market price
90 Minimum amount is 0.1 , sell price cannot be lower 1% than market price
92 Buy price cannot be higher 10% than market price
93 Sell price cannot be lower 10% than market price
120 Maximum BTC amount is 500
122 Maximum trade price is 100,000
... ...
rafalkrupinski commented 10 years ago

@sutra I don't think we should cover every possible limitation, just the most common + useful.

We can only use constant values. We can't check if a order price is within percentage limits relative to current market price, because the market price might change between calls.

nimrood commented 10 years ago

@timmolter, @rafalkrupinski -- I have a busy week this week, but I briefly reviewed the proposed changes and they look reasonable.

I do wonder want an actual implementation might look like; and for exchanges that provide no information -- will XChange be hard-coding? or will XChange push responsibility to the client? In my opinion, hard-coded within XChange does not scale, and potentially introduces a bad user-experience if a user is unfamiliar with the hard-code.

Fees are a separate, but related topic. Fees are not required to validate an order, but fees are closely related to the completion of an order, and also for calculating an order to-be-placed. Fees may warrant their own interface because exchanges are varied (no fee info via API, maker-taker fees, fees by account-type/level, etc.).

rafalkrupinski commented 10 years ago

potentially introduces a bad user-experience if a user is unfamiliar with the hard-code.

Not sure if I understand. We would hardcode the missing data to avoid duplication in the clients.

I'll extend the interface with fees and provide some example implementations

nimrood commented 10 years ago

@rafalkrupinski By bad experience, I mean this:

Most of us (I do) track the mainline development of XChange. We have our XChange sandboxes, and branches, and then we have our application's sandboxes and branches.

What if I'm simply using XChange's precompiled releases from Maven? What if that release's hard-coded values are a few months old and no longer valid? I imagine the user could experience some frustrating problems using the XChange API with out-dated hard-coded values..

rafalkrupinski commented 10 years ago

@nimrood I guess they would do the same as if the exchange had introduced an incompatible change - update the XChange. Until today (the release) for some time our users were unable to use Bitcurex and I think one other exchange, because they introduced incompatible changes and the working code was only available in the snapshot.

I think this proposal already contains a compromise - validation wouldn't be part of placing order. It would be an optional operation, that would require another call on XChange interface.

timmolter commented 10 years ago

@nimrood I think hardcoding some things makes sense when it's not provided directly by the exchange dynamically because once something breaks only one developer needs to update XChange, push it, and everyone benefits. And as was said XChange would not be validating/verifying automatically behind the scenes. It would be up to the client to individually decide whether or not to use the "metadata" code and verify/validate.

In the beginning, XChange had hardcoded verify calls to check valid currency pairs embedded in the normal REST calls, and that definitely did cause problems. We then took it all out.

nimrood commented 10 years ago

@rafalkrupinski & @timmolter -- By day, I work on a massive piece of software (1+ million LOC, and in several different languages and technologies) --- I understand hard-coding is sometimes a necessary evil. And, sometimes, it is completely appropriate.

I'm just playing Devil's Advocate here -- generally speaking, and in my experience, hard-coding often results in even more problems in the future. I'm not opposed -- I'm just trying to make sure we consider all aspects and ramifications.

glabmoris commented 10 years ago

Having fees integrated with XChange would be an awesome feature for my use cases.

Coming from a similar background, I would agree with nimrood. Hardcoding is the source of way too many evils to be considered good practice. It can be justified by the usual excuses such as lack of resources (time included), "will-fix-it-later" , "it works when using [specific case X]" ,etc. The core issue here is that there's no direct link between functionalities and pricelists/fees. My suggestion would be to decouple the code from the pricelist/fees. Maybe something like a CDN-hosted file or something similar would do the trick without resorting to the evil that is hardcoding. Maybe we can get a feature request going to discuss how to add this feature ? :)

jsCoin commented 10 years ago

I have dealt with this issue in my own API implementations by having configuration files for each exchange. If there is an exchange API to get pair information from then the configuration file is disregarded by overloading the loadConfig method with a call to the pair information API. A single config file per exchange is much easier to deal with than hardcoding these values into the implemenation. Where the data comes from can be implemented separately for each exchange with the default being either provided by users or read from config files.

rafalkrupinski commented 10 years ago

I think keeping the data in resource file instead of actual hardcoding would be a good solution. We could release the file independently of XChange

nimrood commented 10 years ago

I believe a config file approach makes sense. It avoids the hard-coding and clients maintain control over the accuracy of the information (regardless of whether they are using Stable or SNAPSHOT releases).

Achterhoeker commented 10 years ago

This is a great discussion.

This issue is also important for my own trading bot. Currently i've implemented for each exchange i use hardcoded settings in a configuration file, or i extract the needed values from the exchange raw api. I'm only interested in:

I think that a separate configuration file with hard-coded values for the exchanges that don't deliver the needed information in their api is a good approach. Now every user needs to hardcode it himself, because a real trading bot needs this information.

jsCoin commented 10 years ago

Bitfinex has a maker/taker fee policy.

On Tue, Oct 14, 2014 at 12:44 PM, Rafał Krupiński notifications@github.com wrote:

ANX just dropped trading fees. Any other exchanges with maker-taker fee policy, or should I drop it from the fee API?

— Reply to this email directly or view it on GitHub https://github.com/timmolter/XChange/issues/379#issuecomment-59113220.

jsCoin 1KRDP78m77J415g7BcPh712KAe2ZWrK5LR

rafalkrupinski commented 10 years ago

Thanks Anyway, my bad, ANX has only dropped deposit/withdraw for BTC, not actual trading fee.

Another thing is that it seems ANX applies maker fee to all limit orders and taker fee to all market orders. Bitfinex, however, implements (or at least declares to) maker-taker policy properly, which makes the fee impossible to predict.

This leaves only the ANX that has different fees for market and limit orders.

I would leave only the max (taker) fee, and maybe add a user option to return maker fee for bots that always make applicable orders.

rafalkrupinski commented 10 years ago

https://github.com/rafalkrupinski/XChange/commits/metadata

I have implemented metadata for ANX and Kraken. Comments?

rafalkrupinski commented 10 years ago

OK, @timmolter , what will it take to make it a good PR? :)

timmolter commented 10 years ago

Well, it sounds like the consensus is a config file. Makes sense to me. Having said that, what's the best way to load it? Have it on the classpath and redeploy your app? Load from disk/cloud on command? Also is a properties file enough or would a YAML file with hierarchy be needed?

rafalkrupinski commented 10 years ago

In the latest commit I'm using properties files for ANX and Kraken. You can see the example files are not that complex. The approach is that there is properties file for each exchange that needs one, and it's loaded in the constructor.

What we could do is a singleton service that reads a single config file on the first use and on demand. The config file would be in xchange-core resources, and if the user wants to download the latest version, they would have to provide a local path for the override file. This file, if exists, would be used to replace the one included in xchange-core.

We could update the file from a known location here on github, though it's rather slow to download a single file, and if we don't want to download the same version twice, we need to keep the ETag (github doesn't support if-modified-since).

I don't think it should be updated automatically at startup, because in case of problem with github, it could take up to 30 seconds (connection timeouts).

What would be advantage of YAML vs JSON, which we use everywhere already?

Also, It would be great if someone suggested what exchange to extend with MarketMetadata, or better yet, if someone would implement it, so we knew if anything is missing or is otherwise unsuitable.

timmolter commented 10 years ago

Sounds good. Are you suggesting putting all the configs in core or a single config in each appropriate exchange module?

I think each module should have a config file in 'resources'. In most cases it can be assumed that the most recent version is the appropriate default one to use. For custom cases, a custom file could be loaded from a file somewhere.

Does anyone else have comments on this?

rafalkrupinski commented 10 years ago

Now it's a file per exchange. I'm suggesting putting it all in a single file in the core module, so it's easier and quicker to update.

rafalkrupinski commented 10 years ago

In the last two commits to https://github.com/rafalkrupinski/XChange/commits/metadata I've added ConfigurationManager class that reads xchange.properties, allows user to override the file or update it from remote location, configuring it either programmatically or through API.

I also extended ANX with maximum trade amount, as an example of extenion of the BaseMarketMetadata.

I think that because the metadata like scales, minima and maxima are rather static, as opposed to trading fees, which may change based on turnover, trading fee could be extracted from MarketMetadata.

Comments?