-
-
Notifications
You must be signed in to change notification settings - Fork 513
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
Add OKX exchange integration #5328
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #5328 +/- ##
===========================================
+ Coverage 70.96% 71.01% +0.05%
===========================================
Files 980 983 +3
Lines 73490 74062 +572
Branches 9039 9170 +131
===========================================
+ Hits 52150 52593 +443
- Misses 19737 19866 +129
Partials 1603 1603
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution. I checked the backend and someone else will check the frontend. Can you also add an entry in the docs/changelog
file saying that support for OKX was added? Also wrote a question in discord (priv) about the tests
Also @anukul linting is failing. Check it before pushing |
Hey @anukul I am not reviewing yet but just a note. We don't merge develop back to the branch. Instead we always rebase feature branches on top of develop. Also make commits that do one big thing, but not so small as 1 commit per lint error or suggestion aplication. |
The frontend part is okay. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @anukul, left some minor comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @anukul thanks a lot for such a nice and big PR. Left some comments/questions.
rotkehlchen/exchanges/okx.py
Outdated
else: | ||
raise AssertionError(f'Unexpected {self.name} endpoint type: {endpoint}') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should remove the else branch. Turn the last elif
to else
. And at the start of the else code branch add an `assert endpoint == 'withdrawals', 'only case left, should be withdrawals'
Why?
- We use type checking for a reason
- assertions are fine for debugging and in production since we produce optimized code they are removed. But
raise AssertionError()
is not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some more things that need changing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two more small comments
rotkehlchen/exchanges/okx.py
Outdated
data = self._api_query_list(endpoint='balance') | ||
try: | ||
currencies_data = data[0]['details'] | ||
except (IndexError, KeyError, TypeError) as e: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
continuing the TypeError
discussion do you see the problem here?
Such a TypeError
would read: "TypeError: string indices must be integers"
Which would translate to: "OKX balance API request failed due to unexpected response TypeError: string indices must be integers".
We won't have any information as to what went wrong or which attribute it is that failed for us.
I think it's much better if you check that data[0]
is a dict
here and if not raise a RemoteError
saying that the "details"
key of the OKX balances response was not a dict as expected, and even include the raw response so it can be studied in the logs.
What we usually do with this is:
- Put as few info as needed in the
RemoteError
as this is usually propagated high up, caught by the API and shown to the user. - At the same time log an error, with the raw response
data
so that when we study the logs of the user we can see both the error and the raw response and figure out what happened.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You missed to do (2).
Logging the error containing the raw response so we, the developers, can debug it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a small comment you forgot, but can also be added in another PR.
Other than that looks good. So will merge.
The single failing test is timeout due to ethereum remote node query
Congrats, your important contribution to this open-source project has earned you a GitPOAP! GitPOAP: 2023 rotki Contributor: Head to gitpoap.io & connect your GitHub account to mint! Learn more about GitPOAPs here. |
Added integration for asset balances, trades and deposits & withdrawals
Screen.Recording.2022-12-31.at.6.50.51.AM.mov
Note: