search

attestantio / go-eth2-client

Apache License 2.0
102 stars 59 forks source link

Fix problems with new Electra structs #136

Closed jtraglia closed 2 months ago

jtraglia commented 2 months ago

This PR fixes the issues discovered by testing with the Electra reference structures.

One of the tests is still failing, but I'm pretty sure the ref test is wrong here.

I sent this message to hww:

image
mcdee commented 2 months ago

Extra data of 0x is an empty value. In YAML strings don't have to be quoted, as far as I'm aware. You will find that consensus and execution nodes are a little lax when it comes to formatting, so it's likely that you will have to handle that situation.

jtraglia commented 2 months ago

@mcdee I'm not entirely sure how to fix this 0x issue. I can fix our marshaled output, but the test code will marshal it to a string, which has a different value. It will marshal it to 0 (not 0x) there.

func testYAMLFormat(input []byte) string {
    val := make(map[string]any)
    if err := yaml.UnmarshalWithOptions(input, &val, yaml.UseOrderedMap()); err != nil {
        panic(err)
    }
        ...

For example, the following. The top line is the expected output, the bottom line is the actual output:

image

Any advice here?

jtraglia commented 2 months ago

Ah I found that you did this for Capella. I'll copy this:

image
jtraglia commented 2 months ago

Pushed a commit which handles that edge case. Also, Hsiao-Wei has acknowledged the issue. I'm going to make a PR to the consensus-specs to get that fixed. The fix might not make it into the next release though.