-
Notifications
You must be signed in to change notification settings - Fork 106
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
API Signing for discovery provider #532
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.
This is looking really good @vicky-g! I have a few asks to try to move towards unifying the response data in health_check and version and also extra metadata fields to add, but this is looking very close
One more thing here. for now we can probably hardcode some wallet and private key into the configs. otherwise the process will err for local runs if no values are set right? |
@dmanjunath yeah, the disc prov won't start without some wallet and private key. i'll add some dummy values to this pr! |
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.
Looking really good! I have one nit but this looks almost good enough to merge
also @vicky-g tests are failing |
# Override the default method | ||
def default(self, o): # pylint: disable=E0202 | ||
if isinstance(o, (datetime.date, datetime.datetime)): | ||
# TODO - the Z is required in JS date format |
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.
@dmanjunath is this TODO still relevant?
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 was meant more as a comment, not a TODO
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 can remove the TODO
|
||
def error_response(error, error_code=500): | ||
return jsonify({'success': False, 'error': error}), error_code | ||
|
||
# Create a response dict with just data, signature, and timestamp | ||
# This response will contain a duplicate of response_entity | ||
def success_response_backwards_compat(response_entity=None, status=200): |
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.
@vicky-g jw - does this mean the new success_response isn't backwards compatible right? its just differently formatted than success_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.
looks great, awesome work @vicky-g!
No description provided.