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.

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

feat(encoding): remove weird type overhead thing #1691

Merged
merged 4 commits into from
Jul 4, 2024
Merged

Conversation

itsdevbear
Copy link
Contributor

@itsdevbear itsdevbear commented Jul 4, 2024

Summary by CodeRabbit

  • Bug Fixes

    • Fixed several import paths to ensure compatibility and prevent errors in various modules.
  • Refactor

    • Simplified function signatures and removed unnecessary parameters to streamline code and improve maintainability.
  • Chores

    • Added a new unit benchmark testing job to the CI pipeline for more robust testing.
    • Updated test cases to improve clarity and reliability.
    • Introduced a new target in the build scripts for running Golang unit benchmarks.

@itsdevbear itsdevbear requested a review from ocnc as a code owner July 4, 2024 16:06
Copy link
Contributor

coderabbitai bot commented Jul 4, 2024

Warning

Rate limit exceeded

@itsdevbear has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 0 minutes and 26 seconds before requesting another review.

How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

Commits

Files that changed from the base of the PR and between 3761865 and efe0631.

Walkthrough

The recent changes primarily involve updating import paths, removing unnecessary parameters and globals, and refining error handling mechanisms across various packages. Notable updates include the replacement of the hex package with encoding/hex, modifications to JSON unmarshaling, and the introduction of a new unit benchmarking job in the pipeline. These changes streamline the codebase, improve clarity, and enhance testing capabilities.

Changes

Files/Packages Change Summary
mod/primitives/pkg/bytes/... Updated imports from pkg/hex to encoding/hex; refactored serialization methods.
mod/primitives/pkg/eip4844/... Removed reflection in UnmarshalJSON functions for Blob and KZGCommitment.
mod/primitives/pkg/encoding/hex/... Simplified decode functions and error handling; updated import paths.
mod/primitives/pkg/math/... Modified import paths; refactored UnmarshalJSON (e.g., u64.go, u256.go).
.github/workflows/pipeline.yml Added a new job for unit bench testing to the CI pipeline.
build/scripts/testing.mk Introduced test-unit-bench target for running Golang unit benchmarks.
mod/storage/pkg/filedb/range_db.go Updated hex package import paths.
mod/primitives/pkg/merkle/tree_test.go Replaced global treeDepth constant with local variable assignments.

Poem

In code we trust, with bytes we play,
From hex to encoding, we've found our way.
Unmarshaled JSON, reflections we break,
New paths to traverse, improvements we make.
With tests that benchmark, our spirits quick,
The pipeline is strong, the code is slick.
Let's hop ahead, with ears held high,
For in this code, our dreams can fly.
🐇✨


Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai generate interesting stats about this repository and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

codecov bot commented Jul 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.45%. Comparing base (2b03b61) to head (3761865).

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##             main    #1691       +/-   ##
===========================================
+ Coverage   22.18%   70.45%   +48.27%     
===========================================
  Files         268        8      -260     
  Lines       11767       88    -11679     
  Branches       18       18               
===========================================
- Hits         2610       62     -2548     
+ Misses       9043       22     -9021     
+ Partials      114        4      -110     

see 259 files with indirect coverage changes

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

Review details

Configuration used: CodeRabbit UI
Review profile: ASSERTIVE

Commits

Files that changed from the base of the PR and between 2b03b61 and 2f091e2.

Files selected for processing (22)
  • mod/primitives/pkg/bytes/b.go (2 hunks)
  • mod/primitives/pkg/bytes/b20.go (1 hunks)
  • mod/primitives/pkg/bytes/b32.go (1 hunks)
  • mod/primitives/pkg/bytes/b4.go (1 hunks)
  • mod/primitives/pkg/bytes/b48.go (1 hunks)
  • mod/primitives/pkg/bytes/b8.go (1 hunks)
  • mod/primitives/pkg/bytes/b96.go (1 hunks)
  • mod/primitives/pkg/bytes/utils.go (2 hunks)
  • mod/primitives/pkg/eip4844/blob.go (2 hunks)
  • mod/primitives/pkg/eip4844/kzg_commitment.go (2 hunks)
  • mod/primitives/pkg/encoding/hex/bytes.go (2 hunks)
  • mod/primitives/pkg/encoding/hex/bytes_test.go (1 hunks)
  • mod/primitives/pkg/encoding/hex/errors.go (1 hunks)
  • mod/primitives/pkg/encoding/hex/hex_test.go (1 hunks)
  • mod/primitives/pkg/encoding/hex/json.go (1 hunks)
  • mod/primitives/pkg/encoding/hex/u64_test.go (1 hunks)
  • mod/primitives/pkg/math/u256.go (1 hunks)
  • mod/primitives/pkg/math/u64.go (2 hunks)
  • mod/primitives/pkg/math/u64_test.go (1 hunks)
  • mod/primitives/pkg/net/jwt/jwt.go (1 hunks)
  • mod/primitives/pkg/net/jwt/jwt_test.go (1 hunks)
  • mod/storage/pkg/filedb/range_db.go (1 hunks)
