da: add jsonrpc proxy #54
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
WalkthroughThe recent changes involve a significant overhaul of the Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #54 +/- ##
==========================================
- Coverage 78.01% 75.54% -2.47%
==========================================
Files 3 5 +2
Lines 191 274 +83
==========================================
+ Hits 149 207 +58
- Misses 32 52 +20
- Partials 10 15 +5 ☔ View full report in Codecov by Sentry. |
This was referenced Mar 5, 2024
rootulp
reviewed
Mar 6, 2024
tuxcanfly
changed the title
![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: 4
Configuration used: CodeRabbit UI
Files ignored due to path filters (2)
go.modis excluded by:!**/*.modgo.sumis excluded by:!**/*.sum
Files selected for processing (4)
- proxy-grpc/proxy_test.go (1 hunks)
- proxy-jsonrpc/client.go (1 hunks)
- proxy-jsonrpc/proxy_test.go (1 hunks)
- proxy-jsonrpc/server.go (1 hunks)
Check Runs (4)
test / Run Unit Tests completed (1)
lint / golangci-lint completed (1)
lint / yamllint completed (1)
lint / markdown-lint completed (1)
Additional comments: 2
proxy-grpc/proxy_test.go (2)
- 11-11: The renaming of the imported package to
github.com/rollkit/go-da/proxy-grpcaligns with the PR's objective to distinguish between different proxy implementations. Ensure that this renaming is consistently applied across all relevant files in the project.- 8-14: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [13-26]
The usage of the renamed package
proxy-grpcwithin the test function appears to be correct and consistent with the PR's objectives. Ensure that the interface provided byproxy-grpcremains compatible with the existing test code.
MSevey
requested changes
Mar 13, 2024
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (3)
- proxy-jsonrpc/proxy_test.go (1 hunks)
- proxy-jsonrpc/server.go (1 hunks)
- proxy/client.go (1 hunks)
Check Runs (1)
codecov/patch failure (15)
- proxy-jsonrpc/server.go: 55-57: Added lines #L55 - L57 were not covered by tests
- proxy-jsonrpc/server.go: 60-61: Added lines #L60 - L61 were not covered by tests
- proxy-jsonrpc/server.go: 75-77: Added lines #L75 - L77 were not covered by tests
- proxy-jsonrpc/server.go: 80-81: Added lines #L80 - L81 were not covered by tests
Files skipped from review as they are similar to previous changes (1)
- proxy-jsonrpc/proxy_test.go
Additional comments: 2
proxy-jsonrpc/server.go (2)
- 49-67: The implementation of the
Startmethod correctly handles concurrent calls, ensuring that the server can only be started once. The logging for an already started server is a good practice. However, consider the following improvements:
- Error Handling: When
net.Listenfails, the error is returned immediately. It might be beneficial to log this error for easier debugging.- Testing Coverage: The static analysis hints at missing test coverage for this method. Ensure that tests cover the concurrent behavior and error scenarios to maintain reliability.
+ log.Error("failed to listen on address", "address", s.srv.Addr, "error", err)
- 69-85: The
Stopmethod's implementation and the added documentation for handling concurrent calls are commendable. Similar to theStartmethod, consider logging the error fromsrv.Shutdownfor better traceability. Additionally, ensure that tests cover the concurrent behavior and error scenarios for this method as well.+ log.Error("failed to shutdown server", "error", err)
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files selected for processing (8)
- proxy/client.go (1 hunks)
- proxy/grpc/client.go (1 hunks)
- proxy/grpc/proxy_test.go (2 hunks)
- proxy/grpc/server.go (1 hunks)
- proxy/grpc/util.go (1 hunks)
- proxy/jsonrpc/client.go (1 hunks)
- proxy/jsonrpc/proxy_test.go (1 hunks)
- proxy/jsonrpc/server.go (1 hunks)
Check Runs (1)
codecov/patch failure (15)
- proxy/grpc/client.go: 28-29: Added lines #L28 - L29 were not covered by tests
- proxy/grpc/client.go: 36-37: Added lines #L36 - L37 were not covered by tests
- proxy/grpc/client.go: 41-47: Added lines #L41 - L47 were not covered by tests
- proxy/grpc/client.go: 72-73: Added lines #L72 - L73 were not covered by tests
- proxy/grpc/client.go: 86-87: Added lines #L86 - L87 were not covered by tests
- proxy/grpc/client.go: 101-102: Added lines #L101 - L102 were not covered by tests
- proxy/grpc/client.go: 117-118: Added lines #L117 - L118 were not covered by tests
- proxy/jsonrpc/client.go: 34-35: Added lines #L34 - L35 were not covered by tests
- proxy/jsonrpc/client.go: 85-88: Added lines #L85 - L88 were not covered by tests
- proxy/jsonrpc/client.go: 92-93: Added lines #L92 - L93 were not covered by tests
- proxy/jsonrpc/client.go: 109-110: Added lines #L109 - L110 were not covered by tests
- proxy/jsonrpc/server.go: 55-57: Added lines #L55 - L57 were not covered by tests
- proxy/jsonrpc/server.go: 60-61: Added lines #L60 - L61 were not covered by tests
- proxy/jsonrpc/server.go: 75-77: Added lines #L75 - L77 were not covered by tests
- proxy/jsonrpc/server.go: 80-81: Added lines #L80 - L81 were not covered by tests
Files skipped from review due to trivial changes (3)
- proxy/grpc/proxy_test.go
- proxy/grpc/server.go
- proxy/grpc/util.go
Additional comments: 2
proxy/jsonrpc/proxy_test.go (1)
- 22-39: The test suite for the JSONRPC service is a good start. Consider the following improvements:
- Mocking External Services: If the JSONRPC service relies on external services, use mocking to ensure tests are not flaky and can run in isolation.
- Error Paths: Include tests for error paths to ensure robust error handling.
Consider using a mocking framework to simulate external dependencies and add tests for error scenarios.
proxy/jsonrpc/server.go (1)
- 49-66: The server start logic is well-implemented, but consider the following:
- Error Handling: Improve error messages for better clarity, especially when the server is already started.
- Concurrency: Ensure that concurrent calls to
Startdo not lead to race conditions or unexpected behavior.- log.Warn("cannot start server: already started") + log.Warn("Server start attempted but it's already running.")Ensure that the
startedatomic.Bool effectively prevents race conditions in concurrentStartcalls.
MSevey
reviewed
Mar 15, 2024
MSevey
previously approved these changes
Mar 15, 2024
gupadhyaya
reviewed
Mar 15, 2024
|
looks good, just a minor comment. @tuxcanfly |
gupadhyaya
approved these changes
Mar 15, 2024
MSevey
approved these changes
Mar 18, 2024
github-merge-queue bot
pushed a commit
to rollkit/rollkit
that referenced
this pull request
Mar 18, 2024
## Overview This PR adds jsonrpc da client. Depends on rollkit/go-da#54 ## Checklist - [x] New and updated code has appropriate documentation - [x] New and updated code has new and/or updated testing - [x] Required CI checks are passing - [x] Visual proof for any user facing features like CLI or documentation updates - [x] Linked issues closed with keywords <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Added a new flag for specifying a data availability layer (DA) auth token. - Introduced support for connecting via gRPC or JSON-RPC transports with configuration options including DA address, auth token, and namespace. - **Bug Fixes** - Updated default DA address format and protocol to ensure consistency and correct connection handling. - **Refactor** - Simplified DA client initialization with default timeout values and new client creation methods. - Renamed and restructured timeout constants and fields within the `DAClient` struct. - Replaced direct gRPC server setup with simplified proxy server initialization for both gRPC and JSONRPC. - Updated references and variable names across various tests to align with new addressing and namespace configurations. - **Chores** - Removed unused imports across multiple files to clean up the codebase. - **Documentation** - Updated documentation to reflect new DA connection options and configurations. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Overview
This PR adds a jsonrpc proxy.
Checklist
Summary by CodeRabbit
New Features
Refactor
proxy/grpc/proxy_test.gofor consistency and clarity.Tests