feat: support BitMEX wallet history imports #6426
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #6426 +/- ##
===========================================
- Coverage 74.62% 74.43% -0.20%
===========================================
Files 1120 1121 +1
Lines 120505 120695 +190
Branches 11435 11474 +39
===========================================
- Hits 89924 89834 -90
- Misses 28988 29253 +265
- Partials 1593 1608 +15
Flags with carried forward coverage won't be shown. Click here to find out more.
|
I see margin position in the screenshot. So it's quite possible it due to something weird with this. We don't really do proper margin trades at all in rotki. Had some half-assed poloniex specific work for their margin trades since I used it a tiny bit in 2015 but we removed most of the logic. The Here it would probably make sense to put some debugging breakpoints into the pnl report process and check what happens. We want to do radical changes in the way PnL report is generated in the future, to make it easier to maintain: #6427 |
|
Turns out the issue with taxable PnL being 0 for some rows is because I didn't have a cost basis for them. After manually adding a large buy before the trade period, taxable PnL is correct for all rows. |
So is this reviewable now? Also seems to need a rebase already, with a conflict in changelog 😅 |
Yes It's ready for review. Will I rebase now? |
Yeah please do it, since I have not looked at this yet anyway, to run tests again and be mergable |
IanMichaelHarper
force-pushed
the
bitmex-csv-import
branch
from
dcde1db
to
4bed2ed
Compare
Rebased. Ready for review now. |
|
|
||
| event_identifier = f'BMEX_{hash_csv_row(csv_row)}' | ||
| usd_price = PriceHistorian().query_historical_price(A_BTC, A_USD, close_time) | ||
| abs_amount = realised_pnl.__abs__() # pylint: disable=no-member |
There was a problem hiding this comment.
Wouldn't this work?
| abs_amount = realised_pnl.__abs__() # pylint: disable=no-member | |
| abs_amount = abs(realised_pnl) |
There was a problem hiding this comment.
Ah cool it does. I'll change it.
| @@ -44,7 +44,7 @@ def deserialize_fee(fee: Optional[str]) -> Fee: | |||
|
|
|||
| Can throw DeserializationError if the fee is not as expected | |||
| """ | |||
| if fee is None: | |||
| if fee is None or fee == 'null': | |||
There was a problem hiding this comment.
Hmm I don't like this. The string here should only be a number. Can you handle the extra 'null' case in the single source case where you encountered it?
IanMichaelHarper
force-pushed
the
bitmex-csv-import
branch
from
4bed2ed
to
792ef5c
Compare
|
@LefterisJP I updated the PR with changes from your comments. While I was working on them I realised an |
|
Frontend part are okay |
There was a problem hiding this comment.
Looks good, just needs to remove a now unneeded change.
While I was working on them I realised an AssetMovement was being added for for a Canceled withdrawal, so I just added this line to skip it. Do we want to show cancelled/failed events in general? If so, maybe work here could be paired with #6169 in terms of the design of how to show incompleted events.
The issues are different. One is in the API of kraken and the way we query it. Since we query only historical ledger entries and not the specific withdrawal info we don't get the "Canceled" information. So it actually shows very wrong.
While here you already see it's canceled from the CSV status. I don't think you need to let the user know anything. There is no need to display or take canceled events into account. Is there?
| @@ -67,6 +67,7 @@ class HistoryEventType(SerializableEnumNameMixin): | |||
| INFORMATIONAL = auto() | |||
| MIGRATE = auto() | |||
| RENEW = auto() | |||
| MARGIN = auto() | |||
There was a problem hiding this comment.
Can you remove this then since it's not used? Let's maybe add it when we need it. Not sure yet how we would like to represent it. On the type or subtype.
Depends how complete we want the data to be. I'd say most users won't care about cancelled/failed events, they might even find them annoying and want to hide them, but others might want the full picture to see exactly what they did in the past. Overall giving the user the option would be good, but I agree should probably be left as a stretch goal. |
There was a problem hiding this comment.
LGTM.
Test that failed is rotkehlchen/tests/unit/test_inquirer.py::test_saddle_oracle[ which is being fixed by yabir.
But I want 2 things:
- I suspect there is an unused Enum being introduced by the PR. Am I blind or is it not being used anywhere?
- Generally keep commits limited/combined if they target the same thing. So here both commits would need to be squashed into one.
| log = RotkehlchenLogsAdapter(logger) | ||
|
|
||
|
|
||
| class BitMEXWalletHistoryColnames(Enum): |
There was a problem hiding this comment.
I added it after typing out all the column names when accessing csv_row but I never ended up using it. Do you think it's better to use something like this or just access columns of csv_row by typing them out directly?
There was a problem hiding this comment.
I added it after typing out all the column names when accessing
csv_rowbut I never ended up using it. Do you think it's better to use something like this or just access columns ofcsv_rowby typing them out directly?
Hmmm if it's used in multiple places maybe this is not bad, but using them directly if they are just used in the parser is fine. No need to create in memory structures like this enum which will persists for the entire lifetime of the app.
There was a problem hiding this comment.
ok, thanks. I wasn't sure so good to know. I'll remove it, squash my commits and push again
IanMichaelHarper
force-pushed
the
bitmex-csv-import
branch
from
e5284f1
to
8c826e1
Compare
LefterisJP
temporarily deployed
to
cassette-merge
GitHub Actions
Inactive
Bitmex Wallet History CSV Import
The lack of this feature is one of the reasons I never used Rotki for my own taxes, as I had traded on BitMEX a bunch in the past but then got locked out of my account and only had the
walletHistory.csvfile.I am not sure if the file I have is the "official format" for the file. I can't remember if I got it via download or just copy and pasting into an excel sheet, but it looks sufficiently similar to what is used in the bitmex api test to base this functionality off. It is missing a few of the fields that exist in that example string (such as
currencyandtransactID) so this may need to be iterated on based on demands from users and if they have data in a different format. For now, all rows are assumed to be denominated in satoshis and onlyDeposit,WithdrawalandRealisedPNLevents are supported.Importing the
walletHistory.csvfile allowsRealisedPNLevents to show up onHistory Eventstab as profit from a margin trade. A profit is marked as aReceivewhilst a loss is marked as aSpend. Both are shown as positive values (should theSpends be negative?). Deposits and withdrawals show up on their tab but theExch. Tradestab is not populated (as these are not really exchange trades, but profit or loss from an exchange trade. Is is more akin to income/capital gains in that regard). themargin_positionstable is populated in the DB and is used for the Profit and Loss Report.Testing
I did a bunch of user acceptance testing, importing different files with different date formats with and without custom date format field checked. Seems to work fine. History events look correct, Deposits and Withdrawals look correct. Nothing in Exch. Trades. PnL Report is successfully generated. Values look realistic. No double counting between History Events and Margin Positions. Everything looks good except sometimes the taxable PnL sometimes shows 0 for some entries (see image below). I can't figure it out, maybe someone could point me in the right direction there?
Checklist