Adjust semantic tests to work on both EOF and legacy #15768
Conversation
| new B1{value: 10}(); | ||
| new B2{value: 10}(); | ||
| new B3{value: 10}(); | ||
| new B1{value: 10, salt: hex"00"}(); | ||
| new B2{value: 10, salt: hex"01"}(); | ||
| new B3{value: 10, salt: hex"02"}(); |
There was a problem hiding this comment.
Cases like this fail on EOF, because the default salt is 0 and therefore unsalted new always generates the same address. And deploying to the same address causes a revert unless the initcode is identical.
| uint[4] memory input; | ||
| uint[6] memory input; | ||
| input[0] = p1.X; | ||
| input[1] = p1.Y; | ||
| input[2] = p2.X; | ||
| input[3] = p2.Y; | ||
| bool success; | ||
| assembly { | ||
| success := call(sub(gas(), 2000), 6, 0, input, 0xc0, r, 0x60) | ||
| // Use "invalid" to make gas estimation work | ||
| switch success case 0 { invalid() } | ||
| } | ||
| (bool success, bytes memory encodedResult) = address(6).call(abi.encode(input)); |
There was a problem hiding this comment.
This is something I don't really understand. According to https://www.evm.codes/precompiled ecadd (precompile at address 0x06) accepts 4 uints. However the original code was passing in 6 of them (0xc0 bytes). It reverts if I try to pass in 4.
Similarly, ecmul below seems to expect 4 rather than 3 parameters.
There was a problem hiding this comment.
BTW, the reason I'm removing the assembly blocks in this file is of course that call() is only available on legacy and the equivalent extcall() only on EOF. We currently can't have assembly that works on both, but we can use <address>.call() in Solidity, which gets automatically translated to the right opcode.
This contract seems to be using assembly only to work around some problem related to gas estimation (and maybe also to make the call cheaper), which is not an issue in our tests.
There was a problem hiding this comment.
This one was almost identical to gas_and_value_basic.sol (modulo some formatting/naming). I think that it was created based on the other one when we introduced the brace syntax. But then when we removed that syntax, we updated the original test instead of removing it.
| // gas legacyOptimized: 180756 | ||
| // getAddress() -> 0x137aa4dfc0911524504fcd4d98501f179bc13b4a | ||
| // balance: 0x137aa4dfc0911524504fcd4d98501f179bc13b4a -> 500 | ||
| // balance: 0x0000000000000000000000000000000000001234 -> 500 |
There was a problem hiding this comment.
Here the problem was that the balance helper requires hard-coding the address and, if we get that address by deploying a contract, it will be different on EOF.
To test balance, we don't really need a new contract though. We can just send some value to a randomly chosen address and then it won't change on EOF.
| @@ -6,19 +6,19 @@ contract Succeeds { | |||
| } | |||
|
|
|||
| contract C { | |||
| function f() public returns (Reverts x, uint, string memory txt) { | |||
There was a problem hiding this comment.
Not sure what the uint was for. It's unused so it does not make any difference other than adding an extra zero in expectations. It does seems too relevant to the test so I removed it.
| } | ||
|
|
||
| function sendAmount(uint256 amount) public returns (uint256 bal) { | ||
| return h.getBalance{value: amount + 3, gas: 1000}(); |
There was a problem hiding this comment.
The name of this test is so vague that it's useless. I couldn't tell what this was supposed to test just from that, but looking at the oldest version, it was for multiple applications of .value():
return h.getBalance.value(amount).gas(1000).value(amount + 3)();// overwrite valueThis is no longer possible with the {value: ...} syntax so the test is obsolete and should have been removed when we deprecated the old syntax.
- The ones in `functionCall` were testing multiple applications of `.value()`, which is no longer possible with the `{value: ...}` syntax.
- The ones in `various` became identical when the `.value()` syntax was deprecated.
…th EOF and legacy
cameel
force-pushed
the
adjust-semantic-tests-to-work-on-eof-too
branch
from
43d9396 to
f5f54f5
Compare
Prerequisite for #15665.
We have some semantic tests which don't pass on EOF as is, but can fairly easily be adjusted to pass without losing the original purpose of the test. This PR does that for the cases I noticed while reviewing #15665.
It also removes some tests for the old
.value()syntax that are now redundant.