ccxt / ccxt

A JavaScript / TypeScript / Python / C# / PHP cryptocurrency trading API with support for more than 100 bitcoin/altcoin exchanges
https://docs.ccxt.com
MIT License
32.54k stars 7.47k forks source link

kucoin - Implement getting withdraw fees on #4938

Closed MarcelBeining closed 1 month ago

MarcelBeining commented 5 years ago

There is an endpoint to get the withdrawal fee of currencies on kucoin which can be accesses with ccxt via privateGetWithdrawalsQuotas(params={'currency':'SOMECOIN'}) or via fetchFundingFee('SOMECOIN'). So far it seems this is not implemented in the loadFees structure, because even after executing loadFees, the dictionary at kucoinObj.fees['funding']['withdraw'] is empty (compared e.g. to binance where all the fees can be seen in there). Would be cool if the withdrawal fee information is automatically provided for kucoin =)

kroitor commented 5 years ago

Will add this and #4880 asap, hopefully, this week.

frosty00 commented 5 years ago

loadFees only checks for fetchFundingFees. Unfortunately it is pretty difficult to implement this unless we use asyncio.gather. We could write some code that batches requests to attempt to solve this. We also have some fetchFundingFees that should be fetchFundingFee. WIll get back to you...

frosty00 commented 5 years ago

https://github.com/ccxt/ccxt/pull/4691#discussion_r257815563

kroitor commented 5 years ago

In general, I consider propagating the fees to the .fees property – a design flaw.

We probably should not support that codepath and should rather drop it. I believe, the .fee should be a property of a market or a property of a currency, but not a property of an entire exchange.

If the lib encourages the user to work with fees accurately (not in a bunch, but per-market and per-currency specifically), then the design of the lib gets much more straightforward.

So, if we drop the propagation logic, then the logical conflict in the load* chain gets self-resolved, because the user won't have to load all fees, he will just load the fees for the market of interest (or a number of them) – and that is how it should be.

Fees should only be hardcoded on the exchange instance if there's no other way of fetching them using the exchange API. In that case the direction of propagation should be from exchange.fees → exchange.markets and exchange.fees → exchange.currencies, but not the other way around.

I think there should be just one source of truth for fees – that is the info inside markets and currencies. If we keep that info duplicated in two separate locations (in the .fees property of the exchange and in markets/currencies of the same exchange), then fees become ambiguous for the user – hard to know which of them are actual, which other are hardcoded, so, if we duplicate them, it becomes harder to manage them consistently and harder to understand them.

It all started when someone added a few hardcoded withdrawal fees to one exchange... From there people continued populating those fees all around... I think we should indicate proper ways of working with fees via markets and currencies in the Manual, because the lack of documentation is the root cause of the confusion.

Thus, if we don't propagate to exchange.fees and only propagate from exchange.fees, then the implementation boils down to a simple call to fetchTradingFee (symbol) - works equally well for markets with hardcoded fees and for markets with fees fetched from a remote endpoint. Same story with funding fees – fetchFundingFee (currency). And we'll keep the optional methods fetchTradingFees / loadTradingFees / fetchFundingFees / loadFundingFees for hardcoded exchanges and the exchanges that allow fetching them all at once, or don't differentiate the fees per market or per currency...

Let me know your thoughts.

frosty00 commented 5 years ago

We probably should not support that codepath and should rather drop it. I believe, the .fee should be a property of a market or a property of a currency, but not a property of an entire exchange.

I think the issue resides in there being two separate endpoints to fetch the currencies and the fees. So if you want to store fees inside the .currency property then you need to make two calls to in fetchCurrencies, or load them from some cache.

The issue is that we have bidirectional propagation right now, from .markets -> .fees['trading'] and .currencies -> .fees['funding'] and .fees['trading'] -> .markets and .fees['funding'] -> .currencies. We do need a single source of truth. I think it pretty much boils down to not storing hardcoded fees in the .fees attribute, and naming them something else. If we had two separate properties for loadedFees from the exchange and hardcoded fees we could do a this.extend (this.fees['funding'], this.hardcoded_fees, this.loaded_fees) and get the best of both worlds.

ccxt fees

We could make the red line execute in loadTradingFees / loadFundingFees, or we could delete the red line altogether which would make the graph unidirectional and is pretty much what you are trying to say:

Thus, if we don't propagate to exchange.fees and only propagate from exchange.fees

frosty00 commented 5 years ago

4948