Additional comments not posted (25)
mod/primitives/pkg/eip4844/blob.go (1)

32-32: LGTM! Ensure consistent usage of the updated method.

The code changes are approved.

However, ensure that all function calls to UnmarshalJSON match the new signature.

Verification successful

No other usages of UnmarshalJSON for Blob were found.

The verification confirms that the only instance of UnmarshalJSON for the Blob type is in the file under review (mod/primitives/pkg/eip4844/blob.go). There are no other calls to UnmarshalJSON that need to be updated.

  • mod/primitives/pkg/eip4844/blob.go: Verified the single instance.
Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify all function calls to `UnmarshalJSON` in the `Blob` type match the new signature.

# Test: Search for the function usage. Expect: Only occurances of the new signature.
rg --type go --pattern $'Blob' | xargs -I {} rg --type go --pattern $'UnmarshalJSON' {}

Length of output: 499


Script:

#!/bin/bash
# Description: Verify all function calls to `UnmarshalJSON` in the `Blob` type match the new signature.

# Search for files containing the `Blob` type definition
blob_files=$(rg --type go --files-with-matches 'type Blob' || exit 1)

# Search for `UnmarshalJSON` function calls in the identified files
for file in $blob_files; do
    rg --type go 'UnmarshalJSON' "$file"
done

Length of output: 1863

mod/primitives/pkg/encoding/hex/json.go (1)

34-36: LGTM! Ensure consistent usage of the updated function.

The code changes are approved.

However, ensure that all function calls to UnmarshalJSONText match the new signature.

Verification successful

All function calls to UnmarshalJSONText match the new signature.

The function calls in the following files have been verified:

  • mod/primitives/pkg/bytes/b.go
  • mod/primitives/pkg/math/u64.go
  • mod/primitives/pkg/encoding/hex/hex_test.go
Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify all function calls to `UnmarshalJSONText` match the new signature.

# Test: Search for the function usage. Expect: Only occurances of the new signature.
rg --type go --pattern $'UnmarshalJSONText'

Length of output: 454


Script:

#!/bin/bash
# Description: Verify all function calls to `UnmarshalJSONText` match the new signature.

# Test: Search for the function usage. Expect: Only occurrences of the new signature.
rg 'UnmarshalJSONText'

Length of output: 520


Script:

#!/bin/bash
# Description: Extract lines containing `UnmarshalJSONText` to verify the signatures.

# Extract lines containing `UnmarshalJSONText` from the relevant files.
rg --no-line-number 'UnmarshalJSONText' mod/primitives/pkg/encoding/hex/json.go mod/primitives/pkg/encoding/hex/hex_test.go mod/primitives/pkg/math/u64.go mod/primitives/pkg/bytes/b.go

Length of output: 682


Script:

#!/bin/bash
# Description: Extract additional context around the usage of `UnmarshalJSONText` in `hex_test.go`.

# Extract lines around the usage of `UnmarshalJSONText` in `hex_test.go`.
rg -C 3 'UnmarshalJSONText' mod/primitives/pkg/encoding/hex/hex_test.go

Length of output: 356

mod/primitives/pkg/bytes/b.go (2)

25-25: Update the import path.

The import path for the hex package has been updated.


39-39: LGTM! Ensure consistent usage of the updated method.

The code changes are approved.

However, ensure that all function calls to UnmarshalJSON match the new signature.

mod/primitives/pkg/encoding/hex/bytes.go (1)

60-68: LGTM! But verify the function usage in the codebase.

The code changes are approved.

However, ensure that all function calls to DecodeFixedText match the new signature.

mod/primitives/pkg/net/jwt/jwt.go (1)

