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

eof: Semantic tests update #15665

Merged
merged 1 commit into from
Jan 31, 2025
Merged

Conversation

rodiazet
Copy link
Contributor

@rodiazet rodiazet commented Dec 19, 2024

  • Disable some tests which don't make sense for EOF.
  • Modify tests to make them EOF compatible and move them to separated folders
  • Adjust test expectation to avoid using deployed contract address as it can change between optimized and non-optimized versions for EOF

Depends on: #15768 Merged

This comment was marked as resolved.

Copy link
Member

@cameel cameel left a comment

Choose a reason for hiding this comment

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

I found one case where disabling the test actually hides broken behavior (delete on an external function pointer).

In most other cases I commented on we need an EOF version or a TODO to mark the test to be dealt with later.

There were also many tests where a separate EOF version was not really necessary - we can avoid duplication by adjusting the original to work on both EOF and legacy. I actually did that myself while going over the PR (see #15768) so you only need to rebase on that, remove the unnecessary copies for EOF and re-enable the original tests.

Copy link
Member

Choose a reason for hiding this comment

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

This one should have an EOF version.

This comment was marked as resolved.

Copy link
Member

Choose a reason for hiding this comment

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

Also:

  • salted_create/prediction_example.sol: calculation on EOF is different, but should still be covered.
  • various/create_calldata.sol: on EOF the calldata accessible via msg.data does contain constructor arguments and this should be tested.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Problem with event_emit_from_other_contact is that the address in Deposit event expectation depends on contract bytecode. Which makes testing this in semantic tests impossible.

Regarding prediction_example I don't see any option to jest it as we don't have access to contract bytecode and more over the bytecode differs between opt and non-opt versions.

Copy link
Member

Choose a reason for hiding this comment

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

Problem with event_emit_from_other_contact is that the address in Deposit event expectation depends on contract bytecode.

Ah, true. I guess we can't test that now.

Regarding prediction_example I don't see any option to jest it as we don't have access to contract bytecode and more over the bytecode differs between opt and non-opt versions.

Right, my bad. That will only be possible when address calculation in eofcreate gets changed not to include the bytecode hash.

I mean, we could technically consider allowing access to type(C).creationCode, because we know the bytecode, but we probably shouldn't.

@rodiazet rodiazet force-pushed the eof-semantic-tests branch 4 times, most recently from ce53f63 to 7edff9d Compare January 27, 2025 12:55
@cameel cameel added the has dependencies The PR depends on other PRs that must be merged first label Jan 27, 2025
@cameel
Copy link
Member

cameel commented Jan 30, 2025

#15768 has been merged. Please rebase.

@rodiazet rodiazet force-pushed the eof-semantic-tests branch 5 times, most recently from c0bf6f1 to bf5bd87 Compare January 30, 2025 15:53
@rodiazet rodiazet requested a review from cameel January 30, 2025 15:54
@rodiazet rodiazet force-pushed the eof-semantic-tests branch 4 times, most recently from e5235c1 to 6bb5aa5 Compare January 31, 2025 10:53
@rodiazet rodiazet requested a review from cameel January 31, 2025 14:58
Copy link
Member

Choose a reason for hiding this comment

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

I removed value_complex.sol and value_insane.sol (see #15768 (comment) for context), so we no longer need their EOF versions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

cameel
cameel previously approved these changes Jan 31, 2025
Copy link
Member

@cameel cameel left a comment

Choose a reason for hiding this comment

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

Already approving since there are just two minor tweaks needed before we can merge.

@cameel cameel enabled auto-merge January 31, 2025 15:17
@cameel cameel merged commit be30965 into ethereum:develop Jan 31, 2025
72 of 73 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
EOF external contribution ⭐ has dependencies The PR depends on other PRs that must be merged first testing 🔨
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants