Skip to content

Commit

Permalink
Do not force mutate eth accounts to be checksummed
Browse files Browse the repository at this point in the history
Fix rotki#519.

This should no longer happen. The very first upgrade of the DB turned
all address entries into checksummed entries. Since then we make sure
to always add addresses as checksummed in the DB and the API also
makes sure that all address input is checksummed.

So this commit closes the chapter by warning the user if a
non-checksummed eth account is found in his DB. But the only way this
happens is if the user manually edits it.
  • Loading branch information
LefterisJP committed Dec 24, 2019
1 parent 9c9612b commit dbca15d
Show file tree
Hide file tree
Showing 3 changed files with 45 additions and 6 deletions.
5 changes: 0 additions & 5 deletions rotkehlchen/blockchain.py
Expand Up @@ -57,12 +57,7 @@ def __init__(
self.ethchain = ethchain
self.msg_aggregator = msg_aggregator
self.owned_eth_tokens = owned_eth_tokens

self.accounts = blockchain_accounts
# go through ETH accounts and make sure they are EIP55 encoded
# TODO: really really bad thing here. Should not have to force mutate
# a named tuple. Move this into the named tuple constructor
self.accounts._replace(eth=[to_checksum_address(x) for x in self.accounts.eth])

# Per account balances
self.balances: Balances = defaultdict(dict)
Expand Down
10 changes: 9 additions & 1 deletion rotkehlchen/db/dbhandler.py
Expand Up @@ -8,7 +8,7 @@
from json.decoder import JSONDecodeError
from typing import Any, Dict, List, Optional, Set, Tuple, cast

from eth_utils import to_checksum_address
from eth_utils import is_checksum_address, to_checksum_address
from pysqlcipher3 import dbapi2 as sqlcipher
from typing_extensions import Literal

Expand Down Expand Up @@ -644,6 +644,14 @@ def get_blockchain_accounts(self) -> BlockchainAccounts:

for entry in query:
if entry[0] == S_ETH:
if not is_checksum_address(entry[1]):
self.msg_aggregator.add_warning(
f'Non-checksummed eth address {entry[1]} detected in the DB. This '
f'should not happen unless the DB was manually modified. '
f' Skipping entry. This needs to be fixed manually. If you '
f' can not do that alone ask for help in the issue tracker',
)
continue
eth_list.append(entry[1])
elif entry[0] == S_BTC:
btc_list.append(entry[1])
Expand Down
36 changes: 36 additions & 0 deletions rotkehlchen/tests/db/test_db.py
Expand Up @@ -900,3 +900,39 @@ def test_add_ethereum_transactions(data_dir, username):
assert len(warnings) == 0
returned_transactions = data.db.get_ethereum_transactions()
assert returned_transactions == [tx1, tx2, tx3]


def test_non_checksummed_eth_account_in_db(database):
"""
Regression test for https://github.com/rotki/rotki/issues/519
This is a test for an occasion that should not happen in a normal run.
Only if the user manually edits the DB and modifies a blockchain account
to be non-checksummed then this scenario will happen.
This test verifies that the user is warned and the address is skipped.
"""
# Manually enter three blockchain ETH accounts one of which is only valid
cursor = database.conn.cursor()
valid_address = '0x9531C059098e3d194fF87FebB587aB07B30B1306'
non_checksummed_address = '0xe7302e6d805656cf37bd6839a977fe070184bf45'
invalid_address = 'dsads'
cursor.executemany(
'INSERT INTO blockchain_accounts(blockchain, account) VALUES (?, ?)',
(
('ETH', non_checksummed_address),
('ETH', valid_address),
('ETH', invalid_address)),
)
database.conn.commit()

blockchain_accounts = database.get_blockchain_accounts()
eth_accounts = blockchain_accounts.eth
assert len(eth_accounts) == 1
assert eth_accounts[0] == valid_address
errors = database.msg_aggregator.consume_errors()
warnings = database.msg_aggregator.consume_warnings()
assert len(errors) == 0
assert len(warnings) == 2
assert f'Non-checksummed eth address {non_checksummed_address}' in warnings[0]
assert f'Non-checksummed eth address {invalid_address}' in warnings[1]

0 comments on commit dbca15d

Please sign in to comment.