Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix IndexedAttestation decoding #17

Closed
wants to merge 2 commits into from

Conversation

jtraglia
Copy link
Collaborator

@jtraglia jtraglia commented Jul 7, 2022

📝 Summary

When reviewing builder types, I noticed that the json encoding for AttestingIndices is missing the string option. The beacon API states (see reference for example) that this should be a list of uint64 strings.

AttestingIndices []uint64 `json:"attesting_indices" ssz-max:"2048"` // MAX_VALIDATORS_PER_COMMITTEE

I was expecting to see something like json:"attesting_indices,string" but I know that won't work.

Other uint64 fields are told to be strings, for example:

Slot uint64 `json:"slot,string"`

I made a test to verify my theory. I've included it here. Keep in mind it will currently fail with this message:

make test
go test ./...
ok  	github.com/flashbots/go-boost-utils/bls	(cached)
--- FAIL: TestIndexedAttestation (0.00s)
    builder_test.go:231:
        	Error Trace:	builder_test.go:231
        	Error:      	Received unexpected error:
        	            	json: cannot unmarshal string into Go struct field IndexedAttestation.attesting_indices of type uint64
        	Test:       	TestIndexedAttestation
FAIL
FAIL	github.com/flashbots/go-boost-utils/types	0.114s
FAIL
make: *** [test] Error 1

It will pass if you change attesting_indices from this to that:

"attesting_indices": [
        "1", "2", "3"
      ],
"attesting_indices": [
        1, 2, 3
      ],

Opening as a draft because I'm not entirely sure what the proper fix it. Would like to discuss.

⛱ Motivation and Context

There's a minor problem with IndexedAttestation encoding/decoding that should be fixed.

📚 References


✅ I have run these commands

  • make lint
  • make test
  • go mod tidy

@jtraglia
Copy link
Collaborator Author

jtraglia commented Jul 8, 2022

I think we need to add a U64Str type to common.go with MarshalText, UnmarshalJSON, and UnmarshalText methods. I've spent a little time trying to figure this out, but I can't seem to get it working. Since this isn't that straight-foward, I'm going close this PR, make an issue, and let someone else figure out how to fix it.

@jtraglia jtraglia closed this Jul 8, 2022
@metachris metachris reopened this Jul 8, 2022
@metachris
Copy link
Collaborator

Reopening this PR because the test is useful and we can reuse that in the proper solution.

@metachris
Copy link
Collaborator

See also #18

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants