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

[Feature Request]: make structs using app_version self contained #3977

Open
zvolin opened this issue Dec 2, 2024 · 1 comment
Open

[Feature Request]: make structs using app_version self contained #3977

zvolin opened this issue Dec 2, 2024 · 1 comment
Labels
enhancement New feature or request external Issues created by non node team members

Comments

@zvolin
Copy link

zvolin commented Dec 2, 2024

Implementation ideas

Background

We are maintaining celestia ecosystem in Rust. We have crates like celestia-types which provides basic types and traits used in celestia, like eg. ExtendedDataSquare, and celestia-rpc which provides RPC client methods definitions for https://node-rpc-docs.celestia.org/. Those are meant to be a standalone building blocks for anything celestia related, whether it's just some application working with offline data or a fully fledged node.

app_version is used to put some constrains for types in celestia ecosystem. In example it puts a limit on square size or specifies parameters required for commitment generation. This effectively means that an EDS created in version 2 may not be valid in version 1. As app version is not stored within Blob and EDS this information needs to be routed somehow to perform a validation.

Problem

We're trying our best to make celestia ecosystem feel idiomatic in rust and one of the guidelines in Rust is to make invalid state inconstructible. This means that if we want to return ExtendedDataSquare to a user, we need to know the app_version to check if it's valid. Storing app_version inside Blob and EDS would make them self-contained instead, allowing for validation eg. during parsing them from json.

Example of issue

Due to the recent changes to celestia-node, the RPC api requires only height and not the whole ExtendedHeader in share.GetEDS. The most widely used json rpc crate in rust allows for expressing APIs using interfaces, which can then be called as a method on any client, to separately abstract out transport layer and the actual interface.

#[rpc]
trait Share {
    #[method(name = "share.GetEDS")]
    async fn share_get_eds(&self, height: u64) -> Result<ExtendedDataSquare, Error>;
}

let client = my_custom_http_client(); // or my_custom_ws_client() or my_custom_browser_fetch_client()
client.share_get_eds(1).await

But here we don't have enough information to construct the ExtendedDataSquare if we want to make sure the state is valid. There are some workarounds to that but none of them is great:

  • return RawExtendedDataSquare instead, and require user to pass app_version to transform it into ExtendedDataSquare
  • have height to app_version mapping, but it'd be needed to have it for all networks (and still unsure how to make it work with custom private networks). This also doesn't really work because there is no network information in this method call
  • provide a dedicated stateful rpc client which would maintain network information or store headers to use them for validation, but this loses all the generic nature of client and would require us to provide all possible configuration parameters for users (example just for http)
  • diverge from node-rpc-api and keep requiring ExtendedHeader to have a source of app_version - creates mismatch between real api and one exposed in rust, and may be less ergonomic to use (likely the reason for the change in celestia-node) (this is what we've chosen to do for now)

Solution

In my opinion the best solution is to have ExtendedHeader and Blob self contained. This could be done in celestia-app level (but is likely consensus breaking) or just introduce the app_version on celestia-node level (the blob is already modified wrt the struct present in app).

The example above is just a symptom that showed up in RPC, but the issue would be the same eg. when reading this data from filesystem, external services etc.

@zvolin zvolin added the enhancement New feature or request label Dec 2, 2024
@github-actions github-actions bot added the external Issues created by non node team members label Dec 2, 2024
@jcstein
Copy link
Member

jcstein commented Dec 3, 2024

cc: @celestiaorg/celestia-node 🙏 for visibility

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request external Issues created by non node team members
Projects
None yet
Development

No branches or pull requests

2 participants