#5 Electrum wallet support with missing fees #11
Conversation
Merging from master
It serves as a base for other csv-files exported from wallets that do not cointain fee information.
Co-Authored-By: cristobaltapia <crtapia@gmail.com>
Co-Authored-By: cristobaltapia <crtapia@gmail.com>
|
Thanks for the corrections and improvements ;) |
| pdict[key] = val | ||
|
|
||
| # Get missing fees if requested | ||
| if fill_missing_fees and pdict['kind'] == 'Withdrawal': |
There was a problem hiding this comment.
Just want to mention here something about pdict['kind'] == 'Withdrawal':
This will only work because the only two wallets so far that actually use fill_missing_fees (Trezor and Electrum) will during import actually call withdrawals 'Withdrawal'. But some exchanges call withdrawals differently or use a different language or whatever, so this might become an issue if one wants to use this function at some point with a different wallet. That is the reason why I always checked if buy_amount is zero to make sure we are dealing with a withdrawal. (Latest developments introduced 'Payment' kind, so this also is not the best way anymore). Anyway, we can keep this line just as it is, but would prefer a cleaner solution. Any suggestions how to improve the issue? Maybe enforcing calling withdrawals 'Withdrawal' during import for all wallets/exchanges?
There was a problem hiding this comment.
*Sorry, Electrum only. But my question still stands. :-)
|
|
||
| # Get missing fees if requested | ||
| if fill_missing_fees and pdict['kind'] == 'Withdrawal': | ||
| tx = pdict['mark'] |
There was a problem hiding this comment.
My idea with the 'mark' field was actually to allow marking (flagging/labeling) the trade in some way in future. Like for example mark it as a payment, or just any special thing (IIRC, I think I used it in Poloniex import to mark trades as initiated from the maker as 'buy' or 'sell' or something). So I'd rather keep it reserved for future use.
I used the 'comment' field for transaction IDs. But I see you actually used comment already in Electrum and Trezor wallet. Are those real comments? Should we rather introduce a new field, 'TransID' or something, then? (Sorry I didn't mention it earlier when the tplocs for these wallets were introduced - I didn't notice it at that time)
There was a problem hiding this comment.
I think a txid field would be a useful addition. Not all the exchanges I import from provide those in their exports, but it would be nice to have a record of them in the resultant data. Additionally, if txids are present, it could be another way (in the future) to help identify the correct deposit following a withdrawal.
And thanks for clarifying what the 'mark' column was intended for. I had been wondering 😄
There was a problem hiding this comment.
Are those real comments? <
Yes, in the wallet you can write some comments to each transaction and the exported csv files contain them.
| # Get missing fees if requested | ||
| if fill_missing_fees and pdict['kind'] == 'Withdrawal': | ||
| tx = pdict['mark'] | ||
| fee_curr = pdict['fee_currency'] |
There was a problem hiding this comment.
If there is no fee_amount, fee_currency might be empty. Rather use sell_currency here.
| fee = tx['fees'] / 10**8 # Smallest unit of measure for LTC | ||
| feeval = Decimal(fee) | ||
| except AssertionError: | ||
| print('Transaction id %s not valid' % txid) |
There was a problem hiding this comment.
please use log.warning instead of print.
| except AssertionError: | ||
| print('Transaction id %s not valid' % txid) | ||
| return None | ||
| print('Wrong Transaction ID.') |
| fee = from_satoshis(tx['fees'], currency.lower()) | ||
| feeval = Decimal(fee) | ||
| elif currency == 'LTC': | ||
| fee = tx['fees'] / 10**8 # Smallest unit of measure for LTC |
There was a problem hiding this comment.
This might already introduce a floating point rounding error. The better way to do it is Decimal(tx['fees']) / 10**8. I believe tx['fees'] is an integer?
This might also happen in from_satoshis. How is it implemented? Can't we do it the same way it is done with LTC?
| :param fill_missing_fees: bool: | ||
| Indicate whether fees should be obtained with 'blockcypher' in case | ||
| the csv-file does not contain them. This will be done based on the | ||
| transaction-ID (TX). (Default: `False`) |
There was a problem hiding this comment.
Please add a warning, something like: "Please note that using this introduces a privacy leak: Since 'blockcypher' will be asked about all your withdrawals, they will be able to exactly tell which wallets belong to your IP."
| :param add_missing_fees: bool | ||
| Indicates whether the missing fees should be obtained using the | ||
| 'blockcypher' API. |
There was a problem hiding this comment.
Here also, please add a warning, same as above, something like: "Please note that using this introduces a privacy leak: Since 'blockcypher' will be asked about all your withdrawals, they will be able to exactly tell which wallets belong to your IP."
3 participants
fixes #5