Binary / gRPC server #7
Conversation
tzdybal
force-pushed
the
tzdybal/binary
branch
6 times, most recently
from
5145415
to
7f6e7de
Compare
Welcome to Codecov 🎉Once merged to your default branch, Codecov will compare your coverage reports and display the results in this comment. Thanks for integrating Codecov - We've got you covered ☂️ |
|
I added auto-merge with merge instead of squash, as this PR contains many little fixes, and commit history is detailed. |
tzdybal
force-pushed
the
tzdybal/binary
branch
from
9b98332
to
a68a244
Compare
tzdybal
force-pushed
the
tzdybal/binary
branch
from
66fe6da
to
59f76e2
Compare
WalkthroughThe changes primarily focus on streamlining the CI/CD process by modifying the workflow and linter configurations, and updating the Celestia DA layer's implementation and its corresponding tests. The changes also include updates to the documentation references. Changes
Poem
TipsChat with CodeRabbit Bot (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files ignored due to filter (2)
- go.mod
- go.sum
Files selected for processing (8)
- .github/workflows/ci_release.yml (1 hunks)
- .github/workflows/lint.yml (2 hunks)
- .golangci.yml (1 hunks)
- .markdownlint.yaml (1 hunks)
- celestia.go (3 hunks)
- celestia_test.go (1 hunks)
- cmd/celestia-da/main.go (1 hunks)
- specs/src/celestia-da.md (2 hunks)
Files skipped from review due to trivial changes (5)
- .github/workflows/ci_release.yml
- .github/workflows/lint.yml
- .golangci.yml
- .markdownlint.yaml
- specs/src/celestia-da.md
Additional comments: 21
celestia_test.go (6)
1-23: The import section looks fine. All the imported packages are being used in the code.
25-32: The
TestSuitestruct is well defined with necessary fields for the test suite.86-90: The
TearDownSuitefunction is correctly purging the Docker resource and handling errors.92-94: The
TestIntegrationTestSuitefunction is correctly running the test suite.96-105: The
TestCelestiaDAfunction is correctly setting up the RPC client, initializing the blob namespace, and running the DA test suite. It also handles errors and context timeouts properly.107-109: The
getRPCAddressfunction is correctly returning the RPC address.cmd/celestia-da/main.go (3)
21-59: The
mainfunction is well-structured and uses thecobralibrary for command-line flag handling. However, the TODO comments indicate that there are plans to extract the configuration and main command from themainfunction and read the configuration from a file. This would be a good improvement for maintainability and flexibility.61-90: The
servefunction sets up the Celestia DA instance and the gRPC server, and serves it on a network listener. It handles errors appropriately by logging them and terminating the program when necessary. However, the TODO comment on line 78 indicates that there are plans to add configuration options for encryption. This is a crucial aspect for security and should be implemented as soon as possible.81-81: The network listener is set up to listen on a random port because the port is not specified in the
net.Listenfunction. If a specific port is required, it should be specified here.- lis, err := net.Listen("tcp", "") + lis, err := net.Listen("tcp", ":8080") // replace 8080 with the desired portcelestia.go (12)
2-16: The import section looks clean and organized. All the necessary packages are imported for the implementation of the
CelestiaDAstruct and its methods.18-23: The
CelestiaDAstruct has been updated to remove theheightfield. This is in line with the changes mentioned in the summary.25-32: The
NewCelestiaDAfunction has been updated to remove theheightparameter. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.34-42: The
Getfunction now takes a list ofda.IDinstead of[]byte. This change is in line with the summary. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.48-62: The
GetIDsfunction now takes theheightparameter. This change is in line with the summary. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.64-68: The
Commitfunction now returns[]da.Commitmentinstead of[]byte. This change is in line with the summary. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.70-96: The
Submitfunction now returns[]da.IDand[]da.Proofinstead of[]byte. This change is in line with the summary. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.98-116: The
blobsAndCommitmentsfunction is well implemented and handles the conversion of[]da.Blobto[]*blob.Bloband generates corresponding[]da.Commitment.128-160: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [118-139]
The
Validatefunction now takes[]da.IDand[]da.Proofinstead of[][]byte. This change is in line with the summary. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.
146-151: The
makeIDfunction has been added. It combinesheightandcommitmentinto a singleda.ID.153-158: The
splitIDfunction has been added. It splits ada.IDintoheightandcommitment.160-160: The
CelestiaDAstruct is confirmed to implement theda.DAinterface. This is a good practice to ensure that the struct meets the interface requirements.
| func (t *TestSuite) SetupSuite() { | ||
| pool, err := dockertest.NewPool("") | ||
| if err != nil { | ||
| t.Failf("Could not construct docker pool", "error: %v\n", err) | ||
| } | ||
| t.pool = pool | ||
|
|
||
| // uses pool to try to connect to Docker | ||
| err = pool.Client.Ping() | ||
| if err != nil { | ||
| t.Failf("Could not connect to Docker", "error: %v\n", err) | ||
| } | ||
|
|
||
| // pulls an image, creates a container based on it and runs it | ||
| resource, err := pool.Run("ghcr.io/rollkit/local-celestia-devnet", "4ecd750", []string{}) | ||
| if err != nil { | ||
| t.Failf("Could not start resource", "error: %v\n", err) | ||
| } | ||
| t.resource = resource | ||
|
|
||
| // exponential backoff-retry, because the application in the container might not be ready to accept connections yet | ||
| pool.MaxWait = 60 * time.Second | ||
| if err := pool.Retry(func() error { | ||
| resp, err := http.Get(fmt.Sprintf("http://localhost:%s/balance", resource.GetPort("26659/tcp"))) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| bz, err := io.ReadAll(resp.Body) | ||
| _ = resp.Body.Close() | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if strings.Contains(string(bz), "error") { | ||
| return errors.New(string(bz)) | ||
| } | ||
| return nil | ||
| }); err != nil { | ||
| log.Fatalf("Could not start local-celestia-devnet: %s", err) | ||
| } | ||
|
|
||
| opts := dockertest.ExecOptions{} | ||
| buf := new(bytes.Buffer) | ||
| opts.StdOut = buf | ||
| opts.StdErr = buf | ||
| _, err = resource.Exec([]string{"/bin/celestia", "bridge", "auth", "admin", "--node.store", "/home/celestia/bridge"}, opts) | ||
| if err != nil { | ||
| t.Failf("Could not execute command", "error: %v\n", err) | ||
| } | ||
|
|
||
| t.token = buf.String() | ||
| } |
There was a problem hiding this comment.
The SetupSuite function is setting up the Docker pool and resource correctly. It also handles errors properly. However, it's recommended to use t.Fatalf instead of t.Failf followed by log.Fatalf to fail the test immediately when an error occurs.
|
|
||
| da := celestia.NewCelestiaDA(client, namespace, ctx) | ||
| // TODO(tzdybal): add configuration options for encryption | ||
| srv := proxy.NewServer(da, grpc.Creds(insecure.NewCredentials())) |
There was a problem hiding this comment.
The gRPC server is set up with insecure credentials. This is a security risk and should be replaced with secure credentials as soon as possible.
- srv := proxy.NewServer(da, grpc.Creds(insecure.NewCredentials()))
+ // replace with secure credentials
+ creds, _ := credentials.NewServerTLSFromFile("cert.pem", "key.pem")
+ srv := proxy.NewServer(da, grpc.Creds(creds))Commitable suggestion (Beta)
| srv := proxy.NewServer(da, grpc.Creds(insecure.NewCredentials())) | |
| // replace with secure credentials | |
| creds, _ := credentials.NewServerTLSFromFile("cert.pem", "key.pem") | |
| srv := proxy.NewServer(da, grpc.Creds(creds)) |
Overview
Resolves #5.
Resolves #9.
Checklist
Summary by CodeRabbit
Refactor:
protojob from the release workflow and eliminating Dockerfile and protobuf linting jobs.celestia.gofor improved data handling.New Features:
cmd/celestia-da/main.go.Tests:
celestia_testpackage with new imports, test functions, and a test suite struct for more comprehensive testing.Style:
.golangci.ymland.markdownlint.yamlfor more specific code and markdown linting.Documentation:
specs/src/celestia-da.md) to reflect the new API documentation location.