29-29: LGTM! But verify the import path in the codebase.

The code changes are approved.

However, ensure that the updated import path github.com/berachain/beacon-kit/mod/primitives/pkg/encoding/hex is correct and there are no broken imports.

Verification successful

Import Path Verification Successful

The import path github.com/berachain/beacon-kit/mod/primitives/pkg/encoding/hex is correct and consistently used across multiple files in the codebase. The hex package exists at the specified path, confirming that there are no broken imports.

  • Verified import path usage in:
    • mod/storage/pkg/filedb/range_db.go
    • mod/primitives/pkg/encoding/hex/u64_test.go
    • mod/primitives/pkg/encoding/hex/bytes_test.go
    • mod/primitives/pkg/encoding/hex/hex_test.go
    • mod/primitives/pkg/net/jwt/jwt_test.go
    • mod/primitives/pkg/net/jwt/jwt.go
    • mod/primitives/pkg/math/u256.go
    • mod/primitives/pkg/math/u64.go
    • mod/primitives/pkg/math/u64_test.go
    • mod/primitives/pkg/bytes/b32.go
    • mod/primitives/pkg/bytes/b48.go
    • mod/primitives/pkg/bytes/b96.go
    • mod/primitives/pkg/bytes/b8.go
    • mod/primitives/pkg/bytes/b20.go
    • mod/primitives/pkg/bytes/b.go
    • mod/primitives/pkg/bytes/utils.go
    • mod/primitives/pkg/bytes/b4.go
Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the updated import path `github.com/berachain/beacon-kit/mod/primitives/pkg/encoding/hex`.

# Test: Search for the import path usage. Expect: Only occurrences of the new import path.
rg --type go $'github.com/berachain/beacon-kit/mod/primitives/pkg/encoding/hex'

Length of output: 1812


Script:

#!/bin/bash
# Description: Verify the existence of the `hex` package or module at the path `mod/primitives/pkg/encoding/`.

# Test: Search for the `hex` package or module.
fd 'hex' mod/primitives/pkg/encoding/

Length of output: 114

mod/primitives/pkg/encoding/hex/u64_test.go (1)

26-26: LGTM! But verify the import path in the codebase.

The code changes are approved.

However, ensure that the updated import path github.com/berachain/beacon-kit/mod/primitives/pkg/encoding/hex is correct and there are no broken imports.

Verification successful

The import path is verified and correct.

The import path github.com/berachain/beacon-kit/mod/primitives/pkg/encoding/hex exists and is used consistently across the codebase.

  • mod/primitives/pkg/encoding/hex/ directory exists.
Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the updated import path `github.com/berachain/beacon-kit/mod/primitives/pkg/encoding/hex`.

# Test: Search for the import path usage. Expect: Only occurrences of the new import path.
rg --type go $'github.com/berachain/beacon-kit/mod/primitives/pkg/encoding/hex'

Length of output: 1812


Script:

#!/bin/bash
# Verify the existence of the directory `mod/primitives/pkg/encoding/hex`.
fd --type d 'hex' --search-path mod/primitives/pkg/encoding

Length of output: 92

mod/primitives/pkg/eip4844/kzg_commitment.go (1)

Line range hint 51-54:
LGTM! But verify the method usage in the codebase.

The code changes are approved.

However, ensure that all method calls to UnmarshalJSON in the KZGCommitment struct match the new signature.

Verification successful

The search results show multiple occurrences of the UnmarshalJSON method across different files. However, we need to specifically verify the UnmarshalJSON method calls for the KZGCommitment struct.

Let's narrow down our search to ensure that all calls to UnmarshalJSON in the KZGCommitment struct match the new signature.


To confirm that the UnmarshalJSON method in the KZGCommitment struct matches the new signature in all its usages, let's extract the relevant lines from kzg_commitment_test.go to verify the usage.


To fully verify the UnmarshalJSON call within the TestKZGCommitmentUnmarshalJSON function, we need to extract additional lines from kzg_commitment_test.go.


Method usage verified

The UnmarshalJSON method in the KZGCommitment struct is correctly used with the new signature in all relevant test cases.

  • The test function TestKZGCommitmentUnmarshalJSON in mod/primitives/pkg/eip4844/kzg_commitment_test.go confirms that the method is called with the correct signature.
Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify all method calls to `UnmarshalJSON` in the `KZGCommitment` struct match the new signature.

