Wrapper around generic serdes methods #15451
Conversation
encoding/ssz/list.go
Outdated
| if nElems == 0 { | ||
| return nil, nil | ||
| } |
There was a problem hiding this comment.
This cannot happen, you already errored out if first < offsetLen and you are guaranteed to be a multiple by the check in line 208, so nElems is guaranteed to be >= 1
encoding/ssz/list.go
Outdated
| // provide the `newt` function that returns a new instance of the type [T] to be unmarshaled. | ||
| // This func will be called for each element in the list to create a new instance of [T]. | ||
| // | ||
| // UnmarshalListFixedElement should be used for fixed-size elements. |
There was a problem hiding this comment.
This comment repeats the one for Variable size, do you want this to be variable sized?
There was a problem hiding this comment.
Actually it's the other way around - my intention was for eg UnmarshalListFixedElement to refer you to UnmarshalListVariableElement for variable elements, and UnmarshalListVariableElement to refer you to UnmarshalListFixedElement for fixed elements.
Now I've noticed 😱 // UnmarshalListFixedElement unmarshals a ssz-encoded list of variable-sized elements.... I will double check all the comment and clean them up, thanks.
| // looking ahead +1 (starting with a `buf` that has already had the first offset sliced off), | ||
| // with the final element handled as a special case outside the loop (using the size of the entire buffer | ||
| // as the ending bound). | ||
| previous := first |
There was a problem hiding this comment.
nit but you can just reuse first here since it is not used again.
There was a problem hiding this comment.
At first I just had a variable called previous, but I thought assignment to a new variable name improved readability.
| // SSZ Lists have different encoding rules depending whether their elements are fixed- or variable-sized, | ||
| // and we can't differentiate them by the ssz interface, so it is the caller's responsibility to | ||
| // pick the correct method. | ||
| func UnmarshalListVariableElement[T Unmarshalable](buf []byte, newt func() T) ([]T, error) { |
There was a problem hiding this comment.
Checked the logic of this function and it looks good and safe to me except the discussion offline
kasey
force-pushed
the
generic-list-serdes-wrapper
branch
2 times, most recently
from
ba7d971 to
b51b06b
Compare
Co-authored-by: Bastin <[email protected]>
kasey
force-pushed
the
generic-list-serdes-wrapper
branch
from
b51b06b to
fa6a09b
Compare
What type of PR is this?
Feature
What does this PR do? Why is it needed?
#15447 adds a set of generic methods for serdes on ssz lists of fixed or variable sized elements. This PR shows how we could wrap those methods up for more convenient use. The
MarshalandUnmarshalmethods retain the type safety of the direct methods but are less cumbersome, because they encapsulate the constructor function. As demonstrated here, we could just create one package level value for each type that needs this functionality.Other notes for review
This PR is built directly on top of 15447 and we should only merge one of the two. I prefer this extra encapsulation but am making separate PRs for feedback on the design.
Acknowledgements