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

Da blob api #487

Merged
merged 9 commits into from
Nov 1, 2023
Merged

Da blob api #487

merged 9 commits into from
Nov 1, 2023

Conversation

al8n
Copy link
Contributor

@al8n al8n commented Oct 30, 2023

No description provided.

@al8n al8n added the enhancement New feature or request label Oct 30, 2023
@al8n al8n added this to the Nomos testnet (playground) milestone Oct 30, 2023
@al8n al8n self-assigned this Oct 30, 2023
Comment on lines 127 to 132
) -> impl IntoResponse {
match da::da_mempool_status(&store.da, items).await {
match da::da_mempool_status(&store.da_mempool, items).await {
Ok(status) => (StatusCode::OK, Json(status)).into_response(),
Err(e) => (StatusCode::INTERNAL_SERVER_ERROR, e.to_string()).into_response(),
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same comment as in other PRs not sure why do we return IntoResponse when we already transform everything into a response inside.

Copy link
Contributor Author

@al8n al8n Oct 30, 2023

Choose a reason for hiding this comment

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

Because the type returned in ok arm and err arm are not actually the same, we have to call into_response to convert to Response.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes I guessed that, but why return IntoResponse later when we already got a response? Is it to avoid boxing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yes, we can use Response as the returned type in our case.

Comment on lines 136 to 141
path = "/da/blob",
responses(
(status = 200, description = "Query the mempool status of the da service", body = Vec<Blob>),
(status = 500, description = "Internal server error", body = String),
)
)]
Copy link
Collaborator

Choose a reason for hiding this comment

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

If we return all blobs it is better that the path is /da/blobs. Also the description would be something like Get pending blobs.

@@ -14,6 +18,12 @@ pub(crate) type DaMempoolService = MempoolService<
CertDiscriminant,
>;

pub type DataAvailability = DataAvailabilityService<
Copy link
Contributor

Choose a reason for hiding this comment

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

better to reuse the type in nomos-node

Copy link
Collaborator

Choose a reason for hiding this comment

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

@al8n, refactor for this will be done later on? Let's open an issue for tracking it please!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, will open a new PR soon for this.

@al8n al8n merged commit aff903a into info-api Nov 1, 2023
6 checks passed
@al8n al8n deleted the da-blob-api branch November 1, 2023 07:25
al8n added a commit that referenced this pull request Nov 1, 2023
* Info api

* Da blob api (#487)

* Add storage api for new http backend (#488)

* Mempool add APIs (#489)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants