fix(da): add jsonrpc da client #1587
Merged
Merged
Conversation
Contributor
WalkthroughThe recent updates represent a significant shift towards enhancing connectivity and configuration flexibility within the system. Key changes include the adoption of JSON-RPC alongside gRPC for data availability (DA) client-server communication, streamlined client initialization with new default settings, and improved configuration options including authentication tokens and namespace adjustments. This evolution underscores a broader strategy to accommodate diverse network protocols and enhance security measures, ensuring a more adaptable and robust framework. 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 (
|
5 tasks
tuxcanfly
changed the title
tuxcanfly
marked this pull request as ready for review
March 13, 2024 07:12
![coderabbitai[bot]](https://cdn.github.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files ignored due to path filters (2)
go.modis excluded by:!**/*.modgo.sumis excluded by:!**/*.sum
Files selected for processing (9)
- cmd/rollkit/commands/run_node.go (4 hunks)
- cmd/rollkit/docs/rollkit_start.md (1 hunks)
- config/config.go (4 hunks)
- da/da.md (2 hunks)
- da/da_test.go (6 hunks)
- da/mock/cmd/main.go (2 hunks)
- node/full.go (3 hunks)
- node/node_test.go (1 hunks)
- rpc/json/helpers_test.go (1 hunks)
Check Runs (5)
lint / hadolint / hadolint completed (1)
lint / markdown-lint completed (1)
lint / yamllint completed (1)
lint / golangci-lint completed (1)
test / Run Unit Tests completed (1)
Files skipped from review as they are similar to previous changes (6)
- cmd/rollkit/commands/run_node.go
- config/config.go
- da/mock/cmd/main.go
- node/full.go
- node/node_test.go
- rpc/json/helpers_test.go
Additional comments: 8
da/da.md (2)
- 7-7: The transition from gRPC to JSON-RPC for the
DAClientis well-documented here. It's important to ensure that all references and links related to JSON-RPC and thejson-rpc-proxyare accurate and accessible.- 32-37: The addition of
[json-rpc-proxy]in the references section is a good practice for documentation clarity. Ensure that the URL provided (https://github.com/rollkit/go-da/tree/main/proxy) is correct and that the repository is publicly accessible.cmd/rollkit/docs/rollkit_start.md (1)
- 34-34: The addition of the
--rollkit.da_auth_tokenflag is a crucial update for supporting authentication with the DA layer. Ensure that the documentation clearly explains how to obtain and use this token for users unfamiliar with the authentication process.da/da_test.go (5)
- 18-18: Renaming the
proxypackage toproxy-jsonrpcaligns with the transition to JSON-RPC. Ensure that all references to this package are updated across the entire codebase to prevent import errors.Verification successful
The renaming of the
proxypackage toproxy-jsonrpchas been correctly applied across the codebase, as indicated by the absence of the old package name and the presence of the new one in several files. This aligns with the transition to JSON-RPC and ensures consistency in package references, preventing import errors.* 72-76: The initialization of the `rpcClient` using `startMockDAClient` demonstrates the transition to JSON-RPC for testing. Verify that the mock DA client correctly mimics the behavior of the actual JSON-RPC DA client to ensure test reliability. * 98-100: Starting the mock DA server with `proxy.NewServer` is a key part of testing the JSON-RPC integration. Ensure that the server address (`localhost:7980`) does not conflict with other services during testing. * 124-126: The temporary adjustment of `submitTimeout` for testing the submit timeout behavior is a good practice. However, ensure that this modification is thread-safe if tests are run in parallel. * 273-278: The tests for submitting large blocks and handling overflow are critical for ensuring the DA client behaves correctly under various conditions. Verify that these tests cover all edge cases, including partial submissions and retries.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for old package name usage across the codebase. Expecting no results. rg --type go 'github.com/rollkit/go-da/proxy'Length of output: 298
Contributor
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- da/da_test.go (6 hunks)
- node/full.go (3 hunks)
Check Runs (5)
lint / hadolint / hadolint completed (1)
test / Run Unit Tests completed (1)
lint / yamllint completed (1)
lint / golangci-lint completed (1)
lint / markdown-lint completed (1)
Files skipped from review as they are similar to previous changes (2)
- da/da_test.go
- node/full.go
Contributor
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to path filters (2)
go.modis excluded by:!**/*.modgo.sumis excluded by:!**/*.sum
Files selected for processing (1)
- node/full.go (3 hunks)
Check Runs (5)
lint / hadolint / hadolint completed (1)
test / Run Unit Tests completed (1)
lint / golangci-lint completed (1)
lint / yamllint completed (1)
lint / markdown-lint completed (1)
Files skipped from review as they are similar to previous changes (1)
- node/full.go
MSevey
suggested changes
Mar 13, 2024
Contributor
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- node/full.go (3 hunks)
Check Runs (5)
lint / hadolint / hadolint completed (1)
test / Run Unit Tests completed (1)
lint / yamllint completed (1)
lint / golangci-lint completed (1)
lint / markdown-lint completed (1)
Files skipped from review as they are similar to previous changes (1)
- node/full.go
Contributor
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- config/defaults.go (1 hunks)
- da/da.md (2 hunks)
Additional comments: 2
config/defaults.go (1)
- 29-29: The update to
DAAddressto include "grpc://" seems inconsistent with the PR's objective of transitioning to JSON-RPC for DA interactions. Please verify if this is the intended protocol and address format, especially since the PR aims to move away from gRPC.da/da.md (1)
- 7-11: The documentation updates accurately reflect the PR's objectives by mentioning support for both gRPC and JSON-RPC transports. However, the default DA address is prefixed with "grpc://", which might be misleading given the focus on JSON-RPC. Consider clarifying the intended default protocol and address format for users.
tzdybal
suggested changes
Mar 13, 2024
Contributor
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (10)
- cmd/rollkit/commands/run_node.go (5 hunks)
- da/da_test.go (6 hunks)
- da/mock/cmd/main.go (1 hunks)
- node/full.go (3 hunks)
- node/full_client_test.go (7 hunks)
- node/full_node_integration_test.go (3 hunks)
- node/full_node_test.go (2 hunks)
- node/helpers_test.go (1 hunks)
- node/node_test.go (2 hunks)
- rpc/json/helpers_test.go (2 hunks)
Files skipped from review as they are similar to previous changes (4)
- cmd/rollkit/commands/run_node.go
- da/mock/cmd/main.go
- node/full.go
- rpc/json/helpers_test.go
Additional comments: 19
node/helpers_test.go (1)
- 21-22: The update from
MockNamespacetoMockDANamespacealigns with the PR's objectives to enhance JSON-RPC DA client integration. This change is clear and straightforward.node/node_test.go (2)
- 20-23: The updates to
MockDAAddressandMockDANamespaceare consistent with the PR's objectives to enhance JSON-RPC DA client integration. These changes properly reflect the transition from gRPC to JSON-RPC.- 68-68: Ensure that the updated
MockDAAddressandMockDANamespaceare consistently used throughout the test suite to avoid any connectivity or namespace resolution issues.da/da_test.go (4)
- 21-22: Transitioning from
proxy-grpctoproxy-jsonrpcaligns with the PR's objectives to enhance JSON-RPC DA client integration. This change is a key part of moving away from gRPC towards a JSON-RPC based approach.- 114-124: The setup for the mock DA server using gRPC has been correctly replaced with a JSON-RPC implementation. This change is crucial for testing the new JSON-RPC client functionality.
- 127-133: The initialization of the mock DA client for gRPC has been correctly updated to reflect the transition to JSON-RPC. This ensures consistency with the new DA interaction method.
- 311-316: Consider revisiting the removal of assertions regarding
resp.Codein the test functions. Ensuring that the response code is as expected is important for validating the behavior of the DA client.node/full_node_test.go (2)
- 197-197: Updating the namespace reference from
MockNamespacetoMockDANamespacein theDAClientstruct aligns with the PR's objectives to enhance JSON-RPC DA client integration. This change is appropriate and reflects the transition from gRPC to JSON-RPC.- 284-285: Ensure that the updated
MockDANamespaceis consistently used throughout the test suite to avoid any namespace resolution issues.node/full_node_integration_test.go (3)
- 63-63: The
DAAddressandDANamespacefields have been updated toMockDAAddressandMockDANamespacerespectively. This change aligns with the PR's objective to transition from gRPC to JSON-RPC for DA interactions. Ensure thatMockDAAddressandMockDANamespaceare correctly defined and represent valid JSON-RPC endpoints and namespaces for testing purposes.- 186-187: Similar to the previous comment, the
DAAddressandDANamespacefields are updated here for the lazy aggregator test scenario. It's crucial to verify that these mock values are appropriately set up for integration testing, reflecting the new JSON-RPC DA client's requirements.- 651-652: In the
createNodefunction, theDAAddressandDANamespacefields are updated, which is consistent with the changes across the test file. This function is pivotal for setting up nodes in various test scenarios, so it's essential to ensure that the mock values used here are correctly initialized and represent the intended JSON-RPC endpoints and namespaces.node/full_client_test.go (7)
- 72-73: The update of
DAAddressandDANamespacetoMockDAAddressandMockDANamespacerespectively in thegetRPCfunction aligns with the PR's objective to shift towards JSON-RPC for DA interactions. This change ensures that the tests are configured to use the new mock addresses and namespaces for data availability, reflecting the transition from gRPC to JSON-RPC.- 175-175: The same update in the
TestGenesisChunkedfunction ensures consistency across test configurations, aligning with the new JSON-RPC based approach for data availability.- 547-548: The updates in the
TestTxfunction to useMockDAAddressandMockDANamespaceare consistent with the overall shift towards JSON-RPC for DA interactions. This change is crucial for ensuring that the tests reflect the new method of interaction with the data availability layer.- 806-807: In the
TestMempool2Nodesfunction, updating the DA configurations to use mock values is appropriate for testing the functionality in a controlled environment. This change is part of the broader update to accommodate the new JSON-RPC DA client.- 818-819: Similarly, the update in the
TestMempool2Nodesfunction for the second node's configuration ensures that both nodes in the test are aligned with the new data availability interaction method. This consistency is key for accurate and relevant testing.- 881-882: The update in the
TestStatusfunction to useMockDAAddressandMockDANamespaceensures that the status checks and related functionalities are tested against the new JSON-RPC based DA client. This change is aligned with the PR's objectives and ensures that the tests are relevant to the new implementation.- 1017-1018: Finally, the update in the
TestFutureGenesisTimefunction to useMockDAAddressandMockDANamespaceis consistent with the shift towards JSON-RPC for DA interactions. This ensures that the tests accurately reflect the new method of interaction with the data availability layer.
Contributor
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- cmd/rollkit/docs/rollkit_start.md (1 hunks)
- da/da_test.go (5 hunks)
Files skipped from review as they are similar to previous changes (1)
- da/da_test.go
Additional comments: 3
cmd/rollkit/docs/rollkit_start.md (3)
- 33-34: The addition of the
--rollkit.da_auth_token stringflag is clearly documented, enhancing the user's ability to provide a DA auth token when starting Rollkit. This is a valuable addition for users needing to authenticate with a data availability layer.- 30-37: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [9-9]
Consider clarifying the description for
--p2p.laddr stringto ensure it's understood that this is the address the node listens on. For example, "node listen address" could be expanded to "node listen address (specify the IP and port for the node to listen on for incoming connections)."
- 30-37: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [50-50]
For the
--log_level stringoption, consider using a hyphen in "set the log-level" to clarify that "log-level" is a compound adjective modifying "default."
Contributor
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files selected for processing (2)
- da/da.md (2 hunks)
- da/mock/cmd/main.go (1 hunks)
Additional comments: 7
da/mock/cmd/main.go (4)
- 4-11: Unused imports such as
netandsyscallhave been correctly removed, aligning with the PR's objectives to clean up dependencies. However, ensure that the removal ofsyscalldoes not impact the handling of interrupt signals for graceful shutdown.- 13-13: The transition to
proxy-jsonrpcfromproxy(previously likelyproxy-grpc) is a key change that aligns with the PR's objective of moving from gRPC to JSON-RPC. This change is correctly implemented.- 27-29: Parsing the address using
url.Parseand extracting the host and port is a good practice for flexibility. However, ensure that the default address (MockDAAddress) is correctly updated to a JSON-RPC endpoint if necessary, as mentioned in a previous comment.- 38-42: Handling interrupt signals for graceful shutdown is correctly implemented. This addresses the previous comment by
tzdybalregarding gentle shutdown of the proxy server.da/da.md (3)
- 7-11: The documentation correctly highlights the
DAClient's ability to connect via both gRPC and JSON-RPC transports. This is a significant update that aligns with the PR's objectives. Ensure that the example URLs and flags are consistent with the actual implementation and default values in the code.- 4-14: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [13-24]
The explanation of the
SubmitBlocksandRetrieveBlocksmethods is clear and informative. However, consider adding a note about the transition from gRPC to JSON-RPC and its impact on these methods, if any, to provide readers with a complete understanding of the changes.Consider adding a section or note about the transition from gRPC to JSON-RPC and its impact on the
SubmitBlocksandRetrieveBlocksmethods for completeness.
- 36-44: The references section is correctly updated to include both
proxy-grpcandproxy-jsonrpc, reflecting the dual support for gRPC and JSON-RPC transports. Ensure that the URLs provided are accurate and lead to the correct resources.
Contributor
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (6)
- block/manager_test.go (1 hunks)
- da/da.go (4 hunks)
- da/da_test.go (5 hunks)
- node/full.go (3 hunks)
- node/full_node_test.go (2 hunks)
- node/helpers_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (3)
- node/full.go
- node/full_node_test.go
- node/helpers_test.go
Additional comments: 11
da/da.go (5)
- 20-24: The renaming and restructuring of timeout constants to
defaultSubmitTimeoutanddefaultRetrieveTimeoutimprove clarity and maintainability by providing default values for timeouts in a centralized location.- 98-105: The addition of
SubmitTimeoutandRetrieveTimeoutfields to theDAClientstruct allows for customizable timeouts for submitting and retrieving blocks, enhancing flexibility and adaptability to different network conditions.- 107-117: The
NewDAClientfunction provides a clear and concise way to initialize aDAClientwith default timeout values, ensuring that all necessary fields are properly set. This approach enhances code readability and maintainability.- 151-151: Using
context.WithTimeoutinSubmitBlocksmethod ensures that the block submission process respects the specified timeout, preventing potential hangs in network communication. This is a good practice for handling external calls with unpredictable response times.- 218-218: Similarly, the use of
context.WithTimeoutinRetrieveBlocksmethod is commendable for ensuring that block retrieval operations are bounded by the specified timeout, enhancing the robustness of the data retrieval process.da/da_test.go (5)
- 21-22: Renaming the
proxypackage toproxy-grpcandproxy-jsonrpcand adjusting imports accordingly improves clarity and better reflects the underlying protocol used by each proxy implementation. This change enhances code readability and maintainability.- 114-124: The
startMockDAServGRPCfunction demonstrates a good practice by creating a new gRPC server with the appropriate credentials and serving it asynchronously. This setup is essential for testing gRPC-related functionality in isolation.- 127-133: The
startMockDAClientGRPCfunction correctly initializes a gRPC client and connects it to the mock server, ensuring that the client is ready for testing. The use ofgrpc.WithTransportCredentialsfor secure communication is a good practice.- 136-142: The
startMockDAServJSONRPCfunction effectively starts a mock JSON-RPC server, showcasing a straightforward approach to testing JSON-RPC server functionality. The use ofcontext.TODO()here is acceptable given the test context, but it's generally good to use more specific contexts.- 146-151: The
startMockDAClientJSONRPCfunction demonstrates a clear and concise way to initialize a JSON-RPC client for testing purposes. The separation of client initialization into its own function enhances test code organization and reusability.block/manager_test.go (1)
- 32-32: The modification in the
getManagerfunction to useda.NewDAClientfor initializingdalcwith specific parameters enhances code clarity and ensures that theDAClientis correctly configured for testing. This change improves the test setup process by making it more explicit and maintainable.
Contributor
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files selected for processing (1)
- da/da_test.go (5 hunks)
Additional comments: 7
da/da_test.go (7)
- 21-22: Renaming the
proxypackage toproxy-grpcandproxy-jsonrpcis a good practice for clarity and specificity. This change helps in distinguishing between the two different protocol implementations.- 28-37: The constants defined for mock DA addresses and namespace are well-structured and clearly named. However, ensure that the port numbers (
7980for gRPC and7988for JSONRPC) do not conflict with any commonly used ports or other services in your environment to avoid potential issues during testing.- 65-65: The instantiation of
NewDAClientwith negative timeout values (-1) in the test functionsTestMockDAErrorsis concerning. While this might be intentional to simulate certain conditions, using negative values for timeouts is unconventional and could lead to confusion or unintended behavior. If this is a deliberate choice for testing specific scenarios, consider adding a comment to clarify the rationale behind using negative values.Also applies to: 83-83
- 91-101: The setup for testing different DA clients (
dummy,jsonrpc,grpc) is comprehensive and well-organized. This approach ensures that each client type is tested under the same conditions, enhancing the test coverage for the new JSONRPC functionality alongside the existing gRPC functionality. Good job on structuring these tests to cover multiple scenarios.- 145-151: The
startMockDAServJSONRPCfunction demonstrates good practice by starting a mock JSONRPC server and handling potential startup errors. This approach ensures robustness and reliability in the test setup phase for JSONRPC-related tests.- 171-171: Setting a specific
SubmitTimeoutvalue (50 * time.Millisecond) in thedoTestSubmitTimeoutfunction is a good practice for testing timeout behavior. However, ensure that this value is representative of realistic scenarios and does not lead to flaky tests, especially in environments with variable network latency or processing power.- 220-220: The use of
rand.Int63() % MockDABlockTime.Milliseconds()to introduce variability in block submission timing is an interesting approach to simulate real-world conditions. However, be cautious with the use of randomness in tests as it can introduce non-determinism and potentially make tests flaky. Consider if there's a more deterministic way to achieve the desired test coverage.
Contributor
There was a problem hiding this comment.
Review Status
Actionable comments generated: 4
Configuration used: CodeRabbit UI
Files selected for processing (10)
- cmd/rollkit/commands/run_node.go (5 hunks)
- da/da_test.go (5 hunks)
- da/mock/cmd/main.go (1 hunks)
- node/full_client_test.go (7 hunks)
- node/full_node_integration_test.go (3 hunks)
- node/full_node_test.go (2 hunks)
- node/helpers_test.go (1 hunks)
- node/node_test.go (2 hunks)
- rpc/json/helpers_test.go (2 hunks)
- test/mocks/util.go (1 hunks)
Files skipped from review due to trivial changes (1)
- test/mocks/util.go
Files skipped from review as they are similar to previous changes (8)
- cmd/rollkit/commands/run_node.go
- da/mock/cmd/main.go
- node/full_client_test.go
- node/full_node_integration_test.go
- node/full_node_test.go
- node/helpers_test.go
- node/node_test.go
- rpc/json/helpers_test.go
Additional comments: 2
da/da_test.go (2)
- 150-155: In the
startMockDAClientJSONRPCfunction, handling the error fromproxyjsonrpc.NewClientis done correctly. However, consider adding a comment explaining the choice of an empty string for the second parameter ofNewClient, as it's not immediately clear what this represents (presumably, an authentication token or API key). This would improve code readability and maintainability.Consider adding a comment to clarify the purpose of the empty string parameter in the
proxyjsonrpc.NewClientcall for future maintainability.
- 215-215: Using
rand.Int63() % MockDABlockTime.Milliseconds()to introduce variability in block submission timing is a clever way to simulate real-world conditions. However, usingrand.Int63()without a seed can lead to predictable sequences of random numbers, which might not be desirable in tests where randomness is expected to introduce variability in test execution. Consider seeding the random number generator (e.g., withrand.Seed(time.Now().UnixNano())) at the beginning of the test or test suite to ensure more unpredictable sequences.Consider seeding the random number generator at the beginning of the test suite to ensure unpredictable sequences of random numbers.
Contributor
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 (17)
- block/manager_test.go (1 hunks)
- cmd/rollkit/commands/run_node.go (5 hunks)
- cmd/rollkit/docs/rollkit_start.md (1 hunks)
- config/config.go (4 hunks)
- config/defaults.go (1 hunks)
- da/da.go (4 hunks)
- da/da.md (2 hunks)
- da/da_test.go (5 hunks)
- da/mock/cmd/main.go (1 hunks)
- node/full.go (3 hunks)
- node/full_client_test.go (7 hunks)
- node/full_node_integration_test.go (3 hunks)
- node/full_node_test.go (2 hunks)
- node/helpers_test.go (1 hunks)
- node/node_test.go (2 hunks)
- rpc/json/helpers_test.go (2 hunks)
- test/mocks/util.go (1 hunks)
Files skipped from review as they are similar to previous changes (14)
- block/manager_test.go
- cmd/rollkit/commands/run_node.go
- config/config.go
- config/defaults.go
- da/da.go
- da/mock/cmd/main.go
- node/full.go
- node/full_client_test.go
- node/full_node_integration_test.go
- node/full_node_test.go
- node/helpers_test.go
- node/node_test.go
- rpc/json/helpers_test.go
- test/mocks/util.go
Additional comments: 4
da/da.md (2)
- 7-11: The documentation clearly explains the new configuration options for connecting to the DA service using either gRPC or JSON-RPC. This addition enhances the flexibility of the
DAClient.- 36-44: References to the
proxy-grpcandproxy-jsonrpcimplementations are correctly linked, providing useful resources for further exploration. It's good practice to ensure these links remain up-to-date.Verification successful
The links to
proxy-grpcandproxy-jsonrpcimplementations are valid and lead to the correct resources, as confirmed by the HTTP/2 200 responses. This ensures that the references are correctly linked, providing useful resources for further exploration.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the links to proxy-grpc and proxy-jsonrpc are valid and lead to the correct resources. curl --head https://github.com/rollkit/go-da/tree/tux/jsonrpc/proxy-grpc curl --head https://github.com/rollkit/go-da/tree/tux/jsonrpc/proxy-jsonrpcLength of output: 9044
cmd/rollkit/docs/rollkit_start.md (1)
- 33-34: The addition of the
--rollkit.da_auth_token stringflag is well-documented, providing clear instructions for users on how to supply the DA auth token when starting Rollkit. This enhances the security and flexibility of DA connections.da/da_test.go (1)
- 21-22: Renaming the
proxypackage toproxy-grpcandproxy-jsonrpcis a good practice for clarity and specificity. This change makes it easier to distinguish between the two implementations.
tzdybal
suggested changes
Mar 15, 2024
tzdybal
left a comment
tzdybal
left a comment
Contributor
There was a problem hiding this comment.
LGTM, except using grpc with 26650 (which seems like an error)
Contributor
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files ignored due to path filters (2)
go.modis excluded by:!**/*.modgo.sumis excluded by:!**/*.sum
Files selected for processing (16)
- block/manager_test.go (1 hunks)
- cmd/rollkit/commands/run_node.go (6 hunks)
- cmd/rollkit/docs/rollkit_start.md (1 hunks)
- config/config.go (4 hunks)
- config/defaults.go (1 hunks)
- da/da.go (4 hunks)
- da/da.md (2 hunks)
- da/da_test.go (5 hunks)
- da/mock/cmd/main.go (1 hunks)
- node/full.go (3 hunks)
- node/full_client_test.go (7 hunks)
- node/full_node_integration_test.go (3 hunks)
- node/full_node_test.go (2 hunks)
- node/helpers_test.go (1 hunks)
- node/node_test.go (4 hunks)
- rpc/json/helpers_test.go (2 hunks)
Files skipped from review as they are similar to previous changes (13)
- block/manager_test.go
- cmd/rollkit/commands/run_node.go
- config/config.go
- config/defaults.go
- da/da.go
- da/mock/cmd/main.go
- node/full.go
- node/full_client_test.go
- node/full_node_integration_test.go
- node/full_node_test.go
- node/helpers_test.go
- node/node_test.go
- rpc/json/helpers_test.go
Additional comments: 4
da/da.md (2)
- 7-11: The configuration options are clearly outlined, but it's important to ensure that the default value for
--rollkit.da_addressis consistent with the actual implementation. Given the previous comments about a potential proto/port mismatch and the suggestion to use Celestia node's endpoint as default, please verify that the default value and the protocol (grpc://) are correctly set and match the implementation.- 43-44: The documentation provides a helpful overview of the
DAClient's capabilities and configuration. However, it would be beneficial to include examples or more detailed explanations of how to use theDAClientwith both gRPC and JSON-RPC transports, especially for users unfamiliar with these protocols.cmd/rollkit/docs/rollkit_start.md (1)
- 33-34: The addition of the
--rollkit.da_auth_tokenflag is clearly documented. It's important to ensure that the documentation also mentions any requirements or constraints related to the DA auth token, such as format or length, to assist users in correctly configuring Rollkit.da/da_test.go (1)
- 161-166: The
startMockDAClientJSONRPCfunction correctly handles errors fromproxyjsonrpc.NewClient. This is a good practice, ensuring that the test setup is robust and can gracefully handle initialization failures.
gupadhyaya
previously approved these changes
Mar 18, 2024
gupadhyaya
enabled auto-merge
March 18, 2024 15:40
MSevey
suggested changes
Mar 18, 2024
Contributor
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to path filters (2)
go.modis excluded by:!**/*.modgo.sumis excluded by:!**/*.sum
Files selected for processing (1)
- da/da.md (2 hunks)
Additional comments: 4
da/da.md (4)
- 7-11: The configuration options introduced for the
DAClientare clear and well-documented. However, it's important to ensure that the default value for--rollkit.da_addressis aligned with the project's standards and the feedback from previous comments regarding the use of a more appropriate default endpoint.- 9-9: Based on previous comments, there was a suggestion to use Celestia node's endpoint as the default for
--rollkit.da_address. It's crucial to verify if this change has been implemented to ensure consistency and alignment with project standards.Verification successful
The verification process confirms that the Celestia node's endpoint, "grpc://localhost:26650", has been set as the default for
--rollkit.da_addressin theda/da.mdfile. This aligns with the expectations set in the review comment, ensuring consistency and alignment with project standards.* 4-14: > 📝 **NOTE** > This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [13-24]Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for the presence of Celestia node's endpoint as the default DA address. grep "grpc://localhost:26650" da/da.mdLength of output: 130
The descriptions of
SubmitBlocksandRetrieveBlocksmethods are detailed and informative. However, it would be beneficial to include examples or code snippets to illustrate how these methods are used in practice. This addition would enhance the documentation's usefulness to developers and improve clarity.Consider adding example usage code snippets for
SubmitBlocksandRetrieveBlocksto provide practical guidance to developers.
- 33-44: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [28-44]
The references section is well-organized, providing easy access to related resources. However, ensure that all links are functional and lead to the intended destinations. Broken links can significantly hinder the usefulness of documentation.
Verification successful
The verification process confirms that all links in the references section are functional, leading to the intended destinations as indicated by the HTTP status code 200 for each link.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the functionality of links in the references section. curl -o /dev/null --silent --head --write-out '%{http_code}\n' https://github.com/rollkit/go-da curl -o /dev/null --silent --head --write-out '%{http_code}\n' https://github.com/rollkit/celestia-da curl -o /dev/null --silent --head --write-out '%{http_code}\n' https://github.com/rollkit/go-da/tree/main/proxy/grpc curl -o /dev/null --silent --head --write-out '%{http_code}\n' https://github.com/rollkit/go-da/tree/main/proxy/jsonrpcLength of output: 458
Contributor
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- cmd/rollkit/commands/run_node.go (6 hunks)
Files skipped from review as they are similar to previous changes (1)
- cmd/rollkit/commands/run_node.go
gupadhyaya
approved these changes
Mar 18, 2024
MSevey
approved these changes
Mar 18, 2024
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
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 jsonrpc da client.
Depends on evstack/go-da#54
Checklist
Summary by CodeRabbit
DAClientstruct.