celestiaorg/celestia-core

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

Closed this issue · 4 comments

Ferret-san commented

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 commented

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 commented

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 commented

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 commented

I am using this 🤚

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