feat(payload): Move to holiman lib #1821
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe recent changes enhance the handling of the Changes
Poem
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 as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1821 +/- ##
=======================================
Coverage ? 32.45%
=======================================
Files ? 7
Lines ? 114
Branches ? 19
=======================================
Hits ? 37
Misses ? 76
Partials ? 1 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Outside diff range comments (1)
mod/engine-primitives/pkg/engine-primitives/payload_env_test.go (1)
[!TIP]
Codebase VerificationEnsure test coverage is maintained.
The removal of the
TestExecutionPayloadEnvelopefunction has significantly reduced the test coverage for theExecutionPayloadEnvelopestruct. No equivalent tests were found elsewhere in the codebase. Please add tests to cover the functionality of theExecutionPayloadEnvelopestruct to maintain adequate test coverage.
- Files to review:
mod/engine-primitives/pkg/engine-primitives/payload_env_test.goAnalysis chain
Line range hint
1-1:
Ensure test coverage is maintained.The removal of the
TestExecutionPayloadEnvelopefunction significantly reduces the test coverage for theExecutionPayloadEnvelopestruct. Confirm if the tests have been relocated or if the functionality is no longer necessary.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify if the `TestExecutionPayloadEnvelope` function or equivalent tests exist elsewhere in the codebase. # Test: Search for the test function. Expect: Occurrences of similar or equivalent tests. rg --type go 'TestExecutionPayloadEnvelope'Length of output: 43
Script:
#!/bin/bash # Description: Verify if the `ExecutionPayloadEnvelope` struct is being tested elsewhere in the codebase. # Test: Search for references to `ExecutionPayloadEnvelope` in test files. rg --type go 'ExecutionPayloadEnvelope' -g '*_test.go'Length of output: 54
Script:
#!/bin/bash # Description: Verify if the `ExecutionPayloadEnvelope` struct is referenced elsewhere in the codebase. # Test: Search for references to `ExecutionPayloadEnvelope` in the entire codebase. rg --type go 'ExecutionPayloadEnvelope'Length of output: 1388
Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Files selected for processing (23)
- mod/chain-spec/pkg/chain/chain_spec.go (1 hunks)
- mod/cli/pkg/commands/genesis/payload.go (1 hunks)
- mod/consensus-types/go.mod (2 hunks)
- mod/consensus-types/pkg/genesis/genesis.go (1 hunks)
- mod/consensus-types/pkg/state/deneb/deneb_test.go (2 hunks)
- mod/consensus-types/pkg/types/block_test.go (1 hunks)
- mod/consensus-types/pkg/types/body_test.go (1 hunks)
- mod/consensus-types/pkg/types/payload.go (8 hunks)
- mod/consensus-types/pkg/types/payload_header.go (8 hunks)
- mod/consensus-types/pkg/types/payload_header_test.go (3 hunks)
- mod/consensus-types/pkg/types/payload_test.go (3 hunks)
- mod/engine-primitives/go.mod (2 hunks)
- mod/engine-primitives/pkg/engine-primitives/mocks/built_execution_payload_env.mock.go (3 hunks)
- mod/engine-primitives/pkg/engine-primitives/payload_env.go (3 hunks)
- mod/engine-primitives/pkg/engine-primitives/payload_env_test.go (2 hunks)
- mod/engine-primitives/pkg/engine-primitives/requests.go (3 hunks)
- mod/engine-primitives/pkg/engine-primitives/requests_test.go (1 hunks)
- mod/execution/pkg/engine/types.go (1 hunks)
- mod/primitives/pkg/math/u256.go (1 hunks)
- mod/primitives/pkg/math/u64.go (3 hunks)
- mod/primitives/pkg/math/u64_test.go (2 hunks)
- mod/state-transition/pkg/core/types.go (1 hunks)
- testing/quick/compare_test.go (2 hunks)
Additional comments not posted (42)
mod/primitives/pkg/math/u256.go (3)
34-34: Refactoring: Simplified type definition.The
U256type is now an alias foruint256.Int, simplifying its declaration and usage.
37-38: New constructor function:NewU256.The
NewU256function creates a newU256from auint64, leveraginguint256.NewInt.
42-43: New constructor function:NewU256FromBigInt.The
NewU256FromBigIntfunction facilitates creating aU256from abig.Int, usinguint256.MustFromBig.mod/execution/pkg/engine/types.go (1)
43-43: Refactor: Changed return type forGetBaseFeePerGas.The return type of the
GetBaseFeePerGasmethod in theExecutionPayloadinterface has been changed to*math.U256. This change improves memory efficiency and flexibility but requires handling potential nil values.mod/engine-primitives/pkg/engine-primitives/payload_env.go (2)
76-76: LGTM! But verify all usages of theGetValuemethod.The change to use a pointer type aligns with the updated field type and method signature.
Ensure that all usages of the
GetValuemethod in theExecutionPayloadEnvelopestruct are updated accordingly.
60-60: LGTM! But verify all usages of theBlockValuefield.The change to use a pointer type aligns with the updated method signature and can improve memory efficiency.
Ensure that all usages of the
BlockValuefield in theExecutionPayloadEnvelopestruct are updated accordingly.testing/quick/compare_test.go (2)
57-57: LGTM! But verify the initialization ofBaseFeePerGas.The change aligns with the new type for
BaseFeePerGasand likely enhances precision.Ensure that the initialization of the
BaseFeePerGasfield usingmath.NewU256(123)is correct and consistent throughout the codebase.
65-65: LGTM! But verify the processing ofBaseFeePerGas.The change aligns with the new type for
BaseFeePerGasand likely improves handling of large numbers.Ensure that the processing of the
BaseFeePerGasfield usingSetFromBig(payload.BaseFeePerGas.ToBig())is correct and consistent throughout the codebase.mod/engine-primitives/go.mod (1)
11-11: LGTM!The change ensures explicit inclusion of the required version of
uint256, likely to utilize its functionalities for handling large integers. This improves clarity and maintainability of the module's dependencies.mod/consensus-types/pkg/types/body_test.go (1)
31-44: LGTM!The initialization of
BaseFeePerGasusingmath.NewU256(0)enhances the functionality of theExecutionPayloadby setting a meaningful default value.mod/consensus-types/go.mod (1)
12-12: Dependency added:github.com/holiman/uint256 v1.3.0The addition of this dependency as a direct requirement enhances the module's functionality related to handling large unsigned integers.
mod/consensus-types/pkg/genesis/genesis.go (1)
154-154: Simplified initialization ofBaseFeePerGas.The change from
math.MustNewU256LFromBigInttomath.NewU256FromBigIntsimplifies the initialization process and enhances maintainability.mod/consensus-types/pkg/types/block_test.go (1)
55-55: LGTM! The addition ofBaseFeePerGasis appropriate.The
BaseFeePerGasfield has been correctly added and initialized withmath.NewU256(0).mod/engine-primitives/pkg/engine-primitives/requests_test.go (1)
72-73: LGTM! The return type change to*math.U256is appropriate.The method
GetBaseFeePerGasnow returns a pointer tomath.U256, which is consistent with better memory management practices.mod/consensus-types/pkg/state/deneb/deneb_test.go (1)
99-99: LGTM! The initialization ofBaseFeePerGaswithmath.NewU256(3906250)is appropriate.The use of
math.NewU256forBaseFeePerGasis consistent with better numerical handling practices.mod/cli/pkg/commands/genesis/payload.go (1)
179-179: Verify the change in error handling forBaseFeePerGas.The change from
math.MustNewU256LFromBigInttomath.NewU256FromBigIntsuggests a change in error handling. Ensure that the new method handles errors gracefully and does not introduce any unexpected behavior.mod/engine-primitives/pkg/engine-primitives/mocks/built_execution_payload_env.mock.go (2)
154-154: LGTM! But verify the usage ofReturnin the codebase.The change to return a
*uint256.Intinstead ofmath.U256Lis approved.However, ensure that all function calls to
Returnare updated to handle the new return type.
159-159: LGTM! But verify the usage ofRunAndReturnin the codebase.The change to return a
*uint256.Intinstead ofmath.U256Lis approved.However, ensure that all function calls to
RunAndReturnare updated to handle the new return type.mod/state-transition/pkg/core/types.go (1)
163-163: LGTM! But verify the correct handling of the new return type.The change to return a pointer improves memory efficiency and allows for
nilvalues. Ensure that all usages ofGetBaseFeePerGashandle the new pointer type correctly.Verification successful
LGTM! The new pointer return type is handled correctly.
The change to return a pointer improves memory efficiency and allows for
nilvalues. All usages ofGetBaseFeePerGashandle the new pointer type correctly.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all usages of `GetBaseFeePerGas` handle the new pointer type correctly. # Test: Search for the function usage. Expect: Correct handling of the new pointer type. rg --type go -A 5 $'GetBaseFeePerGas'Length of output: 6718
Script:
#!/bin/bash # Description: Verify that all usages of `GetBaseFeePerGas` handle the new pointer type correctly. # Test: Search for the function usage and check for proper handling of the pointer type. rg --type go -A 5 'GetBaseFeePerGas' | grep -E '(\*|\&|\.)'Length of output: 6708
mod/engine-primitives/pkg/engine-primitives/requests.go (2)
49-49: LGTM! But verify the correct handling of the new return type.The change to return a pointer improves memory efficiency and allows for
nilvalues. Ensure that all usages ofGetBaseFeePerGashandle the new pointer type correctly.
89-89: LGTM! But verify the correct handling of the new return type.The change to return a pointer improves memory efficiency and allows for
nilvalues. Ensure that all usages ofGetBaseFeePerGashandle the new pointer type correctly.mod/consensus-types/pkg/types/payload_test.go (3)
60-60: LGTM! But verify the correct initialization of theBaseFeePerGasfield.The change to use a pointer type for
BaseFeePerGasis correctly reflected in the function. Ensure that the field is initialized correctly throughout the codebase.Verification successful
The
BaseFeePerGasfield is correctly initialized throughout the codebase.The initialization of the
BaseFeePerGasfield is consistent and correct, using methods that return a pointer tomath.U256.
mod/consensus-types/pkg/types/payload_test.gomod/engine-primitives/pkg/engine-primitives/requests.gomod/cli/pkg/commands/genesis/payload.gomod/consensus-types/pkg/state/deneb/deneb_test.gomod/consensus-types/pkg/types/block_test.gomod/consensus-types/pkg/types/body_test.gomod/consensus-types/pkg/types/payload_header_test.gomod/consensus-types/pkg/genesis/genesis.goScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the `BaseFeePerGas` field is initialized correctly throughout the codebase. # Test: Search for the initialization of `BaseFeePerGas`. Expect: Correct initialization with pointer type. rg --type go -A 5 $'BaseFeePerGas'Length of output: 21008
194-194: LGTM! But verify the correct handling of the new pointer type in the test function.The change to use a pointer type for
BaseFeePerGasis correctly reflected in the test function. Ensure that the new pointer type is handled correctly in all test cases.
134-134: LGTM! But verify the correct handling of the new pointer type in the test function.The change to use a pointer type for
BaseFeePerGasis correctly reflected in the test function. Ensure that the new pointer type is handled correctly in all test cases.Verification successful
The new pointer type for
BaseFeePerGasis handled correctly in the test cases.The test cases involving
BaseFeePerGascorrectly handle the new pointer type, ensuring that the field is set and verified as a pointer.
testing/quick/compare_test.gomod/consensus-types/pkg/types/payload_test.gomod/engine-primitives/pkg/engine-primitives/requests_test.gomod/consensus-types/pkg/types/payload_header_test.goScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the new pointer type for `BaseFeePerGas` is handled correctly in all test cases. # Test: Search for the test cases involving `BaseFeePerGas`. Expect: Correct handling of the new pointer type. rg --type go -A 5 $'BaseFeePerGas'Length of output: 21008
mod/consensus-types/pkg/types/payload_header_test.go (4)
42-45: LGTM! Initialization changes improve test robustness.The initialization of
StateRoot,ReceiptsRoot,LogsBloom, andRandomwith123byte arrays provides more meaningful default values for testing. The change ofBaseFeePerGasto a pointer type aligns with the new representation of gas fees.
80-80: LGTM! Assertion change aligns with new initialization.The assertion for
GetBaseFeePerGashas been updated to check againstmath.NewU256(1), aligning with the new initialization ofBaseFeePerGasas a pointer type.
119-122: LGTM! Initialization change improves test robustness.Initializing the
unmarshalledvariable withnew(types.ExecutionPayloadHeader).Empty(version.Deneb)ensures that it is properly initialized before deserialization, improving the robustness of the test.
51-51: Verify the function usage in the codebase.Ensure that all function calls to
generateExecutionPayloadHeaderare updated to handle the new initialization values, especially the pointer type forBaseFeePerGas.Verification successful
Function usage verified.
All instances of
generateExecutionPayloadHeadercorrectly handle the initialization ofBaseFeePerGasas a pointer type.
mod/consensus-types/pkg/types/payload_header_test.goScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `generateExecutionPayloadHeader` are updated to handle the new initialization values. # Test: Search for the function usage. Expect: Only occurrences of the new initialization values. rg --type go -A 5 $'generateExecutionPayloadHeader'Length of output: 9655
Script:
#!/bin/bash # Extract the full definition of the `generateExecutionPayloadHeader` function ast-grep --lang go --pattern $'func generateExecutionPayloadHeader() *types.ExecutionPayloadHeader { $$$ }' --json # Extract the usage context of `generateExecutionPayloadHeader` to verify initialization rg --type go -A 10 -B 5 'generateExecutionPayloadHeader'Length of output: 22091
mod/primitives/pkg/math/u64_test.go (1)
Line range hint
1-1:
Verify ifTestU64functionality is covered elsewhere.The removal of the
TestU64function suggests a shift in focus away from validating themath.U64type's characteristics within this context. Ensure that these tests are either covered elsewhere or deemed unnecessary.Verification successful
Verify if
TestU64functionality is covered elsewhere.The
TestU64functionality is covered by other specific tests such asTestU64_MarshalSSZ,TestU64_UnmarshalSSZ,TestU64_RoundTripSSZ,TestU64_MarshalText,TestU64_UnmarshalJSON,TestU64_NextPowerOfTwo, and others in the same file.
- mod/primitives/pkg/math/u64_test.go: Multiple functions testing various aspects of
math.U64.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify if the `TestU64` functionality is covered elsewhere. # Test: Search for the `TestU64` function usage. Expect: Occurrences of similar tests elsewhere. rg --type go -A 5 $'TestU64'Length of output: 5194
mod/chain-spec/pkg/chain/chain_spec.go (1)
Line range hint
1-1:
LGTM! Minor cleanup enhances readability.The removal of the blank line from the
Specinterface definition enhances readability and maintainability without affecting functionality.mod/consensus-types/pkg/types/payload_header.go (6)
71-71: LGTM!The change to use
*uint256.IntforBaseFeePerGasimproves memory management and flexibility.
141-141: LGTM!The update to use
ssz.DefineUint256forBaseFeePerGasensures proper serialization of the new type.
242-246: LGTM!Marshaling
BaseFeePerGasbefore adding it to the hash walker ensures consistent serialization.
290-290: LGTM!Updating the
BaseFeePerGasfield type to*math.U256in theMarshalJSONmethod aligns with the new type.
334-334: LGTM!Updating the
BaseFeePerGasfield type to*math.U256in theUnmarshalJSONmethod ensures correct handling during unmarshaling.
525-525: LGTM!Returning a pointer to
math.U256in theGetBaseFeePerGasmethod allows for more efficient handling of the value.mod/consensus-types/pkg/types/payload.go (6)
66-66: LGTM!The change to use
*math.U256forBaseFeePerGasimproves memory management and flexibility.
112-112: LGTM!The update to use
ssz.DefineUint256forBaseFeePerGasensures proper serialization of the new type.
213-217: LGTM!Marshaling
BaseFeePerGasbefore adding it to the hash walker ensures consistent serialization.
291-291: LGTM!Updating the
BaseFeePerGasfield type to*math.U256in theMarshalJSONmethod aligns with the new type.
338-338: LGTM!Updating the
BaseFeePerGasfield type to*math.U256in theUnmarshalJSONmethod ensures correct handling during unmarshaling.
526-526: LGTM!Returning a pointer to
math.U256in theGetBaseFeePerGasmethod allows for more efficient handling of the value.
| @@ -31,7 +31,7 @@ type BuiltExecutionPayloadEnv[ExecutionPayloadT any] interface { | |||
| // GetExecutionPayload retrieves the associated execution payload. | |||
| GetExecutionPayload() ExecutionPayloadT | |||
| // GetValue returns the Wei value of the block in the execution payload. | |||
| GetValue() math.Wei | |||
| GetValue() *math.U256 | |||
There was a problem hiding this comment.
Tip
Codebase Verification
Inconsistent GetValue Implementations Detected
The GetValue method has varying return types across different files, including *math.U256 and *uint256.Int. Please update all implementations of the GetValue method to return *math.U256 to ensure consistency.
mod/primitives/pkg/encoding/ssz/schema/field.go:func (f Field[T]) GetValue() Tmod/engine-primitives/pkg/engine-primitives/payload_env.go:func (e *ExecutionPayloadEnvelope[ExecutionPayloadT, BlobsBundleT]) GetValue() *math.U256mod/engine-primitives/pkg/engine-primitives/mocks/built_execution_payload_env.mock.go:func (_m *BuiltExecutionPayloadEnv[ExecutionPayloadT]) GetValue() *uint256.Int
Please verify and update the return types accordingly.
Analysis chain
LGTM! But verify all implementations and usages of the GetValue method.
The change to return a pointer type enhances memory efficiency and flexibility.
Ensure that all implementations and usages of the GetValue method in the BuiltExecutionPayloadEnv interface are updated accordingly.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify all implementations and usages of the `GetValue` method in the `BuiltExecutionPayloadEnv` interface.
# Test: Search for the method implementations and usages. Expect: Only occurrences of the new signature.
rg --type go -A 5 $'GetValue() *math.U256'
Length of output: 43
Script:
#!/bin/bash
# Description: Verify all implementations and usages of the `GetValue` method in the `BuiltExecutionPayloadEnv` interface.
# Test: Search for all occurrences of the `GetValue` method, regardless of return type.
rg --type go -A 5 'GetValue()'
Length of output: 7126
mod/primitives/pkg/math/u64.go
Outdated
| func (u Gwei) ToWei() *U256 { | ||
| return (&U256{}).Mul(NewU256(uint64(u)), NewU256(GweiPerWei)) |
There was a problem hiding this comment.
Tip
Codebase Verification
Update test cases to handle new return type *U256
The test cases in mod/primitives/pkg/math/u64_test.go expect the ToWei function to return a *big.Int, but the function now returns a *U256. Update the test cases to handle the new return type.
- File:
mod/primitives/pkg/math/u64_test.go- Lines: 7, 10, 15
Analysis chain
LGTM! But verify the usage of ToWei in the codebase.
The change to return a *U256 instead of a *big.Int is approved.
However, ensure that all function calls to ToWei are updated to handle the new return type.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify all function calls to `ToWei` are updated to handle the new return type.
# Test: Search for the function usage. Expect: Only occurrences of the new return type.
rg --type go 'ToWei' -A 5
Length of output: 954
| func (_m *BuiltExecutionPayloadEnv[ExecutionPayloadT]) GetValue() *uint256.Int { | ||
| ret := _m.Called() | ||
|
|
||
| if len(ret) == 0 { | ||
| panic("no return value specified for GetValue") | ||
| } | ||
|
|
||
| var r0 math.U256L | ||
| if rf, ok := ret.Get(0).(func() math.U256L); ok { | ||
| var r0 *uint256.Int | ||
| if rf, ok := ret.Get(0).(func() *uint256.Int); ok { | ||
| r0 = rf() | ||
| } else { | ||
| if ret.Get(0) != nil { | ||
| r0 = ret.Get(0).(math.U256L) | ||
| r0 = ret.Get(0).(*uint256.Int) |
There was a problem hiding this comment.
Tip
Codebase Verification
Inconsistent return type for GetValue function
The GetValue function in payload_env.go still returns *math.U256, which is inconsistent with the mock implementation returning *uint256.Int.
- File:
mod/engine-primitives/pkg/engine-primitives/payload_env.go- Lines:
// GetValue returns the Wei value of the block in the execution payload. GetValue() *math.U256 ... // GetValue returns the value of the ExecutionPayloadEnvelope. func (e *ExecutionPayloadEnvelope[ ExecutionPayloadT, BlobsBundleT, ]) GetValue() *math.U256 { return e.BlockValue }
- Lines:
Please update the return type of GetValue in payload_env.go to *uint256.Int to ensure consistency across the codebase.
Analysis chain
LGTM! But verify the usage of GetValue in the codebase.
The change to return a *uint256.Int instead of math.U256L is approved.
However, ensure that all function calls to GetValue are updated to handle the new return type.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify all function calls to `GetValue` are updated to handle the new return type.
# Test: Search for the function usage. Expect: Only occurrences of the new return type.
rg --type go 'GetValue' -A 5
Length of output: 7122
Summary by CodeRabbit
New Features
BaseFeePerGasfield to theBeaconBlock, enhancing transaction cost management.BaseFeePerGasacross various structures to improve handling of transaction fees.Bug Fixes
ExecutionPayloadHeaderandExecutionPayloadstructs by transitioning to pointer types.Documentation
Chores
Tests
BaseFeePerGas, enhancing the robustness of the test suite.