-
Notifications
You must be signed in to change notification settings - Fork 6k
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
eof: Semantic tests update #15665
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
There was a problem hiding this 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.
test/libsolidity/semanticTests/constructor/eof/no_callvalue_check.sol
Outdated
Show resolved
Hide resolved
test/libsolidity/semanticTests/externalContracts/eof/deposit_contract.sol
Outdated
Show resolved
Hide resolved
test/libsolidity/semanticTests/functionCall/eof/call_options_overload.sol
Outdated
Show resolved
Hide resolved
test/libsolidity/semanticTests/functionTypes/eof/function_external_delete_storage.sol
Outdated
Show resolved
Hide resolved
test/libsolidity/semanticTests/isoltestTesting/eof/balance_other_contract.sol
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
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.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
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 viamsg.data
does contain constructor arguments and this should be tested.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 inDeposit
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.
test/libsolidity/semanticTests/deployedCodeExclusion/subassembly_deduplication.sol
Outdated
Show resolved
Hide resolved
test/libsolidity/semanticTests/builtinFunctions/eof/keccak256_packed_complex_types.sol
Outdated
Show resolved
Hide resolved
ce53f63
to
7edff9d
Compare
7edff9d
to
48d7d81
Compare
#15768 has been merged. Please rebase. |
test/libsolidity/semanticTests/events/event_emit_from_other_contract.sol
Outdated
Show resolved
Hide resolved
test/libsolidity/semanticTests/functionCall/eof/calling_nonexisting_contract_throws.sol
Outdated
Show resolved
Hide resolved
c0bf6f1
to
bf5bd87
Compare
test/libsolidity/semanticTests/builtinFunctions/keccak256_packed_complex_types.sol
Outdated
Show resolved
Hide resolved
e5235c1
to
6bb5aa5
Compare
6bb5aa5
to
152f4af
Compare
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
There was a problem hiding this 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.
152f4af
to
c2f1425
Compare
Depends on: #15768Merged