feat: add keyName #89
Conversation
WalkthroughThe changes involve modifying the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Server
participant DA
Client->>Server: Submit(blobs, gasPrice, namespace, keyName)
Server->>DA: Submit(ctx, blobs, gasPrice, namespace, keyName)
DA-->>Server: Response(ID, error)
Server-->>Client: Response(ID, error)
Assessment against linked issues
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 Configuration File (
|
RollkitBot
requested review from
a team,
MSevey,
tuxcanfly and
tzdybal
and removed request for
a team
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (8)
- README.md (1 hunks)
- da.go (2 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)
Additional context used
GitHub Check: lint / golangci-lint
proxy/grpc/server.go
[failure] 73-73:
request.Keyringkeyname undefined (type *"github.com/rollkit/go-da/types/pb/da".SubmitRequest has no field or method Keyringkeyname)
[failure] 74-74:
request.Keyringkeyname undefined (type *"github.com/rollkit/go-da/types/pb/da".SubmitRequest has no field or method Keyringkeyname) (typecheck)
[failure] 73-73:
request.Keyringkeyname undefined (type *"github.com/rollkit/go-da/types/pb/da".SubmitRequest has no field or method Keyringkeyname)
[failure] 74-74:
request.Keyringkeyname undefined (type *"github.com/rollkit/go-da/types/pb/da".SubmitRequest has no field or method Keyringkeyname)) (typecheck)proxy/grpc/client.go
[failure] 116-116:
req.Keyringkeyname undefined (type *"github.com/rollkit/go-da/types/pb/da".SubmitRequest has no field or method Keyringkeyname)
[failure] 116-116:
undefined: pbda.Keyringkeyname
[failure] 116-116:
req.Keyringkeyname undefined (type *"github.com/rollkit/go-da/types/pb/da".SubmitRequest has no field or method Keyringkeyname)
[failure] 116-116:
undefined: pbda.Keyringkeyname
GitHub Check: test / Run Unit Tests
proxy/grpc/server.go
[failure] 73-73:
request.Keyringkeyname undefined (type *"github.com/rollkit/go-da/types/pb/da".SubmitRequest has no field or method Keyringkeyname)
[failure] 74-74:
request.Keyringkeyname undefined (type *"github.com/rollkit/go-da/types/pb/da".SubmitRequest has no field or method Keyringkeyname)proxy/grpc/client.go
[failure] 116-116:
req.Keyringkeyname undefined (type *"github.com/rollkit/go-da/types/pb/da".SubmitRequest has no field or method Keyringkeyname)
[failure] 116-116:
undefined: pbda.Keyringkeyname
LanguageTool
README.md
[duplication] ~23-~23: Possible typo: you repeated a word
Context: ... |[]bool| |Submit|blobs []Blob, gasPrice float64, namespace Namespace, keyringkeyname Keyringkeyname | `[...(ENGLISH_WORD_REPEAT_RULE)
[duplication] ~23-~23: Possible typo: you repeated a word
Context: ...gasPrice float64, namespace Namespace, keyringkeyname Keyringkeyname |[]ID| NOTE: TheNamespace` pa...(ENGLISH_WORD_REPEAT_RULE)
Additional comments not posted (10)
da.go (2)
39-42: LGTM!The new type alias
Keyringkeynameis correctly defined.The code changes are approved.
29-29: Verify impact on existing implementations.The
Submitmethod signature has been updated to include an additional parameterkeyringkeyname. Ensure that all existing implementations of theDAinterface are updated accordingly to handle this new parameter.Run the following script to identify all implementations of the
Submitmethod:proto/da/da.proto (2)
111-112: LGTM!The addition of the
Keyringkeynamefield in theSubmitRequestmessage enhances the functionality by allowing it to carry additional metadata related to key management.The code changes are approved.
114-116: LGTM!The
Keyringkeynamemessage is correctly defined and enhances the Protocol Buffers definitions by allowing the inclusion of keyring key names.The code changes are approved.
proxy/grpc/client.go (2)
108-108: LGTM!The
Submitfunction signature is correctly updated to include the new parameterkeyringkeyname.The code changes are approved.
115-117: Verify thekeyringkeynamefield definition and usage.The implementation correctly handles the new parameter. However, the static analysis tools indicate that the
keyringkeynamefield might be undefined.Run the following script to verify the
keyringkeynamefield definition and usage:Verification successful
The
keyringkeynamefield is correctly defined and used.The
Keyringkeynamefield is defined as amessagein theproto/da/da.protofile and is used consistently across the codebase, including in theSubmitfunction. The initial concern about it being potentially undefined is unfounded.
- Definition:
proto/da/da.proto- Usage: Various function signatures, including
Submitinproxy/grpc/client.go.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the `keyringkeyname` field definition and usage. # Test: Search for the `keyringkeyname` field definition. Expect: Only valid definitions. rg --type proto 'Keyringkeyname'Length of output: 62
Script:
#!/bin/bash # Description: Search for the `Keyringkeyname` field definition and usage across all files. # Search for the `Keyringkeyname` field in the entire codebase. rg 'Keyringkeyname'Length of output: 1542
Tools
GitHub Check: lint / golangci-lint
[failure] 116-116:
req.Keyringkeyname undefined (type *"github.com/rollkit/go-da/types/pb/da".SubmitRequest has no field or method Keyringkeyname)
[failure] 116-116:
undefined: pbda.Keyringkeyname
[failure] 116-116:
req.Keyringkeyname undefined (type *"github.com/rollkit/go-da/types/pb/da".SubmitRequest has no field or method Keyringkeyname)
[failure] 116-116:
undefined: pbda.KeyringkeynameGitHub Check: test / Run Unit Tests
[failure] 116-116:
req.Keyringkeyname undefined (type *"github.com/rollkit/go-da/types/pb/da".SubmitRequest has no field or method Keyringkeyname)
[failure] 116-116:
undefined: pbda.Keyringkeynameproxy/jsonrpc/client.go (2)
29-29: LGTM!The
Submitfunction signature is correctly updated to include the new parameterkeyringkeyname.The code changes are approved.
64-65: LGTM!The implementation correctly handles the new parameter
keyringkeyname.The code changes are approved.
test/dummy.go (1)
125-125: Verify the necessity and usage of the new parameter.The new parameter
keyringkeynameis added to the function signature but is not used within the function. Verify if this is intentional or if the parameter should be utilized within the function.Please confirm if the
keyringkeynameparameter is intended to be used within theSubmitfunction. If not, consider removing it to avoid confusion.test/test_suite.go (1)
43-43: Verify the usage of the new parameter.The
Submitfunction call now includes an additional parameter,nil. Ensure that this aligns with the intended usage of theSubmitfunction and thatnilis an appropriate value for the new parameter.Please confirm if
nilis the intended value for the new parameter in theSubmitfunction call. If not, provide the appropriate value.
proxy/grpc/server.go
Outdated
| var keyringkeyname *da.Keyringkeyname | ||
| if request.Keyringkeyname != nil { | ||
| keyringkeyname = (*da.Keyringkeyname)(&request.Keyringkeyname.Value) | ||
| } | ||
|
|
There was a problem hiding this comment.
Fix undefined request.Keyringkeyname.
The request.Keyringkeyname field is undefined. Ensure that the SubmitRequest type in the pbda package includes the Keyringkeyname field.
Apply this diff to define the Keyringkeyname field in the SubmitRequest type:
// In the `pbda` package, update the `SubmitRequest` type definition to include the `Keyringkeyname` field.
type SubmitRequest struct {
// Other fields...
Keyringkeyname *Keyringkeyname `protobuf:"bytes,5,opt,name=keyringkeyname" json:"keyringkeyname,omitempty"`
}Committable suggestion was skipped due to low confidence.
Tools
GitHub Check: lint / golangci-lint
[failure] 73-73:
request.Keyringkeyname undefined (type *"github.com/rollkit/go-da/types/pb/da".SubmitRequest has no field or method Keyringkeyname)
[failure] 74-74:
request.Keyringkeyname undefined (type *"github.com/rollkit/go-da/types/pb/da".SubmitRequest has no field or method Keyringkeyname) (typecheck)
[failure] 73-73:
request.Keyringkeyname undefined (type *"github.com/rollkit/go-da/types/pb/da".SubmitRequest has no field or method Keyringkeyname)
[failure] 74-74:
request.Keyringkeyname undefined (type *"github.com/rollkit/go-da/types/pb/da".SubmitRequest has no field or method Keyringkeyname)) (typecheck)
GitHub Check: test / Run Unit Tests
[failure] 73-73:
request.Keyringkeyname undefined (type *"github.com/rollkit/go-da/types/pb/da".SubmitRequest has no field or method Keyringkeyname)
[failure] 74-74:
request.Keyringkeyname undefined (type *"github.com/rollkit/go-da/types/pb/da".SubmitRequest has no field or method Keyringkeyname)
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- README.md (1 hunks)
Additional context used
LanguageTool
README.md
[duplication] ~23-~23: Possible typo: you repeated a word
Context: ... |[]bool| |Submit|blobs []Blob, gasPrice float64, namespace Namespace, keyringkeyname Keyringkeyname| `[...(ENGLISH_WORD_REPEAT_RULE)
Additional comments not posted (1)
README.md (1)
23-23: Fix possible typo.There is a possible typo in the
Submitmethod signature documentation. Ensure that the method signature is correctly documented.Apply this diff to fix the typo:
- | `Submit` | `blobs []Blob, gasPrice float64, namespace Namespace, keyringkeyname Keyringkeyname` | `[]ID` | + | `Submit` | `blobs []Blob, gasPrice float64, namespace Namespace, keyringkeyname Keyringkeyname` | `[]ID` |Likely invalid or redundant comment.
Tools
LanguageTool
[duplication] ~23-~23: Possible typo: you repeated a word
Context: ... |[]bool| |Submit|blobs []Blob, gasPrice float64, namespace Namespace, keyringkeyname Keyringkeyname| `[...(ENGLISH_WORD_REPEAT_RULE)
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #89 +/- ##
==========================================
+ Coverage 75.54% 78.26% +2.71%
==========================================
Files 5 5
Lines 274 230 -44
==========================================
- Hits 207 180 -27
+ Misses 52 35 -17
Partials 15 15 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 0
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 (2)
- proto/da/da.proto (1 hunks)
- test/test_suite.go (3 hunks)
Files skipped from review as they are similar to previous changes (2)
- proto/da/da.proto
- test/test_suite.go
tuxcanfly
left a comment
tuxcanfly
left a comment
There was a problem hiding this comment.
Since node's jsonrpc library doesn't support optional params, older jsonrpc clients go-da <= 0.5.0 will not be compatible as they'll break with:
RPC error (-32602): wrong param count (method 'da.Submit'): 3 != 4
One alternative might be to add a new method SubmitWithOptions which includes an opaque []byte SubmitOptions.
type SubmitOptions = []byte
type DA interface {
...
SubmitWithOptions(ctx context.Context, blobs []Blob, namespace Namespace, options SubmitOptions) ([]ID, error)
...
}
In celestia-node we can pass this directly to TxConfig.UnmarshalJSON to support all other txconfig options such as signer_address, fee_granter_address as well e.g.:
func (s *Service) SubmitWithOptions(
ctx context.Context,
daBlobs []da.Blob,
options da.SubmitOptions)
) ([]da.ID, error) {
blobs, _, err := s.blobsAndCommitments(daBlobs, namespace)
if err != nil {
return nil, err
}
opts := state.NewTxConfig()
err = opts.UnmarshalJSON(options)
This allows older clients to continue working while newer clients can support new features such as multiple accounts, fee granter etc.
da.go
Outdated
|
|
||
| // Keyringkeyname is an optional parameter used to set the keyring keyname for | ||
| // the DA service. | ||
| type Keyringkeyname = []byte |
There was a problem hiding this comment.
KeyName sounds less repetitive.
There was a problem hiding this comment.
OK. I was just trying to match UX on node.
|
Got it! I understand, and thank you for the review. Confirming my understanding, that |
Co-authored-by: Javed Khan <tuxcanfly@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 3
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- da.go (2 hunks)
- proto/da/da.proto (1 hunks)
- proxy/grpc/client.go (1 hunks)
- proxy/grpc/server.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- proto/da/da.proto
Additional context used
GitHub Check: test / Run Unit Tests
da.go
[failure] 29-29:
undefined: keyName
GitHub Check: lint / golangci-lint
da.go
[failure] 29-29:
undefined: keyName (typecheck)
[failure] 29-29:
undefined: keyName) (typecheck)
[failure] 29-29:
undefined: keyName) (typecheck)proxy/grpc/server.go
[failure] 77-77:
request.KeyName undefined (type *da.SubmitRequest has no field or method KeyName) (typecheck)proxy/grpc/client.go
[failure] 113-113:
undefined: pbda.KeyName (typecheck)
golangci-lint
proxy/grpc/server.go
77-77: request.KeyName undefined (type *da.SubmitRequest has no field or method KeyName)
(typecheck)
proxy/grpc/client.go
113-113: undefined: pbda.KeyName
(typecheck)
Additional comments not posted (1)
da.go (1)
39-41: LGTM!The type alias
Keyringkeynameis correctly defined.The code changes are approved.
proxy/grpc/server.go
Outdated
There was a problem hiding this comment.
Fix undefined KeyName.
The request.KeyName field is undefined. Ensure that the correct field name is used.
Apply this diff to fix the issue:
ids, err := p.target.Submit(ctx, blobs, request.GasPrice, request.Namespace.GetValue(), keyringkeyname)Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var keyringkeyname *da.Keyringkeyname | |
| if request.Keyringkeyname != nil { | |
| keyringkeyname = (*da.Keyringkeyname)(&request.Keyringkeyname.Value) | |
| } | |
| ids, err := p.target.Submit(ctx, blobs, request.GasPrice, request.Namespace.GetValue(), request.KeyName.GetValue()) | |
| var keyringkeyname *da.Keyringkeyname | |
| if request.Keyringkeyname != nil { | |
| keyringkeyname = (*da.Keyringkeyname)(&request.Keyringkeyname.Value) | |
| } | |
| ids, err := p.target.Submit(ctx, blobs, request.GasPrice, request.Namespace.GetValue(), keyringkeyname) |
Tools
golangci-lint
77-77: request.KeyName undefined (type *da.SubmitRequest has no field or method KeyName)
(typecheck)
GitHub Check: lint / golangci-lint
[failure] 77-77:
request.KeyName undefined (type *da.SubmitRequest has no field or method KeyName) (typecheck)
proxy/grpc/client.go
Outdated
There was a problem hiding this comment.
Fix undefined KeyName.
The KeyName field is undefined. Ensure that the correct field name is used.
Apply this diff to fix the issue:
Namespace: &pbda.Namespace{Value: namespace},
Keyringkeyname: &pbda.Keyringkeyname{Value: *keyringkeyname},Committable suggestion was skipped due to low confidence.
Tools
golangci-lint
113-113: undefined: pbda.KeyName
(typecheck)
GitHub Check: lint / golangci-lint
[failure] 113-113:
undefined: pbda.KeyName (typecheck)
There was a problem hiding this comment.
Actionable comments posted: 6
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (7)
- README.md (1 hunks)
- da.go (2 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)
Files skipped from review as they are similar to previous changes (2)
- proxy/jsonrpc/client.go
- test/dummy.go
Additional context used
GitHub Check: test / Run Unit Tests
da.go
[failure] 29-29:
undefined: keyName
GitHub Check: lint / golangci-lint
da.go
[failure] 29-29:
undefined: keyName (typecheck)
[failure] 29-29:
undefined: keyName) (typecheck)proxy/grpc/server.go
[failure] 73-73:
request.KeyName undefined (type *da.SubmitRequest has no field or method KeyName) (typecheck)
[failure] 77-77:
request.KeyName undefined (type *da.SubmitRequest has no field or method KeyName) (typecheck)proxy/grpc/client.go
[failure] 113-113:
undefined: pbda.KeyName (typecheck)
[failure] 117-117:
req.KeyName undefined (type *da.SubmitRequest has no field or method KeyName) (typecheck)
golangci-lint
proxy/grpc/server.go
73-73: request.KeyName undefined (type *da.SubmitRequest has no field or method KeyName)
(typecheck)
77-77: request.KeyName undefined (type *da.SubmitRequest has no field or method KeyName)
(typecheck)
proxy/grpc/client.go
113-113: undefined: pbda.KeyName
(typecheck)
117-117: req.KeyName undefined (type *da.SubmitRequest has no field or method KeyName)
(typecheck)
LanguageTool
README.md
[duplication] ~23-~23: Possible typo: you repeated a word
Context: ... |[]bool| |Submit|blobs []Blob, gasPrice float64, namespace Namespace, keyName keyName|[]ID| NOTE: ...(ENGLISH_WORD_REPEAT_RULE)
buf
proto/da/da.proto
111-111: Field name "keyName" should be lower_snake_case, such as "key_name".
(FIELD_LOWER_SNAKE_CASE)
Additional comments not posted (3)
da.go (1)
39-42: LGTM!The type alias
KeyNameis correctly defined as a byte slice.proto/da/da.proto (1)
114-115: LGTM!The
KeyNamemessage is correctly defined and enhances the functionality.The code changes are approved.
proxy/grpc/client.go (1)
108-108: LGTM!The addition of the
keyNameparameter enhances the functionality of theSubmitfunction.The code changes are approved.
| // This method is synchronous. Upon successful submission to Data Availability layer, it returns the IDs identifying blobs | ||
| // in DA. | ||
| Submit(ctx context.Context, blobs []Blob, gasPrice float64, namespace Namespace) ([]ID, error) | ||
| Submit(ctx context.Context, blobs []Blob, gasPrice float64, namespace Namespace, keyName keyName) ([]ID, error) |
There was a problem hiding this comment.
Fix undefined keyName.
The keyName parameter is undefined. Ensure that the parameter name matches the type alias KeyName.
Apply this diff to fix the issue:
- Submit(ctx context.Context, blobs []Blob, gasPrice float64, namespace Namespace, keyName keyName) ([]ID, error)
+ Submit(ctx context.Context, blobs []Blob, gasPrice float64, namespace Namespace, keyName KeyName) ([]ID, error)Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Submit(ctx context.Context, blobs []Blob, gasPrice float64, namespace Namespace, keyName keyName) ([]ID, error) | |
| Submit(ctx context.Context, blobs []Blob, gasPrice float64, namespace Namespace, keyName KeyName) ([]ID, error) |
Tools
GitHub Check: test / Run Unit Tests
[failure] 29-29:
undefined: keyName
GitHub Check: lint / golangci-lint
[failure] 29-29:
undefined: keyName (typecheck)
[failure] 29-29:
undefined: keyName) (typecheck)
proxy/grpc/server.go
Outdated
There was a problem hiding this comment.
Fix undefined request.KeyName.
The request.KeyName field is undefined. Ensure that the SubmitRequest type in the pbda package includes the KeyName field.
Apply this diff to define the KeyName field in the SubmitRequest type:
// In the `pbda` package, update the `SubmitRequest` type definition to include the `KeyName` field.
type SubmitRequest struct {
// Other fields...
KeyName *KeyName `protobuf:"bytes,5,opt,name=keyName" json:"keyName,omitempty"`
}Committable suggestion was skipped due to low confidence.
Tools
golangci-lint
73-73: request.KeyName undefined (type *da.SubmitRequest has no field or method KeyName)
(typecheck)
77-77: request.KeyName undefined (type *da.SubmitRequest has no field or method KeyName)
(typecheck)
GitHub Check: lint / golangci-lint
[failure] 73-73:
request.KeyName undefined (type *da.SubmitRequest has no field or method KeyName) (typecheck)
[failure] 77-77:
request.KeyName undefined (type *da.SubmitRequest has no field or method KeyName) (typecheck)
| | `Commit` | `blobs []Blob, namespace Namespace` | `[]Commitment` | | ||
| | `Validate` | `ids []Blob, proofs []Proof, namespace Namespace` | `[]bool` | | ||
| | `Submit` | `blobs []Blob, gasPrice float64, namespace Namespace` | `[]ID` | | ||
| | `Submit` | `blobs []Blob, gasPrice float64, namespace Namespace, keyName keyName` | `[]ID` | |
There was a problem hiding this comment.
Fix possible typo.
There is a possible typo in the Submit method signature documentation. Ensure that the method signature is correctly documented.
Apply this diff to fix the typo:
- | `Submit` | `blobs []Blob, gasPrice float64, namespace Namespace, keyName keyName` | `[]ID` |
+ | `Submit` | `blobs []Blob, gasPrice float64, namespace Namespace, keyName KeyName` | `[]ID` |Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| | `Submit` | `blobs []Blob, gasPrice float64, namespace Namespace, keyName keyName` | `[]ID` | | |
| | `Submit` | `blobs []Blob, gasPrice float64, namespace Namespace, keyName KeyName` | `[]ID` | |
Tools
LanguageTool
[duplication] ~23-~23: Possible typo: you repeated a word
Context: ... |[]bool| |Submit|blobs []Blob, gasPrice float64, namespace Namespace, keyName keyName|[]ID| NOTE: ...(ENGLISH_WORD_REPEAT_RULE)
| repeated Blob blobs = 1; | ||
| double gas_price = 2; | ||
| Namespace namespace = 3; | ||
| KeyName keyName = 4; | ||
| } |
There was a problem hiding this comment.
Use lower_snake_case for field names.
The field name keyName should be in lower_snake_case to follow protobuf naming conventions. Change it to key_name.
Apply this diff to fix the naming convention:
message SubmitRequest {
repeated Blob blobs = 1;
double gas_price = 2;
Namespace namespace = 3;
- KeyName keyName = 4;
+ KeyName key_name = 4;
}Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| repeated Blob blobs = 1; | |
| double gas_price = 2; | |
| Namespace namespace = 3; | |
| KeyName keyName = 4; | |
| } | |
| repeated Blob blobs = 1; | |
| double gas_price = 2; | |
| Namespace namespace = 3; | |
| KeyName key_name = 4; |
Tools
buf
111-111: Field name "keyName" should be lower_snake_case, such as "key_name".
(FIELD_LOWER_SNAKE_CASE)
There was a problem hiding this comment.
This is perfectly valid point and should be reworked.
There was a problem hiding this comment.
@tzdybal, thanks for confirming! The change to lower_snake_case for the field name will ensure consistency with protobuf naming conventions. Let's proceed with reworking it as suggested.
proxy/grpc/client.go
Outdated
There was a problem hiding this comment.
Fix incorrect field usage.
The field req.KeyName is incorrectly used, causing type errors and undefined field issues. Use the correct field name key_name.
Apply this diff to fix the issue:
if keyName != nil {
- req.KeyName = &pbda.KeyName{Value: *keyName}
+ req.key_name = &pbda.KeyName{Value: *keyName}
}Committable suggestion was skipped due to low confidence.
Tools
golangci-lint
117-117: req.KeyName undefined (type *da.SubmitRequest has no field or method KeyName)
(typecheck)
GitHub Check: lint / golangci-lint
[failure] 117-117:
req.KeyName undefined (type *da.SubmitRequest has no field or method KeyName) (typecheck)
proxy/grpc/client.go
Outdated
There was a problem hiding this comment.
Fix incorrect field usage.
The field Namespace is incorrectly used for KeyName, causing type errors and undefined field issues. Use the correct field name KeyName.
Apply this diff to fix the issue:
req := &pbda.SubmitRequest{
Blobs: blobsDA2PB(blobs),
GasPrice: gasPrice,
Namespace: &pbda.Namespace{Value: namespace},
- Namespace: &pbda.KeyName{Value: keyName},
+ KeyName: &pbda.KeyName{Value: keyName},
}Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Namespace: &pbda.KeyName{Value: keyName}, | |
| } | |
| req := &pbda.SubmitRequest{ | |
| Blobs: blobsDA2PB(blobs), | |
| GasPrice: gasPrice, | |
| Namespace: &pbda.Namespace{Value: namespace}, | |
| KeyName: &pbda.KeyName{Value: keyName}, | |
| } |
Tools
golangci-lint
113-113: undefined: pbda.KeyName
(typecheck)
GitHub Check: lint / golangci-lint
[failure] 113-113:
undefined: pbda.KeyName (typecheck)
tzdybal
left a comment
tzdybal
left a comment
There was a problem hiding this comment.
I'm trying to be aggressively neutral here.
Key name seems to be something quite specific to celestia-node. It probably could be translated to other implementations as well, but I can also imagine system where you can specify actual key (not nick-name).
And to be even more generic, maybe we should pass just some bytes, that could be passed by rollkit via go-da and interpreted by specific da implementations?
| // posted to, for DA layers supporting the functionality. | ||
| type Namespace = []byte | ||
|
|
||
| // KeyName is an optional parameter used to set the keyring keyname for |
There was a problem hiding this comment.
TBH it's not "optional" as you have to pass something in Go call.
There was a problem hiding this comment.
To make it actually optional, we should implement Option pattern and accept them using ... in functions.
| repeated Blob blobs = 1; | ||
| double gas_price = 2; | ||
| Namespace namespace = 3; | ||
| KeyName keyName = 4; | ||
| } |
There was a problem hiding this comment.
This is perfectly valid point and should be reworked.
|
Replaced by #95 |
|
worth a shot! thank you for making #95 and apologies for letting this go stale |
3 participants
Overview
Matches evstack/ev-node#1820
Partially resolves evstack/ev-node#1821
Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Documentation