Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reorganize pair converter code #218

Merged
merged 7 commits into from
Oct 22, 2024
Merged

Reorganize pair converter code #218

merged 7 commits into from
Oct 22, 2024

Conversation

eprbell
Copy link
Owner

@eprbell eprbell commented Dec 23, 2023

This is preparatory work for my experiments writing a CCXT-based version of the Coinbase plugin (which has been broken by Coinbase, as described in #216).

This PR moves graph/optimization code from abstract_pair_converter_plugin.py to abstract_ccxt_input_plugin.py (no functional change). This is because:

  • the code was interfering with simple plugins like Historic-Crypto, which was printing the following message: "No Fiat Access Key. Fiat pricing will NOT be available. To enable fiat pricing, an access key from exchangerate.host is required.". This was confusing because Historic-Crypto has no dependency on exchangerate.host.
  • it's cleaner to have a bare bone top class (abstract_pair_converter_plugin.py) and a graph/optimization-aware subclass (abstract_ccxt_input_plugin.py). This way, simple plugins don't have to inherit the extra complexity of optimization.

This PR also contains minor changes to exception handling: ExchangeError doesn't need to retry.

Moved graph/optimization code from abstract_pair_converter_plugin.py to abstract_ccxt_input_plugin.py. This is because:
- the code was interfering with simple plugins like Historic-Crypto, which was printing the following message: "No Fiat Access Key. Fiat pricing will NOT be available. To enable fiat pricing, an access key from exchangerate.host is required.". This was confusing because Historic-Crypto has no dependency on exchangerate.host.
- it's cleaner to have a bare bone top class (abstract_pair_converter_plugin.py) and a graph/optimization-aware subclass (abstract_ccxt_input_plugin.py). This way, simple plugins don't have to inherit the extra complexity of optimization.
@eprbell eprbell requested a review from macanudo527 December 23, 2023 18:32
@macanudo527
Copy link
Collaborator

@eprbell Sorry, I did see this. I just need to patch code to see if it works. (Right now I can't get accurate prices because things are out of date).

I had to update the fiat pricing and kraken pricing. I think I have it now. I'm going to do some cleaning and submit it. I might need to skip writing tests for now, though. Trying to get it all done before the tax deadline.

@eprbell
Copy link
Owner Author

eprbell commented Jan 11, 2024

@eprbell Sorry, I did see this. I just need to patch code to see if it works. (Right now I can't get accurate prices because things are out of date).

I had to update the fiat pricing and kraken pricing. I think I have it now. I'm going to do some cleaning and submit it. I might need to skip writing tests for now, though. Trying to get it all done before the tax deadline.

No worries! Thanks for reviewing my PR: it's good to turn things around :-)

@macanudo527
Copy link
Collaborator

@eprbell did you mean to create a separate cache for CCXT and the abstract pair plugin? in CCXT there is self._cache, but the abstract has self.__cache

@eprbell
Copy link
Owner Author

eprbell commented May 13, 2024

@eprbell did you mean to create a separate cache for CCXT and the abstract pair plugin? in CCXT there is self._cache, but the abstract has self.__cache

I think the _cache property should probably be in the top abstract class (i.e. abstract_pair_converter_plugin.py) and be inherited by all subclasses including CCXT-based ones.

@macanudo527
Copy link
Collaborator

I think the _cache property should probably be in the top abstract class (i.e. abstract_pair_converter_plugin.py) and be inherited by all subclasses including CCXT-based ones.

OK, that's what I was thinking. I'll fix my code in #225 to reflect that.

@macanudo527
Copy link
Collaborator

@eprbell This should finally be ready to go. I had to patch the code a little to fix mypy issues 93018c4 Let me know your thoughts.

@eprbell
Copy link
Owner Author

eprbell commented Oct 22, 2024

@eprbell This should finally be ready to go. I had to patch the code a little to fix mypy issues 93018c4 Let me know your thoughts.

This looks good to me! I can't approve it though, since it's my PR.

@macanudo527
Copy link
Collaborator

This looks good to me! I can't approve it though, since it's my PR.

Right. Lol

Copy link
Collaborator

@macanudo527 macanudo527 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@macanudo527 macanudo527 merged commit a40888c into main Oct 22, 2024
36 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants