-
-
Notifications
You must be signed in to change notification settings - Fork 7.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Unified deposit and withdrawal transaction history #1896
Comments
Not yet, this is the final part of the Unified API, we are going to address this aspect immediately after covering the funding fees (and unifying all fees in general, this work is in progress now, as you can see from pull-requests #1669, #1814, #1835). Your contributions are very welcome, as always ) I would propose to start with one or two experimental implementations for one or two major exchanges to devise unified structures for deposits and withdrawals. I am going to upload them shortly, so that all developers could carry on from there. |
@kroitor Is there a proposal on this implementation? |
@ZohaibAhmed not yet, but we think it should be pretty much similar to the unified trades api... Here it is: // returns deposits and withdrawals
fetchTransactions (currency = undefined, since = undefined, limit = undefined, params = {})
// returns deposit transactions
fetchDeposits (currency = undefined, since = undefined, limit = undefined, params = {})
// returns withdrawal transactions
fetchWithdrawals (currency = undefined, since = undefined, limit = undefined, params = {}) A transaction would look similar to a trade:
From the above it follows that there are still questions to be discussed. Your proposals and suggestions are welcome. |
@kroitor that looks fine except the fees. Not all exchanges (e.g. Bittrex) include fees in their withdrawal/deposit API. Should we omit that for now? |
I need to implement deposits & withdrawals for Binance. @ZohaibAhmed have you implemented this? I want to get started on this but don't want to duplicate any work already being done. I also want to make sure it matches the standard api. I can model the unified trades api as a starting point. |
@grandmore, I haven't begun implementing this. One catch while working with withdrawal/deposit data is that the amount might be positive or negative for withdrawals. The current trades API doesn't show negative amounts, it converts them to positive ones. Let's make sure we can add that to the spec? |
@ZohaibAhmed It's an interesting point. We either track them as +/- (credit/debit) style or flag them as deposit/withdrawal. I am going to push some code to a fork in a few hours. I have created a Binance version, while trying to stick to the spec. A lot of the exchanges do not have the finesse required, so a lot of the work will need to be done in the library. |
Just what's on this thread...
Just make sure to read this carefully: https://github.com/ccxt/ccxt/blob/master/CONTRIBUTING.md#how-to-contribute-code Thx! |
@kroitor Yes, what's in this thread, as its all there is. As well as modelling the other functions in the library. There are some clever concepts in the library. Anyway I will push to a fork so it will not impact the library directly. If it's useful you can merge it. I will push it to a fork tomorrow as I'm running late. |
@kroitor I have pushed the first draft of Binance to this fork: transactions-for-withdraw-and-deposit I have only created the withdraw and deposits following this spec
The remaining unified fetchTransactions I will do after I clear up some questions.
There is very little compatability from the exchanges I look at. |
Kraken API also has |
Bitstamp's deposits and withdrawals are combined with the trade history, so the current implementation for |
@kroitor I am writing code for fetchDeposits & fetchWithdrawals because we need them now. I would follow the specification above. May I write this code and make pull requests for some of the exchanges? Do you think you could merge this changes? |
@lfern your contributions are welcome ) |
@lfern I am in the same position. I need to implement now, hence why I stared at the weekend. I have a working prototype and right now I am comparing all of the exchanges to find the common data structure. After I finish I was going to propose the shape of the data that can be common and some approaches to some of the variations. One of the primary issues I see is that some of the exchanges do not have an id number to uniquely identify the transaction. Because of this I have been thinking about creating a hash of the data that is common, to create a key that always matches the transaction. I was going to complete this tomorrow so I will have a complete withdraw/deposit functionality for Binance. Assuming it gets accepted I was going to roll this forward and do the first half dozen exchanges. @kroitor what do you need to get this code into the main repository? Its awkward for me to write multiple branches without using them together, which means I need to merge the branches into my fork. Is there a better way? |
@grandmore basically, just make a pull request and from there we should be able to resolve it quickly... we need just one js file of the exchange ) thx ) |
@kroitor Let me get the code finished and I will do it tomorrow. It's actually 2 files as Exchange.js needs to hold the common parseTransactions() parseTransactions (transactions, side = undefined, market = undefined, since = undefined, limit = undefined) {
let result = Object.values (transactions || []).map (transaction => this.parseTransaction (transaction, side))
result = sortBy (result, 'timestamp')
let symbol = (typeof market !== 'undefined') ? market['symbol'] : undefined
return this.filterBySymbolSinceLimit (result, symbol, since, limit)
} |
@kroitor @grandmore I am writing cobinhood code for transactions. What do you think about posible values for returned status for transactions? parseTransactionStatus (status) {
if (status in ['tx_pending_two_factor_auth', 'tx_pending_email_auth', 'tx_pending_approval']) {
return 'pending_user_action';
} else if (status === 'tx_approved') {
return 'approved';
} else if (status in ['tx_processing', 'tx_pending', 'tx_sent']) {
return 'pending';
} else if (status in ['tx_timeout', 'tx_invalid', 'tx_cancelled', 'tx_rejected']) {
return 'canceled';
} else if (status in ['tx_confirmed']) {
return 'ok';
} else {
'unkown';
}
} |
@ifern Each exchange has different responses so we can either standardise them of the leave the exchanges response, or find a middle ground. I like your approach of bundling, essentially the same errors into one response type This is what I have done for Binance. parseTransactionStatus (status, side) {
if (!(side === 'deposit' || side === 'withdraw'))
throw new ExchangeError (this.id + 'deposit/withdraw side set incorrectly: ' + side);
if (side === 'deposit') {
let statuses = {
'0': 'pending',
'1': 'complete',
};
return (status in statuses) ? statuses[status] : status.toLowerCase ();
} else {
let statuses = {
'0': 'email sent',
'1': 'cancelled',
'2': 'awaiting approval',
'3': 'rejected',
'4': 'processing',
'5': 'failure',
'6': 'complete',
};
return (status in statuses) ? statuses[status] : status.toLowerCase ();
}
} |
Maybe it would be fine to differentiate 'rejected' and 'failure' from 'cancelled'. Also, I would like to differentiate 'pending' when the exchange is processing the request, and 'pending' when the exchange is waiting for additional steps from user. But I don't know if this additional state would be really useful for others.
|
I would go with as little variety of statuses as possible, namely:
To me the above codes seem aligned to what we would get from the exchanges' APIs on average... The rest of the statuses – not so sure... Still open to discussion and arguments. |
@kroitor I've create a synthetic id so that transactions can be identified even when the exchange does not return an id. I need to be able to identify transactions and this seemed to be the best way to achieve it.
The id is computed so it will always return the same id when recomputed. But I have 2 questions.
This
or this
How do I organise a pull request? |
I would prefer the id to be
In general: https://help.github.com/articles/creating-a-pull-request/ You fork the repo to your github. Then make the changes. Then there is a button to create a pull-request from those changes to the original repository. Regarding ccxt: https://github.com/ccxt/ccxt/blob/master/CONTRIBUTING.md#how-to-contribute-code Let us know if you have further questions. Thx! |
For the |
@clarkmoody our standard fee structure (you can find it among orders and trades) throughout the library is:
) |
Great! I was just wondering since the example data structure earlier in this thread didn't have all those fields. |
Hi @kroitor,
In python, |
because, you said it yourself:
) In other words, if |
P.S. Of course, the |
@kroitor question for those exchanges that don't have different methods to fetch withdrawls and fetch deposits, such as:
Is it
I see that
I propose to add a new parameter in the method, for the
E.g. on bitfinex you'd call this method without specifying the type and you'd get all deposits and withdrawals. Or call it 2 times. First with On bitstamp you'd call this method without specifying the type and you'd get all deposits and withdrawals (as for bitfinex). Since trades are not expected to be returned in this method response, they should be filtered. What do you think? I'd be happy to contribute in this area, as I'm writing an accounting software that needs to fetch exchanges histories. I can begin with bitstamp as it should be easy to determine type of transaction without hassle (see discussion in PR #3736) |
Yep )
I don't really think that we should do that. I'd suggest that we better keep filtering outside of the library (in userland). To explain it, think this way: if there are no native So, in cases when we only have // JavaScript
// native js syntax filters
transactions = await exchange.fetchTransactions ()
deposits = transactions.filter (t => t.type === 'deposit')
withdrawals = transactions.filter (t => t.type === 'withdrawal')
// alternatively, filter with ccxt
grouped = exchange.groupBy (transactions, 'type')
console.log (grouped['deposit'])
console.log (grouped['withdrawal']) # Python
# native python syntax filters
transactions = exchange.fetch_transactions ()
deposits = [t for t in transactions if t['type'] == 'deposit']
withdrawals = [t for t in transactions if t['type'] == 'withdrawal']
# alternatively, filter with ccxt
grouped = exchange.group_by(transactions, 'type')
print(grouped['deposit'])
print(grouped['withdrawal']) // PHP
// native php syntax filters
$transactions = $exchange->fetch_transactions ();
$deposits = array_filter ($transactions, function ($t) { return ($t['type'] === 'deposit'; });
$withdrawals = array_filter ($transactions, function ($t) { return ($t['type'] === 'withdrawal'; });
// alternatively, filter with ccxt
$grouped = $exchange->group_by($transactions, 'type');
echo print_r($grouped['deposit'], true) . "\n";
echo print_r($grouped['withdrawal'], true) . "\n"; So, as you can see from above, there's very little point in having the filter inside the library – imagine every library would duplicate all those filters in its source code reinventing the wheel over and over again... This isn't what we really want in terms of library design. The idea behind it is: if it's easily doable with native language syntax in a very short way or with a one-liner clause – then it does not belong to the library codebase. Hopefully, this explains why we would not add a filter for On the other hand, you could wrap Thus, instead of calling
Your help with resolving the bitstamp issue is still very welcome! Actually, any help is still welcome, so if the above does not make sense to you and you want to try unifying that aspect in a more elegant way – we'd be happy to review and merge it. Let me know if you got more questions. Thx so much! |
Thanks @kroitor for your nice answer and references to the manual, wow this library is really complex, well documented and there is always something new to learn. It makes totally sense your explanation about not filtering stuff and let that be in userland. The bitstamp case is a bit tricky then. Because in #3749 the user didn't expect deposits and withdrawals to appear in As a user of CCXT I'd try to use the methods without checking the code and each exchange documentation. I expect the library to be easy to use and to provide me as much information as possible, so I'd not need to go and read the official documentation of the exchange API. That said, when a method doesn't return what the method name says, the method should be documented so that the user would know what it doesn't, when he overs or clicks it from his IDE. I see 2 solutions for bitstamp.
I'd suggest solution nr 2 because the bitstamp endpoint to be used is called If there are many exchanges that do like bitstamp and have only one endpoint to fetch all transactions, maybe a new "unified" method could be created called
Like this one would see everything false, but fetchAllTransactions is true, that means that's an endpoint like the bitstamp one, fetching everything in one go. Like this it would be clear also for the user that filed #3749 that he should use one of the What do you think? Update: downside of solution 2 is that it will break things for all people relying on fetchMyTrades. But you can tell them they should always check if a feature is available, if not check the next etc. like if I had to automate checking a bunch of exchanges deposit history I'd implement in my userland a helper function to check exactly that. |
I'd suggest a compromise – we could go for solution1, keeping both fetchMyTrades and fetchTransactions (both mapped to the same common method, basically), and we'd return async fetchMyTrades (...) {
return await this.fetchMyTradesAndTransactions (type, ...); // type might not be needed
}
async fetchTransactions (...) {
return await this.fetchMyTradesAndTransactions (type, ...); // type might not be needed
}
async fetchMyTradesAndTransactions (type, ...) {
if (this.options['fetchMyTradesAndTransactionsWarning']) {
const warning = this.id + " " +
"fetchMyTrades / fetchTransactions will return all trades and transactions " +
"mixed in the same array. Make sure you add proper handling and set this.options['fetchMyTradesAndTransactionsWarning'] = false " +
"or add { 'options': { 'fetchMyTradesAndTransactionsWarning': false }} to exchange constructor " +
"to acknowledge this message and turn off this warning.";
throw new ExchangeError (warning);
}
...
// parsing overrides to handle them both
return ...;
} The user just won't be able to skip it and we'll force the awareness about the above exchange-specific behavior that way. How does that sound to you? |
I like the exception, it's quite smart way to make a user aware of the unusual situation without forcing him to check the official exchange docu, and provide an option to get rid of the warning. So this one would be the exceptional exchange, the one that will filter/group the transactions based on the type? Or what do you mean in:
? You mean to call parseTrades and then also call parseTransactions? |
I did an initial draft of the bitstamp refactoring. I didn't understand exactly your intention with type: transactions or trades. When I call the common method from:
then:
Used like this we could have used also a bool, e.g. fetchTransactions true|false. Like this the only thing I still need to do is to implement the |
Ok, my intent was probably not clear, let's clarify this a bit more ) According to their docs, the Back to our unified API, we have Every unified trade structure has a Every unified transaction structure also has a Because both Thinking a bit further about it, I guess, we should do the following: By default, I would also keep a friendly warning inside each of the two methods to make sure that the user is aware of the possible pagination issues. The user who would want all transactions, would just call the implicit method directly. To me that feels like a clean approach to Bitstamp for now. How about you? We could also add some flags to |
Looks good, I'm implementing exactly like that. Btw I tried to use the
Doesn't transpile... I still get response.filter (with the arrow function) in python... which doesn't work of course. Workarounds for this? Or is this a transpiler bug? |
let result = this.filterBy (result, 'type', '2'); // this will transpile properly To recap, we can't really use |
I find lots of examples of filterBy, but not examples to filter by more than one value. In our case filterBy 2 ok, to get myTrades. But to get deposits (0) AND withdrawals (1) I need to filter by 0 and 1. Do we have a method like that? If not, maybe can you create one? Else I can try to make one myself, should be a simple loop. UPDATE: or maybe not to make life complicated: I filter first by 0, then by 1, then combine the 2 arrays into one. Do we have a helper method that transpiles correctly, to combine 2 arrays? |
// ...
let groups = this.groupBy (results, 'type');
let transactions = this.arrayConcat (groups['0'], groups['1']); // 0 (deposits) + 1 (withdrawals)
transactions = this.sortBy (transactions, 'timestamp');
let trades = this.sortBy (groups['2'], 'timestamp');
// ... Actual field names and key-values depend on whether you're operating with results from the exchange not parsed yet, or you're operating with already-parsed/unified results. |
@firepol sorry, I forgot that I made the |
Err... actually, the example above is a bad example of how not to use filterByArray, as it does multipass traversion of the array, whereas in that particular case with bibox shown on the screenshot in my previous message, all that can be done in just one pass: parseTickers (rawTickers, symbols = undefined) {
let tickers = [];
for (let i = 0; i < rawTickers.length; i++) {
tickers.push (this.parseTicker (rawTickers[i]));
}
return this.filterByArray (tickers, 'symbol', symbols);
} ↓ parseTickers (rawTickers, symbols = undefined) {
let tickers = [];
for (let i = 0; i < rawTickers.length; i++) {
let ticker = this.parseTicker (rawTickers[i]);
if ((typeof symbols !== 'undefined') || (this.inArray (ticker['symbol'], symbols))) {
tickers.push (ticker);
}
}
return tickers;
} But anyways, you have a complete set of tools and you can combine them how you like. |
[RANT] Did I tell you that I kinda hate the bitstamp API? So far it's the worst ones I've seen and used after lykke and cex, nr 3 in the top 3 ugliest crypto APIs seen so far... [/RANT] I'm giving up for tonight, I pushed my WIP implementation here: https://github.com/firepol/ccxt/tree/bitstamp-fetch-transactions I will file a bug related to the sucky bitstamp in a second. UPDATE: actually I guess I can override parseTransactions instead of filing a bug. If you are curious, the problem can be reproduced (in my branch I just pushed in my fork, linked above) as follows (in python): bitstamp = ccxt.bitstamp('...')
test1 = bitstamp.fetch_transactions('BTC') # this won't work, not a valid market apparently
test2 = bitstamp.fetch_transactions('BTC/USD') In test2, BTC/USD becomes btcusd, which is not a valid currency, thus in the exchange.parse_transactions it fails with this error:
|
@firepol we usually use some flavor of the following: let symbol = 'BTC/USD';
let market = this.market (symbol); // market['id'] == 'btcusd'
let base = market['base']; // BTC, also baseId = btc
let quote = market['quote']; // USD, also quoteId = usd This helps in situations where you need to convert a currency code to a market symbol and back. |
I have a little problem, the error happens in the exchange.fetch_transactions method. If I copy paste it in bitstamp.js to override it, I get another syntax error because the transpiler doesn't transpile correctly the first line:
Sorry to bother you with so many questions, but bitstamp is really a pain, and the transpiler also is quite harsh... Can you help? |
@firepol sure! The issue is that we can't break the following rules: https://github.com/ccxt/ccxt/blob/master/CONTRIBUTING.md#derived-exchange-classes, this one in particular:
As I said before, we can't transpile syntax that involves closures – that is all syntax with js
However, you don't really have to worry about, as I'm going to edit it and clean it up upon merging anyways. Let me know if you have more ) |
Voila, done. I may add an interesting discovery I just made: refer to the api docs: https://www.bitstamp.net/api/ USER TRANSACTIONS endpoint: https://www.bitstamp.net/api/v2/user_transactions/{currency_pair}/ You may think that you can get just the deposits (type 0) or withdrawals (type 1) but you can't. In order to get them you have to fetch everything. I mean the history of all trades for all currencies (all pairs/markets). That's why I implemented how I did. I had to fight for a long time to figure everything out. Now it's tested and working. |
@firepol after thinking a bit more on it, i decided to revert to your initial proposal, and I've uploaded a merged version of it, should be ok as of 1.17.253, and removed some parts to make it a bit cleaner implementation. Let me know if you think we should add or edit something beyond that. Thx again for your help, your involvement is very much appreciated! |
JFYI we think adding the |
Has the unified deposit and withdrawal history been implemented in any
of the exchanges yet (or base classes for that matter)? I was hoping to find one to use as an example.
The text was updated successfully, but these errors were encountered: