Skip to content
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

Closed
rbn920 opened this issue Feb 14, 2018 · 64 comments
Closed

Unified deposit and withdrawal transaction history #1896

rbn920 opened this issue Feb 14, 2018 · 64 comments
Assignees

Comments

@rbn920
Copy link
Contributor

rbn920 commented Feb 14, 2018

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.

@kroitor kroitor self-assigned this Feb 14, 2018
@kroitor
Copy link
Member

kroitor commented Feb 14, 2018

Has the unified deposit and withdrawal history been implemented in any
of the exchanges yet (or base classes for that matter)?

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.

@ZohaibAhmed
Copy link

@kroitor Is there a proposal on this implementation?

@kroitor
Copy link
Member

kroitor commented Mar 11, 2018

@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:

    {
        'info':       { ... },                  // the original decoded JSON as is
        'id':        '12345-67890:09876/54321', // string transaction id
        'txid':      'ddsjfdlskjflksdjfkldsjf', // txid in terms of corresponding currency
        'timestamp':  1502962946216,            // Unix timestamp in milliseconds
        'datetime':  '2017-08-17 12:42:48.000', // ISO8601 datetime with milliseconds
        'currency':  'ETH',                     // currency code 
        'status':    'pending',                 // status of the transaction, "pending", "ok"... to be discussed
        'side':      'deposit',                 // direction of the transaction, 'deposit' or 'withdraw'
        'price':      0.06917684,               // float price in quote currency
        'amount':     1.5,                      // absolute amount of base currency
        'fee': {
            'cost': ..., // we also need to somehow designate if...
            'rate': ..., // ...it is a network fee or the exchange fee or both
        },
        ... ? ...
    },

From the above it follows that there are still questions to be discussed. Your proposals and suggestions are welcome.

@ZohaibAhmed
Copy link

@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?

@grandmore
Copy link
Contributor

I need to implement deposits & withdrawals for Binance.

@ZohaibAhmed have you implemented this?
@kroitor do you have a spec or only what is on this thread?

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.

@ZohaibAhmed
Copy link

@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?

@grandmore
Copy link
Contributor

grandmore commented Mar 18, 2018

@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.

@kroitor
Copy link
Member

kroitor commented Mar 19, 2018

@grandmore

do you have a spec or only what is on this thread?

Just what's on this thread...

I am going to push some code to a fork in a few hours.

Just make sure to read this carefully: https://github.com/ccxt/ccxt/blob/master/CONTRIBUTING.md#how-to-contribute-code

Thx!

@grandmore
Copy link
Contributor

@kroitor Yes, what's in this thread, as its all there is. As well as modelling the other functions in the library.
I have of course read all the documentation as well as the CONTRIBUTING.md docs and experimented with the library over the last few days.

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.

@grandmore
Copy link
Contributor

@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

// returns deposit transactions
fetchDeposits (currency = undefined, since = undefined, limit = undefined, params = {})

// returns withdrawal transactions
fetchWithdrawals (currency = undefined, since = undefined, limit = undefined, params = {})

The remaining unified fetchTransactions I will do after I clear up some questions.

// returns deposits and withdrawals
fetchTransactions (currency = undefined, since = undefined, limit = undefined, params = {})

There is very little compatability from the exchanges I look at.
Binance api
Poloniex Api
Coinbase Api
GDAX Api

@clarkmoody
Copy link
Contributor

Kraken API also has /private/Ledgers with account history.

@clarkmoody
Copy link
Contributor

Bitstamp's deposits and withdrawals are combined with the trade history, so the current implementation for fetchMyTrades on Bitstamp is incorrect. Deposits and withdrawals are denoted by the type field in the entry. I'm not sure they provide txid for crypto transactions through their API, though this information is available through the logged-in Account interface.

@lfern
Copy link
Contributor

lfern commented Mar 22, 2018

@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?

@kroitor
Copy link
Member

kroitor commented Mar 22, 2018

@lfern your contributions are welcome )

@grandmore
Copy link
Contributor

grandmore commented Mar 22, 2018

@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?

@kroitor
Copy link
Member

kroitor commented Mar 22, 2018

@kroitor what do you need to get this code into the main repository?

@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 )

@grandmore
Copy link
Contributor

@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)
} 

@lfern
Copy link
Contributor

lfern commented Mar 23, 2018

@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';
        }
    }

@grandmore
Copy link
Contributor

@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 ['tx_timeout', 'tx_invalid', 'tx_cancelled', 'tx_rejected'] = cancelled.

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 ();
  }
}

@lfern
Copy link
Contributor

lfern commented Mar 23, 2018

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.

type TransactionStates = (
  'ok' |
  'cancelled' |
  'pending' |
  'approved' |
  'rejected' |
  'failure' |
  'pending-user-action');

@kroitor
Copy link
Member

kroitor commented Mar 23, 2018

What do you think about posible values for returned status for transactions?

I would go with as little variety of statuses as possible, namely:

  • ok (deposit credited or funded/received or withdraw has been sent, has been approved for sending, etc...)
  • pending (deposit pending/confirming or withdrawal awaiting for confirmation or approval)
  • error (whatever the reason, rejected by whatever party or failed)
  • canceled (most users will probably want it, however, many exchanges will not report canceled historical transactions)

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.

@grandmore
Copy link
Contributor

grandmore commented Mar 24, 2018

@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.

const syntheticId = this.hash (timestamp.toString () + currency.id + side + amount + address);

The id is computed so it will always return the same id when recomputed. But I have 2 questions.

  1. The hash id has to be identical when created from any of the supported languages, I am using the standard this.hash() I do not code in python so I wanted to check with you if this works in python.

  2. I can either return the synthetic id in place of the id or return a nulled id and add a hash id for example. What would you prefer?

This

let result = {
  ...
  'id': null, 
  'hashid': syntheticId,
  ...
};

or this

let result = {
  ...
  'id': syntheticId, 
  ...
};

How do I organise a pull request?

@kroitor
Copy link
Member

kroitor commented Mar 25, 2018

I can either return the synthetic id in place of the id or return a nulled id and add a hash id for example. What would you prefer?

I would prefer the id to be undefined if it is unknown from the exchange and to keep the hash id in your userland outside of the library, as having the hash id is your user-specific task. Not all users need the excessive calls. In other words I would prefer normalized data where normalized means that the data should not be duplicated, and if a field is always reconstructed from the same data, I would prefer not to denormalize it (not to add more fields or special cases).

How do I organise a pull request?

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!

@clarkmoody
Copy link
Contributor

For the fees object, have you seen any exchanges that charge fees in a different currency than the transaction currency? Even if not, it may be nice if the fees object were the same type across multiple classes, so the currency would be included on fees even if the same as the withdrawal.

@kroitor
Copy link
Member

kroitor commented Mar 29, 2018

@clarkmoody our standard fee structure (you can find it among orders and trades) throughout the library is:

fee: {
    'cost': 123.45,
    'rate': 0.001,
    'currency': 'ETH',
}

)

@clarkmoody
Copy link
Contributor

our standard fee structure

Great! I was just wondering since the example data structure earlier in this thread didn't have all those fields.

@grandmore
Copy link
Contributor

@kroitor I have create a pull request for the Binance deposit & withdraws code. (#2526)

@firepol
Copy link
Contributor

firepol commented Aug 25, 2018

Hi @kroitor,

renamed side field to type field

In python, type is the name of a built-in, so I think it's a good idea not to use that name that overrides the build-in and stick to side. This should not be a problem if you use it inside a json as it seem to be the current usage, but it can be confusing as all methods will use "side" and in the json it's not side but type. Also people are used to the side terminology to identify buy/sell, so why not keep the same terminology also for deposit/withdraw? Maybe you wanted to separate the 2 concepts of buy/sell and deposit/withdraw, thus using a different field... but I think it's not really necessary... Thoughts?

@kroitor
Copy link
Member

kroitor commented Aug 25, 2018

so why not keep the same terminology also for deposit/withdraw?

because, you said it yourself:

people are used to the side terminology to identify buy/sell,

) In other words, if side is either buy or sell everywhere, we don't really want to add a third and a fourth possible side to the list of possible values for that field. This will add to confusion, I think. Therefore type.

@kroitor
Copy link
Member

kroitor commented Aug 25, 2018

P.S. Of course, the type field itself is also used with orders (limit, market), so it's not the best name for the funding direction either. We were also thinking of calling it direction, however, we ended up with type for now as we think it would be semantically close to what exchanges themselves reply with.

@firepol
Copy link
Contributor

firepol commented Sep 6, 2018

@kroitor question for those exchanges that don't have different methods to fetch withdrawls and fetch deposits, such as:

  • bitstamp (can get all history, including trades)
  • bitfinex (deposits + withdrawals)
  • ...

Is it fetchTransactions to be set like this?

  • 'fetchTransactions': true,
  • 'fetchWithdrawals': false,
  • 'fetchDeposits': false,

I see that fetchTransactions is implemented for these exchanges:

