Implement Binance.US REST API Data Loader Plugin

eprbell commented 2 years ago

Writing a DaLI data loader plugin has high impact on the functionality and usefulness of both DaLI and RP2. Data loader plugins are well-defined, encapsulated modules that translate native exchange REST API-based data into DaLI's standard format. As such they are good first issues for newcomers to the the project.

Before starting, please read the contributing guidelines.

Plugin development is described in the developer documentation, in particular read the Dali Internals and Plugin Development sections, which contain all the information needed to build a new data loader plugin.

Binance.US REST API is documented here: https://docs.binance.us/#introduction

The Coinbase plugin can be used as an example.

Before submitting a PR for the new plugin, make sure to go through the DaLI Plugin Laundry List.

eprbell commented 2 years ago

It may be worth experimenting with the idea of basing the implementation on ccxt (https://docs.ccxt.com/en/latest/manual.html): if it works well we could write one abstract ccxt plugin and subclass multiple times to get support for many exchanges almost for free (all the exchanges supported by ccxt).

macanudo527 commented 2 years ago

Ccxt seems to be very robust and allows for a lot of expandability. I haven't seen too many issues adapting it into an abstract plugin, but I did notice at least one issue with it. There isn't a way to check if you can fetch 'dust' orders. And at least for Binance, it treats these as a separate category of transactions and not with the other transactions. They will have to be pulled separately.

I guess I can bring this up with ccxt? Also, I'll be working with Binance.com (not .US) data since that is where I trade. I think they both have very similar APIs though and if anything the .com API has more features than the .US one.

eprbell commented 2 years ago

Thanks for taking on an issue! I'm thinking the Ccxt abstract plugin deserves a separate issue (I'll open one shortly). Then we can build the Binance plugin as a subclass of it and learn how to implement whatever is missing from Ccxt.

The way I see it there are two possibilities:

Ccxt works well enough, in which case this will become the new recommended way to build DaLI plugins or
Ccxt has too many limitations, in which case we'll have to revert back to native implementations, similar to the existing Coinbase or Coinbase Pro plugin.

What are dust orders? I'm not familiar with them, but they sound like potentially an example of unsupported Ccxt transaction, which makes them interesting. BTW: Binance.com is fine, just work with the exchange you have an account for and, like you say, Binance.us is probably not too different.

eprbell commented 2 years ago

Just found this on CCXT and Binance dust trades: issue https://github.com/ccxt/ccxt/issues/4887 (implemented in PR https://github.com/ccxt/ccxt/pull/5006)

eprbell commented 2 years ago

Added CCXT abstract plugin issue #22 .

eprbell commented 2 years ago

Thinking some more on this. Maybe the best way to avoid throw-away work here is:

implement Binance plugin with CCXT and add enough implementation to convince ourselves that it works,
finish the implementation of Binance plugin (without abstract superclass),
when the Binance plugin is finished, extract the CCXT superclass.

This would allow us to focus on the most important part of the work (implementing a CCXT-based plugin), without worrying about making the class hierarchy happy until the end. Just a thought...

macanudo527 commented 2 years ago

@eprbell ,

That's kind of what I was thinking. I could try to hash out what was needed and how to implement the CCXT functions using a real world example / data (Binance) and then extract from there. Binance will probably have all of the features that other exchanges would have since it's the biggest exchange in the world. And it has numerous quirks (e.g. You have to pull trades by trading pair and not all of your personal trades).

I don't do any futures / margin trades, but I can try with the stubbed data they give.

eprbell commented 2 years ago