# Test: Search for the method usage. Expect: Only occurrences of the new signature.
rg --type go $'UnmarshalJSON'

Length of output: 14277


Script:

#!/bin/bash
# Extracting all UnmarshalJSON method calls specifically for KZGCommitment

# Search for the KZGCommitment struct definition
rg --type go 'type KZGCommitment struct' -A 10

# Search for all occurrences of UnmarshalJSON calls related to KZGCommitment
rg --type go 'KZGCommitment.*UnmarshalJSON'

Length of output: 301


Script:

#!/bin/bash
# Extract relevant lines from kzg_commitment_test.go to verify UnmarshalJSON usage

# Display the lines around the TestKZGCommitmentUnmarshalJSON function
rg --type go 'func TestKZGCommitmentUnmarshalJSON' -A 20 mod/primitives/pkg/eip4844/kzg_commitment_test.go

Length of output: 733


Script:

#!/bin/bash
# Extract more lines from kzg_commitment_test.go to verify UnmarshalJSON usage

# Display more lines around the TestKZGCommitmentUnmarshalJSON function
rg --type go 'func TestKZGCommitmentUnmarshalJSON' -A 50 mod/primitives/pkg/eip4844/kzg_commitment_test.go

Length of output: 1444

mod/primitives/pkg/bytes/b32.go (1)

25-25: LGTM! Ensure consistency of the import path.

The import path for the hex package has been updated correctly.

Verify that the import path is consistent across the project.

Verification successful

Import Path Consistency Verified

The import path for the hex package has been consistently updated to github.com/berachain/beacon-kit/mod/primitives/pkg/encoding/hex across the project.

  • No occurrences of the old import path github.com/berachain/beacon-kit/mod/primitives/pkg/hex were found.
  • Multiple occurrences of the new import path were found in various files.
Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the consistency of the import path for the `hex` package across the project.

# Test: Search for the old import path. Expect: No occurrences.
rg --type go 'github.com/berachain/beacon-kit/mod/primitives/pkg/hex'

# Test: Search for the new import path. Expect: Multiple occurrences.
rg --type go 'github.com/berachain/beacon-kit/mod/primitives/pkg/encoding/hex'

Length of output: 1882

mod/primitives/pkg/bytes/b20.go (1)

25-25: LGTM! Ensure consistency of the import path.

The import path for the hex package has been updated correctly.

Verify that the import path is consistent across the project.

mod/primitives/pkg/bytes/b4.go (1)

25-25: LGTM! Ensure consistency of the import path.

The import path for the hex package has been updated correctly.

Verify that the import path is consistent across the project.

mod/primitives/pkg/bytes/b8.go (1)

25-25: LGTM! Ensure consistency of the import path.

The import path for the hex package has been updated correctly.

Verify that the import path is consistent across the project.

mod/primitives/pkg/bytes/b48.go (1)

25-25: Import Path Update.

The import path for the hex package has been updated to github.com/berachain/beacon-kit/mod/primitives/pkg/encoding/hex. Ensure that this new path is correct and that the package is accessible.

mod/primitives/pkg/bytes/utils.go (3)

25-25: Import Path Update.

The import path for the hex package has been updated to github.com/berachain/beacon-kit/mod/primitives/pkg/encoding/hex. Ensure that this new path is correct and that the package is accessible.


109-110: Function Signature Update: UnmarshalFixedJSON.

The UnmarshalFixedJSON function signature has been simplified by removing the typ parameter. Ensure that this change aligns with the overall goal of reducing reliance on reflection and that the function still performs as expected.


116-117: Function Signature Update: UnmarshalFixedText.

The UnmarshalFixedText function signature has been simplified by removing the typename parameter. Ensure that this change aligns with the overall goal of reducing reliance on reflection and that the function still performs as expected.

mod/primitives/pkg/bytes/b96.go (1)

25-25: Import Path Update.

The import path for the hex package has been updated to github.com/berachain/beacon-kit/mod/primitives/pkg/encoding/hex. Ensure that this new path is correct and that the package is accessible.

mod/storage/pkg/filedb/range_db.go (1)

29-29: Import Path Update.

The import path for the hex package has been updated to github.com/berachain/beacon-kit/mod/primitives/pkg/encoding/hex. Ensure that this new path is correct and that the package is accessible.

mod/primitives/pkg/net/jwt/jwt_test.go (1)

