EIP-7549 beacon spec #13946
EIP-7549 beacon spec #13946
Conversation
| indexedAtt, err := attestation.ConvertToIndexed(ctx, att, committee) | ||
| if err != nil { | ||
| return err | ||
| if att.Version() < version.Electra { |
There was a problem hiding this comment.
we should decide the direction of these checks for more consistency reviewing, some other parts do the reverse where it checks the version is >= version.Electra. just something we can align on.
| indexedAtt, err := attestation.ConvertToIndexed(ctx, att, committee) | ||
| if err != nil { | ||
| return err | ||
| if att.Version() < version.Electra { |
There was a problem hiding this comment.
Same feedback as my other comment, could you refactor the old logic to its own method and put the old spec definition there?
There was a problem hiding this comment.
It's not as straightforward here because the other function was basically replaced in whole. Here only a small subset of validation in the middle of the function is different.
| indexedAtt, err := attestation.ConvertToIndexed(ctx, att, committee) | ||
| if err != nil { | ||
| return err | ||
| if att.Version() < version.Electra { |
There was a problem hiding this comment.
This might be a good place to refactor the pre electra and post electra functions into two different functions
There was a problem hiding this comment.
Not sure about this, see #13946 (comment)
| slices.Sort(attesters) | ||
| return slices.Compact(attesters), nil |
There was a problem hiding this comment.
We will want to test this for performance. Sorting is O(nlogn) where using a map would be O(n) at the cost of additional O(n)
github-merge-queue
bot
removed this pull request from the merge queue due to no response for status checks
github-merge-queue
bot
removed this pull request from the merge queue due to no response for status checks
* EIP-7549 beacon spec * reviews * change signature of AttestingIndices
Implementation of the following changes to beacon spec: