Change native execution value to Gwei Uint64 #12291
Conversation
api/client/builder/bid.go
Outdated
| v := big.NewInt(0).SetBytes(bytesutil.ReverseByteOrder(b.p.Value)) | ||
| gweiPerEth := big.NewInt(int64(params.BeaconConfig().GweiPerEth)) | ||
| v.Div(v, gweiPerEth) | ||
| return blocks.WrappedExecutionPayloadHeaderCapella(b.p.Header, v.Uint64()) |
There was a problem hiding this comment.
Thinking this may be better to be its own helper
There was a problem hiding this comment.
yeah I feel like we should have helper of the form
func (z *Int) ToGwei() uint64
| @@ -160,8 +161,10 @@ func WrappedBuilderBidCapella(p *ethpb.BuilderBidCapella) (Bid, error) { | |||
| // Header returns the execution data interface. | |||
| func (b builderBidCapella) Header() (interfaces.ExecutionData, error) { | |||
| // We have to convert big endian to little endian because the value is coming from the execution layer. | |||
| v := bytesutil.ReverseByteOrder(b.p.Value) | |||
| return blocks.WrappedExecutionPayloadHeaderCapella(b.p.Header, big.NewInt(0).SetBytes(v)) | |||
| v := big.NewInt(0).SetBytes(bytesutil.ReverseByteOrder(b.p.Value)) | |||
There was a problem hiding this comment.
Should protect for p == nil here.
api/client/builder/bid.go
Outdated
| v := big.NewInt(0).SetBytes(bytesutil.ReverseByteOrder(b.p.Value)) | ||
| gweiPerEth := big.NewInt(int64(params.BeaconConfig().GweiPerEth)) | ||
| v.Div(v, gweiPerEth) | ||
| return blocks.WrappedExecutionPayloadHeaderCapella(b.p.Header, v.Uint64()) |
There was a problem hiding this comment.
yeah I feel like we should have helper of the form
func (z *Int) ToGwei() uint64
| @@ -258,10 +258,9 @@ func TestClient_HTTP(t *testing.T) { | |||
| require.DeepEqual(t, want.ExecutionPayload.PrevRandao.Bytes(), pb.PrevRandao) | |||
| require.DeepEqual(t, want.ExecutionPayload.ParentHash.Bytes(), pb.ParentHash) | |||
|
|
|||
| v, err := resp.Value() | |||
| v, err := resp.ValueInGwei() | |||
There was a problem hiding this comment.
I feel it's fine to keep this one as Value() all values in the CL are Gwei after this change
There was a problem hiding this comment.
resp is already interface.ExecutionData. Do you want me to have both Value and ValueInGwei getters for ExecutionData?
| vs := &Server{ | ||
| ExecutionEngineCaller: &powtesting.EngineClient{PayloadIDBytes: id, ExecutionPayloadCapella: &v1.ExecutionPayloadCapella{BlockNumber: 1, Withdrawals: withdrawals}, BlockValue: big.NewInt(18395081606530051)}, | ||
| ExecutionEngineCaller: &powtesting.EngineClient{PayloadIDBytes: id, ExecutionPayloadCapella: &v1.ExecutionPayloadCapella{BlockNumber: 1, Withdrawals: withdrawals}, BlockValue: v.Uint64()}, |
There was a problem hiding this comment.
Simply but the Gwei value for the test and avoid the big Int arithmetics here.
| @@ -331,7 +333,7 @@ func TestNoWeiOverflow(t *testing.T) { | |||
|
|
|||
| blk, err := blocks.NewSignedBeaconBlock(util.NewBeaconBlockCapella()) | |||
| require.NoError(t, err) | |||
| vs.ExecutionEngineCaller = &powtesting.EngineClient{PayloadIDBytes: id, ExecutionPayloadCapella: &v1.ExecutionPayloadCapella{BlockNumber: 3, Withdrawals: withdrawals}, BlockValue: big.NewInt(162927606e9)} // 0.16 ETH | |||
| vs.ExecutionEngineCaller = &powtesting.EngineClient{PayloadIDBytes: id, ExecutionPayloadCapella: &v1.ExecutionPayloadCapella{BlockNumber: 3, Withdrawals: withdrawals}, BlockValue: 162927606} // 0.16 ETH | |||
There was a problem hiding this comment.
In fact probably this test can be completely removed as there's no overflow to test anymore.
There was a problem hiding this comment.
I'll just remove the test
| @@ -323,7 +322,7 @@ func createWrappedPayloadHeaderCapella(t testing.TB) interfaces.ExecutionData { | |||
| BlockHash: make([]byte, fieldparams.RootLength), | |||
| TransactionsRoot: make([]byte, fieldparams.RootLength), | |||
| WithdrawalsRoot: make([]byte, fieldparams.RootLength), | |||
| }, big.NewInt(11)) | |||
| }, 11) | |||
There was a problem hiding this comment.
This number is not used right? cause I don't see it tested anywhere, and if it was 11 Wei before, it should be 0 Gwei now I guess
There was a problem hiding this comment.
Good point. Changed it to 0s
Following up #12290, Default execution value type to uin64. Consensus layer uses gwei as the primitive type so it's better to have that as the native first class citizen rather than using big.Int