Plugin for binance.us csv tax report #65
Conversation
|
Awesome to hear that @macanudo527's hard work is starting to pay off: thanks for trying his Binance plugin! I will review this in the next few days, but meanwhile check my comments on |
|
Has the CCXT plugin worked for Binance.us? Just to be clear, I've tested / worked with it for Binance.COM. Hopefully we can get it finalized and I can add official support for your plugins. |
There was a problem hiding this comment.
Since I don't have a Binance account I can't tell if the CSV file is as complete as the REST output: @macanudo527 could you also review this plugin, since you know much more than I do about Binance?
I'm particularly curious about a couple of things:
- does the transaction id contain the transaction hash or is it an internal binance id (see https://github.com/eprbell/dali-rp2/blob/main/docs/developer_faq.md#how-to-fill-the-unique-id-field)?
- is the CSV as complete as the REST API or is it missing data (e.g. the Coinbase CSV files are missing a lot of data)?
@iloveitaly, your efforts on CSV plugins are bringing up an interesting design discussion. CSV files are sometimes incomplete (not always, e.g. Trezor is complete AFAIK): when is it worth having a CSV plugin? Here's what I'm thinking so far (input and comments are welcome):
- if there is a REST plugin and the CSV files are incomplete, then the CSV plugin can probably be skipped
- if the there is a REST plugin and the CSV files are complete, then CSV plugin is OK to have (it's an alternative)
- if there is no REST plugin, CSV plugin is good to have regardless of whether CSV files are complete or not (it's better than nothing)
Thanks everyone!
src/dali/plugin/input/csv/binance.py
Outdated
| "assets": line[self.__BASE_ASSET_INDEX].strip(), | ||
| "crypto_out_no_fee": line[self.__BASE_ASSET_AMOUNT_INDEX].strip(), | ||
| "spot_price": line[self.__BASE_ASSET_AMOUNT_SPOT_PRICE_INDEX].strip(), | ||
| # TODO it's unclear if `crypto_fee` is the USD amount of the crypto-paid fee |
There was a problem hiding this comment.
See my other comments on fees above.
iloveitaly
force-pushed
the
binance-csv-plugin
branch
from
8eb40ef to
6029bfd
Compare
|
I formalized the CSV vs REST discussion we started here into a FAQ: https://github.com/eprbell/dali-rp2/blob/main/docs/developer_faq.md#should-i-implement-a-csv-or-a-rest-data-loader-plugin According to the reasoning in the FAQ, this CSV plugin may fall in one of these three cases:
If bullet 3 or 4 are true we can proceed with the binance.us CSV plugin, but if bullet 2 is true we should abandon it. So in order to decide, the questions to be answered are:
@macanudo527, can you help with the above questions? |
|
Sorry, I guess Binance.US and Binance.com have very different CSV export processes. For .com, I have to export each kind of transaction separately. So, to get deposits, I have to go -> [Wallet] -> [Fiat & Spot] -> [Transaction History] (which defaults to deposit history) -> [Export Deposit History] And that will give me the deposit address, txnId, amount, asset, etc..., so basically, everything you need for IntraTransactions to resolve. Is it different for .US? That exported file does not include buys or sells, I have to go to a different menu item to get trades. I don't have access to .US to test. |
|
Ok, thanks for the info. Are the txids in binance.com CSVs actual transaction hashes or are they internal Binance ids? |
|
The transaction IDs (excluding USD deposits) only have 8 characters, which isn't enough for a txn hash right? My knowledge of the underlying network hashes is very fuzzy. I think it's safe to say that the unique_id is unknown in this plugin.
From my experience with my crypto bot (https://github.com/iloveitaly/crypto-index-fund-bot) the APIs between .com & .us are materially different. I can't confirm if .us is a subset of .com, but from what I've seen .us is a fork which has diverged from .com in enough ways that they should be thought of as separate exchanges. I could be wrong.
Yup, binance.us has a special tax report which includes all transactions. |
Right: it's too short to be a transaction hash (at least for BTC, ETH and many others).
Ok, however we use CCXT as a foundation and @macanudo527 said the CCXT binance.us class is a subclass of the CCXT binance.com one, so for our purposes we can probably consider .us a subset of .com. Since the .us CSV is incomplete (no hashes) and the CCXT binance.us REST plugin is probably relatively simple to obtain from the .com one (or from its abstract superclass), I'm leaning toward skipping the .us CSV plugin and just focus on the REST version. Thoughts? |
iloveitaly
force-pushed
the
binance-csv-plugin
branch
from
7a67a8e to
b766b6b
Compare
|
That seems like a reasonable approach! My gut is the .us API started as a subset of .com but has meaningfully diverged—even if the spec looks/is the same, I have a sense that the implementation/nuances of the API are different. Using the API makes sense, but I don't have the time to help here now that I have the CSV importer working :( I've fixed in the mypy errors here in case you want to merge this in! |
|
That's good thinking: let's keep this open. If we find out that the REST version of the binance.us plugin isn't as easy to obtain as we're currently thinking we can merge this PR instead. Thanks for helping out and driving the discussion in interesting directions! |
|
hey @eprbell what is the status on this PR can this be merged? were you able to determine if the REST version of the binance.us was easily obtainable? |
@gbtorrance has recently started working on the REST version of the Binance.us data loader. The reason I preferred waiting instead of merging this is that apparently Binance.us CSV files are missing transaction hashes, which are essential information. This makes transfer transactions unresolvable and as a result the missing information needs to be edited manually by users. Could you try this version with your taxes and let us know how it goes? If you find it is useful enough we might go ahead and merge it, while the REST variant is being worked on. |
|
Thanks for the update, ah gotcha. Yeah I was planning on testing this one out however I pulled this PR and attempted to install via the dev page https://github.com/eprbell/dali-rp2/blob/main/README.dev.md but I keep running into this error: Not sure if this is a known issue or one isolated to this PR as this is my first time looking at this repo but will take a closer look over this weekend. |
|
That means it's not finding RP2: are you sure you're not missing some of the setup steps? A few questions:
|
|
@eprbell just pushed some updates here! What are your thoughts on getting this merged? |
The Binance.us plugin didn't happen yet, so I'm OK with proceeding with this. I'll review the code in the next few days. Thanks for following up! |
CSV import plugin for binance.us tax report. There's some TODOs which I'd love to resolve if folks have tips.
By the way, the cctx plugin has been working great for me.