I propose to add a new parameter in the method, for the type.

  • undefined: all kind of transactions (deposits, withdrawals, trades)
  • withdrawal: only return withdrawals
  • deposit: only return deposits

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 type = 'deposit', then with type = 'withdrawal'.

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.
Or call it 2 times, as for bitfinex: First with type = 'deposit', then with type = 'withdrawal'.
To get mytrades, the method fetchMyTrades should be called instead, and fetchMyTrades should filter out the (withdrawl and deposit) transactions, not expected in that method call.

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)

@kroitor
Copy link
Member

kroitor commented Sep 6, 2018

Is it fetchTransactions to be set like this?

  • 'fetchTransactions': true,
  • 'fetchWithdrawals': false,
  • 'fetchDeposits': false,

Yep )

I propose to add a new parameter in the method, for the type.

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 fetchDeposits and fetchWithdrawals and only fetchTransactions is available, it means that the exchange in question will output all of them mixed, without the filter. Otherwise, if the exchange allowed to filter them, native fetchDeposits or fetchWithdrawals would be true, and then the user would just call the corresponding method in the first place.

So, in cases when we only have fetchTransactions CCXT will first download the entire response (containing mix-typed transactions). Then, the library has to parse each of the downloaded transactions, to find out their actual unified types. Note that we will parse all deposits and withdrawals anyway. At this point, we got all transfers parsed, so, having done the hardest part of the job already, why would a library filter them? Sounds like an artificial (unnecessary) limitation of results, whereas we actually handled all of them. We'd rather return all parsed results and let the user decide. Moreover, we offer a useful set of methods, and most languages offer syntax sugar for filters and maps, like so:

// 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 fetchTransactions. We'd just have fetchTransactions without a type and that's it.

On the other hand, you could wrap fetchTransactions with has['fetchDeposits'] = 'emulated', as described in the Manual on exchange has properties: https://github.com/ccxt/ccxt/wiki/Manual#exchange-properties and with an additional basic
wrapper that would call fetchDeposits → fetchTransactions+filter and fetchWithdrawals → fetchTransactions+filter.

Thus, instead of calling fetchTransactions with a type specified, you would rather call emulated versions of fetchDeposits or fetchWithdrawals wrappers. However, because of pagination, this can really become tricky (to pay respect for limit and since with filtered results). So, in the end, we would not bother emulating anything that is not very well supported by the exchange in question (not worth the effort, we 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)

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!

@firepol
Copy link
Contributor

firepol commented Sep 6, 2018

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 fetchMyTrades and I agree with him.

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.

  1. Implement fetchTransactions and basically it's the same as fetchMyTrades.
  2. Get rid of fetchMyTrades and have only fetchTransactions.

I'd suggest solution nr 2 because the bitstamp endpoint to be used is called transactions so if a user will look it up online he'd know he should search something called "transactions" and not "fetchmytrades".

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 fetchAllTransactions that will justify fetchMyTrades: false, e.g. for bitstamp it would be:

'fetchMyTrades': false,
'fetchTransactions': false,
'fetchWithdrawals': false,
'fetchDeposits': false,
'fetchAllTransactions': true,

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 grouped or filtering methods in the native programming language and filter what he needs from the huge list containing everything.

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.

@kroitor
Copy link
Member

kroitor commented Sep 6, 2018

What do you think?

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 type: undefined/None/null, type:'limit', type: 'market', type:'withdrawal', type:' 'deposit' from both fetchMyTrades and fetchTransactions, where/if possible. And I would add an exception-warning that would make this code self-documented, something like:

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?

@firepol
Copy link
Contributor

firepol commented Sep 6, 2018

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:

    // parsing overrides to handle them both
    return ...;

? You mean to call parseTrades and then also call parseTransactions?

@firepol
Copy link
Contributor

firepol commented Sep 7, 2018

I did an initial draft of the bitstamp refactoring.

I didn't understand exactly your intention with type but I used it in this way:

type: transactions or trades.

When I call the common method from:

fetchMyTrades I set type = 'myTrades'
fetchTransactions I set type = 'transactions'

then:

...
response = call the API
if (type === 'myTrades') {
    result = filter to get only trades
    return parseOrders(result)
} else {
    result = filter to get only trades
    return parseTransactions(result)
}

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 parseTransaction method and do the proper filtering of the response. Please correct me if you intended something else, I'll probably work on this after 20:30 CET time so no rush in answering ;)

@kroitor
Copy link
Member

kroitor commented Sep 7, 2018

I didn't understand exactly your intention with type but I used it in this way:

Ok, my intent was probably not clear, let's clarify this a bit more ) According to their docs, the user_transactions endpoint returns all ledger rows, that includes trades, funding transactions (deposits and withdrawals) and internal transfers.

bitstamp-transactions

