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

Make distinct go modules for the /pkg directory and celestia-app modules in /x #2570

Closed
evan-forbes opened this issue Sep 22, 2023 · 2 comments
Assignees
Labels
refactor optional label for items that are related to implementation work and do not change functionality

Comments

@evan-forbes
Copy link
Member

In order to help downstream teams avoid dependency hell, we should begin using go modules for some of our packages similar to what most rust projects do.

We should at least do this for the /pkg and /x directories, but could go further by including a go.mod in each subdirectory of /x.

ref celestiaorg/rsmt2d#248 + a bunch of issues in node and rollkit

@evan-forbes evan-forbes added refactor optional label for items that are related to implementation work and do not change functionality go.mod labels Sep 22, 2023
@evan-forbes evan-forbes self-assigned this Sep 25, 2023
@rootulp rootulp added this to the Post-mainnet milestone Oct 9, 2023
@cmwaters cmwaters self-assigned this Oct 11, 2023
cmwaters added a commit that referenced this issue Oct 16, 2023
Ref: #2570

This PR creates a `pkg/blob` where the protos for `Blob` and `BlobTx` is
created. The intention is that this becomes it's own go.mod to avoid
needing to depend on `celestia-core` and our `cosmos-sdk` fork. Other
common structures like namespace should do the same so we can achieve
some cannonical representation of the structs between node and core/app

The reason for creating the types here instead of in `celestia-core` is
so that only `celestia-app` becomes a multi go.mod repo as opposed to
either. Additionally, `Blob` and `BlobTx` were never part of the
upstream CometBFT.

I also made the decision to have a single representation of blobs rather
than a proto and go native. The reason being is one of simplification
and performance. With large blobs, copying the data across to another
type can lead to a noticeable impact. Having a single struct however
does have the dowside of potential coversion errors from uint32 to uint8
so data needs to be validated before it can be used (this is also the
same as before).

This PR also ports over the proofs. I have put this all under a
common/v1 proto package.
@rootulp
Copy link
Collaborator

rootulp commented Oct 23, 2023

Note: this should be easier to do for pkg/shares because #2659 merged.

@evan-forbes evan-forbes removed this from the Post-mainnet milestone Nov 12, 2023
cmwaters added a commit to celestiaorg/go-square that referenced this issue Dec 14, 2023
Ref: celestiaorg/celestia-app#2570

This PR creates a `pkg/blob` where the protos for `Blob` and `BlobTx` is
created. The intention is that this becomes it's own go.mod to avoid
needing to depend on `celestia-core` and our `cosmos-sdk` fork. Other
common structures like namespace should do the same so we can achieve
some cannonical representation of the structs between node and core/app

The reason for creating the types here instead of in `celestia-core` is
so that only `celestia-app` becomes a multi go.mod repo as opposed to
either. Additionally, `Blob` and `BlobTx` were never part of the
upstream CometBFT.

I also made the decision to have a single representation of blobs rather
than a proto and go native. The reason being is one of simplification
and performance. With large blobs, copying the data across to another
type can lead to a noticeable impact. Having a single struct however
does have the dowside of potential coversion errors from uint32 to uint8
so data needs to be validated before it can be used (this is also the
same as before).

This PR also ports over the proofs. I have put this all under a
common/v1 proto package.
@rootulp
Copy link
Collaborator

rootulp commented Jan 21, 2024

This seems close-able because we extracted https://github.com/celestiaorg/go-square . Note: go-square doesn't include the /x modules stated in the OP but I think those are less useful for downstream apps compared to the /pkg modules. Please re-open or open a new issue if you feel differently @cmwaters @evan-forbes

@rootulp rootulp closed this as completed Jan 21, 2024
0xchainlover pushed a commit to celestia-org/celestia-app that referenced this issue Aug 1, 2024
Ref: celestiaorg/celestia-app#2570

This PR creates a `pkg/blob` where the protos for `Blob` and `BlobTx` is
created. The intention is that this becomes it's own go.mod to avoid
needing to depend on `celestia-core` and our `cosmos-sdk` fork. Other
common structures like namespace should do the same so we can achieve
some cannonical representation of the structs between node and core/app

The reason for creating the types here instead of in `celestia-core` is
so that only `celestia-app` becomes a multi go.mod repo as opposed to
either. Additionally, `Blob` and `BlobTx` were never part of the
upstream CometBFT.

I also made the decision to have a single representation of blobs rather
than a proto and go native. The reason being is one of simplification
and performance. With large blobs, copying the data across to another
type can lead to a noticeable impact. Having a single struct however
does have the dowside of potential coversion errors from uint32 to uint8
so data needs to be validated before it can be used (this is also the
same as before).

This PR also ports over the proofs. I have put this all under a
common/v1 proto package.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor optional label for items that are related to implementation work and do not change functionality
Projects
None yet
Development

No branches or pull requests

3 participants