29-29: LGTM! Import path update is correct.

The import path for the hex package has been updated as per the PR objectives.

mod/primitives/pkg/math/u256.go (1)

27-27: LGTM! Import path update is correct.

The import path for the hex package has been updated as per the PR objectives.

mod/primitives/pkg/encoding/hex/bytes_test.go (1)

29-29: LGTM! Import path update is correct.

The import path for the hex package has been updated as per the PR objectives.

mod/primitives/pkg/math/u64.go (2)

29-29: LGTM! Import path update is correct.

The import path for the hex package has been updated as per the PR objectives.


115-115: LGTM! Simplification of UnmarshalJSON method is correct.

The method has been simplified by removing reflection-related parameters, aligning with the PR objectives.

mod/primitives/pkg/encoding/hex/hex_test.go (1)

32-32: Verify the correctness of the new import path.

Ensure that the new import path github.com/berachain/beacon-kit/mod/primitives/pkg/encoding/hex is correct and matches the new structure.

Verification successful

The new import path is correct.

The import path github.com/berachain/beacon-kit/mod/primitives/pkg/encoding/hex is consistently used across multiple files in the codebase, indicating its correctness.

  • mod/primitives/pkg/encoding/hex/hex_test.go
  • mod/primitives/pkg/encoding/hex/bytes_test.go
  • mod/primitives/pkg/encoding/hex/u64_test.go
  • mod/primitives/pkg/bytes/utils.go
  • mod/primitives/pkg/bytes/b96.go
  • mod/primitives/pkg/bytes/b4.go
  • mod/primitives/pkg/bytes/b20.go
  • mod/primitives/pkg/bytes/b.go
  • mod/primitives/pkg/bytes/b48.go
  • mod/primitives/pkg/bytes/b32.go
  • mod/primitives/pkg/bytes/b8.go
  • mod/storage/pkg/filedb/range_db.go
  • mod/primitives/pkg/net/jwt/jwt_test.go
  • mod/primitives/pkg/math/u64.go
  • mod/primitives/pkg/math/u256.go
  • mod/primitives/pkg/net/jwt/jwt.go
Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the new import path is correct.

# Test: Search for the new import path. Expect: The new import path should be consistent across the codebase.
rg --type go 'github.com/berachain/beacon-kit/mod/primitives/pkg/encoding/hex'

Length of output: 1812

mod/primitives/pkg/math/u64_test.go (1)

28-28: Verify the correctness of the new import path.

Ensure that the new import path github.com/berachain/beacon-kit/mod/primitives/pkg/encoding/hex is correct and matches the new structure.

Verification successful

The new import path is correct and used consistently across the codebase.

The import path github.com/berachain/beacon-kit/mod/primitives/pkg/encoding/hex is verified to be correct.

  • No further action is required.
Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the new import path is correct.

# Test: Search for the new import path. Expect: The new import path should be consistent across the codebase.
rg --type go 'github.com/berachain/beacon-kit/mod/primitives/pkg/encoding/hex'

Length of output: 1812

