Skip to content
This repository was archived by the owner on May 7, 2025. It is now read-only.

feat: add keyName #89

Closed
wants to merge 6 commits into from
Closed

feat: add keyName #89

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
1d6b911
feat: add keyringkeyname
jcstein Aug 27, 2024
483f494
Update README.md
jcstein Aug 27, 2024
fe2e2ee
feat: update protobuf files
jcstein Aug 27, 2024
de917c7
fix: add nil to d.Submit
jcstein Aug 27, 2024
3f7312e
Apply suggestions from code review
jcstein Aug 28, 2024
f22f875
style: use KeyName instead of KeyringKeyname
jcstein Aug 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ go-da defines a generic Data Availability interface for modular blockchains.
| `GetProofs` | `ids []id, namespace Namespace` | `[]Proof` |
| `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` |
Copy link

@coderabbitai coderabbitai bot Aug 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
| `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)


NOTE: The `Namespace` parameter in the interface methods is optional and used
only on DA layers that support the functionality, for example Celestia
Expand Down
6 changes: 5 additions & 1 deletion da.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ type DA interface {
//
// 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)
Copy link

@coderabbitai coderabbitai bot Aug 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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)


// Validate validates Commitments against the corresponding Proofs. This should be possible without retrieving the Blobs.
Validate(ctx context.Context, ids []ID, proofs []Proof, namespace Namespace) ([]bool, error)
Expand All @@ -36,6 +36,10 @@ type DA interface {
// posted to, for DA layers supporting the functionality.
type Namespace = []byte

// KeyName is an optional parameter used to set the keyring keyname for
Copy link
Member

@tzdybal tzdybal Aug 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TBH it's not "optional" as you have to pass something in Go call.

Copy link
Member

@tzdybal tzdybal Aug 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To make it actually optional, we should implement Option pattern and accept them using ... in functions.

// the DA service.
type KeyName = []byte

// Blob is the data submitted/received from DA interface.
type Blob = []byte

Expand Down
11 changes: 8 additions & 3 deletions proto/da/da.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,9 +105,14 @@ message CommitResponse {

// SubmitRequest is the request type for the Submit rpc method.
message SubmitRequest {
repeated Blob blobs = 1;
double gas_price = 2;
Namespace namespace = 3;
repeated Blob blobs = 1;
double gas_price = 2;
Namespace namespace = 3;
KeyName keyName = 4;
}
Comment on lines +108 to +112
Copy link

@coderabbitai coderabbitai bot Aug 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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)

Copy link
Member

@tzdybal tzdybal Aug 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is perfectly valid point and should be reworked.

Copy link

@coderabbitai coderabbitai bot Aug 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.


message KeyName {
bytes value = 1;
}

// SubmitResponse is the response type for the Submit rpc method.
Expand Down
7 changes: 6 additions & 1 deletion proxy/grpc/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,11 +105,16 @@ func (c *Client) Commit(ctx context.Context, blobs []da.Blob, namespace da.Names
}

// Submit submits the Blobs to Data Availability layer.
func (c *Client) Submit(ctx context.Context, blobs []da.Blob, gasPrice float64, namespace da.Namespace) ([]da.ID, error) {
func (c *Client) Submit(ctx context.Context, blobs []da.Blob, gasPrice float64, namespace da.Namespace, keyName da.KeyName) ([]da.ID, error) {
req := &pbda.SubmitRequest{
Blobs: blobsDA2PB(blobs),
GasPrice: gasPrice,
Namespace: &pbda.Namespace{Value: namespace},
Namespace: &pbda.KeyName{Value: keyName},
}
Comment on lines +113 to +114
Copy link

@coderabbitai coderabbitai bot Aug 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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)


if keyName != nil {
req.KeyName = &pbda.KeyName{Value: *keyName}
Comment on lines +116 to +117
Copy link

@coderabbitai coderabbitai bot Aug 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

}

resp, err := c.client.Submit(ctx, req)
Expand Down
7 changes: 6 additions & 1 deletion proxy/grpc/server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,12 @@ func (p *proxySrv) GetProofs(ctx context.Context, request *pbda.GetProofsRequest
func (p *proxySrv) Submit(ctx context.Context, request *pbda.SubmitRequest) (*pbda.SubmitResponse, error) {
blobs := blobsPB2DA(request.Blobs)

ids, err := p.target.Submit(ctx, blobs, request.GasPrice, request.Namespace.GetValue())
var keyName da.KeyName
if request.KeyName != nil {
keyName = (da.KeyName)(&request.KeyName.Value)
}

ids, err := p.target.Submit(ctx, blobs, request.GasPrice, request.Namespace.GetValue(), request.KeyName.GetValue())
Comment on lines +72 to +77
Copy link

@coderabbitai coderabbitai bot Aug 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

if err != nil {
return nil, err
}
Expand Down
6 changes: 3 additions & 3 deletions proxy/jsonrpc/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ type API struct {
GetProofs func(ctx context.Context, ids []da.ID, ns da.Namespace) ([]da.Proof, error) `perm:"read"`
Commit func(ctx context.Context, blobs []da.Blob, ns da.Namespace) ([]da.Commitment, error) `perm:"read"`
Validate func(context.Context, []da.ID, []da.Proof, da.Namespace) ([]bool, error) `perm:"read"`
Submit func(context.Context, []da.Blob, float64, da.Namespace) ([]da.ID, error) `perm:"write"`
Submit func(context.Context, []da.Blob, float64, da.Namespace, da.KeyName) ([]da.ID, error) `perm:"write"`
}
}

Expand Down Expand Up @@ -61,8 +61,8 @@ func (api *API) Validate(ctx context.Context, ids []da.ID, proofs []da.Proof, ns
}

// Submit submits the Blobs to Data Availability layer.
func (api *API) Submit(ctx context.Context, blobs []da.Blob, gasPrice float64, ns da.Namespace) ([]da.ID, error) {
return api.Internal.Submit(ctx, blobs, gasPrice, ns)
func (api *API) Submit(ctx context.Context, blobs []da.Blob, gasPrice float64, ns da.Namespace, keyName da.KeyName) ([]da.ID, error) {
return api.Internal.Submit(ctx, blobs, gasPrice, ns, keyName)
}

// Client is the jsonrpc client
Expand Down
2 changes: 1 addition & 1 deletion test/dummy.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,7 @@ func (d *DummyDA) Commit(ctx context.Context, blobs []da.Blob, _ da.Namespace) (
}

// Submit stores blobs in DA layer.
func (d *DummyDA) Submit(ctx context.Context, blobs []da.Blob, gasPrice float64, _ da.Namespace) ([]da.ID, error) {
func (d *DummyDA) Submit(ctx context.Context, blobs []da.Blob, gasPrice float64, _ da.Namespace, keyName *da.KeyName) ([]da.ID, error) {
d.mu.Lock()
defer d.mu.Unlock()
ids := make([]da.ID, len(blobs))
Expand Down
10 changes: 5 additions & 5 deletions test/test_suite.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,15 +40,15 @@ func BasicDATest(t *testing.T, d da.DA) {
msg2 := []byte("message 2")

ctx := context.TODO()
id1, err := d.Submit(ctx, []da.Blob{msg1}, 0, testNamespace)
id1, err := d.Submit(ctx, []da.Blob{msg1}, 0, testNamespace, nil)
assert.NoError(t, err)
assert.NotEmpty(t, id1)

id2, err := d.Submit(ctx, []da.Blob{msg2}, 0, testNamespace)
id2, err := d.Submit(ctx, []da.Blob{msg2}, 0, testNamespace, nil)
assert.NoError(t, err)
assert.NotEmpty(t, id2)

id3, err := d.Submit(ctx, []da.Blob{msg1}, 0, testNamespace)
id3, err := d.Submit(ctx, []da.Blob{msg1}, 0, testNamespace, nil)
assert.NoError(t, err)
assert.NotEmpty(t, id3)

Expand Down Expand Up @@ -92,7 +92,7 @@ func GetIDsTest(t *testing.T, d da.DA) {
msgs := [][]byte{[]byte("msg1"), []byte("msg2"), []byte("msg3")}

ctx := context.TODO()
ids, err := d.Submit(ctx, msgs, 0, testNamespace)
ids, err := d.Submit(ctx, msgs, 0, testNamespace, nil)
assert.NoError(t, err)
assert.Len(t, ids, len(msgs))

Expand Down Expand Up @@ -147,7 +147,7 @@ func ConcurrentReadWriteTest(t *testing.T, d da.DA) {
go func() {
defer wg.Done()
for i := uint64(1); i <= 100; i++ {
_, err := d.Submit(ctx, [][]byte{[]byte("test")}, 0, testNamespace)
_, err := d.Submit(ctx, [][]byte{[]byte("test")}, 0, testNamespace, nil)
assert.NoError(t, err)
}
}()
Expand Down
Loading