-
Notifications
You must be signed in to change notification settings - Fork 204
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
Support blob and batch fetch APIs #966
Conversation
"type": "integer" | ||
}, | ||
"cumulative_payment": { | ||
"description": "TODO: we are thinking the contract can use uint128 for cumulative payment,\nbut the definition on v2 uses uint64. Double check with team.", |
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.
Do we really want this in the swagger doc?
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's the cost of reusing data types in "core", we otherwise have to duplicate it in "dataapi". I may do a manual fix on it to handle this as an exception.
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'm going to leave it for now until all APIs are implemented, otherwise each time we generate docs it will involve this manual edit.
disperser/dataapi/docs/swagger.json
Outdated
@@ -136,7 +182,7 @@ | |||
"200": { | |||
"description": "OK", | |||
"schema": { | |||
"$ref": "#/definitions/dataapi.BlobMetadataResponse" | |||
"$ref": "#/definitions/dataapi.BlobResponse" |
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.
The v2 path is overwriting the v1, which is not desired.
The path naming may happen to need some improvements, i.e.
/feed/blobs
->/blob/feed
, and/feed/blobs/<blob_key>
-> /blob/<blob_key>`/feed/batches
->/batch/feed
, and/feed/batch/<batch_header_hash>
->/batch/<batch_header_hash>
This seems to be a more natural ordering. WDYT?
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.
Hmm, is the base url not taken into account here?
Anyway, that makes sense to me
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.
Yea maybe adding "v1" / "v2" to the path will avoid this. But it seems more logical to have "/blob/..." or "/batch/..." than organizing around "/feed/...".
"type": "integer" | ||
}, | ||
"cumulative_payment": { | ||
"description": "TODO: we are thinking the contract can use uint128 for cumulative payment,\nbut the definition on v2 uses uint64. Double check with team.", |
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's the cost of reusing data types in "core", we otherwise have to duplicate it in "dataapi". I may do a manual fix on it to handle this as an exception.
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.
lgtm
disperser/dataapi/docs/swagger.json
Outdated
@@ -136,7 +182,7 @@ | |||
"200": { | |||
"description": "OK", | |||
"schema": { | |||
"$ref": "#/definitions/dataapi.BlobMetadataResponse" | |||
"$ref": "#/definitions/dataapi.BlobResponse" |
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.
Hmm, is the base url not taken into account here?
Anyway, that makes sense to me
Why are these changes needed?
Implementing dataapi v2 (#955)
Checks