-
Notifications
You must be signed in to change notification settings - Fork 5
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
✨ feature calculate_pnl via rotki #626
Conversation
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 @mraniki - I've reviewed your changes and found some issues that need to be addressed.
Blocking issues:
- Hard-coded endpoint URL should be avoided in configuration files. (link)
- Hard-coded endpoint URL should be avoided, even in commented code. (link)
- Avoid including hard-coded URLs in the codebase. (link)
- Hard-coded URLs in configuration files are a security risk. (link)
- Hard-coded URLs should be replaced with configurable options. (link)
Here's what I looked at during the review
- 🟡 General issues: 3 issues found
- 🔴 Security: 5 blocking issues
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Docstrings: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
if self.rotki_report_endpoint is None: | ||
return 0 |
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.
suggestion (code_clarification): Consider raising an exception or logging when 'rotki_report_endpoint' is None.
Returning 0 might mask the issue that 'rotki_report_endpoint' is not set, which could lead to harder-to-debug problems later. Raising an exception or at least logging a warning could provide clearer feedback on the configuration issue.
if self.rotki_report_endpoint is None: | |
return 0 | |
if self.rotki_report_endpoint is None: | |
raise ValueError("rotki_report_endpoint is not set.") |
if response.status != 200: | ||
logger.error(f"Received non-200 status code: {response.status}") | ||
return 0 |
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.
suggestion (code_refinement): Consider handling different HTTP status codes with specific actions or logs.
Different non-200 status codes can indicate various issues (e.g., 404 not found, 500 server error). Handling these explicitly could improve debugging and user feedback.
if response.status != 200: | |
logger.error(f"Received non-200 status code: {response.status}") | |
return 0 | |
if response.status == 404: | |
logger.error("Resource not found: 404 error") | |
elif response.status == 500: | |
logger.error("Server error: 500 error") | |
else: | |
logger.error(f"Unexpected status code received: {response.status}") | |
return 0 |
try: | ||
free_amount = float(amounts.get("free", "0")) |
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.
suggestion (edge_case_not_handled): Validate 'free' field presence before conversion to float.
Directly converting the 'free' field to float without checking its presence might lead to unexpected errors if the field is missing. Consider validating its presence first.
try: | |
free_amount = float(amounts.get("free", "0")) | |
free_amount_str = amounts.get("free") | |
if free_amount_str is not None: | |
free_amount = float(free_amount_str) | |
else: | |
free_amount = 0.0 |
@@ -59,6 +60,8 @@ block_explorer_api = "" # your block explorer api | |||
mapping = [ # mapping to use to change instrument per exchange | |||
{ id = "BTC", alt = "WBTC" ,enable = true }, | |||
] | |||
rotki_report_endpoint= "http://localhost:5042/api/v1/reports/1" |
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.
🚨 issue (security): Hard-coded endpoint URL should be avoided in configuration files.
Consider using environment variables or configuration management systems to handle sensitive or environment-specific data securely.
@@ -83,6 +86,7 @@ mapping = [ # mapping to use to change instrument per exchange | |||
# mapping = [ | |||
# { id = "BTC", alt = "WBTC" }, | |||
# ] | |||
# rotki_report_endpoint= "http://localhost:5042/api/1/pnl" |
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.
🚨 issue (security): Hard-coded endpoint URL should be avoided, even in commented code.
This can lead to accidental exposure of endpoints when uncommenting or refactoring code. Use more secure methods to manage such data.
@@ -83,6 +86,7 @@ mapping = [ # mapping to use to change instrument per exchange | |||
# mapping = [ | |||
# { id = "BTC", alt = "WBTC" }, | |||
# ] | |||
# rotki_report_endpoint= "http://localhost:5042/api/1/pnl" |
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.
🚨 issue (security): Avoid including hard-coded URLs in the codebase.
Even if commented out, these can be mistakenly used or exposed. Use configuration management for such values.
@@ -83,6 +86,7 @@ mapping = [ # mapping to use to change instrument per exchange | |||
# mapping = [ | |||
# { id = "BTC", alt = "WBTC" }, | |||
# ] | |||
# rotki_report_endpoint= "http://localhost:5042/api/1/pnl" |
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.
🚨 issue (security): Hard-coded URLs in configuration files are a security risk.
It's recommended to use environment-specific configuration files or environment variables to manage such data.
@@ -83,6 +86,7 @@ mapping = [ # mapping to use to change instrument per exchange | |||
# mapping = [ | |||
# { id = "BTC", alt = "WBTC" }, | |||
# ] | |||
# rotki_report_endpoint= "http://localhost:5042/api/1/pnl" |
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.
🚨 issue (security): Hard-coded URLs should be replaced with configurable options.
This helps in maintaining security and flexibility across different environments.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #626 +/- ##
==========================================
- Coverage 80.16% 77.47% -2.70%
==========================================
Files 11 11
Lines 595 617 +22
==========================================
+ Hits 477 478 +1
- Misses 118 139 +21 ☔ View full report in Codecov by Sentry. |
No description provided.