RPC API: ensure hash field included in batches json #1684
Conversation
WalkthroughThe codebase has been updated to enhance the Changes
TipsChat with CodeRabbit Bot (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 4
Configuration used: CodeRabbit UI
Files selected for processing (3)
- go/common/headers.go (2 hunks)
- integration/networktest/actions/publicdata/tenscan_data.go (1 hunks)
- integration/networktest/tests/tenscan/tenscan_rpc_test.go (1 hunks)
Additional comments: 3
go/common/headers.go (2)
49-53: The addition of the
Hashfield to thebatchHeaderEncodingstruct aligns with the PR objectives to include the hash field in the batch header JSON output.73-79: The update to the
MarshalJSONmethod to marshal the newHashfield is consistent with the PR objectives and ensures the hash field is present in the JSON output.integration/networktest/tests/tenscan/tenscan_rpc_test.go (1)
- 14-24: The
TestRPCfunction has been correctly implemented to verify and debug Tenscan's RPC endpoints using thenetworktestframework. It is designed to run only in an IDE environment, which is suitable for local development and debugging purposes.
| // 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, |
There was a problem hiding this comment.
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.
| // 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 | |
| } |
There was a problem hiding this comment.
Not necessary because hash is a calculated field.
There was a problem hiding this comment.
@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 -->
| // 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) | ||
| } | ||
| if batchListing.BatchesData[0].Number.Cmp(batchListing.BatchesData[1].Number) < 0 { | ||
| return fmt.Errorf("expected batches to be sorted by height descending") | ||
| } | ||
|
|
||
| return nil | ||
| }) | ||
| } |
There was a problem hiding this comment.
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.
| 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) | ||
| } |
There was a problem hiding this comment.
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") | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- integration/obscuroscan/obscuroscan_test.go (1 hunks)
Additional comments: 1
integration/obscuroscan/obscuroscan_test.go (1)
- 136-142: The added assertions correctly check for the descending order of batch numbers and the presence of the "hash" field in the JSON response, aligning with the PR's objective to ensure the hash field is included in the batch header JSON output.
(cherry picked from commit 0fd3773)
Why this change is needed
Tenscan is showing N/A for hash column on batches tables because the field is missing from the json responses. It was lost as part of the json marshalling fixes recently.
What changes were made as part of this PR
Ensure hash field included in the batch header json output.
PR checks pre-merging
Please indicate below by ticking the checkbox that you have read and performed the required
PR checks