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

simulators/ethereum/engine: struct inputs and TransitionConfigurationV1 tests #559

Closed
wants to merge 4 commits into from

Conversation

MarcusWentz
Copy link
Member

Testing inputs for Engine API struct parameters (including TransitionConfigurationV1):
-prefix (0x00)
-long
-short

TransitionConfigurationV1 tests check to see if CL == EL for parameters:
-TTD
-TerminalBlockNumber
-TerminalBlockHash

Note: cannot directly test Uint64/BigInt parameters due to marshaling restriction.
Could possibly be tested with building raw packets/RLP but also should be covered by Engine API Fuzz tests.

@MarcusWentz MarcusWentz changed the title Engine API struct inputs and TransitionConfigurationV1 tests simulators/ethereum/engine: struct inputs and TransitionConfigurationV1 tests Jun 1, 2022
Comment on lines 251 to 252
// TransitionConfigurationV1
type ExchangeTransitionConfigurationResponseExpectObject struct {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This type of name is pretty common in the Java world, but in go-lang, people usually tend to use shorter names. Also, the docstring is wrong -- it should always begin with the name of the struct it is describing.

Could you please write a brief description of what this struct is/does, and then maybe we can make the name a bit briefer.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shortened name and routine docstring updated below.

}
}

type ExchangeTransitionConfigurationBytesResponseExpectObject struct {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand the need for these types of objects.. ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The original struct was not compatible with the modified bytes version.

The original struct was not used for any tests, so I have removed the original version but kept the bytes version below.

@@ -230,6 +248,34 @@ func (exp *GetPayloadResponseExpectObject) ExpectTimestamp(expectedTimestamp uin
}
}

// ExchangeTransitionConfigurationV1 in bytes format.
type ExTransConfigResponseExpectObject struct {
*TestEnv
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Docstring corrected and routine name abbreviated.

Rewriting types as bytes in this context allows test size inputs beyond the original struct data types.

@MarcusWentz
Copy link
Member Author

Closing PR since Geth patch added for logsBloom ethereum/go-ethereum#24946

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.

None yet

2 participants