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

[Altair] Add regression test for parsing non-Altair StandardConfig #2343

Closed
wants to merge 3 commits into from

Conversation

paulhauner
Copy link
Member

@paulhauner paulhauner commented May 14, 2021

Issue Addressed

NA

Proposed Changes

I was wondering why StandardConfig was wrapped in a SuperStruct, so I figured I'd let the compiler tell me. I'll let this run against CI to see if there's anything I've missed.

Adds a regression test to ensure we can parse a StandardConfig without Altair params.

Additional Info

NA

@paulhauner paulhauner added the work-in-progress PR is a work-in-progress label May 14, 2021
@michaelsproul
Copy link
Member

This is for backwards compatibility, and I added it relatively recently. I wanted to be able to parse a "base-only" config from JSON, as this was required to work with the Pyrmont Teku nodes hosted by Infura.

With Proto's config overhaul we'll need to restructure again, so I'm against changing this for now

@michaelsproul
Copy link
Member

Proto's PR: ethereum/consensus-specs#2390

@paulhauner
Copy link
Member Author

paulhauner commented May 14, 2021

This is for backwards compatibility, and I added it relatively recently. I wanted to be able to parse a "base-only" config from JSON, as this was required to work with the Pyrmont Teku nodes hosted by Infura.

Ooh right, I didn't pick up that the serde(untagged) was achieving this, given the SuperStruct stuff in macro-land.

I've reverted my previous commit and added a regression test to prevent this happening again.

@paulhauner paulhauner changed the title [Altair] Remove SuperStruct from StandardConfig [Altair] Add regression test for parsing non-Altair StandardConfig May 14, 2021
@paulhauner paulhauner added ready-for-review The code is ready for review and removed work-in-progress PR is a work-in-progress labels May 14, 2021
@paulhauner paulhauner marked this pull request as ready for review May 14, 2021 05:41
@paulhauner
Copy link
Member Author

Oh, this test already exists as decode_no_altair, my bad. I'll close this.

@paulhauner paulhauner closed this May 14, 2021
@michaelsproul
Copy link
Member

No worries, I appreciate the attention to detail

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-for-review The code is ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants