feat: add SubmitWithOptions method to the API #95
Conversation
tzdybal
requested review from
MSevey and
yarikbratashchuk
and removed request for
a team
WalkthroughThe pull request introduces a new method, Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant DA
participant gRPC
participant JSONRPC
Client->>DA: SubmitWithOptions(blobs, gasPrice, namespace, options)
DA->>gRPC: Send SubmitRequest(blobs, gasPrice, namespace, options)
gRPC-->>DA: Response with IDs
DA-->>Client: Return IDs
Assessment against linked issues
Poem
Tip OpenAI O1 model for chat
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Outside diff range, codebase verification and nitpick comments (2)
da.go (1)
31-35: Approve the addition ofSubmitWithOptionsmethod.The method
SubmitWithOptionshas been correctly added to theDAinterface, aligning with the PR objectives to enhance flexibility for multi-account support. The method signature is consistent and clear.However, consider using a more structured type than
[]byteforoptionsto improve type safety and clarity on what the options can include, such as using a struct or predefined constants.proxy/jsonrpc/client.go (1)
69-72: Approve the addition ofSubmitWithOptions.The implementation of
SubmitWithOptionsin theAPIstruct is straightforward and aligns with the PR's objectives to enhance flexibility. Consider adding more detailed documentation or examples in the code comments to illustrate how clients can utilize this new method effectively.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
types/pb/da/da.pb.gois excluded by!**/*.pb.go
Files selected for processing (6)
- da.go (1 hunks)
- proto/da/da.proto (1 hunks)
- proxy/grpc/client.go (1 hunks)
- proxy/jsonrpc/client.go (2 hunks)
- test/dummy.go (1 hunks)
- test/test_suite.go (1 hunks)
Additional context used
golangci-lint
test/dummy.go
[warning] 135-135: exported: exported method DummyDA.SubmitWithOptions should have comment or be unexported
(revive)
GitHub Check: lint / golangci-lint
test/dummy.go
[failure] 135-135:
exported: exported method DummyDA.SubmitWithOptions should have comment or be unexported (revive)
Additional comments not posted (1)
proto/da/da.proto (1)
111-111: Approve the addition ofoptionsfield with a note on compatibility.The addition of the
optionsfield to theSubmitRequestmessage is a good enhancement, aligning with the PR objectives to allow for more flexible submissions.However, please verify the use of
optionalinproto3, as it requires field presence to be enabled in the protobuf version used. If not supported, consider using a default value or a wrapper type.
This commit introduces a new method `SubmitWithOptions` for submitting blobs with additional options.
tzdybal
force-pushed
the
tzdybal/new_api
branch
from
c718596 to
4521f9a
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
proxy/grpc/server.go (1)
72-72: Update the method documentation.Consider adding a TODO comment to update the
Submitmethod documentation to reflect the new behavior and the additionaloptionsparameter.+// TODO: Update the method documentation to reflect the new behavior and the additional `options` parameter. func (p *proxySrv) Submit(ctx context.Context, request *pbda.SubmitRequest) (*pbda.SubmitResponse, error) {
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
types/pb/da/da.pb.gois excluded by!**/*.pb.go
Files selected for processing (7)
- da.go (1 hunks)
- proto/da/da.proto (1 hunks)
- proxy/grpc/client.go (1 hunks)
- proxy/grpc/server.go (1 hunks)
- proxy/jsonrpc/client.go (2 hunks)
- test/dummy.go (1 hunks)
- test/test_suite.go (1 hunks)
Files skipped from review as they are similar to previous changes (5)
- da.go
- proxy/grpc/client.go
- proxy/jsonrpc/client.go
- test/dummy.go
- test/test_suite.go
Additional comments not posted (2)
proxy/grpc/server.go (1)
72-72: Verify the implementation of theSubmitWithOptionsmethod.The change to call the
SubmitWithOptionsmethod aligns with the PR objective of supporting multi-account scenarios. However, it's important to verify the implementation and behavior of this new method.Run the following script to verify the
SubmitWithOptionsmethod:Verification successful
The
SubmitWithOptionsmethod is correctly implemented and used.The verification process confirms that the
SubmitWithOptionsmethod is properly defined in thedapackage interface and consistently implemented across the codebase. The change inproxy/grpc/server.gocorrectly calls this method with all required parameters, including the newrequest.Options. The method is also being used in test files, which helps ensure its correct behavior.While the
DummyDAimplementation ignores theoptionsparameter, this is acceptable for a test implementation. However, it's worth noting that real implementations should make proper use of this parameter to support multi-account scenarios as intended.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of the `SubmitWithOptions` method. # Test: Search for the method declaration. Expect: The method should be defined in the `da` package. rg --type go $'func \(d \*[a-zA-Z]+\) SubmitWithOptions\(' # Test: Search for the method usage. Expect: The method should be called with the correct parameters. rg --type go -A 5 $'SubmitWithOptions\('Length of output: 2686
proto/da/da.proto (1)
111-111: LGTM!The addition of the
bytes optionsfield to theSubmitRequestmessage aligns with the PR objective of introducing a new method calledSubmitWithOptionsto enable users to pass additional options, including key names. Thebytestype is a generic type that can be used to pass various options, which is one of the potential solutions outlined in the linked issue. The field nameoptionsclearly conveys its purpose of carrying optional data, and the field number4follows the existing field numbering sequence in the message.
yarikbratashchuk
left a comment
yarikbratashchuk
left a comment
There was a problem hiding this comment.
Looks alright to me
Overview
Resolves #93
Summary by CodeRabbit
New Features
SubmitWithOptions, allowing users to submit blobs with additional options.SubmitRequestmessage to include an optional field for extra configuration data.Bug Fixes
Submitmethod to accommodate the new options parameter, improving its functionality.