chore(types): remove ExecutionPayloadHeader version#2701
chore(types): remove ExecutionPayloadHeader version#2701
Conversation
…nding-partial-withdrawals
…hain/beacon-kit into pppw-gindexes
…hain/beacon-kit into pppw-gindexes
…hain/beacon-kit into pppw-gindexes
| ); err != nil { | ||
| return err | ||
| // NOTE: marshalling this struct is NOT affected by it's own fork version. | ||
| if version.IsBefore(forkVersion, version.Electra()) { |
There was a problem hiding this comment.
Can delay the hard fork by making this version.Electra1() or something else in the future
There was a problem hiding this comment.
This refactored code is still backwards compatible
There was a problem hiding this comment.
super nit: consider inverting test to simplfiy code. Something like
if !version.IsBefore(forkVersion, version.Electra()) {
// post Electra we simplified marshalling and we do not need to store version
return nil
}
...…into remove-eph-version
calbera
changed the title
|
Agree with this change for a later fork. Will review properly after all bectra related work is done |
| eth1DepositIndex sdkcollections.Item[uint64] | ||
| // latestExecutionPayloadVersion stores the latest execution payload | ||
| // version. | ||
| // version. Kept for backwards compatibility for versions before Electra. |
There was a problem hiding this comment.
I am afraid we really need to do that. One of the downside of a blockchain
| // the BeaconStore. | ||
| func (kv *KVStore) SetLatestExecutionPayloadHeader( | ||
| payloadHeader *ctypes.ExecutionPayloadHeader, | ||
| payloadHeader *ctypes.ExecutionPayloadHeader, forkVersion common.Version, |
There was a problem hiding this comment.
spitballing here: an alternative could be to have a SetLatestExecutionPayloadHeaderPreElectra and a SetLatestExecutionPayloadHeader.
From Pectra on we would call only the latter, but clients of this method would have to decide which method to call.
Probably does not make much of a different, just pointing it out
abi87
left a comment
abi87
left a comment
There was a problem hiding this comment.
Content is fine by me. I agree it's probably slightly best to include this change in a sequent fork instead of Electra, but not by much.
3 participants
A non-critical proposal to clean up tech debt. This PR can be merged now but chosen to be kept as a simple refactor instead of a hard fork by changing this line