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

Wrap the return of ProveShares into a new type ResultProveShares that lives inside of rpc/core/types/responses/go #1306

Closed
Ferret-san opened this issue Apr 16, 2024 · 4 comments · Fixed by #1313
Assignees

Comments

@Ferret-san
Copy link

Ferret-san commented Apr 16, 2024

Feature Request

Wrap the return of ProveShares into a new type ResultProveShares that lives inside of rpc/core/types/responses/go.

Summary

Every single RPC request in the codebase follows a pattern in which the responses for an rpc method get wrapped into a ResultX type. For example, the DataRootInclusionProof request returns:

type ResultDataRootInclusionProof struct {
	Proof merkle.Proof `json:"proof"`
}

But ProveShares doesn't, it simple returns a type from types.go

Problem Definition

Returning types.go/ShareProof without wrapping it into a result breaks the pattern that is established across RPC methods, and without proper documentation, it leads into errors and confusion when trying to interact with celestia core through any request method that does not involve using celestia-core's Go client.

People will have to change the structure of their responses if they are making their own requests to celestia core using their own code, and people using the http client of previous versions of celestia core to make requests will have to update to a newer version.

Proposal

Add a ResultProveShares type to rpc/core/types.go that wraps around SharesProof like:

type ResultProveShares struct {
   Proof SharesProof `json:"proof"`
}
@rootulp
Copy link
Collaborator

rootulp commented Apr 18, 2024

Why does this issue need the WS: v2 label? It can be fixed in a non-breaking way and released much sooner than v2 is released. To fix this in a non-breaking way:

  1. Create a new endpoint that wraps the response in a result
  2. Deprecate the existing endpoint

@rach-id
Copy link
Member

rach-id commented Apr 18, 2024

We don't need to worry about breaking this since no one is using this. Also, we have to release this as part of V2. So it's fine to have the breaking change instead of introducing new endpoints etc

@Ferret-san
Copy link
Author

Ferret-san commented Apr 18, 2024

We don't need to worry about breaking this since no one is using this

I am using this 🤚

But I think since it's already implemented as an rpc method in Go and Rust (most used languages for blockchains it seems), its not something that requires an immediate fix imo (i.e doesn't seem to be blocking anything for anyone)

@rach-id
Copy link
Member

rach-id commented Apr 18, 2024

I am using this 🤚

Meant external downstream teams with whom we should discuss the change and worry if they won't like it 😆

rach-id added a commit that referenced this issue Apr 19, 2024
## Description

This PR wraps the `ShareProof` into a `ResultShareProof` in the API.
However, It doesn't change the custom query response since it's internal
data and there is no need to wrap it.

Closes #1306
rach-id added a commit that referenced this issue May 8, 2024
## Description

This PR wraps the `ShareProof` into a `ResultShareProof` in the API.
However, It doesn't change the custom query response since it's internal
data and there is no need to wrap it.

Closes #1306

(cherry picked from commit c89f6f9)
rach-id added a commit that referenced this issue May 9, 2024
## Description

Contributes to #1306

---------

Co-authored-by: Rootul P <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants