-
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: node api proxy #92
Conversation
5abd5b4
to
2f00813
Compare
@@ -41,3 +41,39 @@ def get_address_search( | |||
except HathorCoreTimeout: | |||
self.node_api_gateway.blacklist_address(address) | |||
return ADDRESS_BLACKLIST_RESPONSE |
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 like we are using this same response for a lot of cases, even the ones that are not timeout.
And the variable name does not represent its content Timeout due to too many transaction
Is that right?
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.
Yes, the previous response was "This address is blacklisted" but the reason of the blacklist is a timeout due to too many transactions. This message was changed because it is showed to the user on the frontend so it should reflect the actual reason.
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.
but the reason of the blacklist is a timeout due to too many transactions
I'm not sure I understand this sentence.
I brought an example from line 18 in this file to make my point clearer:
if self.node_api_gateway.is_blacklisted_address(address):
return ADDRESS_BLACKLIST_RESPONSE
I don't understand why in this case we return Timeout due to too many transaction
to the user.
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.
Other than @luislhl comments, this looks good to me
Acceptance criteria