Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RPC API: ensure hash field included in batches json #1684

Merged
merged 2 commits into from
Dec 7, 2023
Merged

RPC API: ensure hash field included in batches json #1684

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f904c8f
RPC API: ensure hash field included in batches json
BedrockSquirrel Dec 7, 2023
c9b3f04
review comments: add tests
BedrockSquirrel Dec 7, 2023
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: 2 additions & 0 deletions go/common/headers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ type BatchHeader struct {
}

type batchHeaderEncoding struct {
Hash common.Hash `json:"hash"`
ParentHash L2BatchHash `json:"parentHash"`
Root common.Hash `json:"stateRoot"`
TxHash common.Hash `json:"transactionsRoot"`
Expand All @@ -72,6 +73,7 @@ type batchHeaderEncoding struct {
// MarshalJSON custom marshals the BatchHeader into a json
func (b *BatchHeader) MarshalJSON() ([]byte, error) {
return json.Marshal(batchHeaderEncoding{
b.Hash(),
b.ParentHash,
b.Root,
b.TxHash,
Comment on lines 73 to 79
Copy link

@coderabbitai coderabbitai bot Dec 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

The UnmarshalJSON method should be updated to handle the new Hash field to maintain symmetry with the MarshalJSON method.

func (b *BatchHeader) UnmarshalJSON(data []byte) error {
	dec := new(batchHeaderEncoding)
	err := json.Unmarshal(data, dec)
	if err != nil {
		return err
	}

+	b.Hash = dec.Hash
	b.ParentHash = dec.ParentHash
	// ... rest of the fields ...
	return nil
}
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.

Suggested change
// MarshalJSON custom marshals the BatchHeader into a json
func (b *BatchHeader) MarshalJSON() ([]byte, error) {
return json.Marshal(batchHeaderEncoding{
b.Hash(),
b.ParentHash,
b.Root,
b.TxHash,
func (b *BatchHeader) UnmarshalJSON(data []byte) error {
dec := new(batchHeaderEncoding)
err := json.Unmarshal(data, dec)
if err != nil {
return err
}
b.Hash = dec.Hash
b.ParentHash = dec.ParentHash
// ... rest of the fields ...
return nil
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not necessary because hash is a calculated field.

Copy link

Choose a reason for hiding this comment

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

@BedrockSquirrel Understood, the Hash field is computed using the Hash() method and does not need to be unmarshaled from JSON. Thank you for the clarification.

(•ㅅ•) 
/ 　 づ```

<!-- This is an auto-generated reply by CodeRabbit -->

Expand Down
41 changes: 41 additions & 0 deletions integration/networktest/actions/publicdata/tenscan_data.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
package publicdata

import (
"context"
"fmt"

"github.com/ten-protocol/go-ten/go/common"
"github.com/ten-protocol/go-ten/go/obsclient"
"github.com/ten-protocol/go-ten/integration/networktest"
"github.com/ten-protocol/go-ten/integration/networktest/actions"
)

// VerifyBatchesDataAction tests the batches data RPC endpoint
func VerifyBatchesDataAction() networktest.Action {
return actions.VerifyOnlyAction(func(ctx context.Context, network networktest.NetworkConnector) error {
client, err := obsclient.Dial(network.ValidatorRPCAddress(0))
if err != nil {
return err
}

pagination := common.QueryPagination{
Offset: 0,
Size: 20,
}
batchListing, err := client.GetBatchesListing(&pagination)
if err != nil {
return err
}
if len(batchListing.BatchesData) != 20 {
return fmt.Errorf("expected 20 batches, got %d", len(batchListing.BatchesData))
}
if batchListing.Total <= 10 {
return fmt.Errorf("expected more than 10 batches, got %d", batchListing.Total)
}
Comment on lines +21 to +34
Copy link

Choose a reason for hiding this comment

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

The hardcoded values for pagination (Size: 20) and batch count checks (expected 20 batches, expected more than 10 batches) may not be suitable for all test scenarios. Consider parameterizing these values to make the function more flexible and capable of handling different testing requirements.

if batchListing.BatchesData[0].Number.Cmp(batchListing.BatchesData[1].Number) < 0 {
return fmt.Errorf("expected batches to be sorted by height descending")
}
Comment on lines +35 to +37
Copy link

Choose a reason for hiding this comment

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

The sorting check only compares the first two batches for descending order. To ensure robustness, consider iterating over the entire list of batches to verify that they are sorted correctly by height in descending order.


return nil
})
}
Comment on lines +13 to +41
Copy link

Choose a reason for hiding this comment

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

The function VerifyBatchesDataAction correctly sets up a client and performs validation checks on the retrieved batch data. However, it does not check for the presence of the hash field in the batch data, which is the main objective of the PR. Consider adding a check to ensure that the hash field is present and correctly populated for each batch in the listing.

24 changes: 24 additions & 0 deletions integration/networktest/tests/tenscan/tenscan_rpc_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
package tenscan

import (
"testing"

"github.com/ten-protocol/go-ten/integration/networktest"
"github.com/ten-protocol/go-ten/integration/networktest/actions"
"github.com/ten-protocol/go-ten/integration/networktest/actions/publicdata"
"github.com/ten-protocol/go-ten/integration/networktest/env"
)

// Verify and debug the RPC endpoints that Tenscan relies on for data in various environments

func TestRPC(t *testing.T) {
networktest.TestOnlyRunsInIDE(t)
networktest.Run(
"tenscan-rpc-data",
t,
env.LocalDevNetwork(),
actions.Series(
publicdata.VerifyBatchesDataAction(),
),
)
}
4 changes: 4 additions & 0 deletions integration/obscuroscan/obscuroscan_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,10 @@ func TestObscuroscan(t *testing.T) {
assert.NoError(t, err)
assert.LessOrEqual(t, 9, len(batchlistingObj.Result.BatchesData))
assert.LessOrEqual(t, uint64(9), batchlistingObj.Result.Total)
// check results are descending order (latest first)
assert.LessOrEqual(t, batchlistingObj.Result.BatchesData[1].Number.Cmp(batchlistingObj.Result.BatchesData[0].Number), 0)
// check "hash" field is included in json response
assert.Contains(t, string(body), "\"hash\"")

statusCode, body, err = fasthttp.Get(nil, fmt.Sprintf("%s/items/blocks/?offset=0&size=10", serverAddress))
assert.NoError(t, err)
Expand Down
Loading