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

user.get_trade_volume() says it supports multiple currencies as a list, but it does not seem to. #115

Closed
andyDoucette opened this issue Jun 10, 2023 · 8 comments
Labels
Bug Something isn't working

Comments

@andyDoucette
Copy link

Ok, a couple issues here.

  1. V1.3.0 of the documentation doesn't list user.get_trade_volume() in the docs, but it's in 1.2.0 and the function is in 1.3.0's code.
  2. The docstring in 1.3.0 for user.get_trade_volume() says: the acceptable pair types include a list of strings: pair: Optional[Union[str, List[str]]] = None,

Yet, check out the following pdb session:

'1INCHEUR'

(Pdb) pprint(user.get_trade_volume(all_currency_pairs[0]))
{'currency': 'ZUSD',
 'fees': {'1INCHEUR': {'fee': '0.2000',
                       'maxfee': '0.2600',
                       'minfee': '0.1000',
                       'nextfee': '0.1800',
                       'nextvolume': '500000.0000',
                       'tiervolume': '250000.0000'}},
 'fees_maker': {'1INCHEUR': {'fee': '0.1000',
                             'maxfee': '0.1600',
                             'minfee': '0.0000',
                             'nextfee': '0.0800',
                             'nextvolume': '500000.0000',
                             'tiervolume': '250000.0000'}},
 'volume': '282801.4725'}
 
(Pdb) all_currency_pairs[0:1]
['1INCHEUR']

(Pdb) pprint(user.get_trade_volume(all_currency_pairs[0:1]))
*** kraken.exceptions.KrakenException.KrakenInvalidAPIKeyError: An invalid API-Key header was supplied.
Details: {'error': ['EAPI:Invalid key']}

It seems like this is not supported yet.

I'm trying to get the trade_volume for all currency pairs at once so I do not have to use so many API requests (which affect my rate limit) to get them.

@andyDoucette andyDoucette added the Bug Something isn't working label Jun 10, 2023
@andyDoucette
Copy link
Author

andyDoucette commented Jun 10, 2023

Note: pprint(user.get_trade_volume(",".join(all_currency_pairs[0:5]))) DOES work, so maybe this could be added to any functions that claim to accept a list of strings for pairs but don't yet?

if isinstance(pair, list): 
    pair=','.join(pair)

@btschwertfeger
Copy link
Owner

I'm not sure which version you use, but in v1.3.0 there is no problem when passing a list to the pair parameter, since this decorator ensures that all passed lists will be transformed into a string like "elem1,elem2,elem3", see kraken/spot/user/init.py#L766.

I will check out why some functions are hidden in the documentation. Thanks for that note.

@btschwertfeger
Copy link
Owner

I found out that the documentation for functions that use a custom decorator are missing, since decorators modify the signature of the underlaying function which will confuse the sphinx doc building tool. This will be fixed in #106.

@andyDoucette
Copy link
Author

I am using 1.3.0 now, straight from this repo. Can you switch to 1.3.0 and try print(user.get_trade_volume(['1INCHEUR']))? That should settle things one way or another.

@btschwertfeger
Copy link
Owner

btschwertfeger commented Jun 10, 2023

pair is a keyword argument - try the following:

print(User("KEY", "SECRET").get_trade_volume(pair=["1INCHEUR"]))

It results in (using v1.3.0):

{
    "currency": "ZUSD",
    "volume": "654.3318",
    "fees": {
        "1INCHEUR": {
            "fee": "0.2600",
            "minfee": "0.1000",
            "maxfee": "0.2600",
            "nextfee": "0.2400",
            "tiervolume": "0.0000",
            "nextvolume": "50000.0000"
        }
    },
    "fees_maker": {
        "1INCHEUR": {
            "fee": "0.1600",
            "minfee": "0.0000",
            "maxfee": "0.1600",
            "nextfee": "0.1400",
            "tiervolume": "0.0000",
            "nextvolume": "50000.0000"
        }
    }
}

@andyDoucette
Copy link
Author

Thank you for sharing that! Looks like that's another work-around. But here's the thing. Even keyword arguments can usually be used as positional arguments if you get them in the right order.

Since user.get_trade_volume("1INCHEUR") works, something is wrong if user.get_trade_volume(["1INCHEUR"]) doesn't work. There may be multiple work-arounds, (some of which I'm using), but I hope something can be fixed still so that the next person isn't confused. I don't think it's a documentation fix. I think maybe it would be better if @ensure_string("pair") became @ensure_string([0], ["pair"]) and ensured both arg[0] and kwarg['pair'] were strings. That way, we can keep the expected behavior of being able to choose if you label each argument as a keyword or use them positionally, just like we usually can.

@andyDoucette
Copy link
Author

If through some inspection thing, @ensure_string could accept just the name and it would look up the position, that would be even less work for the maintainers. But from a user perspective, having the desired behavior when the function is used either way is what matters. Thanks again for your great work on this library! It's just what I needed.

@btschwertfeger
Copy link
Owner

I can understand if a solution like the one you propose is considered desirable. Python does allow regular arguments to be addressed both by position and by keyword.

It may be my personal bias, but for compatibility and ease of refactoring, it is always best to use arguments by name. This way, for example, unwanted behavior can be avoided if the order of the arguments changes or new ones are added.

Within the python-kraken-sdk, the names of the arguments are always specified as well - this does not mean, of course, that the users have to adapt this in their code, but when deviating from the given norm, individual solutions must be found.

PEP 3102 introduces keyword-only arguments. This would be a consideration to avoid the behavior you found here. I will try that out.

Thank you for your contribution!

@btschwertfeger btschwertfeger added Wontfix This will not be worked on and removed Wontfix This will not be worked on labels Jun 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants