Skip to content

chore(core/types): add Test_Body_hasOptionalFields to enforce hasLaterOptionalField is up to date #113

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

Closed
wants to merge 5 commits into from

Conversation

qdm12
Copy link

@qdm12 qdm12 commented Feb 3, 2025

Why this should be merged

To enforce updating the hasLaterOptionalField field if more optional fields are added to Body

How this works

Use some reflect magic in a unit test

How this was tested

@ARR4N
Copy link
Collaborator

ARR4N commented Feb 3, 2025

Any fields added in the future will have to be optional. The change-detector will pick up on these too.

@qdm12
Copy link
Author

qdm12 commented Feb 3, 2025

The change-detector will pick up on these too.

yes but it would not pick up if we forget to update the hasLaterOptionalField expression. I thought about it working (/messing around) on the header, where we have many optional fields and the expression becomes an or of != nil checks.

@@ -146,6 +146,10 @@ func (b *Body) EncodeRLP(dst io.Writer) error {
})
}

func (b *Body) hasOptionalFields() bool {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
func (b *Body) hasOptionalFields() bool {
func (b *Body) hasOptionalFieldsSet() bool {

Copy link
Author

Choose a reason for hiding this comment

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

Renamed to hasAnyOptionalFieldSet since it should be true if any (not all) optional field is set.

field := v.Field(i)
before := field.Interface()
switch field.Kind() {
case reflect.Slice:
Copy link
Collaborator

Choose a reason for hiding this comment

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

This needs support for reflect.Pointer as the optional fields can be struct pointers.

Copy link
Author

Choose a reason for hiding this comment

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

We can just add them later on a add-when-needed basis. If it's not defined in the switch, the test would fail

@qdm12 qdm12 requested a review from ARR4N February 4, 2025 14:35
Base automatically changed from arr4n/body-rlp-override to main February 5, 2025 10:52
@qdm12 qdm12 force-pushed the qdm12/body-rlp-override-optional-test branch from 03e6373 to a0cec28 Compare February 5, 2025 12:38
@qdm12 qdm12 force-pushed the qdm12/body-rlp-override-optional-test branch from a0cec28 to 24b3aec Compare February 5, 2025 12:38
@ARR4N
Copy link
Collaborator

ARR4N commented Feb 6, 2025

This will likely be made obsolete by #120

@qdm12 qdm12 added the Status: 🔴 DO NOT MERGE This PR is not meant to be merged in its current state label Feb 7, 2025
@ARR4N
Copy link
Collaborator

ARR4N commented Feb 7, 2025

Unnecessary following merge of #120

@ARR4N ARR4N closed this Feb 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: 🔴 DO NOT MERGE This PR is not meant to be merged in its current state
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants