search

eprbell / dali-rp2

DaLI (Data Loader Interface) is a data loader and input generator for RP2 (https://pypi.org/project/rp2), the privacy-focused, free, open-source cryptocurrency tax calculator: DaLI removes the need to manually prepare RP2 input files. Just like RP2, DaLI is also free, open-source and it prioritizes user privacy.
https://pypi.org/project/dali-rp2/
Apache License 2.0
66 stars 42 forks source link

Fix Errors caused by Updates #262

Closed macanudo527 closed 1 month ago

macanudo527 commented 1 month ago

What?

Pylint wants to punish us for too many positional arguments. I don't think we need this linter.

I also ran Black to add/delete some spaces.

I also monkey patched historic_crypto to use the new Coinbase interface.

eprbell commented 1 month ago

Test results are strange: some platforms fully succeeded, while others still break.

macanudo527 commented 1 month ago

Test results are strange: some platforms fully succeeded, while others still break.

Yeah, I'm not sure what is going on. I was going to switch python version locally and see.

macanudo527 commented 1 month ago

@eprbell I guess we just had to jiggle the lock. I think these errors are being caused by the call to coinbase. I'll try to put a VCR tape together so they aren't flakey anymore.

EDIT: Actually, this is much more complex since the test runs the program from the command line. Is there way to run it directly in Python?

eprbell commented 1 month ago

@eprbell I guess we just had to jiggle the lock. I think these errors are being caused by the call to coinbase. I'll try to put a VCR tape together so they aren't flakey anymore.

EDIT: Actually, this is much more complex since the test runs the program from the command line. Is there way to run it directly in Python?

I think we have to understand why the tests are failing only occasionally (I'm not sure which lock you're referring to: can you clarify?). If the problem is indeed Coinbase one quick workaround would be to pass to the tests a config file that uses one of the CCXT-based converters instead of the CB-based one. But the question remains if the Coinbase pair converter is still reliable or not (if not we could try to fix it or remove it altogether).

macanudo527 commented 1 month ago

@eprbell jiggle the lock -> keep trying it until it works basically. If you keep re-running the failed tests, they finally go through.

The problem we have is if the historic_crypto plugin fails (or for the matter any pair converter plugin fails) the resulting error message is ambiguous and hard to troubleshoot because dali-rp2 simply exits with a 1 exit code. Because of that, the test ini/ods files aren't generated and that results in the tests failing.

If you can trigger the generation of the files in pure Python, it would be much more easier to troubleshoot.

I personally feel though that none of our tests should rely on an external API being up and available. It just complicates things that don't need to be complicated. If we want to test if the APIs are still available, these tests should be isolated and done with curl or something similar. It just makes things more performant and reliable. I worry about new developers encountering this error and having no idea how to fix it or what is causing it.

For this particular test, we just want to know that the report creation is working. We aren't (and shouldn't be) testing the API call.

Thinking on this for a bit, I think what is happening here is that the new Coinbase API is rate limiting IP addresses. When the test runs on Github, 12 or so tests are run simultaneously. All of them hit the Coinbase API at approximately the same time and trigger a rate limit. We can increase the re-tries of course, but it just adds more time to the test. It may also end up triggering a ban in the future. I really don't think it is a good idea.

Because of that, even if we use the CCXT Coinbase plugin, we will still have the same issue.

If we can't fake the response. We can build a dummy pair converter that generates the same price or a simple CSV pair converter that uses CSV files to generate the prices. The CSV plugin may be more useful for users. Either way, I really don't think we should rely on a connection.

eprbell commented 1 month ago

@eprbell jiggle the lock -> keep trying it until it works basically.

Ah, I didn't get the metaphor: I thought you were referring to some mutex 😄.

If you can trigger the generation of the files in pure Python, it would be much more easier to troubleshoot.

Those tests are meant to be end to end, so they invoke the user-facing command: that's why they need a separate process. However I take your point that in case of failure the error is unhelpful. What these tests should really do is capture the output of the subprocess and print it out in case of failure. We should keep this improvement on the radar.

I personally feel though that none of our tests should rely on an external API being up and available.

I fully agree: unit tests should run in an encapsulated mode (any network calls should be mocked).

If we can't fake the response. We can build a dummy pair converter that generates the same price or a simple CSV pair converter that uses CSV files to generate the prices.

I'm unsure where in unit tests' execution path the price converter is needed: unit tests should all specify spot price, so that price converter shouldn't be invoked. We either have some tests that don't specify spot price or we're calling it unnecessarily. As a quick workaround we can do what you suggest: a test-specific, network less price converter. Then we can investigate the rest as time permits.

Good investigation work!

macanudo527 commented 1 month ago

The price converter is invoked because of the -s flag in the test. I suppose we can just resolve this by putting the actual price of the asset in the CSV files that are read in and removing the -s flag. Let me try that.

macanudo527 commented 1 month ago

@eprbell It looks like that eliminated the flakey tests. I had to convert the two Trezor wallets to manual input. The ODS files were edited to eliminate the note about where the price was pulled, since it is no longer getting pulled. Also, there was a small accuracy error that was caused by the old method.

This now doesn't require the API call and makes the test a little faster. I combine the trezor wallets into one CSV. I didn't see a reason to have them separated for the manual plugin.

eprbell commented 1 month ago

@eprbell It looks like that eliminated the flakey tests. I had to convert the two Trezor wallets to manual input. The ODS files were edited to eliminate the note about where the price was pulled, since it is no longer getting pulled. Also, there was a small accuracy error that was caused by the old method.

This now doesn't require the API call and makes the test a little faster. I combine the trezor wallets into one CSV. I didn't see a reason to have them separated for the manual plugin.

Sounds good: I am currently traveling, but I should be able to review this in the weekend.

macanudo527 commented 1 month ago

@eprbell According to #263, It seems like pre-2023 data is not available via the new API endpoint. It seems like we will have to eventually implement a coingecko or coinmarketcap backup pair converter eventually. It is just incredibly difficult to keep up with these APIs. I'd like a backup of some kind.

macanudo527 commented 1 month ago

@eprbell Is there something else you need for this?