Nice, sounds like we're on the same page. As for futures / margins: I haven't spent much time thinking about them yet (see https://github.com/eprbell/rp2/blob/main/docs/user_faq.md#how-to-handle-futures-and-options). If you figure out how they work and how to represent them it would be a great plus!

macanudo527 commented 2 years ago

Forked the project to start work on Binance.com plugin.

eprbell commented 2 years ago

@macanudo527, I assigned this issue to you, even though you're working on Binance.com, not .us: they're probably close enough that their implementation will be similar.

macanudo527 commented 2 years ago

Looking at the .us docs it looks like binance.us api is just a subset of the functionality of binance.com so I can just 'rip out' what is not - needed.

eprbell commented 2 years ago

Sounds good, however hopefully CCXT abstracts out most exchange-specific behavior and you can write most of the plugin using the CCXT API, rather than the exchange native API/JSON. My hope is that the CCXT-based Binance.com plugin will work (almost) as is on other exchanges as well. That is, if the CCXT abstractions are good enough. There may be some exotic Binance-specific behaviors which require using the native Binance JSON, but hopefully not too many.

macanudo527 commented 2 years ago

Most of the main functions will work. There are a few things that I have to use the underlying 'raw' api calls in CCXT, but haven't had to hand roll any json calls.

I think you can abstract the main stuff out like trades and possibly deposits. Haven't looked at whether or not dividends will work smoothly or not.

What's strange is that CCXT won't pull fiat deposits unless you give it the specific code for the fiat. It has the api call that will pull all fiat deposits though.

Anyway, we will have to go over it function by function. I should have a rough draft available soon.

eprbell commented 2 years ago

Awesome! Looking forward to it.

eprbell commented 2 years ago

After discussion in #42 I saw that your Binance.com plugin is almost ready. It seems to me that the currency pair conversion building block will be needed soon, so I wanted to finalize its spec. Here's what I'm thinking:

add an optional fiat_iso_code parameter to AbstractTransaction constructor. If left unassigned the code will assume the fiat_iso_code to be the one from the country plugin (e.g. USD). Otherwise, plugins can assign it (e.g. JPY) and the transaction will then be denominated in the specified fiat
add a new plugin hierarchy under plugin/price_converter: these plugins will inherit from a new superclass src/dali/abstract_price_converter_plugin.py and implement get_ratio_from_native_source(timestamp: datetime, from_asset: str, to_asset: str), which will return the conversion ratio from_asset -> to_asset on the given timestamp
I will implement the first price_converter plugin by extracting the existing Historic_Crypto implementation from transaction_resolver. You can implement the CCXT-based one
add a new PriceConverter class, which will act as a container for all price_converter plugins. It will have a method like get_ratio(timestamp: datetime, from_asset: str, to_asset: str): it will call get_ratio_from_native_source() on the first plugin and if it can't find the pair it will try the second one and so on
the transaction resolver will check if a transaction has fiat_iso_code set and if so it will convert the transaction's fiat values to the country plugin fiat (USD) and perhaps add some description to the Notes field

Does this spec have everything you need or am I missing something?

We can divide this work in 2 parts:

all of the above, but the PriceConverter.get_ratio() has a fixed, built-in sequence of plugins to try (Historic_Crypto, CCXT, etc.)
implement .ini configuration for price_converter plugins, so that the user can configure plugin search order and parameters

@stevendavis (who has been working on some of these issues): since we'll be needing this fairly soon I propose I work on 1 (which includes adding the new plugin hierarchy) and you work on 2 (which is along the lines of your original idea of adding configuration for historical price converters). Does that sound OK? If you can't work on 2, let me know and I'll work on that.

macanudo527 commented 2 years ago

This is pretty much exactly what I was thinking. I might add that the fiat_iso_code should be required. I just see it causing too many hidden accounting errors otherwise. Or would this cause too many problems with input plugins?

I am about ready to submit the Binance plugin. I just need to add mining and figure out what unit tests to write.

macanudo527 commented 1 year ago

@gbtorrance In theory, the Binance.US plugin should just be a subset of the features available on Binance.com. Of course, you will need to change out the CCXT exchange class for the US one. However, you should just be able to gut it of the .com only features like mining and it should work.

I'm based in Japan, so I'm geolocked out of all US-exchanges, despite being a US citizen, so I can't test anything US-based.

So, does .US have autoinvest, BETH (ETH staking), and lending/savings? I don't think they have mining, do they?

Let me know if you have any questions. I had to work in a few workarounds for missing information, but my numbers seem to match up for .com so far at least.

According to the docs, it appears to be almost the same API as .com, but another user said the APIs are actually quite different, so you might need to do a lot of testing.

gbtorrance commented 1 year ago

@macanudo527 Thanks for the info! I'll take a look.

As to your questions about what it supports, I'm not sure yet. It has been a while since I used it, and I didn't use much more than the basic trading features.

BTW, do you typically use a sandbox for testing or your own data?

gbtorrance commented 1 year ago

I'm trying to get my dev environment set up, but I'm running into some issues. I've followed the instructions to Setup my Mac. No issues there, but when I try to run the unit tests I get this:

(.venv) gtorrance@gtorrance-mbp dali-rp2 % pytest --tb=native --verbose
============================================================================================================================= test session starts =============================================================================================================================
platform darwin -- Python 3.9.16, pytest-7.2.2, pluggy-1.0.0 -- /Users/gtorrance/Data/Repos/Various/dali-rp2/.venv/bin/python
cachedir: .pytest_cache
rootdir: /Users/gtorrance/Data/Repos/Various/dali-rp2
plugins: mock-3.10.0
collected 53 items / 13 errors                                                                                                                                                                                                                                                

=================================================================================================================================== ERRORS ====================================================================================================================================
____________________________________________________________________________________________________________________ ERROR collecting tests/test_cache.py _____________________________________________________________________________________________________________________
import file mismatch:
imported module 'test_cache' has this __file__ attribute:
  /Users/gtorrance/Data/Repos/Various/dali-rp2/out/test/dali-rp2/test_cache.py
which is not the same as the test file we want to collect:
  /Users/gtorrance/Data/Repos/Various/dali-rp2/tests/test_cache.py
HINT: remove __pycache__ / .pyc files and/or use a unique basename for your test file modules

{ ... }

=========================================================================================================================== short test summary info ===========================================================================================================================
ERROR tests/test_cache.py
ERROR tests/test_historical_bar.py
ERROR tests/test_ods_output_diff.py
ERROR tests/test_plugin_binance_com.py
ERROR tests/test_plugin_binance_com_supplemental_csv.py
ERROR tests/test_plugin_bitbank_supplemental_csv.py
ERROR tests/test_plugin_ccxt.py
ERROR tests/test_plugin_coinbase.py
ERROR tests/test_plugin_coinbase_pro.py
ERROR tests/test_plugin_coincheck_supplemental.py
ERROR tests/test_plugin_historic_crypto.py
ERROR tests/test_plugin_pionex.py
ERROR tests/test_transaction_resolver.py
!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!! Interrupted: 13 errors during collection !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
============================================================================================================================= 13 errors in 3.98s ==============================================================================================================================

I found that if I add a __init__.py file in tests, then the result is a lot more positive with 102 tests passing, 1 failure, and 3 errors.

Thoughts? Thanks.

eprbell commented 1 year ago

I haven't seen this error before. A couple of questions:

did you install Homebrew (I assume you did, but just making sure you're not using the Python that comes with Mac)?
can you try removing all __pycache__ directories in the tree (including recursively in the subdirectories)?

The heart of the problem is in this message:

imported module 'test_cache' has this __file__ attribute:
  /Users/gtorrance/Data/Repos/Various/dali-rp2/out/test/dali-rp2/test_cache.py
which is not the same as the test file we want to collect:
  /Users/gtorrance/Data/Repos/Various/dali-rp2/tests/test_cache.py

Can you try this:

ls -al /Users/gtorrance/Data/Repos/Various/dali-rp2/out/test/dali-rp2/test_cache.py
ls -al /Users/gtorrance/Data/Repos/Various/dali-rp2/tests/test_cache.py

The first file seems incorrect and I'm not sure where it's coming from: test_cache.py should be inside the tests directory, not in out/test (actually I'm not sure where out/test is coming from: it's not a DaLI directory).

gbtorrance commented 1 year ago

Thanks for the reply. OK, progress. When I delete the out directory and rerun the tests everything works fine.

It seems out is created by IntelliJ when I do a Build | Build Project from within the IDE. I assume out is it's default target directory.

I don't have a lot of knowledge yet about the Python build process. I assume I can configure IntelliJ to output to a different directory. But what directory? Thoughts? Thanks.

eprbell commented 1 year ago

I am not familiar with IntelliJ, so I can't advise you precisely on how to configure it. Based on the picture you sent it would seem that it is trying to build a distribution for DaLI in the out/production directory (see the egg-info directory), but that is unnecessary because DaLI has its own distribution flow (see the distribution target in the Makefile). It is probably making a copy of sources under out/distribution, which is confusing the Python interpreter when running tests. Check if there is a way to disable that behavior.

gbtorrance commented 1 year ago

Thanks. I think that's all I really need to know for now. I'll just make a point of not running the build process from within IntelliJ. That should be be fine.

gbtorrance commented 1 year ago

BTW, JetBrains IntelliJ is a Java IDE, but I am using the Python plugin, which essentially gives it the same features as JetBrains PyCharm. (No need to respond. Just figure I should clarify.)

gbtorrance commented 1 year ago

Hi @eprbell & @macanudo527. I want to apologize. This has been on my mind for a while and I need to face facts. I am, unfortunately, not going to have the necessary time to work on the Binance US plugin. I really intended to, but realistically it's just not going to happen. Too much else going on, and that will likely be the case for the foreseeable future. I'm sorry for volunteering and then "un-volunteering". That said, I really appreciate the tool and will continue to use it. Thank you for making it available! All the best!

macanudo527 commented 1 year ago

Thanks @gbtorrance for letting us know and not ghosting us. If you do find the time, please let us know.

gbtorrance commented 1 year ago

@macanudo527 Will do.

GanjaRick commented 4 months ago

sorry to intrude here. I could really use a Binance.us plugin. I have minimal programming experience, but I am a fast learner. Is there any way I can help in this development?

eprbell commented 3 months ago

@GanjaRick, thanks for volunteering! I think the best way to start is to use the Binance.com plugin as boilerplate: https://github.com/eprbell/dali-rp2/blob/main/src/dali/plugin/input/rest/binance_com.py. Also check the plugin development docs: https://github.com/eprbell/dali-rp2/blob/main/README.dev.md#plugin-development

CC: @macanudo527 who worked on the Binance.com plugin and its CCXT-based abstract superclass.

macanudo527 commented 3 months ago

@GanjaRick, in theory the Binance.US plugin should just be a stripped down version of the Binance plugin, but since neither of us trade on that platform we can't test or confirm that. Let me know if you have a question.

tpaynter4 commented 3 months ago

I'm also trying to get the binance plugin to work with binance.us. I changed the ccxt exchange class from binsnce to binanceus, and tried to remove all functionality exclusive to binance.com, but when I run it, I get the following error:

  File "/home/thomas/rp2/dev/dali-rp2/src/dali/dali_main.py", line 168, in _dali_main_internal
    result_list = pool.map(_input_plugin_helper, input_plugin_args_list)
  File "/usr/lib/python3.10/multiprocessing/pool.py", line 367, in map
    return self._map_async(func, iterable, mapstar, chunksize).get()
  File "/usr/lib/python3.10/multiprocessing/pool.py", line 774, in get
    raise self._value
  File "/usr/lib/python3.10/multiprocessing/pool.py", line 125, in worker
    result = (True, func(*args, **kwds))
  File "/usr/lib/python3.10/multiprocessing/pool.py", line 48, in mapstar
    return list(map(*args))
  File "/home/thomas/rp2/dev/dali-rp2/src/dali/dali_main.py", line 226, in _input_plugin_helper
    plugin_transactions = input_plugin.load(country)
  File "/home/thomas/rp2/dev/dali-rp2/src/dali/abstract_ccxt_input_plugin.py", line 195, in load
    self._process_withdrawals(intra_transactions)
  File "/home/thomas/rp2/dev/dali-rp2/src/dali/abstract_ccxt_input_plugin.py", line 432, in _process_withdrawals
    processing_result_list = pool.map(self._process_transfer, withdrawals)
  File "/usr/lib/python3.10/multiprocessing/pool.py", line 367, in map
    return self._map_async(func, iterable, mapstar, chunksize).get()
  File "/usr/lib/python3.10/multiprocessing/pool.py", line 774, in get
    raise self._value
  File "/usr/lib/python3.10/multiprocessing/pool.py", line 125, in worker
    result = (True, func(*args, **kwds))
  File "/usr/lib/python3.10/multiprocessing/pool.py", line 48, in mapstar
    return list(map(*args))
  File "/home/thomas/rp2/dev/dali-rp2/src/dali/abstract_ccxt_input_plugin.py", line 697, in _process_transfer
    IntraTransaction(
  File "/home/thomas/rp2/dev/dali-rp2/src/dali/intra_transaction.py", line 50, in __init__
    super().__init__(
  File "/home/thomas/rp2/dev/dali-rp2/src/dali/abstract_transaction.py", line 115, in __init__
    self.__unique_id: str = self._validate_string_field(Keyword.UNIQUE_ID.value, unique_id, raw_data, disallow_empty=True, disallow_unknown=False)
  File "/home/thomas/rp2/dev/dali-rp2/src/dali/abstract_transaction.py", line 53, in _validate_string_field
    raise RP2RuntimeError(f"Internal error: {name} is not a string: {value}\n{raw_data}")
rp2.rp2_error.RP2RuntimeError: Internal error: unique_id is not a string: None

With a json response with transaction information at the bottom. Do you have any idea why the unique_id would not be populated?

macanudo527 commented 3 months ago

With a json response with transaction information at the bottom. Do you have any idea why the unique_id would not be populated?

According to this line:

File "/home/thomas/rp2/dev/dali-rp2/src/dali/abstract_ccxt_input_plugin.py", line 697, in _process_transfer
    IntraTransaction(

Means a transfer is missing it's unique_id. I would add some logger lines there and check it is outputing a transfer id correctly.

tpaynter4 commented 3 months ago

Is there a way for me to use a pair converter without the need for an exchangerate.host access key?

macanudo527 commented 3 months ago

It should only be required if you are dealing with a currency other than USD.

There is actually a few PRs #218 and #225 we are wading through that should do away with the need for an exchangerate.host key.

I just need time to get to them.

tpaynter4 commented 3 months ago

I think I found out why it says I need a pair converter. I've only traded in USD, but according to this, there are some of my transactions using USD, and others using USD4. I think it is trying to convert USD4 -> USD, which makes no sense because USD4 is not an actual currency. USD4 and USD should be linked 1:1. Is there a way to do that in the plugin?

macanudo527 commented 3 months ago

You need to program that into the plugin. Have the plugin convert USD4 transactions to USD transactions when it pulls them.

tpaynter4 commented 3 months ago

I have Implemented a load() method within the binance.us plugin, but I cannot modify any of the attributes in the transactions. Is it possible to modify the attributes? Or should I be doing it in a different way?

eprbell commented 3 months ago

In DALI and RP2 most data structures are immutable as a design choice (this helps remove entire categories of bugs). So the only way to assign values to transaction attributes is at initialization time.

See https://github.com/eprbell/dali-rp2/blob/main/README.dev.md#design-guidelines for more details. Hope this helps.

tpaynter4 commented 3 months ago

Thanks for the info. I overrode the process_buy, process_sell, and process_transfer methods in the plugin to convert USD4 -> USD and that seems to solve the the problem. Is this in any way correct?

macanudo527 commented 3 months ago

Thanks for the info. I overrode the process_buy, process_sell, and process_transfer methods in the plugin to convert USD4 -> USD and that seems to solve the the problem. Is this in any way correct?

That sounds correct, but you'll need to submit a PR for us to say for sure. You can submit a PR as a draft while you work on it. That way we know you are still working on it and then if you have a specific question we can help you better with it.

eprbell / dali-rp2

Implement Binance.US REST API Data Loader Plugin #4