chore(consensus-type): Fixed ExecutionPayload json marshalling #2042
Conversation
WalkthroughThe changes in this pull request primarily involve modifications to the Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2042 +/- ##
=======================================
Coverage 22.23% 22.23%
=======================================
Files 356 356
Lines 16022 16022
Branches 12 12
=======================================
Hits 3563 3563
Misses 12306 12306
Partials 153 153
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range comments (1)
mod/consensus-types/pkg/types/payload.go (1)
Line range hint
318-426: LGTM! The changes improve the robustness of the unmarshaling process.The addition of nil checks for
BlobGasUsedandExcessBlobGasfields enhances the reliability of the JSON unmarshaling process. The casting ofBaseFeePerGasfrom*math.U256Hexto*math.U256maintains consistency with the struct definition.For consistency, consider adding error messages for missing
BlobGasUsedandExcessBlobGasfields, similar to other required fields. For example:if dec.BlobGasUsed != nil { p.BlobGasUsed = *dec.BlobGasUsed +} else { + return errors.New("missing required field 'blobGasUsed' for ExecutionPayload") } if dec.ExcessBlobGas != nil { p.ExcessBlobGas = *dec.ExcessBlobGas +} else { + return errors.New("missing required field 'excessBlobGas' for ExecutionPayload") }This change would make the error handling more consistent across all fields.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (2)
- mod/consensus-types/pkg/types/payload.go (1 hunks)
- mod/consensus-types/pkg/types/payload_test.go (1 hunks)
🔇 Additional comments (3)
mod/consensus-types/pkg/types/payload_test.go (1)
Line range hint
1-174: Overall assessment of changes topayload_test.goThe addition of the
TestExecutionPayload_MarshalJSON_ValueAndPointerfunction is a positive step towards addressing the JSON marshaling inconsistency mentioned in the PR objectives. This test helps ensure that theExecutionPayloadstruct's JSON marshaling behaves consistently for both value and pointer receivers.However, to fully address the PR objectives and ensure robust testing, consider implementing the suggested improvements in the previous comment. These enhancements will provide more comprehensive coverage and explicitly verify the custom
MarshalJSONimplementation.Additionally, it might be beneficial to add tests that cover edge cases or specific scenarios where the marshaling behavior could potentially differ between value and pointer receivers.
The changes are approved, but implementing the suggested improvements would significantly enhance the test coverage and align more closely with the PR objectives.
mod/consensus-types/pkg/types/payload.go (2)
Line range hint
276-316: LGTM! The changes address the PR objectives effectively.The modification of the
MarshalJSONmethod from a pointer receiver to a value receiver is a crucial fix. This change ensures that the custom marshaling is used even when the method is called on a value receiver, preventing unexpected behavior during JSON serialization.The update of
BaseFeePerGasto use*math.U256Hexis consistent with the field type change in the struct definition, maintaining type consistency throughout the codebase.
Line range hint
1-626: Summary: The changes effectively address the PR objectives and improve code robustness.The modifications to the
ExecutionPayloadstruct and its associated methods, particularly theMarshalJSONandUnmarshalJSONfunctions, successfully address the issues outlined in the PR objectives. The change from a pointer receiver to a value receiver inMarshalJSONensures consistent behavior when marshaling JSON, regardless of whether it's called on a pointer or value.The additional nil checks in
UnmarshalJSONforBlobGasUsedandExcessBlobGasfields enhance the robustness of the unmarshaling process. The type changes forBaseFeePerGasare consistently applied throughout the file.These changes should resolve the JSON marshaling issues and improve the overall reliability of the
ExecutionPayloadstruct's serialization and deserialization processes.
| func TestExecutionPayload_MarshalJSON_ValueAndPointer(t *testing.T) { | ||
| val := types.ExecutionPayload{} | ||
|
|
||
| // Marshal on raw val uses default json marshal | ||
| valSerialized, err := json.Marshal(val) | ||
| require.NoError(t, err) | ||
|
|
||
| // Marshal on ptr val uses implemented MarshalJSON | ||
| ptrSerialized, err := json.Marshal(&val) | ||
| require.NoError(t, err) | ||
|
|
||
| require.Equal(t, valSerialized, ptrSerialized) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance test coverage for ExecutionPayload JSON marshaling
The new test function TestExecutionPayload_MarshalJSON_ValueAndPointer is a good addition to verify the consistency of JSON marshaling for both value and pointer receivers of ExecutionPayload. However, consider the following improvements to make the test more robust:
-
Use a non-empty
ExecutionPayloadstruct to better represent real-world scenarios. You can utilize thegenerateExecutionPayload()function for this purpose. -
Explicitly verify that the custom
MarshalJSONmethod is being used for the pointer receiver. This can be done by comparing the output with a known expected JSON structure. -
Check the actual content of the marshaled JSON, not just that the outputs are equal. This ensures that the serialization is correct, not just consistent.
Here's a suggested improvement:
func TestExecutionPayload_MarshalJSON_ValueAndPointer(t *testing.T) {
payload := generateExecutionPayload()
// Marshal on raw val uses default json marshal
valSerialized, err := json.Marshal(*payload)
require.NoError(t, err)
// Marshal on ptr val uses implemented MarshalJSON
ptrSerialized, err := json.Marshal(payload)
require.NoError(t, err)
// Verify that both outputs are equal
require.Equal(t, valSerialized, ptrSerialized)
// Verify the content of the marshaled JSON
var unmarshalled map[string]interface{}
err = json.Unmarshal(ptrSerialized, &unmarshalled)
require.NoError(t, err)
// Add assertions for key fields
require.Equal(t, payload.ParentHash.String(), unmarshalled["parentHash"])
require.Equal(t, payload.FeeRecipient.String(), unmarshalled["feeRecipient"])
// Add more assertions for other fields...
// Verify that custom MarshalJSON is used for pointer receiver
customMarshaled, err := payload.MarshalJSON()
require.NoError(t, err)
require.Equal(t, ptrSerialized, customMarshaled)
}This enhanced version of the test provides more comprehensive coverage and explicitly verifies the custom MarshalJSON implementation.
The
ExecutionPayloadstruct implementsMarshalJSONon a pointer receiver instead of on a value receiver.Due to how the json library handles these definitions, using
json.Marshalon a value receiver will result in attempting to use the default marshalling behavior instead of the custom definedMarshalJSONmethod.This could result in unexpected bytes or invalid round trip conversions if the value receiver is ever used to marshal via json.
Summary by CodeRabbit
New Features
ExecutionPayloadstruct.BaseFeePerGasfield for improved data handling.Bug Fixes
Tests
ExecutionPayload.