Comment on lines +51 to +55
func DecodeFixedJSON(input, out []byte) error {
if !isQuotedString(input) {
return WrapUnmarshalError(ErrNonQuotedString, bytesT)
return ErrNonQuotedString
}
return WrapUnmarshalError(
DecodeFixedText(typ.String(), input[1:len(input)-1], out), typ,
)
return DecodeFixedText(input[1:len(input)-1], out)
Copy link
Contributor

Choose a reason for hiding this comment

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

Tip

Codebase Verification

Update required: Test function TestDecodeFixedJSON uses outdated signature

The test function TestDecodeFixedJSON in mod/primitives/pkg/encoding/hex/bytes_test.go still uses the old signature of DecodeFixedJSON with the typename parameter. Please update the test to match the new function signature.

  • File: mod/primitives/pkg/encoding/hex/bytes_test.go
  • Lines: 11-20
func TestDecodeFixedJSON(t *testing.T) {
	tests := []struct {
		name      string
		input     []byte
		out       []byte
	}{
		// Test cases
	}

	for _, tt := range tests {
		t.Run(tt.name, func(t *testing.T) {
			err := hex.DecodeFixedJSON(tt.input, tt.out)
			// Additional test logic
		})
	}
}

Please remove the typename field and update the test cases accordingly.

Analysis chain

LGTM! But verify the function usage in the codebase.

The code changes are approved.

However, ensure that all function calls to DecodeFixedJSON match the new signature.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify all function calls to `DecodeFixedJSON` match the new signature.

# Test: Search for the function usage. Expect: Only occurrences of the new signature.
rg --type go $'DecodeFixedJSON'

Length of output: 474


Script:

#!/bin/bash
# Description: Verify the context of each usage of `DecodeFixedJSON` to ensure they match the new signature.

# Extract and show lines around the function definition and its usage in bytes.go
rg -A 5 -B 5 'DecodeFixedJSON' mod/primitives/pkg/encoding/hex/bytes.go

# Extract and show lines around the function usage in bytes_test.go
rg -A 5 -B 5 'DecodeFixedJSON' mod/primitives/pkg/encoding/hex/bytes_test.go

# Extract and show lines around the function usage in utils.go
rg -A 5 -B 5 'DecodeFixedJSON' mod/primitives/pkg/bytes/utils.go

Length of output: 1533

Comment on lines +28 to +36
ErrEmptyString = errors.New("empty hex string")
ErrMissingPrefix = errors.New("hex string without 0x prefix")
ErrOddLength = errors.New("hex string of odd length")
ErrNonQuotedString = errors.New("non-quoted hex string")
ErrInvalidString = errors.New("invalid hex string")
ErrLeadingZero = errors.New("hex number with leading zero digits")
ErrEmptyNumber = errors.New("hex string \"0x\"")
ErrUint64Range = errors.New("hex number > 64 bits")
ErrBig256Range = errors.New("hex number > 256 bits")
Copy link
Contributor

Choose a reason for hiding this comment

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

Tip

Codebase Verification

Action Required: Remove or Replace WrapUnmarshalError Reference

The function WrapUnmarshalError has been removed from the codebase, but there is still a reference to it in the test file mod/primitives/pkg/math/u64_test.go. This reference should be removed or replaced to ensure the codebase is consistent and error-free.

  • File: mod/primitives/pkg/math/u64_test.go
  • Line: Reference to WrapUnmarshalError
Analysis chain

LGTM! Verify the removal of WrapUnmarshalError.

The code changes are approved.

However, ensure that the removal of WrapUnmarshalError does not affect error handling in other parts of the codebase.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the removal of `WrapUnmarshalError` and its usage in the codebase.

# Test: Search for the function usage. Expect: No occurrences of `WrapUnmarshalError`.
rg --type go --pattern $'WrapUnmarshalError'

Length of output: 455


Script:

#!/bin/bash
# Description: Verify the removal of `WrapUnmarshalError` and its usage in the codebase.

# Test: Search for the function usage. Expect: No occurrences of `WrapUnmarshalError`.
rg 'WrapUnmarshalError' --type go

Length of output: 96


Script:

#!/bin/bash
# Description: Verify the definition of `WrapUnmarshalError` in the codebase.

# Test: Search for the function definition. Expect: No occurrences of `WrapUnmarshalError`.
rg 'func WrapUnmarshalError' --type go

Length of output: 40

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

Review details

Configuration used: CodeRabbit UI
Review profile: ASSERTIVE

Commits

Files that changed from the base of the PR and between 2f091e2 and d82e171.

Files selected for processing (7)
  • .github/workflows/pipeline.yml (1 hunks)
  • build/scripts/testing.mk (2 hunks)
  • mod/primitives/pkg/bytes/b96_test.go (1 hunks)
  • mod/primitives/pkg/bytes/b_test.go (2 hunks)
  • mod/primitives/pkg/encoding/hex/bytes_test.go (6 hunks)
  • mod/primitives/pkg/encoding/hex/hex_test.go (2 hunks)
  • mod/primitives/pkg/math/u64_test.go (2 hunks)
Additional comments not posted (9)
mod/primitives/pkg/encoding/hex/bytes_test.go (1)

151-151: LGTM!

The test case changes look good and cover various scenarios.

build/scripts/testing.mk (1)

200-203: LGTM!

The new test-unit-bench target is well-defined and follows the structure of other similar targets.

mod/primitives/pkg/bytes/b96_test.go (1)

189-194: LGTM!

The test case changes ensure that both implementations of HashTreeRoot produce the same result.

.github/workflows/pipeline.yml (1)

72-72: LGTM!

The new test-unit-bench job is well-defined and follows the structure of other similar jobs in the workflow.

mod/primitives/pkg/encoding/hex/hex_test.go (1)

31-31: LGTM! Import path updated correctly.

The import path for the hex package has been updated to github.com/berachain/beacon-kit/mod/primitives/pkg/encoding/hex.

mod/primitives/pkg/math/u64_test.go (2)

27-27: LGTM! Import path updated correctly.

The import path for the hex package has been updated to github.com/berachain/beacon-kit/mod/primitives/pkg/encoding/hex.


188-189: LGTM! Error handling updated correctly.

The error handling for invalid hex strings has been updated to use hex.ErrInvalidString.

mod/primitives/pkg/bytes/b_test.go (2)

1340-1340: Verify the function call to UnmarshalFixedJSON.

Ensure that the updated function call to UnmarshalFixedJSON matches the new signature and that the tests cover all edge cases.

Verification successful

The function call to UnmarshalFixedJSON is correct.

The function signature in the test file matches the actual function definition in mod/primitives/pkg/bytes/utils.go.

  • mod/primitives/pkg/bytes/utils.go: func UnmarshalFixedJSON(input, out []byte) error
Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the function signature of `UnmarshalFixedJSON` and ensure it matches the updated call in the test.

# Test: Search for the function definition. Expect: The updated signature.
rg --type go $'func UnmarshalFixedJSON(input, out []byte) error'

Length of output: 211


Script:

#!/bin/bash
# Description: Verify the function signature of `UnmarshalFixedJSON` and ensure it matches the updated call in the test.

# Test: Search for the function definition. Expect: The updated signature.
rg --type go 'func UnmarshalFixedJSON\(input, out \[\]byte\) error'

Length of output: 154


1388-1388: Verify the function call to UnmarshalFixedText.

Ensure that the updated function call to UnmarshalFixedText matches the new signature and that the tests cover all edge cases.

Verification successful

Verified the function call to UnmarshalFixedText.

The function call to UnmarshalFixedText in the test file mod/primitives/pkg/bytes/b_test.go correctly matches the function signature defined in mod/primitives/pkg/bytes/utils.go.

  • mod/primitives/pkg/bytes/utils.go: func UnmarshalFixedText(input, out []byte) error
Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the function signature of `UnmarshalFixedText` and ensure it matches the updated call in the test.

# Test: Search for the function definition. Expect: The updated signature.
rg --type go $'func UnmarshalFixedText(input, out []byte) error'

Length of output: 211


Script:

#!/bin/bash
# Description: Verify the function signature of `UnmarshalFixedText` and ensure it matches the updated call in the test.

# Test: Search for the function definition. Expect: The updated signature.
rg --type go 'func UnmarshalFixedText\(input, out \[\]byte\) error'

Length of output: 154

@itsdevbear itsdevbear merged commit 87c11b2 into main Jul 4, 2024
1 check passed
@itsdevbear itsdevbear deleted the cleanup-encoding branch July 4, 2024 16:25
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

Review details

Configuration used: CodeRabbit UI
Review profile: ASSERTIVE

Commits

Files that changed from the base of the PR and between d82e171 and 3761865.

Files selected for processing (1)
  • mod/primitives/pkg/merkle/tree_test.go (11 hunks)
Additional comments not posted (9)
mod/primitives/pkg/merkle/tree_test.go (9)

33-33: Good practice: Improved test isolation.

Replacing the constant treeDepth with a local variable improves test isolation and clarity.


69-69: Good practice: Improved test isolation.

Replacing the constant treeDepth with a local variable improves test isolation and clarity.


124-124: Good practice: Improved test isolation.

Replacing the constant treeDepth with a local variable improves test isolation and clarity.


168-168: Good practice: Improved test isolation.

Replacing the constant treeDepth with a local variable improves test isolation and clarity.


189-189: Good practice: Improved test isolation.

Replacing the constant treeDepth with a local variable improves test isolation and clarity.


233-233: Good practice: Improved test isolation.

Replacing the constant treeDepth with a local variable improves test isolation and clarity.


253-253: Good practice: Improved test isolation.

Replacing the constant treeDepth with a local variable improves test isolation and clarity.


274-274: Good practice: Improved test isolation.

Replacing the constant treeDepth with a local variable improves test isolation and clarity.


299-299: Good practice: Improved test isolation.

Replacing the constant treeDepth with a local variable improves test isolation and clarity.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant