Skip to content

#5 Electrum wallet support with missing fees #11

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.

Sign up for GitHub

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

Open
cristobaltapia wants to merge 6 commits into
base: master
Choose a base branch
from
Open

#5 Electrum wallet support with missing fees #11

Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
2a8c96b
Merge pull request #2 from probstj/master
cristobaltapia Dec 16, 2018
42ab724
Consider missing fees in electrum wallet.
cristobaltapia Dec 16, 2018
12ff255
Update ccgains/trades.py
anson-vandoren Dec 16, 2018
1df3c63
Apply suggestions from code review
anson-vandoren Dec 16, 2018
ed3555b
Update trades.py
cristobaltapia Dec 16, 2018
a4e59f8
A needed argument was not being passed to the _parse_trade function
cristobaltapia Dec 19, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
68 changes: 61 additions & 7 deletions ccgains/trades.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -213,7 +213,9 @@
'comment': 4
}

def _parse_trade(str_list, param_locs, default_timezone):

def _parse_trade(str_list, param_locs, default_timezone,
fill_missing_fees=False):
"""Parse list of strings *str_list* into a Trade object according
to *param_locs*.

Expand All @@ -236,6 +238,14 @@ def _parse_trade(str_list, param_locs, default_timezone):
datetime string inside str_list. Otherwise the time data will
be interpreted as time given in *default_timezone*.

: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). This only accounts for the blockchain fees, so it
is intend to be used in personal wallets only, as it does not (cannot)
consider the exchange fees. This should be specified in the specific
`append_***_csv` method. (Default: `False`)

:return: Trade object

"""
Expand All @@ -257,9 +267,43 @@ def _parse_trade(str_list, param_locs, default_timezone):
else:
pdict[key] = val

# Get missing fees if requested
if fill_missing_fees and pdict['kind'] == 'Withdrawal':
Copy link
Owner

@probstj probstj Jan 9, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Owner

@probstj probstj Jan 9, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

*Sorry, Electrum only. But my question still stands. :-)

tx = pdict['mark']
Copy link
Owner

@probstj probstj Jan 9, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor

@anson-vandoren anson-vandoren Jan 10, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😄

Copy link
Contributor Author

@cristobaltapia cristobaltapia Jan 10, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are those real comments? <

Yes, in the wallet you can write some comments to each transaction and the exported csv files contain them.

fee_curr = pdict['fee_currency']
Copy link
Owner

@probstj probstj Jan 9, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there is no fee_amount, fee_currency might be empty. Rather use sell_currency here.

pdict['fee_amount'] = _get_tx_fees(tx, fee_curr)

return Trade(default_timezone=default_timezone, **pdict)


def _get_tx_fees(txid, currency):
"""Get missing transaction fees using blockcypher API.

:param txid: str:
The transaction-ID.
:param currency: str:
The currency of the fees.

:return: obtained fees as Decimal object

"""
from blockcypher import get_transaction_details, from_satoshis
try:
tx = get_transaction_details(txid, coin_symbol=currency.lower())
if currency == 'BTC':
fee = from_satoshis(tx['fees'], currency.lower())
feeval = Decimal(fee)
elif currency == 'LTC':
fee = tx['fees'] / 10**8 # Smallest unit of measure for LTC
Copy link
Owner

@probstj probstj Jan 9, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

feeval = Decimal(fee)
except AssertionError:
print('Transaction id %s not valid' % txid)
Copy link
Owner

@probstj probstj Jan 9, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please use log.warning instead of print.

return None
print('Wrong Transaction ID.')
Copy link
Owner

@probstj probstj Jan 9, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

never reached :)


return feeval


class Trade(object):
"""This class holds details about a single transaction, like a trade
between two currencies or a withdrawal of a single currency.
Expand Down Expand Up @@ -581,7 +625,7 @@ def add_missing_transaction_fees(self, raise_on_error=True):

def append_csv(
self, file_name, param_locs=range(11), delimiter=',', skiprows=1,
default_timezone=None):
default_timezone=None, fill_missing_fees=False):
"""Import trades from a csv file and add them to this
TradeHistory.

Expand All @@ -606,6 +650,11 @@ def append_csv(
according to the locale setting; or it must be a tzinfo
subclass (from dateutil.tz or pytz)

: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`)
Copy link
Owner

@probstj probstj Jan 9, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


"""
with open(file_name) as f:
csvlines = f.readlines()
Expand All @@ -622,7 +671,8 @@ def append_csv(
# ignore empty lines
continue
self.tlist.append(
_parse_trade(line, param_locs, default_timezone))
_parse_trade(line, param_locs, default_timezone,
fill_missing_fees))

log.info("Loaded %i transactions from %s",
len(self.tlist) - numtrades, file_name)
Expand Down Expand Up @@ -1030,8 +1080,8 @@ def append_trezor_csv(self, file_name, currency, skiprows=1,
self.append_csv(file_name, TPLOC_TREZOR_WALLET, delimiter=',',
skiprows=skiprows, default_timezone=default_timezone)

def append_electrum_csv(self, file_name, skiprows=1,
default_timezone=None):
def append_electrum_csv(self, file_name, fill_missing_fees=False,
skiprows=1, default_timezone=None):
"""Import trades from a csv file exported from the Electrum Wallet
and add them to this TradeHistory.

Expand All @@ -1041,6 +1091,10 @@ def append_electrum_csv(self, file_name, skiprows=1,

Afterwards, all trades will be sorted by date and time.

:param add_missing_fees: bool
Indicates whether the missing fees should be obtained using the
'blockcypher' API.
Copy link
Owner

@probstj probstj Jan 9, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


:param default_timezone:
This parameter is ignored if there is timezone data in the
csv string. Otherwise, if None, the time data in the csv
Expand All @@ -1052,9 +1106,9 @@ def append_electrum_csv(self, file_name, skiprows=1,
it might change in future.

"""

self.append_csv(file_name, TPLOC_ELECTRUM_WALLET, delimiter=',',
skiprows=skiprows, default_timezone=default_timezone)
skiprows=skiprows, default_timezone=default_timezone,
fill_missing_fees=fill_missing_fees)

def append_coinbase_csv(self, file_name, currency=None, skiprows=4,
delimiter=',', default_timezone=None):
Expand Down