Back to our unified API, we have fetchMyTrades (returns an array of dictionaries) and we also have fetchTransactions (also returns an array of dictionaries, and the method may be missing, or represented by the two other separate methods – fetchDeposits and fetchWithdrawals, if the exchange does not allow to fetch them in one call).

Every unified trade structure has a type (limit or market):
https://github.com/ccxt/ccxt/wiki/Manual#trade-structure

Every unified transaction structure also has a type (deposit or withdraw):
https://github.com/ccxt/ccxt/wiki/Manual#transaction-structure

Because both fetchMyTrades and fetchTransactions return arrays of dictionaries, and because all dictionaries have a type the user can use that type to do the filtering and to distinguish trades from transactions (be it deposits or withdrawals).

Thinking a bit further about it, I guess, we should do the following:

By default, fetchMyTrades would return just the trades (rows that aren't transactions or internal transfers, parsed with parseTrade), and fetchTransactions would return just the transactions (rows that are not trades or internal transfers, parsed with parseTransaction). This is consistent with the design of our unified API. And this is what a user would expect, from description in the Manual.

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 fetchMyTrades / fetchTransactions to return differentiated results, with mix-typed rows parsed by either this or that parser, but I would not bother emulating this aspect, really (trying to fill the gaps in the exchanges' apis with emulated methods often turns out to be a bad decision in terms of both the architecture and the user experience).

@firepol
Copy link
Contributor

firepol commented Sep 7, 2018

Looks good, I'm implementing exactly like that.

Btw I tried to use the filter method, as you suggested, but it doesn't transpile correctly in python.

        if (type === 'myTrades') {
            let result = response.filter (t => t['type'] === '2');
            return this.parseTrades (result, market, since, limit);
        } else {
            let result = response.filter (t => t['type'] === '0' || t['type'] === '1');
            return this.parseTransactions (result, market, since, limit);
        }

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?

@kroitor
Copy link
Member

kroitor commented Sep 7, 2018

Workarounds for this?

let result = this.filterBy (result, 'type', '2'); // this will transpile properly

To recap, we can't really use .filter () inside the lib, because the transpiler is still quite dumb and doesn't know how to do code introspection that it has to do to recognize closures and mappings and convert them to proper python list comprehensions (this involves complex expressions with variables from the outside of the expression scope, which makes it hard to transpile). Therefore, we have a set of helpers: .filterBy, .groupBy, .indexBy, .sortBy and so on. If you search for the word "filter" in source files, you will hit a ton of examples of how it is used in other exchanges for filtering arrays of results by key-value. Hope this helps ) Let me know if not ) Thx! )

@firepol
Copy link
Contributor

firepol commented Sep 7, 2018

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?

@kroitor
Copy link
Member

kroitor commented Sep 7, 2018

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.

@kroitor
Copy link
Member

kroitor commented Sep 7, 2018

@firepol sorry, I forgot that I made the .filterByArray method earlier, so yes, you can do that in just one line )

screen shot 2018-09-08 at 00 33 34

@kroitor
Copy link
Member

kroitor commented Sep 7, 2018

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.

@firepol
Copy link
Contributor

firepol commented Sep 7, 2018

[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:

  File "/home/paul/projects-python/ccxt/python/ccxt/base/exchange.py", line 1293, in parse_transactions
    code = currency['code'] if currency else None
KeyError: 'code'

@kroitor
Copy link
Member

kroitor commented Sep 7, 2018

@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.

@firepol
Copy link
Contributor

firepol commented Sep 8, 2018

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:

  File "/home/paul/projects-python/ccxt/python/ccxt/bitstamp.py", line 456
    result = Object.values(transactions or []).map(transaction => self.parse_transaction(transaction, currency))
                                                                ^
SyntaxError: invalid syntax

Sorry to bother you with so many questions, but bitstamp is really a pain, and the transpiler also is quite harsh... Can you help?

@kroitor
Copy link
Member

kroitor commented Sep 8, 2018

@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:

unfold all maps and comprehensions to basic for-loops

As I said before, we can't transpile syntax that involves closures – that is all syntax with js .filter and js .map, therefore we substitute all .filter/.map/.forEach with either a call to a base method or a basic for-loop. See this example:

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 )

@firepol
Copy link
Contributor

firepol commented Sep 8, 2018

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.

@kroitor
Copy link
Member

kroitor commented Sep 10, 2018

@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!

@kroitor
Copy link
Member

kroitor commented Sep 12, 2018

JFYI we think adding the tag associated with the address for certain currencies and transactions – is a reasonable thing to do, to align it with the createDepositAddress/fetchDepositAddress return, therefore we have started adding the tag to parseTransaction where missing and we've updated the description of a transaction structure in the Manual as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

9 participants