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

Catch all exceptions in wallet API #220

Merged
merged 4 commits into from May 22, 2023
Merged

Conversation

sihamon
Copy link
Collaborator

@sihamon sihamon commented May 15, 2023

Move HTTPException from router to error handler and catch all exceptions in error handler.
Closes #201 .

@codecov
Copy link

codecov bot commented May 18, 2023

Codecov Report

Patch coverage: 69.23% and project coverage change: +0.34 🎉

Comparison is base (08ceeda) 56.06% compared to head (ed64b4c) 56.41%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #220      +/-   ##
==========================================
+ Coverage   56.06%   56.41%   +0.34%     
==========================================
  Files          42       42              
  Lines        3328     3311      -17     
==========================================
+ Hits         1866     1868       +2     
+ Misses       1462     1443      -19     
Impacted Files Coverage Δ
cashu/wallet/api/router.py 76.08% <52.94%> (+6.23%) ⬆️
cashu/wallet/api/app.py 100.00% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Comment on lines 12 to 15
except Exception as exc:
raise HTTPException(
status_code=status.HTTP_400_BAD_REQUEST, detail=str(exc)
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would suggest

Suggested change
except Exception as exc:
raise HTTPException(
status_code=status.HTTP_400_BAD_REQUEST, detail=str(exc)
)
except (AssertionError, KeyboardInterrupt):
# These should never really be caught, 1st is "something real bad" and 2nd is a PITA if something gets stuck
raise
except Exception as exc:
if isinstance(exc, HTTPException):
# Allows optionally setting a custom `status_code` if desired, but otherwise defaulting to below for convenience
raise
raise HTTPException(
status_code=status.HTTP_400_BAD_REQUEST, detail=str(exc)
)

Copy link
Collaborator

Choose a reason for hiding this comment

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

str(exc) probably OK for now, but should be a little cautious about echoing the contents of whatever exception gets raised back to some random person on the internet, it can leak info about the system and make it easier to find vulnerabilities (minor example #228 where the API responses reveal that this mint was running Postgres and CLN specifically)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Would still recommend passing-up a HTTPException as-is if one is encountered, but could easilly be added later if someone needs this

if isinstance(exc, HTTPException):
    # Allows optionally setting a custom `status_code` if desired, but otherwise defaulting to below for convenience
    raise

On the AssertionError, seems assert has been used in a few places so can pass on that suggestion (though IMHO even though assert is shorter, it is best reserved as a thing to actually cause a crash, like if somehow 1 != 1)

cashu/wallet/api/router.py Outdated Show resolved Hide resolved
@@ -32,7 +34,7 @@ async def load_mint(wallet: Wallet, mint: Optional[str] = None):
try:
await wallet.load_mint()
except Exception as e:
raise HTTPException(status_code=status.HTTP_400_BAD_REQUEST, detail=str(e))
raise Exception(str(e))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Jokingly suggest using a custom Exception class Cachuww 🤧

Copy link
Collaborator

Choose a reason for hiding this comment

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

Cash'uwu

Comment on lines 32 to 35
try:
await wallet.load_mint()
except Exception as e:
raise HTTPException(status_code=status.HTTP_400_BAD_REQUEST, detail=str(e))
raise Exception(str(e))
Copy link
Collaborator

Choose a reason for hiding this comment

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

This block can be replaced by simply wallet.load_mint() without try: ... except: ... – achieves the same thing.

Copy link
Collaborator

@callebtc callebtc left a comment

Choose a reason for hiding this comment

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

LGTM (pls check comment)

@callebtc callebtc merged commit 5df0a9a into main May 22, 2023
5 checks passed
@callebtc callebtc deleted the wallet-api-catch-exceptions branch May 23, 2023 11:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Wallet API] Catch all Exception in FastApi
3 participants