Binance.com (not US) plugin #45
Conversation
|
Great! I'll review in the next few days. Looks like you need to add ccxt as a dependency in setup.cfg under |
There was a problem hiding this comment.
This is fantastic: a really well-written plugin! Thanks for coding this.
DaLI plugins provide the highest impact on the RP2-DaLI tax suite and I hope more coders submit new plugins.
I have a few suggestions, comments and questions, but this is a great first implementation.
Additional comments that didn't fit anywhere else:
- to pass the tests on Github, you need to add ccxt as a dependency in setup.cfg under
install_requires. - as for missing type hints in ccxt, the way to fix that is to add a stub file for ccxt (see examples in DaLI): add to it only the functions/data structures you're using, not everything.
|
I'm working through everything needed for mypy and I was wondering if I should create TypedDict for the JSON responses I get from CCXT and Binance. Or should I just mark all these as Any? I think TypedDict is the way to go, but didn't see any in the example code. |
I had a similar issue in RP2: the configuration is a relatively complex JSON file. Over there I just used Any: https://github.com/eprbell/rp2/blob/main/src/rp2/configuration.py#L98. This is to say: don't get blocked on it. It's fine to do things incrementally: you can do a first version with Any and we can add typing information later as a separate effort (same goes for my code above). |
|
BTW, I'm making progress on the new plugin infrastructure for pair conversion. Hopefully I'll have something ready soon. Will update here. |
|
I'm basically done with the new pair conversion plugin infrastructure. One more review and I'll push it. |
|
I pushed the pair conversion changes: please update your tree. Here's a quick overview:
So to summarize, you have to:
The rest should work automatically, but since this is new code, there may be bugs. No tests fail so far (including tests with my personal data, which are not uploaded), but there are probably things to iron out: we can iterate as needed. If you have any questions/comments let me know. |
|
The transaction resolver will eventually need to be able to output values in other currencies as well, not just USD. I think you mentioned something about a country plugin? This is not critical right now. I can finish up the Binance.com plugin and a few other plugins, but I'll eventually need totals in JPY. And, of course, any international user would need it in their native fiat. I know that is really expanding the original goal of RP2, but it would be really useful for me. I'll get started on using |
Yep, totally. RP2 already has experimental country plugin infrastructure (so far it models native fiat and little more but that should be enough for what you are describing): DaLI should just reuse that infra. This way the native fiat would come from the plugin without being hardcoded as USD. I'll work on that. |
|
Just expanding on the last comment: do you need US crypto taxes denominated in JPY or Japanese crypto taxes (meaning taxes generated following Japanese laws) denominated in JPY? |
|
I'll need all the transactions to have estimated JPY totals to file taxes in Japan. Total tax is then calculated by the total average method or the more complicated, but accurate moving average method. There is no specific id accounting in Japan. And everything is basically short-term capital gains (actually, it counts as just regular income). So, basically, that's my part-time job over this year. Building a few more exchange plugins then doing the accounting plugin for Japan. :) |
|
For fiat deposit/withdrawl, should there be an amount for |
Currently fiat deposits are treated as if the fiat was a crypto (a crypto that is guaranteed to always priced at $1): that's why the crypto fields are used. This way it works with the rest of RP2 infrastructure without having to write special code. The Coinbase itself uses the same value in amount and native_amount for fiat deposits. So to answer your question: yes, use the same approach on Binance (and elsewhere). |
Sounds great! |
|
Sorry, I had a bit of time and started reading your new commits and commenting on them. However I realized I should check with you first: do you want me to just start reviews whenever or should I wait for you to give me the go ahead? Thanks! |
|
I just added support for country plugins in DaLI (see latest push). This and the new pair converter plugin architecture should allow us to support any native fiat (e.g. JPY, EUR, etc.), as we discussed. Let me know if you see any problems or if you have any questions. |
|
I'm okay with you giving me feedback when you can. My schedule is irratic at times. I'll just mark what I did as resolved and keep moving through it. |
| def _process_transfer(self, transaction: Any, intra_transaction_list: List[IntraTransaction]) -> None: | ||
| if transaction[_STATUS] == "failed": | ||
| self.__logger.info("Skipping failed transfer %s", json.dumps(transaction)) | ||
| return None |
There was a problem hiding this comment.
I think the pylint error is saying:
- either you remove this
return None - or you add a
return Noneoutside the if statement at the end of the function (next line but one less indentation)
Since _process_transfer() has a None return type I would go for the first option.
There was a problem hiding this comment.
BTW, it's very possible pylint got updated and now it's catching more things.
There was a problem hiding this comment.
I see what happened now. I needed to pull the merge for the dividend fixes before running pylint. The static analysis error was introduced with the merge. It should be good to go now.
|
Also, let's wait for the Binance Supplemental CSV and documents to be ready, and then we can roll them all in. I'm fully back from vacay so should be able to make steady progress again. |
|
Looks like internal order IDs were used in some cases, here's a PR to fix that: macanudo527#6 |
|
I just noticed that there is actually a cleaner pagination mechanism for pulling data, but I'd like to implement it when I abstract this out to a CCXT abstract plugin. After I merge @iloveitaly's contribution, we can roll this through and I'll begin work on the abstraction. I'm drafting a document that attempts to outline how the architecture is going to work out. I'll try to post it to #22 . |
|
Looking forward to all that good stuff! |
setup.cfg
Outdated
| @@ -39,7 +39,7 @@ install_requires = | |||
| pytz>=2021.3 | |||
| requests>=2.26.0 | |||
| ccxt>=1.81.30 | |||
| rp2>=1.0.8 | |||
| rp2>=1.0.9 | |||
There was a problem hiding this comment.
Leave an empty line after last dependency. Also can you move ccxt up so that it's in alphabetical order?
| super().__init__(account_holder, native_fiat) | ||
| self.__logger: logging.Logger = create_logger(f"{self.__BINANCE_COM}/{self.account_holder}") | ||
| self.__cache_key: str = f"{self.__BINANCE_COM.lower()}-{account_holder}" | ||
| self.client: binance = binance( |
There was a problem hiding this comment.
All private instance variables (client, username, markets, algos, start_time and start_time_ms) should be prepended with __
|
Should I just ignore those pylint errors? I need to access the protected member to perform the tests. This is the code right? |
|
I'm confused about the unit tests failure. It looks like somewhere the transaction type is being capitalized. Was this introduced in a new update? It looks like you introduced forced capitalization for the transaction type. |
Yes, I did that to make sure generated files are consistent, otherwise the transaction type was generated as entered by the user. I think the only change needed are in the test introduced in the PR: whenever the test checks the transaction type, it can use either |
|
I merged the other 3 PRs: there are a couple of new conflicts here. After fixing them we'll merge this as well and then we're done! Well we're not really done, there's plenty more to do, but you know what I mean :-D |
|
Also it looks like after merging the other 3 the main branch has a couple of test failures related to transaction type: https://github.com/eprbell/dali-rp2/runs/8025113548?check_suite_focus=true |
Fixed the test failures. |
|
@macanudo527 , congrats on getting all this code in! Onwards! |
|
Miracles are possible I guess! I thought this would be simple when I started. |
|
No miracles, just good work 🙂 |
This is my first draft of the Binance.com Plugin. All of the main features are present. I'm having a lot of trouble with mypy since CCXT is not stubbed at all. I'm wondering how to proceed as I don't have much experience with mypy and type hints.