-
Notifications
You must be signed in to change notification settings - Fork 3
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
feat: report api #57
base: main
Are you sure you want to change the base?
feat: report api #57
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.
I just reviewed the design.
``` | ||
body: | ||
{ | ||
type: str, // 'token' or 'transaction' |
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.
Should we have types of report as well? An initial list of choices might be: "impersonate" and "scam"?
{ | ||
type: str, // 'token' or 'transaction' | ||
id: str, | ||
decription: str |
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.
Should we put a max length here?
body: | ||
{ | ||
type: str, // 'token' or 'transaction' | ||
id: str, |
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.
How is the id calculated? I feel we shouldn't receive it here.
``` | ||
body: | ||
{ | ||
type: str, // 'token' or 'transaction' |
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.
Should we require a captcha to prevent spam? If yes, maybe it must have two steps.
# Rationale and alternatives | ||
[rationale-and-alternatives]: #rationale-and-alternatives | ||
|
||
We can use other way to receive reports instead of an email |
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.
I feel we should add a quick note about why we haven't chose alternative methods.
# Reference-level explanation | ||
[reference-level-explanation]: #reference-level-explanation | ||
|
||
**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.
It seems this is the Guide Level explanation. For the low level, I think we should explain what the API will do.
I feel we should internally save an S3 file for each report, add a report counter to the token/transaction, and send an email. Maybe the email will be disabled in the future in case we start to receive too many reports.
[future-possibilities]: #future-possibilities | ||
|
||
We can store it in a database and have some automation as the volume can increase. | ||
It can be hard to us to manage all reports as it grows. |
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.
Maybe we could use a third-party tool for this in the future. It would decrease the complexity for us.
Sentry had a User Feedback feature, for example: https://docs.sentry.io/api/projects/submit-user-feedback/
:return: if report was successful sent or not | ||
:rtype: bool | ||
""" | ||
print(f"Report sent! Message: {message}") # TODO: THis line will be changed to real messenger provider |
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.
This will be done only in another PR?
Rendered Design