Skip to content

Replace byte arrays with hex encoding for json#552

Merged
GalRogozinski merged 25 commits intomainfrom
hex_encoding
Jul 17, 2025
Merged

Replace byte arrays with hex encoding for json#552
GalRogozinski merged 25 commits intomainfrom
hex_encoding

Conversation

@alan-ssvlabs
Copy link
Contributor

@alan-ssvlabs alan-ssvlabs commented Jun 21, 2025

closes #278

This PR creates custom marshallers and unmarshallers to change the encoding of bytes arrays in test json files to hex encoded strings

Copy link
Contributor

@GalRogozinski GalRogozinski left a comment

Choose a reason for hiding this comment

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

Good job!

I think it looks better now and has less confusing boilerplate code

Some comments I couldn't put in place due to VSCode bugs:

  1. hash changes in beacon types encoding and messages encoding (under investigation)
  2. In json_testutils consider the DRY (don't repeat yourself) approach that helps with future maintainability.

You could add the following helper functions, and then later only edit them as neccessary:

// hexStringFromJSON trims surrounding quotes from a JSON string value.
func hexStringFromJSON(data []byte) string {
	return strings.TrimSuffix(strings.TrimPrefix(string(data), "\""), "\"")
}

// tryUnmarshalByteArray tries to unmarshal a JSON array of bytes of a given length.
func tryUnmarshalByteArray(data []byte, expectedLen int) ([]byte, error) {
	var arr []byte
	if err := json.Unmarshal(data, &arr); err == nil {
		if len(arr) != expectedLen {
			return nil, fmt.Errorf("invalid array length: expected %d, got %d", expectedLen, len(arr))
		}
		return arr, nil
	}
	return nil, fmt.Errorf("not a valid byte array")
}
  1. I spotted DataRound:0 changed to Round:1... we have leftovers form other PRs?
    Maybe it will get better after you fix conflicts

@GalRogozinski GalRogozinski removed the request for review from MatheusFranco99 July 3, 2025 10:35
MatheusFranco99
MatheusFranco99 previously approved these changes Jul 9, 2025
Copy link
Contributor

@MatheusFranco99 MatheusFranco99 left a comment

Choose a reason for hiding this comment

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

Looks good to me! Good job!

@GalRogozinski GalRogozinski requested a review from y0sher July 10, 2025 08:00
@Tom-ssvlabs Tom-ssvlabs removed their request for review July 10, 2025 08:19
GalRogozinski
GalRogozinski previously approved these changes Jul 13, 2025
@Zacholme7
Copy link
Contributor

Lgtm thanks

@Zacholme7
Copy link
Contributor

Follow up: this should also be applied to the entire repository, not just qbft

@GalRogozinski GalRogozinski dismissed stale reviews from MatheusFranco99 and themself via 0a36059 July 17, 2025 09:30
@GalRogozinski GalRogozinski requested a review from MatusKysel July 17, 2025 09:41
@Tom-ssvlabs Tom-ssvlabs removed their request for review July 17, 2025 09:45
@GalRogozinski
Copy link
Contributor

@Zacholme7
I see other folders changed
can you point out an example of a missing change

Copy link
Collaborator

@dknopik dknopik left a comment

Choose a reason for hiding this comment

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

Approving based on Zac's LGTM above

Copy link
Contributor

@y0sher y0sher left a comment

Choose a reason for hiding this comment

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

lgtm

@GalRogozinski GalRogozinski merged commit 5d6c4a7 into main Jul 17, 2025
2 checks passed
@GalRogozinski GalRogozinski deleted the hex_encoding branch July 17, 2025 09:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Encode byte arrays in hex or Base64 when serializing to json

6 participants